这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@xiaochen-z
Copy link
Collaborator

@xiaochen-z xiaochen-z commented Feb 25, 2025

Fix #214.

After some investigations, it seems adding the recalculation step in set the ongoing navigation algorithm makes the most sense.

The only similar feature in the current standard is the navigation API, which has two promises. Their resolution or rejection are done when certain navigation events are fired. See:

However, these promises are only relevant for navigations that are intercepted by the navigation API. When a cross-site navigation is not intercepted by the navigation API, no resolution or rejection steps are run on the promises because they will disappear together with the current document, as stated in the explainer (See the last paragraph in the linked section):

And if they are non-intercepted cross-document navigations, then the returned promises, along with the entire JavaScript global environment, will disappear as the current document gets unloaded.

So simply adding the recalculation steps to where navigation API resolves its promises does not work as the cross-document navigation cases are not handled.

In the implementation, the network status recalculation is done whenever a navigation ends. In other words, when a frame changes from having an ongoing navigation, to not having an ongoing navigation. So it appears set the ongoing navigation algorithm is the best place to add this step because it is the centralized place where the ongoing navigation state is changed.

There was a previous PR that briefly touched on how to spec this, see this comment.


Preview | Diff

@xiaochen-z xiaochen-z added the specification Additions to specifications label Feb 25, 2025
@xiaochen-z xiaochen-z marked this pull request as ready for review March 4, 2025 15:52
@xiaochen-z xiaochen-z requested a review from blu25 March 4, 2025 15:53
@xiaochen-z
Copy link
Collaborator Author

+@blu25 for the first scan. Thank you.

Copy link
Collaborator

@blu25 blu25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xiaochen-z xiaochen-z requested a review from domfarolino March 5, 2025 18:41
@xiaochen-z
Copy link
Collaborator Author

  • @domfarolino for review. I've added some findings in the PR description since I last discussed where to spec this with you. Thanks!

@domfarolino
Copy link
Collaborator

In other words, when a frame changes from having an ongoing navigation, to not having an ongoing navigation

What about going from one ongoing navigation to another (overwriting the previous navigation)?

@xiaochen-z
Copy link
Collaborator Author

It depends on whether the first navigation commits. In the implementation, RenderFrameHostImpl::CalculateUntrustedNetworkStatus() is called in NavigationRequest::DidCommitNavigation(). If the first navigation is overwritten by another navigation without being committed, the network status recalculation will not be triggered.

@domfarolino
Copy link
Collaborator

OK so for example, if you start one navigation that hangs for a while, and then part-way through start another navigation that 204s, the network status of the navigating frame never gets recalculated? Can we write a WPT exercising this scenario?

@xiaochen-z
Copy link
Collaborator Author

OK so for example, if you start one navigation that hangs for a while, and then part-way through start another navigation that 204s, the network status of the navigating frame never gets recalculated? Can we write a WPT exercising this scenario?

After a second look of the implementation, the network status recalculation is also done in FrameTreeNode::ResetNavigationRequest().

For the example you described, I believe the above function should be invoked when another navigation starts. But I will need to confirm by adding a WPT.

@xiaochen-z
Copy link
Collaborator Author

Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this LGTM

@domfarolino
Copy link
Collaborator

I'll let @blu25 merge this once he has reviewed the tests and confirms that it matches the spec.

aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 28, 2025
respect to ongoing navigation changes.

Add a WPT that sets up a fenced frame with a nested iframe. The nested
iframe initiates a navigation to a URL that hangs, and after a while it
starts another navigation that receives a 204 response code. The fenced
frame attempts to revoke untrusted network right after the first
navigation starts. The promise returned by `disableUntrustedNetwork()`
will not resolve until the second navigation completes.

This is to verify the recalculation takes place even if the navigation
does not commit (204). As long as ongoing navigation concludes, the
recalculation should be triggered.

Note if the nested frame is a fenced frame, after the navigation
receives 204 response, the promise at parent FF should not resolve.
This is because it is required to have all descendant fenced frames to
revoke network before the parent fenced frame disable untrusted network
promise can resolve. A test is added to:
disable-untrusted-network.https.html

See: WICG/fenced-frame#215 (comment)

