-
Notifications
You must be signed in to change notification settings - Fork 36
Recalculate network status when navigation ends #215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Recalculate network status when navigation ends #215
Conversation
|
+@blu25 for the first scan. Thank you. |
blu25
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
What about going from one ongoing navigation to another (overwriting the previous navigation)? |
|
It depends on whether the first navigation commits. In the implementation, |
|
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 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. |
domfarolino
left a comment
There was a problem hiding this 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
|
I'll let @blu25 merge this once he has reviewed the tests and confirms that it matches the spec. |
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}
…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}
…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}
|
Merging now that the WPT is in place. |
SHA: c628f90 Reason: push, by blu25 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fix #214.
After some investigations, it seems adding the recalculation step in
set the ongoing navigationalgorithm 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):
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