Fenced frame: Network cutoff callback waits for sub-fenced frames. [chromium/src : main]

0 views
Skip to first unread message

Andrew Verge (Gerrit)

unread,
Apr 9, 2024, 4:35:27 PMApr 9
to Liam Brady, Tricium, Garrett Tanzer, Shivani Sharma, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
Attention needed from Garrett Tanzer, Liam Brady, Nasko Oskov and Shivani Sharma

Andrew Verge added 3 comments

File content/browser/renderer_host/render_frame_host_impl.cc
Line 9233, Patchset 5: // TODO(averge): Credentialless iframes have their own per-`Page` nonce that
Shivani Sharma . unresolved

Wasn't this TODO handled in https://chromium-review.googlesource.com/c/chromium/src/+/5376066 ?

Andrew Verge

+1

`RevokeNetworkForNonces` will only call this function after both nonces have been tagged for revocation, so there's no extra work to do.

Line 9254, Patchset 8 (Latest): bool network_cutoff_ready = all_children_have_network_cutoff &&
Andrew Verge . unresolved

In the case where we have a fenced frame and an iframe nested underneath it, traversal will hit the iframe first. It will use the properties of the closest ancestor, which is the parent fenced frame, and trigger the revocation callback for it.

The iteration will then move up to the fenced frame itself, but all of the work has been done already when examining the iframe. Can we add a clause like this:

```
// If network has already been disabled for this node previously, ignore it.
if (properties->has_disabled_untrusted_network()) {
continue;
}
```

This prevents a fenced frame tree frame which has already had network fully disabled (meaning, bit is set and callback has been run) from being examined again. Might save some cycles?

Line 9269, Patchset 7: continue;
Andrew Verge . unresolved

Do you need continues? Whether you enter this inner if clause or not, you'll still repeat the loop at the end of `if (network_cutoff_ready)`

Same below.

Open in Gerrit

Related details

Attention is currently required from:
  • Garrett Tanzer
  • Liam Brady
  • Nasko Oskov
  • Shivani Sharma
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
Gerrit-Change-Number: 5426603
Gerrit-PatchSet: 8
Gerrit-Owner: Liam Brady <lbr...@google.com>
Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
Gerrit-Reviewer: Liam Brady <lbr...@google.com>
Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
Gerrit-CC: Shivani Sharma <shiva...@chromium.org>
Gerrit-Attention: Garrett Tanzer <gta...@chromium.org>
Gerrit-Attention: Liam Brady <lbr...@google.com>
Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
Gerrit-Attention: Nasko Oskov <na...@chromium.org>
Gerrit-Comment-Date: Tue, 09 Apr 2024 20:35:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Shivani Sharma <shiva...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Liam Brady (Gerrit)

unread,
Apr 9, 2024, 5:21:27 PMApr 9
to Tricium, Garrett Tanzer, Andrew Verge, Shivani Sharma, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
Attention needed from Andrew Verge, Garrett Tanzer, Nasko Oskov and Shivani Sharma

Liam Brady added 7 comments

Commit Message
Line 7, Patchset 5:Fenced frame: Network cutoff callback waits for subframes.
Shivani Sharma . resolved

nit: sub-fenced frames

Liam Brady

Done

Line 13, Patchset 5:accessed until all subframes have also cut off their own network.
Shivani Sharma . resolved

nit: sub-fenced frames here and in other places.

Liam Brady

Done

Line 19, Patchset 5:network cutoff has truly kicked in. In other words, once all subframes
in a fenced frame tree have had their network cut off, only then will
Shivani Sharma . resolved

nested fenced frame start their own FF tree, so this sounds confusing and seems to handle iframes. Suggest rephrasing to:
"once all sub-fenced frame trees have had..."

Liam Brady

Done

Line 34, Patchset 5:The function that determines the network cutoff status involves a
recursive depth-first traversal of the entire frame tree (including
crossing fenced frame boundaries) to see if any subframes still have
Shivani Sharma . resolved

I haven't yet looked at the code, but can we instead recompute the ancestor FFs' disable

Liam Brady

We need to look at each frame, since a fenced frame's same-origin child iframe could've been the frame that called `disableUntrustedNetwork()`, which would require calling the resolve callback housed in that `RenderFrameHost`'s `FencedDocumentData`.

I think we can do what Andrew's suggesting [here](https://chromium-review.googlesource.com/c/chromium/src/+/5426603/comment/cef6b2f2_9efb7e0e/) to avoid repeating work.

File content/browser/renderer_host/render_frame_host_impl.cc
Line 9233, Patchset 5: // TODO(averge): Credentialless iframes have their own per-`Page` nonce that
Shivani Sharma . resolved

Wasn't this TODO handled in https://chromium-review.googlesource.com/c/chromium/src/+/5376066 ?

Andrew Verge

+1

`RevokeNetworkForNonces` will only call this function after both nonces have been tagged for revocation, so there's no extra work to do.

Liam Brady

Done

Line 9254, Patchset 8: bool network_cutoff_ready = all_children_have_network_cutoff &&
Andrew Verge . resolved

In the case where we have a fenced frame and an iframe nested underneath it, traversal will hit the iframe first. It will use the properties of the closest ancestor, which is the parent fenced frame, and trigger the revocation callback for it.