Bug: 397377177
Change-Id: Ic317580754366c2f55ff34639da875a8afee7b2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6394457
Commit-Queue: Xiaochen Zhou <xiaochenzh@chromium.org>
Reviewed-by: Liam Brady <lbrady@google.com>
Cr-Commit-Position: refs/heads/main@{#1439477}
aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 28, 2025
…tion with"

This reverts commit 2c55f3b.

Reason for revert: New test was failing on some ci builders, example of a first failure can be seen here: https://ci.chromium.org/ui/p/chromium/builders/ci/mac13-arm64-rel-tests/46223/overview

Original change's description:
> Fenced frames: Add WPT for verifying network status recalculation with
> respect to ongoing navigation changes.
>
> Add a WPT that sets up a fenced frame with a nested iframe. The nested
> iframe initiates a navigation to a URL that hangs, and after a while it
> starts another navigation that receives a 204 response code. The fenced
> frame attempts to revoke untrusted network right after the first
> navigation starts. The promise returned by `disableUntrustedNetwork()`
> will not resolve until the second navigation completes.
>
> This is to verify the recalculation takes place even if the navigation
> does not commit (204). As long as ongoing navigation concludes, the
> recalculation should be triggered.
>
> Note if the nested frame is a fenced frame, after the navigation
> receives 204 response, the promise at parent FF should not resolve.
> This is because it is required to have all descendant fenced frames to
> revoke network before the parent fenced frame disable untrusted network
> promise can resolve. A test is added to:
> disable-untrusted-network.https.html
>
> See: WICG/fenced-frame#215 (comment)
>
> Bug: 397377177
> Change-Id: Ic317580754366c2f55ff34639da875a8afee7b2f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6394457
> Commit-Queue: Xiaochen Zhou <xiaochenzh@chromium.org>
> Reviewed-by: Liam Brady <lbrady@google.com>
> Cr-Commit-Position: refs/heads/main@{#1439477}

Bug: 397377177
Change-Id: I50bcabe2a465b1e6c6bab4c6449a37c9aade3f13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6408932
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Tim <tjudkins@chromium.org>
Owners-Override: Tim Judkins <tjudkins@google.com>
Reviewed-by: Xiaochen Zhou <xiaochenzh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1439553}
aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 31, 2025
…tion with respect to ongoing navigation changes."

Mark long timeout. The tests are failing because in CI tests, the try
bot is using a smaller timeout limit(6000ms). While in CQ, the try bot
uses 12000ms as timeout limit. So it passes the CQ but fails in CI,
which only runs after submission.

However, here dry run with "Include-Ci-Only-Tests:" footer does not set
try bot's timeout limit to 6000ms. It still uses 12000ms. So without
marking timeout as long, test will still pass.

But I think this confirms that the timeout is the issue here.

This is a reland of commit 2c55f3b

Original change's description:
> Fenced frames: Add WPT for verifying network status recalculation with
> respect to ongoing navigation changes.
>
> Add a WPT that sets up a fenced frame with a nested iframe. The nested
> iframe initiates a navigation to a URL that hangs, and after a while it
> starts another navigation that receives a 204 response code. The fenced
> frame attempts to revoke untrusted network right after the first
> navigation starts. The promise returned by `disableUntrustedNetwork()`
> will not resolve until the second navigation completes.
>
> This is to verify the recalculation takes place even if the navigation
> does not commit (204). As long as ongoing navigation concludes, the
> recalculation should be triggered.
>
> Note if the nested frame is a fenced frame, after the navigation
> receives 204 response, the promise at parent FF should not resolve.
> This is because it is required to have all descendant fenced frames to
> revoke network before the parent fenced frame disable untrusted network
> promise can resolve. A test is added to:
> disable-untrusted-network.https.html
>
> See: WICG/fenced-frame#215 (comment)
>
> Bug: 397377177
> Change-Id: Ic317580754366c2f55ff34639da875a8afee7b2f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6394457
> Commit-Queue: Xiaochen Zhou <xiaochenzh@chromium.org>
> Reviewed-by: Liam Brady <lbrady@google.com>
> Cr-Commit-Position: refs/heads/main@{#1439477}

Bug: 397377177
Change-Id: Icf551fd5ac626ebcd08547cd4ba785a0e95fe2cb
Include-Ci-Only-Tests: *|blink_wpt_tests
Cq-Include-Trybots: luci.chromium.try:mac13-arm64-rel,mac14-arm64-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6409013
Reviewed-by: Liam Brady <lbrady@google.com>
Commit-Queue: Liam Brady <lbrady@google.com>
Cr-Commit-Position: refs/heads/main@{#1440321}
@blu25
Copy link
Collaborator

blu25 commented Apr 1, 2025

Merging now that the WPT is in place.

@blu25 blu25 merged commit c628f90 into WICG:master Apr 1, 2025
2 of 3 checks passed
github-actions bot added a commit that referenced this pull request Apr 1, 2025
SHA: c628f90
Reason: push, by blu25

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

specification Additions to specifications

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Spec] Untrusted network status needs to be recalculated every time a navigation ends.

4 participants