Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2701093002:
PlzNavigate: Cancel ongoing history navigations in the browser when we receive a navigation request… (Closed)
DescriptionPlzNavigate: Cancel ongoing history navigations in the browser when we receive a navigation request from the renderer.
This should fix the http/tests/navigation/back-to-dynamic-iframe.html layout test which
initiates history navigations on the frame and in the process also changes the src tag on the
frame in JS. The expectation is that the JS changes should win. In the PlzNavigate case the history navigations win
because they are marked as browser initiated.
Proposed fix with a lot of help from creis is to cancel ongoing history navigations for child frames when we receive
a navigation request for a frame. Additionally we don't run unload handlers for history navigations for newly created
child frames as they won't have any.
BUG=691168
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2701093002
Cr-Commit-Position: refs/heads/master@{#451507}
Committed: https://chromium.googlesource.com/chromium/src/+/232b610b1a9ae4893764f54a0c952f11e1619686
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address review comments #
Total comments: 2
Patch Set 3 : Move check for is_history_navigation_in_new_child up #
Total comments: 2
Messages
Total messages: 22 (13 generated)
Description was changed from ========== PlzNavigate: Cancel ongoing history navigations in the browser when we receive a navigation request from the renderer. This should fix the http/tests/navigation/back-to-dynamic-iframe.html layout test which initiates history navigations on the frame and in the process also changes the src tag on the frame in JS. The expectation is that the JS changes should win. In the PlzNavigate case the history navigations win because they are marked as browser initiated. Proposed fix with a lot of help from creis is to cancel ongoing history navigations for child frames when we receive a navigation request for a frame. Additionally we don't run unload handlers for history navigations for newly created child frames as they won't have any. BUG=691168 ========== to ========== PlzNavigate: Cancel ongoing history navigations in the browser when we receive a navigation request from the renderer. This should fix the http/tests/navigation/back-to-dynamic-iframe.html layout test which initiates history navigations on the frame and in the process also changes the src tag on the frame in JS. The expectation is that the JS changes should win. In the PlzNavigate case the history navigations win because they are marked as browser initiated. Proposed fix with a lot of help from creis is to cancel ongoing history navigations for child frames when we receive a navigation request for a frame. Additionally we don't run unload handlers for history navigations for newly created child frames as they won't have any. BUG=691168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
ananta@chromium.org changed reviewers: + creis@chromium.org
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
[CC clamy@] Thanks! LGTM with nits. Also, thanks for pointing out that skipping the beforeunload logic increases the chance that the history navigation commits and then the client redirect commits afterward. That's not ideal but probably isn't a huge problem, and I suspect we'd hit that eventually when clamy@ skips beforeunload when there aren't any handlers (for https://crbug.com/365039). If it turns out to be an issue, we can revisit. https://codereview.chromium.org/2701093002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2701093002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:1180: // We don't want to dispatch an unload handler if nit: beforeunload (This is different from an unload handler, which runs later.) https://codereview.chromium.org/2701093002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:1182: // child frame which does not have an unload handler. nit: s/an unload handler/a beforeunload handler/ https://codereview.chromium.org/2701093002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:1186: !is_history_navigation_in_new_child; minor nit: I might put this before the ShouldDispatchBeforeUnload line, just because it seems slightly more related to the is_same_document_history_load case. https://codereview.chromium.org/2701093002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation (right): https://codereview.chromium.org/2701093002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:81: #Bug(none) virtual/mojo-loading/http/tests/navigation/back-to-dynamic-iframe.html [ Failure ] This lines can be removed entirely, right? Or is there a reason to keep them around commented out?
https://codereview.chromium.org/2701093002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2701093002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:1180: // We don't want to dispatch an unload handler if On 2017/02/18 00:05:24, Charlie Reis wrote: > nit: beforeunload > > (This is different from an unload handler, which runs later.) Done. Thanks https://codereview.chromium.org/2701093002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:1182: // child frame which does not have an unload handler. On 2017/02/18 00:05:24, Charlie Reis wrote: > nit: s/an unload handler/a beforeunload handler/ Done. https://codereview.chromium.org/2701093002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:1186: !is_history_navigation_in_new_child; On 2017/02/18 00:05:24, Charlie Reis wrote: > minor nit: I might put this before the ShouldDispatchBeforeUnload line, just > because it seems slightly more related to the is_same_document_history_load > case. Done. https://codereview.chromium.org/2701093002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation (right): https://codereview.chromium.org/2701093002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:81: #Bug(none) virtual/mojo-loading/http/tests/navigation/back-to-dynamic-iframe.html [ Failure ] On 2017/02/18 00:05:24, Charlie Reis wrote: > This lines can be removed entirely, right? Or is there a reason to keep them > around commented out? Yes. I commented them for ensuring the tests run and forgot about it
https://codereview.chromium.org/2701093002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2701093002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:1229: !is_history_navigation_in_new_child) { Oh, sorry! I meant 1 line up from where it was, on 1185 (now 1182).
https://codereview.chromium.org/2701093002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2701093002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:1229: !is_history_navigation_in_new_child) { On 2017/02/18 00:22:52, Charlie Reis wrote: > Oh, sorry! I meant 1 line up from where it was, on 1185 (now 1182). Done.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2701093002/#ps40001 (title: "Move check for is_history_navigation_in_new_child up")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487464766151760,
"parent_rev": "2d744514083dfee8ea2bdde294738e3943a7a02e", "commit_rev":
"232b610b1a9ae4893764f54a0c952f11e1619686"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Cancel ongoing history navigations in the browser when we receive a navigation request from the renderer. This should fix the http/tests/navigation/back-to-dynamic-iframe.html layout test which initiates history navigations on the frame and in the process also changes the src tag on the frame in JS. The expectation is that the JS changes should win. In the PlzNavigate case the history navigations win because they are marked as browser initiated. Proposed fix with a lot of help from creis is to cancel ongoing history navigations for child frames when we receive a navigation request for a frame. Additionally we don't run unload handlers for history navigations for newly created child frames as they won't have any. BUG=691168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Cancel ongoing history navigations in the browser when we receive a navigation request from the renderer. This should fix the http/tests/navigation/back-to-dynamic-iframe.html layout test which initiates history navigations on the frame and in the process also changes the src tag on the frame in JS. The expectation is that the JS changes should win. In the PlzNavigate case the history navigations win because they are marked as browser initiated. Proposed fix with a lot of help from creis is to cancel ongoing history navigations for child frames when we receive a navigation request for a frame. Additionally we don't run unload handlers for history navigations for newly created child frames as they won't have any. BUG=691168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2701093002 Cr-Commit-Position: refs/heads/master@{#451507} Committed: https://chromium.googlesource.com/chromium/src/+/232b610b1a9ae4893764f54a0c95... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/232b610b1a9ae4893764f54a0c95...
Message was sent while issue was closed.
jam@chromium.org changed reviewers: + jam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2701093002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2701093002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:1009: frame_tree_node->ResetNavigationRequest(false); btw I was tracing this code as part of https://codereview.chromium.org/2889703002/, but I don't see it being hit for the layout tests fixed here, or for any other layout or browser test. Is this still needed?
Message was sent while issue was closed.
https://codereview.chromium.org/2701093002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2701093002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:1009: frame_tree_node->ResetNavigationRequest(false); On 2017/05/16 20:13:11, jam wrote: > btw I was tracing this code as part of > https://codereview.chromium.org/2889703002/, but I don't see it being hit for > the layout tests fixed here, or for any other layout or browser test. Is this > still needed? Yep, it's still needed, but some races mean we aren't hitting it in tests. We should add a test for this branch of the race if we can-- I've filed https://crbug.com/724271. See https://codereview.chromium.org/2890613002/#msg15 for details. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