The iteration will then move up to the fenced frame itself, but all of the work has been done already when examining the iframe. Can we add a clause like this:

```
// If network has already been disabled for this node previously, ignore it.
if (properties->has_disabled_untrusted_network()) {
continue;
}
```

This prevents a fenced frame tree frame which has already had network fully disabled (meaning, bit is set and callback has been run) from being examined again. Might save some cycles?

Liam Brady

We still need to run any callbacks registered in the `RenderFrameHost`'s `FencedDocumentData`, but I can add this to save some redundant processing.

Andrew Verge . resolved

Do you need continues? Whether you enter this inner if clause or not, you'll still repeat the loop at the end of `if (network_cutoff_ready)`

Same below.

Liam Brady

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Andrew Verge
  • Garrett Tanzer
  • Nasko Oskov
  • Shivani Sharma
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
Gerrit-Change-Number: 5426603
Gerrit-PatchSet: 9
Gerrit-Owner: Liam Brady <lbr...@google.com>
Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
Gerrit-Reviewer: Liam Brady <lbr...@google.com>
Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
Gerrit-CC: Shivani Sharma <shiva...@chromium.org>
Gerrit-Attention: Garrett Tanzer <gta...@chromium.org>
Gerrit-Attention: Andrew Verge <ave...@chromium.org>
Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
Gerrit-Attention: Nasko Oskov <na...@chromium.org>
Gerrit-Comment-Date: Tue, 09 Apr 2024 21:21:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Andrew Verge <ave...@chromium.org>
Comment-In-Reply-To: Shivani Sharma <shiva...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Shivani Sharma (Gerrit)

unread,
Apr 10, 2024, 4:51:47 PMApr 10
to Liam Brady, Tricium, Garrett Tanzer, Andrew Verge, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
Attention needed from Andrew Verge, Garrett Tanzer, Liam Brady and Nasko Oskov

Shivani Sharma added 2 comments

File content/browser/fenced_frame/fenced_frame_config.h
Line 642, Patchset 10 (Latest): bool is_partition_nonce_revoked_ = false;

// Whether this fenced frame has had its network access cut off (i.e., if this
// is true, untrusted network access should be disabled and access to
// unpartitioned storage should be enabled.)
bool has_disabled_untrusted_network_ = false;
Shivani Sharma . unresolved

both of these are states of the progress of disableUntrustedNetwork so suggest putting them in an enum. Using kCurrentFrameTreeComplete for is_partition_nonce_revoked_

`enum class DisableUntrustedNetworkStatus {
kNotStarted,
kCurrentFrameTreeComplete,
kCurrentAndNestedFrameTreesComplete
}`
Line 586, Patchset 10 (Latest): // The nonce that will be included in the IsolationInfo for network requests
Shivani Sharma . unresolved

replace the rest of the comment with "IsolationInfo, CookiePartitionKey and StorageKey for network, cookie and storage partitioning, respectively. As part of IsolationInfo it is also used to identify which network requests should be disallowed in the network service if the initiator fenced frame tree has had its network cutoff via disableUntrustedNetwork()."

Open in Gerrit

Related details

Attention is currently required from:
  • Andrew Verge
  • Garrett Tanzer
  • Liam Brady
  • Nasko Oskov
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
    Gerrit-Change-Number: 5426603
    Gerrit-PatchSet: 10
    Gerrit-Owner: Liam Brady <lbr...@google.com>
    Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
    Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
    Gerrit-Reviewer: Liam Brady <lbr...@google.com>
    Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
    Gerrit-CC: Shivani Sharma <shiva...@chromium.org>
    Gerrit-Attention: Garrett Tanzer <gta...@chromium.org>
    Gerrit-Attention: Andrew Verge <ave...@chromium.org>
    Gerrit-Attention: Liam Brady <lbr...@google.com>
    Gerrit-Attention: Nasko Oskov <na...@chromium.org>
    Gerrit-Comment-Date: Wed, 10 Apr 2024 20:51:39 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Garrett Tanzer (Gerrit)

    unread,
    Apr 11, 2024, 11:55:52 AMApr 11
    to Liam Brady, Tricium, Andrew Verge, Shivani Sharma, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
    Attention needed from Andrew Verge, Liam Brady and Shivani Sharma

    Garrett Tanzer added 2 comments

    File content/browser/fenced_frame/fenced_frame_config.h
    Line 642, Patchset 10 (Latest): bool is_partition_nonce_revoked_ = false;

    // Whether this fenced frame has had its network access cut off (i.e., if this
    // is true, untrusted network access should be disabled and access to
    // unpartitioned storage should be enabled.)
    bool has_disabled_untrusted_network_ = false;
    Shivani Sharma . unresolved

    both of these are states of the progress of disableUntrustedNetwork so suggest putting them in an enum. Using kCurrentFrameTreeComplete for is_partition_nonce_revoked_

    `enum class DisableUntrustedNetworkStatus {
    kNotStarted,
    kCurrentFrameTreeComplete,
    kCurrentAndNestedFrameTreesComplete
    }`
    Garrett Tanzer

    +1

    File third_party/blink/web_tests/wpt_internal/fenced_frame/disable-untrusted-network-nested.https.html
    Line 7, Patchset 10 (Latest):<script src="resources/utils.js"></script>
    Garrett Tanzer . unresolved

    I think at least some of the WPT test cases could be integrated into the existing shared storage get() tests:

    For example, there is a TODO explicitly for this:
    https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/wpt_internal/fenced_frame/content-shared-storage-get.https.html;l=123

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andrew Verge
    • Liam Brady
    • Shivani Sharma
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
    Gerrit-Change-Number: 5426603
    Gerrit-PatchSet: 10
    Gerrit-Owner: Liam Brady <lbr...@google.com>
    Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
    Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
    Gerrit-Reviewer: Liam Brady <lbr...@google.com>
    Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
    Gerrit-CC: Shivani Sharma <shiva...@chromium.org>
    Gerrit-Attention: Andrew Verge <ave...@chromium.org>
    Gerrit-Attention: Liam Brady <lbr...@google.com>
    Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
    Gerrit-Comment-Date: Thu, 11 Apr 2024 15:55:42 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Shivani Sharma <shiva...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Andrew Verge (Gerrit)

    unread,
    Apr 11, 2024, 3:36:19 PMApr 11
    to Liam Brady, Tricium, Garrett Tanzer, Shivani Sharma, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
    Attention needed from Liam Brady and Shivani Sharma

    Andrew Verge added 1 comment

    File third_party/blink/web_tests/wpt_internal/fenced_frame/disable-untrusted-network-nested.https.html
    Line 25, Patchset 10 (Latest): could_call_get = true;
    Andrew Verge . unresolved

    You can skip all the `could_call_get` stuff by adding an `assert_unreached()` at this line instead. If the get() completes successfully, the assert will be executed next, which will throw immediately.

    Then you can double check the type of error in the catch, and make sure it's not `AssertionError`, I believe

    https://web-platform-tests.org/writing-tests/testharness-api.html#assert_unreached

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Liam Brady
    • Shivani Sharma
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
    Gerrit-Change-Number: 5426603
    Gerrit-PatchSet: 10
    Gerrit-Owner: Liam Brady <lbr...@google.com>
    Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
    Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
    Gerrit-Reviewer: Liam Brady <lbr...@google.com>
    Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
    Gerrit-CC: Shivani Sharma <shiva...@chromium.org>
    Gerrit-Attention: Liam Brady <lbr...@google.com>
    Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
    Gerrit-Comment-Date: Thu, 11 Apr 2024 19:36:04 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Liam Brady (Gerrit)

    unread,
    Apr 11, 2024, 5:47:48 PMApr 11
    to Tricium, Garrett Tanzer, Andrew Verge, Shivani Sharma, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
    Attention needed from Andrew Verge, Garrett Tanzer and Shivani Sharma

    Liam Brady added 4 comments

    File content/browser/fenced_frame/fenced_frame_config.h
    Line 642, Patchset 10: bool is_partition_nonce_revoked_ = false;


    // Whether this fenced frame has had its network access cut off (i.e., if this
    // is true, untrusted network access should be disabled and access to
    // unpartitioned storage should be enabled.)
    bool has_disabled_untrusted_network_ = false;
    Shivani Sharma . resolved

    both of these are states of the progress of disableUntrustedNetwork so suggest putting them in an enum. Using kCurrentFrameTreeComplete for is_partition_nonce_revoked_

    `enum class DisableUntrustedNetworkStatus {
    kNotStarted,
    kCurrentFrameTreeComplete,
    kCurrentAndNestedFrameTreesComplete
    }`
    Garrett Tanzer

    +1

    Liam Brady

    Done

    Line 586, Patchset 10: // The nonce that will be included in the IsolationInfo for network requests
    Shivani Sharma . resolved

    replace the rest of the comment with "IsolationInfo, CookiePartitionKey and StorageKey for network, cookie and storage partitioning, respectively. As part of IsolationInfo it is also used to identify which network requests should be disallowed in the network service if the initiator fenced frame tree has had its network cutoff via disableUntrustedNetwork()."

    Liam Brady

    Done

    File third_party/blink/web_tests/wpt_internal/fenced_frame/disable-untrusted-network-nested.https.html
    Line 7, Patchset 10:<script src="resources/utils.js"></script>
    Garrett Tanzer . resolved

    I think at least some of the WPT test cases could be integrated into the existing shared storage get() tests:

    For example, there is a TODO explicitly for this:
    https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/wpt_internal/fenced_frame/content-shared-storage-get.https.html;l=123

    Liam Brady

    Done

    Line 25, Patchset 10: could_call_get = true;
    Andrew Verge . resolved

    You can skip all the `could_call_get` stuff by adding an `assert_unreached()` at this line instead. If the get() completes successfully, the assert will be executed next, which will throw immediately.

    Then you can double check the type of error in the catch, and make sure it's not `AssertionError`, I believe

    https://web-platform-tests.org/writing-tests/testharness-api.html#assert_unreached

    Liam Brady

    Resolving since I'm merging this test into the test in `content-shared-storage-get.https.html`, which already does that.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andrew Verge
    • Garrett Tanzer
    • Shivani Sharma
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
    Gerrit-Change-Number: 5426603
    Gerrit-PatchSet: 13
    Gerrit-Owner: Liam Brady <lbr...@google.com>
    Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
    Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
    Gerrit-Reviewer: Liam Brady <lbr...@google.com>
    Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
    Gerrit-CC: Shivani Sharma <shiva...@chromium.org>
    Gerrit-Attention: Garrett Tanzer <gta...@chromium.org>
    Gerrit-Attention: Andrew Verge <ave...@chromium.org>
    Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
    Gerrit-Comment-Date: Thu, 11 Apr 2024 21:47:37 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Garrett Tanzer <gta...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Garrett Tanzer (Gerrit)

    unread,
    Apr 12, 2024, 10:08:00 AMApr 12
    to Liam Brady, Tricium, Andrew Verge, Shivani Sharma, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
    Attention needed from Andrew Verge, Liam Brady and Shivani Sharma

    Garrett Tanzer added 3 comments

    File content/browser/fenced_frame/fenced_frame_config.h
    Line 539, Patchset 13 (Latest): bool HasDisabledUntrustedNetwork() const {
    Garrett Tanzer . unresolved

    I would rather keep the names of these accessors similar to the enums. It's not really clear that "HasDisabledUntrustedNetwork" applies to all descendants.

    `HasDisabledNetworkForCurrentFrameTree`
    `HasDisabledNetworkForCurrentAndDescendantFrameTrees`

    Line 216, Patchset 13 (Latest): kCurrentAndNestedFrameTreesComplete
    Garrett Tanzer . unresolved

    can we say `Descendant` instead of `Nested`, since nested could mean 1 level of nesting only

    Line 215, Patchset 13 (Latest): // Set after all descendant fenced frames have had network cutoff.
    Garrett Tanzer . unresolved

    nit: cutoff -> cut off.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andrew Verge
    • Liam Brady
    • Shivani Sharma
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
      Gerrit-Change-Number: 5426603
      Gerrit-PatchSet: 13
      Gerrit-Owner: Liam Brady <lbr...@google.com>
      Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
      Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
      Gerrit-Reviewer: Liam Brady <lbr...@google.com>
      Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
      Gerrit-CC: Shivani Sharma <shiva...@chromium.org>
      Gerrit-Attention: Andrew Verge <ave...@chromium.org>
      Gerrit-Attention: Liam Brady <lbr...@google.com>
      Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
      Gerrit-Comment-Date: Fri, 12 Apr 2024 14:07:48 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Liam Brady (Gerrit)

      unread,
      Apr 12, 2024, 11:31:15 AMApr 12
      to Tricium, Garrett Tanzer, Andrew Verge, Shivani Sharma, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
      Attention needed from Andrew Verge, Garrett Tanzer and Shivani Sharma

      Liam Brady added 3 comments

      File content/browser/fenced_frame/fenced_frame_config.h
      Line 539, Patchset 13: bool HasDisabledUntrustedNetwork() const {
      Garrett Tanzer . resolved

      I would rather keep the names of these accessors similar to the enums. It's not really clear that "HasDisabledUntrustedNetwork" applies to all descendants.

      `HasDisabledNetworkForCurrentFrameTree`
      `HasDisabledNetworkForCurrentAndDescendantFrameTrees`

      Liam Brady

      Done

      Line 216, Patchset 13: kCurrentAndNestedFrameTreesComplete
      Garrett Tanzer . resolved

      can we say `Descendant` instead of `Nested`, since nested could mean 1 level of nesting only

      Liam Brady

      Done

      Line 215, Patchset 13: // Set after all descendant fenced frames have had network cutoff.
      Garrett Tanzer . resolved

      nit: cutoff -> cut off.

      Liam Brady

      Done

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Andrew Verge
      • Garrett Tanzer
      • Shivani Sharma
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Review
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
      Gerrit-Change-Number: 5426603
      Gerrit-PatchSet: 14
      Gerrit-Owner: Liam Brady <lbr...@google.com>
      Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
      Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
      Gerrit-Reviewer: Liam Brady <lbr...@google.com>
      Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
      Gerrit-CC: Shivani Sharma <shiva...@chromium.org>
      Gerrit-Attention: Garrett Tanzer <gta...@chromium.org>
      Gerrit-Attention: Andrew Verge <ave...@chromium.org>
      Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
      Gerrit-Comment-Date: Fri, 12 Apr 2024 15:31:01 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Garrett Tanzer <gta...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Shivani Sharma (Gerrit)

      unread,
      Apr 12, 2024, 12:04:43 PMApr 12
      to Liam Brady, Tricium, Garrett Tanzer, Andrew Verge, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
      Attention needed from Andrew Verge, Garrett Tanzer and Liam Brady

      Shivani Sharma added 5 comments

      File content/browser/fenced_frame/fenced_frame_config.h
      Line 545, Patchset 14 (Latest): void MarkPartitionNonceRevoked() {
      Shivani Sharma . unresolved

      For consistency, MarkDisabledNetworkForCurrentFrameTree() and MarkDisabledNetworkForCurrentAndDescendantFrameTrees() for the next function.

      Line 535, Patchset 14 (Latest): return disable_untrusted_network_status_ !=
      DisableUntrustedNetworkStatus::kNotStarted;
      Shivani Sharma . unresolved

      disable_untrusted_network_status_ == DisableUntrustedNetworkStatus::kCurrentFrameTreeComplete

      File content/browser/renderer_host/render_frame_host_impl.h
      Line 4124, Patchset 14 (Latest): // revocation can only happen when network access has been disabled for its
      // FencedFrameProperties' partition_nonce_ as well as for all of its
      // descendants. This is calculated for all FrameTreeNodes, including nodes
      Shivani Sharma . unresolved

      Since disabling network is not just nonce-based, let's rephrase this to:
      "// Network revocation can only happen when network access has been disabled for this fenced frame tree as well as for all of its descendant fenced frame trees."

      and then the next sentence can be removed since it's redundant.

      File content/browser/renderer_host/render_frame_host_impl.cc
      Line 9283, Patchset 14 (Latest): // If we are not in a frame tree with a FencedFrameProperties, then network
      // cutoff isn't possible.
      Shivani Sharma . unresolved

      what about urn-iframes. They will have FencedFrameProperties but still network cut-off isn't possible?

      Line 9297, Patchset 14 (Latest): fenced_document_data->RunDisabledUntrustedNetworkCallbacks();
      Shivani Sharma . unresolved

      shouldn't RunDisabledUntrustedNetworkCallbacks only get called when properties->MarkUntrustedNetworkDisabled() is called?

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Andrew Verge
      • Garrett Tanzer
      • Liam Brady
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
        Gerrit-Change-Number: 5426603
        Gerrit-PatchSet: 14
        Gerrit-Owner: Liam Brady <lbr...@google.com>
        Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
        Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
        Gerrit-Reviewer: Liam Brady <lbr...@google.com>
        Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
        Gerrit-Reviewer: Shivani Sharma <shiva...@chromium.org>
        Gerrit-Attention: Garrett Tanzer <gta...@chromium.org>
        Gerrit-Attention: Andrew Verge <ave...@chromium.org>
        Gerrit-Attention: Liam Brady <lbr...@google.com>
        Gerrit-Comment-Date: Fri, 12 Apr 2024 16:04:11 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Andrew Verge (Gerrit)

        unread,
        Apr 12, 2024, 12:26:02 PMApr 12
        to Liam Brady, Shivani Sharma, Tricium, Garrett Tanzer, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
        Attention needed from Garrett Tanzer, Liam Brady and Shivani Sharma

        Andrew Verge added 1 comment

        File content/browser/renderer_host/render_frame_host_impl.cc
        Line 9283, Patchset 14 (Latest): // If we are not in a frame tree with a FencedFrameProperties, then network
        // cutoff isn't possible.
        Shivani Sharma . unresolved

        what about urn-iframes. They will have FencedFrameProperties but still network cut-off isn't possible?

        Andrew Verge

        Could we just skip checking the URN iframe properties, and use `GetFencedFrameProperties(kFrameTreeRoot)` unconditionally?

        • If this traversal hits a fenced frame or any of its descendant iframes of any type, the revocation check will be done for the fenced frame root's properties.
        • If a child frame revokes network on behalf of the fenced frame root, all other frames in the tree will hit the clause on line 9291 and continue until we exit the fenced frame
        • If network is not ready to be revoked, then each parent frame will propagate that bit up via `node_ids_with_network_allowed_descendants`, same as right now.
        Open in Gerrit

        Related details

        Attention is currently required from:
        • Garrett Tanzer
        • Liam Brady
        • Shivani Sharma
        Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
        Gerrit-Change-Number: 5426603
        Gerrit-PatchSet: 14
        Gerrit-Owner: Liam Brady <lbr...@google.com>
        Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
        Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
        Gerrit-Reviewer: Liam Brady <lbr...@google.com>
        Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
        Gerrit-Reviewer: Shivani Sharma <shiva...@chromium.org>
        Gerrit-Attention: Garrett Tanzer <gta...@chromium.org>
        Gerrit-Attention: Liam Brady <lbr...@google.com>
        Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
        Gerrit-Comment-Date: Fri, 12 Apr 2024 16:25:49 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        Comment-In-Reply-To: Shivani Sharma <shiva...@chromium.org>
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Garrett Tanzer (Gerrit)

        unread,
        Apr 12, 2024, 12:28:21 PMApr 12
        to Liam Brady, Shivani Sharma, Tricium, Andrew Verge, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
        Attention needed from Andrew Verge, Liam Brady and Shivani Sharma

        Garrett Tanzer added 1 comment

        File content/browser/renderer_host/render_frame_host_impl.cc
        Line 9283, Patchset 14 (Latest): // If we are not in a frame tree with a FencedFrameProperties, then network
        // cutoff isn't possible.
        Shivani Sharma . unresolved

        what about urn-iframes. They will have FencedFrameProperties but still network cut-off isn't possible?

        Andrew Verge

        Could we just skip checking the URN iframe properties, and use `GetFencedFrameProperties(kFrameTreeRoot)` unconditionally?

        • If this traversal hits a fenced frame or any of its descendant iframes of any type, the revocation check will be done for the fenced frame root's properties.
        • If a child frame revokes network on behalf of the fenced frame root, all other frames in the tree will hit the clause on line 9291 and continue until we exit the fenced frame
        • If network is not ready to be revoked, then each parent frame will propagate that bit up via `node_ids_with_network_allowed_descendants`, same as right now.
        Garrett Tanzer

        Yup, we should use kFrameTreeRoot, good catch

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Andrew Verge
        • Liam Brady
        • Shivani Sharma
        Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
        Gerrit-Change-Number: 5426603
        Gerrit-PatchSet: 14
        Gerrit-Owner: Liam Brady <lbr...@google.com>
        Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
        Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
        Gerrit-Reviewer: Liam Brady <lbr...@google.com>
        Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
        Gerrit-Reviewer: Shivani Sharma <shiva...@chromium.org>
        Gerrit-Attention: Andrew Verge <ave...@chromium.org>
        Gerrit-Attention: Liam Brady <lbr...@google.com>
        Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
        Gerrit-Comment-Date: Fri, 12 Apr 2024 16:28:04 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Liam Brady (Gerrit)

        unread,
        Apr 12, 2024, 12:47:27 PMApr 12
        to Shivani Sharma, Tricium, Garrett Tanzer, Andrew Verge, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
        Attention needed from Andrew Verge, Garrett Tanzer and Shivani Sharma

        Liam Brady added 5 comments

        File content/browser/fenced_frame/fenced_frame_config.h
        Line 545, Patchset 14: void MarkPartitionNonceRevoked() {
        Shivani Sharma . resolved

        For consistency, MarkDisabledNetworkForCurrentFrameTree() and MarkDisabledNetworkForCurrentAndDescendantFrameTrees() for the next function.

        Liam Brady

        Done

        Line 535, Patchset 14: return disable_untrusted_network_status_ !=
        DisableUntrustedNetworkStatus::kNotStarted;
        Shivani Sharma . resolved

        disable_untrusted_network_status_ == DisableUntrustedNetworkStatus::kCurrentFrameTreeComplete

        Liam Brady

        For documentation sake, the reason why this is fine is because `CalculateUntrustedNetworkStatus()` has a check for `HasDisabledNetworkForCurrentAndDescendantFrameTrees` with a `continue;` before we check `HasDisabledNetworkForCurrentFrameTree`. There's no point in this CL where we need to check that either of the 2 states are in place, so it's fine to have `HasDisabledNetworkForCurrentFrameTree` be exclusively checking if the current frame tree is disabled but descendant trees are not.

        File content/browser/renderer_host/render_frame_host_impl.h
        Line 4124, Patchset 14: // revocation can only happen when network access has been disabled for its

        // FencedFrameProperties' partition_nonce_ as well as for all of its
        // descendants. This is calculated for all FrameTreeNodes, including nodes
        Shivani Sharma . resolved

        Since disabling network is not just nonce-based, let's rephrase this to:
        "// Network revocation can only happen when network access has been disabled for this fenced frame tree as well as for all of its descendant fenced frame trees."

        and then the next sentence can be removed since it's redundant.

        Liam Brady

        Done

        File content/browser/renderer_host/render_frame_host_impl.cc
        Line 9283, Patchset 14: // If we are not in a frame tree with a FencedFrameProperties, then network

        // cutoff isn't possible.
        Shivani Sharma . resolved

        what about urn-iframes. They will have FencedFrameProperties but still network cut-off isn't possible?

        Liam Brady

        This is referring to the fact that we don't need to calculate anything since there are no ancestors that could potentially have their network cut off. I'll update the comment to clarify.

        I'll also use `kFrameTreeRoot` for the traversal.

        Line 9297, Patchset 14: fenced_document_data->RunDisabledUntrustedNetworkCallbacks();
        Shivani Sharma . resolved

        shouldn't RunDisabledUntrustedNetworkCallbacks only get called when properties->MarkUntrustedNetworkDisabled() is called?

        Liam Brady

        If you have a fenced frame (A1) that has a child iframe (A2), and both frames call `window.fence.disableUntrustedNetwork()`, the promise for both A1 and A2 need to resolve. Since the promise is saved to the `FencedDocumentData`, and since the A1 and A2 documents will have their own distinct `FencedDocumentData`, we need to call the relevant callbacks for each document associated with the `FencedFrameConfig`.

        This is verified with the last test in `content-shared-storage-get-nested.https.html`.

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Andrew Verge
        • Garrett Tanzer
        • Shivani Sharma
        Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement is not satisfiedCode-Review
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
        Gerrit-Change-Number: 5426603
        Gerrit-PatchSet: 15
        Gerrit-Owner: Liam Brady <lbr...@google.com>
        Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
        Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
        Gerrit-Reviewer: Liam Brady <lbr...@google.com>
        Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
        Gerrit-Reviewer: Shivani Sharma <shiva...@chromium.org>
        Gerrit-Attention: Garrett Tanzer <gta...@chromium.org>
        Gerrit-Attention: Andrew Verge <ave...@chromium.org>
        Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
        Gerrit-Comment-Date: Fri, 12 Apr 2024 16:47:13 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        Comment-In-Reply-To: Shivani Sharma <shiva...@chromium.org>
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Andrew Verge (Gerrit)

        unread,
        Apr 16, 2024, 10:54:19 AMApr 16
        to Liam Brady, Shivani Sharma, Tricium, Garrett Tanzer, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
        Attention needed from Garrett Tanzer, Liam Brady and Shivani Sharma

        Andrew Verge voted and added 1 comment

        Votes added by Andrew Verge

        Code-Review+1

        1 comment

        Patchset-level comments
        File-level comment, Patchset 15 (Latest):
        Andrew Verge . resolved

        Giving my LGTM for WPTs, CalculateUntrustedNetworkStatus, and shared_storage_browsertest, since those were requested of me in the initial review comment and/or offline discussion. Will defer to the other reviewers for the remaining files.

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Garrett Tanzer
        • Liam Brady
        • Shivani Sharma
        Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement satisfiedCode-Review
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
        Gerrit-Change-Number: 5426603
        Gerrit-PatchSet: 15
        Gerrit-Owner: Liam Brady <lbr...@google.com>
        Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
        Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
        Gerrit-Reviewer: Liam Brady <lbr...@google.com>
        Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
        Gerrit-Reviewer: Shivani Sharma <shiva...@chromium.org>
        Gerrit-Attention: Garrett Tanzer <gta...@chromium.org>
        Gerrit-Attention: Liam Brady <lbr...@google.com>
        Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
        Gerrit-Comment-Date: Tue, 16 Apr 2024 14:53:57 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        open
        diffy

        Garrett Tanzer (Gerrit)

        unread,
        Apr 18, 2024, 11:16:03 AMApr 18
        to Liam Brady, Andrew Verge, Shivani Sharma, Tricium, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
        Attention needed from Liam Brady and Shivani Sharma

        Garrett Tanzer added 2 comments

        Patchset-level comments
        Garrett Tanzer . resolved

        will lgtm once the WPT comment is addressed

        File third_party/blink/web_tests/wpt_internal/fenced_frame/content-shared-storage-get.https.html
        Line 123, Patchset 15 (Parent): let disable_network_promise = window.fence.disableUntrustedNetwork();
        Garrett Tanzer . unresolved

        As the TODO says, this test should still exist. It should just use a timeout to check that disableUntrustedNetwork doesn't resolve, then attempt to access shared storage get (which should fail, even though network for the current frame tree is disabled).

        You might also want to do a fetch here and in the nested fenced frame to confirm that network is disabled in the parent but not the child.

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Liam Brady
        • Shivani Sharma
        Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Review
          • requirement is not satisfiedNo-Unresolved-Comments
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
          Gerrit-Change-Number: 5426603
          Gerrit-PatchSet: 15
          Gerrit-Owner: Liam Brady <lbr...@google.com>
          Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
          Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
          Gerrit-Reviewer: Liam Brady <lbr...@google.com>
          Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
          Gerrit-Reviewer: Shivani Sharma <shiva...@chromium.org>
          Gerrit-Attention: Liam Brady <lbr...@google.com>
          Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
          Gerrit-Comment-Date: Thu, 18 Apr 2024 15:15:53 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: No
          satisfied_requirement
          unsatisfied_requirement
          open
          diffy

          Liam Brady (Gerrit)

          unread,
          Apr 18, 2024, 1:17:38 PMApr 18
          to Andrew Verge, Shivani Sharma, Tricium, Garrett Tanzer, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
          Attention needed from Garrett Tanzer and Shivani Sharma

          Liam Brady added 1 comment

          File third_party/blink/web_tests/wpt_internal/fenced_frame/content-shared-storage-get.https.html
          Line 123, Patchset 15 (Parent): let disable_network_promise = window.fence.disableUntrustedNetwork();
          Garrett Tanzer . resolved

          As the TODO says, this test should still exist. It should just use a timeout to check that disableUntrustedNetwork doesn't resolve, then attempt to access shared storage get (which should fail, even though network for the current frame tree is disabled).

          You might also want to do a fetch here and in the nested fenced frame to confirm that network is disabled in the parent but not the child.

          Liam Brady

          Done

          Open in Gerrit

          Related details

          Attention is currently required from:
          • Garrett Tanzer
          • Shivani Sharma
          Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Review
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
          Gerrit-Change-Number: 5426603
          Gerrit-PatchSet: 17
          Gerrit-Owner: Liam Brady <lbr...@google.com>
          Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
          Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
          Gerrit-Reviewer: Liam Brady <lbr...@google.com>
          Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
          Gerrit-Reviewer: Shivani Sharma <shiva...@chromium.org>
          Gerrit-Attention: Garrett Tanzer <gta...@chromium.org>
          Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
          Gerrit-Comment-Date: Thu, 18 Apr 2024 17:17:24 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: No
          Comment-In-Reply-To: Garrett Tanzer <gta...@chromium.org>
          satisfied_requirement
          open
          diffy

          Garrett Tanzer (Gerrit)

          unread,
          Apr 18, 2024, 1:54:40 PMApr 18
          to Liam Brady, Andrew Verge, Shivani Sharma, Tricium, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
          Attention needed from Liam Brady and Shivani Sharma

          Garrett Tanzer voted Code-Review+1

          Code-Review+1
          Open in Gerrit

          Related details

          Attention is currently required from:
          • Liam Brady
          • Shivani Sharma
          Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Review
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
          Gerrit-Change-Number: 5426603
          Gerrit-PatchSet: 17
          Gerrit-Owner: Liam Brady <lbr...@google.com>
          Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
          Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
          Gerrit-Reviewer: Liam Brady <lbr...@google.com>
          Gerrit-Reviewer: Nasko Oskov <na...@chromium.org>
          Gerrit-Reviewer: Shivani Sharma <shiva...@chromium.org>
          Gerrit-Attention: Liam Brady <lbr...@google.com>
          Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
          Gerrit-Comment-Date: Thu, 18 Apr 2024 17:54:30 +0000
          Gerrit-HasComments: No
          Gerrit-Has-Labels: Yes
          satisfied_requirement
          open
          diffy

          Liam Brady (Gerrit)

          unread,
          Apr 18, 2024, 2:08:19 PMApr 18
          to Alex Moshchuk, Garrett Tanzer, Andrew Verge, Shivani Sharma, Tricium, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
          Attention needed from Alex Moshchuk and Shivani Sharma

          Liam Brady added 1 comment

          Patchset-level comments
          File-level comment, Patchset 17 (Latest):
          Liam Brady . resolved

          Removing nasko@ from the reviewers list since he's OOO.

          alexmos@: PTAL at the following files:

          • render_frame_host_impl.h
          • render_frame_host_impl.cc
          • shared_storage_browsertest.cc

          Thanks!

          Open in Gerrit

          Related details

          Attention is currently required from:
          • Alex Moshchuk
          • Shivani Sharma
          Submit Requirements:
          • requirement satisfiedCode-Coverage
          • requirement satisfiedCode-Review
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: chromium/src
          Gerrit-Branch: main
          Gerrit-Change-Id: I88522ea9d3a7d261124fc740681c27537b056cdb
          Gerrit-Change-Number: 5426603
          Gerrit-PatchSet: 17
          Gerrit-Owner: Liam Brady <lbr...@google.com>
          Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
          Gerrit-Reviewer: Andrew Verge <ave...@chromium.org>
          Gerrit-Reviewer: Garrett Tanzer <gta...@chromium.org>
          Gerrit-Reviewer: Liam Brady <lbr...@google.com>
          Gerrit-Reviewer: Shivani Sharma <shiva...@chromium.org>
          Gerrit-Attention: Alex Moshchuk <ale...@chromium.org>
          Gerrit-Attention: Shivani Sharma <shiva...@chromium.org>
          Gerrit-Comment-Date: Thu, 18 Apr 2024 18:08:08 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: No
          satisfied_requirement
          open
          diffy

          Alex Moshchuk (Gerrit)

          unread,
          Apr 18, 2024, 7:29:28 PMApr 18
          to Liam Brady, Garrett Tanzer, Andrew Verge, Shivani Sharma, Tricium, Chromium LUCI CQ, chromium...@chromium.org, alexmo...@chromium.org, blink-...@chromium.org, creis...@chromium.org, navigation...@chromium.org
          Attention needed from Liam Brady and Shivani Sharma

          Alex Moshchuk added 5 comments

          File content/browser/fenced_frame/fenced_document_data.h
          Line 76, Patchset 17 (Latest): // vector to account for the web platform having multiple calls to
          Alex Moshchuk . unresolved

          nit: "supporting"?

          File content/browser/fenced_frame/fenced_frame_config.h
          Line 616, Patchset 17 (Latest): // tree has had its network cutoff via disableUntrustedNetwork().
          Alex Moshchuk . unresolved

          nit: "cut off"

          Line 546, Patchset 17 (Latest): CHECK(can_disable_untrusted_network_);
          Alex Moshchuk . unresolved

          Can we check that disable_untrusted_network_status_ isn't kCurrentAndDescendantFrameTreesComplete at this point, i.e. that we aren't relaxing it by accident?

          File content/browser/renderer_host/render_frame_host_impl.cc
          Line 1091, Patchset 17 (Latest): ->HasDisabledNetworkForCurrentAndDescendantFrameTrees()) {
          Alex Moshchuk . unresolved

          Why wouldn't cases like this use HasDisabledNetworkForCurrentFrameTree(), since on first glance, no new navigations should happen in the current FF once it has disabled network, regardless of descendants, and also the error message only talks about the current fenced frame's network? Is this because for HasDisabledNetworkForCurrentFrameTree(), unpartitioned storage hasn't been granted to the current FF yet (due to descendants still having network), and may never be granted if a descendant FF never disables its network, so you don't want to block navigations out of the current FF forever? Seems worth explaining in a comment somewhere, basically when you would use HasDisabledNetworkForCurrentFrameTree() vs HasDisabledNetworkForCurrentAndDescendantFrameTrees() to gate access to features that require network access. (similarly below)

          Line 9312, Patchset 17 (Latest): properties->MarkDisabledNetworkForCurrentAndDescendantFrameTrees();
          Alex Moshchuk . unresolved

          I'm having trouble following this logic for more complex hierarchies. Consider the following case: A(FF1(B(C)),FF2). So, A embeds a fenced frame, which embeds an iframe B and a nested FF2, and B further embeds C. The list of nodes would be {A,FF1,B,FF2,C} (breadth-first, per