Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2475693002:
Do not reset NavigationHandle when navigating same-page (Closed)
|
Created:
4 years, 1 month ago by clamy Modified:
4 years, 1 month ago CC:
chromium-reviews, tyoshino+watch_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, loading-reviews_chromium.org, darin-cc_chromium.org, gavinp+loader_chromium.org, blink-reviews, Nate Chapin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not reset NavigationHandle when navigating same-page
This CL ensures that the NavigationHandle in the RenderFrameHost is not
reset when the frame just navigated same-page. This matches what Blink
is doing, where a same-page navigation will not interrupt a
cross-document one.
BUG=658530
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/3bf35e3c0aed8dbfcdfcbc00e15c650e62838d03
Cr-Commit-Position: refs/heads/master@{#431265}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Addressed comments #
Total comments: 2
Patch Set 3 : Addressed comments + compilation issue #Patch Set 4 : git cl format #Patch Set 5 : Rebase #
Total comments: 19
Patch Set 6 : Rebase #Patch Set 7 : Addressed comments #
Total comments: 10
Patch Set 8 : Rebase #Patch Set 9 : Addressed comments #
Total comments: 1
Patch Set 10 : Rebase + removed DCHECK #Patch Set 11 : Rebase + removed DCHECK #
Total comments: 1
Messages
Total messages: 53 (33 generated)
Description was changed from ========== Do not reset NavigationHandle when navigating same-page This CL ensures that the NavigationHandle in the RenderFrameHost is not reset when the frame just navigated same-page. This matches what Blink is doing, where a same-page navigation will not interrupt a cross-document one. BUG=658530 ========== to ========== Do not reset NavigationHandle when navigating same-page This CL ensures that the NavigationHandle in the RenderFrameHost is not reset when the frame just navigated same-page. This matches what Blink is doing, where a same-page navigation will not interrupt a cross-document one. BUG=658530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by clamy@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...
Description was changed from ========== Do not reset NavigationHandle when navigating same-page This CL ensures that the NavigationHandle in the RenderFrameHost is not reset when the frame just navigated same-page. This matches what Blink is doing, where a same-page navigation will not interrupt a cross-document one. BUG=658530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Do not reset NavigationHandle when navigating same-page This CL ensures that the NavigationHandle in the RenderFrameHost is not reset when the frame just navigated same-page. This matches what Blink is doing, where a same-page navigation will not interrupt a cross-document one. BUG=658530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + creis@chromium.org, japhet@chromium.org, nasko@chromium.org
@nasko, creis, japhet: PTAL
blink LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Ah, subtle. Yes, I think this should work, though it's a bit unfortunate to allow multiple NavigationHandles to exist for the same RFH at the same time. One possible bug with browser-initiated fragment navigations below. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/interstitial_page_navigator_impl.cc (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/interstitial_page_navigator_impl.cc:51: navigation_handle_->DidCommitNavigation(input_params, false, This looks incorrect now. We should assume that same-page navigations are possible on interstitial pages, since other embedders define their own interstitials. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:1096: // host does not have a valid handle. Additionally, coarsely check that: nit: Update comment, since it doesn't matter whether the RFH has a handle anymore. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator.h:75: // The RenderFrameHostImpl has committed a navigation. nit: Mention something about how we shouldn't assume that |navigation_handle| matches RFH's current NavigationHandle, and that DidNavigate is responsible for resetting |navigation_handle| at the end of this method (and shouldn't keep it alive). https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (left): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:672: render_frame_host->SetNavigationHandle(nullptr); Sanity check: This is handled by FindNavigationHandleForCommit now, right? https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (left): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:1208: // base urls. Is there a reason not to preserve this TODO? https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:1185: // load in this renderer yet. Do it now. nit: Add something like: (e.g., for same-page navigations that start in the renderer). https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:3157: // nav_id. nit: pending_nav_entry_id https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:3159: params.url, frame_tree_node_, true, // is_renderer_initiated Hmm, is_renderer_initiated isn't necessarily true for all same-page navigations. You can type "#foo" in the address bar to trigger one. Of course, in that case we would already have a matching NavigationHandle, in which case it's wrong to get inside this block and create another one. Maybe the condition on line 3155 should include a check for whether the current handle matches this? Or rather, move the block from line 3164 to the top of the method? We should include a test for this case. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:3206: // nav_id. If the previous handle was a prematurely aborted navigation loaded nit: pending_nav_entry_id https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:826: std::unique_ptr<NavigationHandleImpl> FindNavigationHandleForCommit( This sounds like an accessor, and it doesn't make it clear that this method resets the RFH's own handle in most cases. Can you clarify? https://codereview.chromium.org/2475693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2475693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:779: if (m_frame->document()->loadEventFinished() && !m_provisionalDocumentLoader) I'm curious-- how is this related to the rest of the CL? I think it might be worth mentioning something in the comment above.
The CQ bit was checked by clamy@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...
Thanks! https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/interstitial_page_navigator_impl.cc (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/interstitial_page_navigator_impl.cc:51: navigation_handle_->DidCommitNavigation(input_params, false, On 2016/11/03 22:02:22, Charlie Reis wrote: > This looks incorrect now. We should assume that same-page navigations are > possible on interstitial pages, since other embedders define their own > interstitials. I'm now checking if the navigation that just committed is not same-page before resetting the NavigationHandle for the interstitial page. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:1096: // host does not have a valid handle. Additionally, coarsely check that: On 2016/11/03 22:02:22, Charlie Reis wrote: > nit: Update comment, since it doesn't matter whether the RFH has a handle > anymore. Done. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator.h:75: // The RenderFrameHostImpl has committed a navigation. On 2016/11/03 22:02:22, Charlie Reis wrote: > nit: Mention something about how we shouldn't assume that |navigation_handle| > matches RFH's current NavigationHandle, and that DidNavigate is responsible for > resetting |navigation_handle| at the end of this method (and shouldn't keep it > alive). Done. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (left): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:672: render_frame_host->SetNavigationHandle(nullptr); On 2016/11/03 22:02:22, Charlie Reis wrote: > Sanity check: This is handled by FindNavigationHandleForCommit now, right? Yes in the cases where we need to reset it. FindNavigationHandleForCommit will transfer ownership of the NavigationHandle to use for commit to this function. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (left): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:1208: // base urls. On 2016/11/03 22:02:22, Charlie Reis wrote: > Is there a reason not to preserve this TODO? Nope, it got lost in code reordering. I brought it back. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:1185: // load in this renderer yet. Do it now. On 2016/11/03 22:02:22, Charlie Reis wrote: > nit: Add something like: (e.g., for same-page navigations that start in the > renderer). Done. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:3157: // nav_id. On 2016/11/03 22:02:22, Charlie Reis wrote: > nit: pending_nav_entry_id Done. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:3159: params.url, frame_tree_node_, true, // is_renderer_initiated On 2016/11/03 22:02:22, Charlie Reis wrote: > Hmm, is_renderer_initiated isn't necessarily true for all same-page navigations. > You can type "#foo" in the address bar to trigger one. > > Of course, in that case we would already have a matching NavigationHandle, in > which case it's wrong to get inside this block and create another one. Maybe > the condition on line 3155 should include a check for whether the current handle > matches this? Or rather, move the block from line 3164 to the top of the > method? > > We should include a test for this case. That's a good point, though in this case we don't have a NavigationHandle, since it is only created when we receive a DidStartProvisionalLoad. However we do have a pending NavigationEntry, so I'm checking against that. We would still miscategorize it if we happened to delete the pending entry in the meantime, though I'm not sure what I can do in that case. I've added a test to check that the NavigationHandle has the right values in this particular case. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:3206: // nav_id. If the previous handle was a prematurely aborted navigation loaded On 2016/11/03 22:02:22, Charlie Reis wrote: > nit: pending_nav_entry_id Done. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:826: std::unique_ptr<NavigationHandleImpl> FindNavigationHandleForCommit( On 2016/11/03 22:02:22, Charlie Reis wrote: > This sounds like an accessor, and it doesn't make it clear that this method > resets the RFH's own handle in most cases. Can you clarify? I've updated the comment and renamed the method. If you have better naming suggestions, please let me know. https://codereview.chromium.org/2475693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2475693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:779: if (m_frame->document()->loadEventFinished() && !m_provisionalDocumentLoader) On 2016/11/03 22:02:22, Charlie Reis wrote: > I'm curious-- how is this related to the rest of the CL? I think it might be > worth mentioning something in the comment above. Without it, we still delete the NavigationHandle of the ongoing navigation when doing the same-page navigation because we will send a DidStopLoading IPC. Sending the IPC is wrong if we're in the middle of another navigation (ie we have a provisional document loader).
clamy@chromium.org changed reviewers: + estark@chromium.org
@estark: PTAL at the changes in chrome/browser/ssl. The issue is that the unit test was actually doing what we classified as a same-page navigation instead of doing a different document navigation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
I have a couple questions, just want to make sure that the code that is being tested in chrome_security_state_model_client_unittest.cc is still doing the right thing: 1. Does this change mean that NavigationHandle::IsSamePage() will return true whenever you navigate to the same page? i.e. will IsSamePage() be true if the user is on http://foo.com and then types http://foo.com in the omnibox and hits enter? Or if JS on http://foo.com does `document.location = http://foo.com`? If so, is there any way to distinguish that kind of navigation from pushState or a fragment change? 2. Not really a question, just wondering if this could probably be related to https://crbug.com/662267. My guess with that bug was that something was going wrong where we aren't caching the certificate correctly on the NavigationHandleImpl. But I haven't had my coffee yet so this may be completely unrelated... https://codereview.chromium.org/2475693002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client_unittest.cc (right): https://codereview.chromium.org/2475693002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:276: void navigate_to_http_2() { NavigateAndCommit(GURL("http://example2.test")); } nit: could you rename this to navigate_to_different_http_page() please?
On 2016/11/04 14:53:59, estark wrote: > I have a couple questions, just want to make sure that the code that is being > tested in chrome_security_state_model_client_unittest.cc is still doing the > right thing: > > 1. Does this change mean that NavigationHandle::IsSamePage() will return true > whenever you navigate to the same page? i.e. will IsSamePage() be true if the > user is on http://foo.com and then types http://foo.com in the omnibox and hits > enter? Or if JS on http://foo.com does `document.location = http://foo.com%60 If > so, is there any way to distinguish that kind of navigation from pushState or a > fragment change? No in fact the issue is that we have some testing code in TestRenderFrameHost that attempts to guess in NavigateAndCommit whether this is a same-page navigation and gets it wrong in this particular case (because we provide the wrong ui::Transition). The things that changed are that when I get a same-page navigation I may associate it with the pending NavigationEntry which was not done before (it would always get a pending entry id of 0) and I also don't delete a NavigationHandle for an ongoing navigation. In that specific case, we would probably have deleted a previously created handle, which may have led to the SSL update even though the NavigationController believed we did a same-page navigation. > > 2. Not really a question, just wondering if this could probably be related to > https://crbug.com/662267. My guess with that bug was that something was going > wrong where we aren't caching the certificate correctly on the > NavigationHandleImpl. But I haven't had my coffee yet so this may be completely > unrelated... I imagine it's possible. IIRC, the SSL info is stored when we get to WillProcessResponse. If we got a same-page navigation while trying to commit the navigation (possible with a push state in the unload handler of the previous page), we would destroy the NavigationHandle, and then re-create one when the navigation commits, which wouldn't have the SSL info. This patch fixes this possible issue. > > https://codereview.chromium.org/2475693002/diff/20001/chrome/browser/ssl/chro... > File chrome/browser/ssl/chrome_security_state_model_client_unittest.cc (right): > > https://codereview.chromium.org/2475693002/diff/20001/chrome/browser/ssl/chro... > chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:276: void > navigate_to_http_2() { NavigateAndCommit(GURL("http://example2.test")); } > nit: could you rename this to navigate_to_different_http_page() please?
Ahh, ok, thanks for the explanations! c/b/ssl lgtm if you rename navigate_to_http_2 to navigate_to_different_http_page On 2016/11/04 15:11:46, clamy (slow) wrote: > On 2016/11/04 14:53:59, estark wrote: > > I have a couple questions, just want to make sure that the code that is being > > tested in chrome_security_state_model_client_unittest.cc is still doing the > > right thing: > > > > 1. Does this change mean that NavigationHandle::IsSamePage() will return true > > whenever you navigate to the same page? i.e. will IsSamePage() be true if the > > user is on http://foo.com and then types http://foo.com in the omnibox and > hits > > enter? Or if JS on http://foo.com does `document.location = http://foo.com%60 > If > > so, is there any way to distinguish that kind of navigation from pushState or > a > > fragment change? > > No in fact the issue is that we have some testing code in TestRenderFrameHost > that attempts to guess in NavigateAndCommit whether this is a same-page > navigation and gets it wrong in this particular case (because we provide the > wrong ui::Transition). The things that changed are that when I get a same-page > navigation I may associate it with the pending NavigationEntry which was not > done before (it would always get a pending entry id of 0) and I also don't > delete a NavigationHandle for an ongoing navigation. In that specific case, we > would probably have deleted a previously created handle, which may have led to > the SSL update even though the NavigationController believed we did a same-page > navigation. > > > > > 2. Not really a question, just wondering if this could probably be related to > > https://crbug.com/662267. My guess with that bug was that something was going > > wrong where we aren't caching the certificate correctly on the > > NavigationHandleImpl. But I haven't had my coffee yet so this may be > completely > > unrelated... > > I imagine it's possible. IIRC, the SSL info is stored when we get to > WillProcessResponse. If we got a same-page navigation while trying to commit the > navigation (possible with a push state in the unload handler of the previous > page), we would destroy the NavigationHandle, and then re-create one when the > navigation commits, which wouldn't have the SSL info. This patch fixes this > possible issue. > > > > > https://codereview.chromium.org/2475693002/diff/20001/chrome/browser/ssl/chro... > > File chrome/browser/ssl/chrome_security_state_model_client_unittest.cc > (right): > > > > > https://codereview.chromium.org/2475693002/diff/20001/chrome/browser/ssl/chro... > > chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:276: void > > navigate_to_http_2() { NavigateAndCommit(GURL("http://example2.test")); } > > nit: could you rename this to navigate_to_different_http_page() please?
The CQ bit was checked by clamy@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 checked by clamy@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...
Thanks! https://codereview.chromium.org/2475693002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client_unittest.cc (right): https://codereview.chromium.org/2475693002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client_unittest.cc:276: void navigate_to_http_2() { NavigateAndCommit(GURL("http://example2.test")); } On 2016/11/04 14:53:59, estark wrote: > nit: could you rename this to navigate_to_different_http_page() please? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by clamy@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...
Thanks! I still have a concern about the interstitial case, but it's generally looking about ready. https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:3159: params.url, frame_tree_node_, true, // is_renderer_initiated On 2016/11/04 14:18:21, clamy (slow) wrote: > On 2016/11/03 22:02:22, Charlie Reis wrote: > > Hmm, is_renderer_initiated isn't necessarily true for all same-page > navigations. > > You can type "#foo" in the address bar to trigger one. > > > > Of course, in that case we would already have a matching NavigationHandle, in > > which case it's wrong to get inside this block and create another one. Maybe > > the condition on line 3155 should include a check for whether the current > handle > > matches this? Or rather, move the block from line 3164 to the top of the > > method? > > > > We should include a test for this case. > > That's a good point, though in this case we don't have a NavigationHandle, since > it is only created when we receive a DidStartProvisionalLoad. However we do have > a pending NavigationEntry, so I'm checking against that. We would still > miscategorize it if we happened to delete the pending entry in the meantime, > though I'm not sure what I can do in that case. Good point. I think missing the canceled-pending-entry case is probably ok. > > I've added a test to check that the NavigationHandle has the right values in > this particular case. Thanks for the test! Why is it disabled in PlzNavigate, though? https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2475693002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:826: std::unique_ptr<NavigationHandleImpl> FindNavigationHandleForCommit( On 2016/11/04 14:18:21, clamy (slow) wrote: > On 2016/11/03 22:02:22, Charlie Reis wrote: > > This sounds like an accessor, and it doesn't make it clear that this method > > resets the RFH's own handle in most cases. Can you clarify? > > I've updated the comment and renamed the method. If you have better naming > suggestions, please let me know. I think that works-- thanks. https://codereview.chromium.org/2475693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2475693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameLoader.cpp:779: if (m_frame->document()->loadEventFinished() && !m_provisionalDocumentLoader) On 2016/11/04 14:18:21, clamy (slow) wrote: > On 2016/11/03 22:02:22, Charlie Reis wrote: > > I'm curious-- how is this related to the rest of the CL? I think it might be > > worth mentioning something in the comment above. > > Without it, we still delete the NavigationHandle of the ongoing navigation when > doing the same-page navigation because we will send a DidStopLoading IPC. > Sending the IPC is wrong if we're in the middle of another navigation (ie we > have a provisional document loader). Acknowledged. https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_navigator_impl.cc (right): https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_navigator_impl.cc:50: if (!navigation_handle->IsSamePage() && navigation_handle_) { Hmm, I think we should look more closely at this. The problem happens when navigation_handle != navigation_handle_, in which case we might be calling DidCommitNavigation on the wrong handle. It seems fragile to assume that's only true if navigation_handle->IsSamePage() returns false. If we are making that assumption, it's safer to do it as a DCHECK or CHECK, with something like: if (navigation_handle == navigation_handle_) { // existing block } else { CHECK(navigation_handle->IsSamePage()); } Additionally, in the navigation_handle->IsSamePage() case, aren't we missing the DidCommitNavigation call on it? https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1095: // to the current navigation handle. Additionally, coarsely check that: nit: s/current/given/ https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6997: // Tests that a same-page browser-initiated navigation is properly rported by nit: reported https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3154: // entry. This is the case for a browser-initiated same-page navigation. nit: Please add: ", which does not cause a NavigationHandle to be created because it does not go through DidStartProvisionalLoad." https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3165: return NavigationHandleImpl::Create( I agree with your reasoning about this case, but it still feels a bit indirect to assume that params.was_within_same_page means this commit cannot ever match navigation_handle_. Can we put in some kind of DCHECK or CHECK to verify this assumption? I imagine it's a bit tricky to phrase, since: 1) There could be either no handle or one for a different navigation. 2) The URL might match (since a pushState can have the same URL as on ongoing different-page navigation). Maybe something like this? // We don't ever expect navigation_handle_ to match, because handles are not created for same-page navigations. DCHECK(!navigation_handle_ || !navigation_handle_->IsSamePage()); https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:812: // Returns ownership the NavigationHandle associated with a navigation that nit: ownership of https://codereview.chromium.org/2475693002/diff/80001/testing/buildbot/filter... File testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter (right): https://codereview.chromium.org/2475693002/diff/80001/testing/buildbot/filter... testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter:9: -NavigationControllerBrowserTest.SamePageBrowserInitiated Why is this disabled for PlzNavigate? (I'm generally ok with fixing for that mode later, but it would be helpful to understand why.) https://codereview.chromium.org/2475693002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2475693002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:801: // document, since a new document is already loading. nit: concurrently navigating away
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by clamy@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...
Thanks! https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_navigator_impl.cc (right): https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_navigator_impl.cc:50: if (!navigation_handle->IsSamePage() && navigation_handle_) { On 2016/11/04 17:30:53, Charlie Reis (OOO til 11-11) wrote: > Hmm, I think we should look more closely at this. > > The problem happens when navigation_handle != navigation_handle_, in which case > we might be calling DidCommitNavigation on the wrong handle. It seems fragile > to assume that's only true if navigation_handle->IsSamePage() returns false. If > we are making that assumption, it's safer to do it as a DCHECK or CHECK, with > something like: > if (navigation_handle == navigation_handle_) { > // existing block > } else { > CHECK(navigation_handle->IsSamePage()); > } > > Additionally, in the navigation_handle->IsSamePage() case, aren't we missing the > DidCommitNavigation call on it? Actually, it is impossible for navigation_handle_ to be navigation_handle. In the case of interstitials, the NavigationHandle is stored in the InterstitialPageNavigatorImpl, so a brand new NavigationHandle would be created in RenderFrameHostImpl ta commit time. I've moved the storage of the NavigationHandle to the RenderFrameHost, so this should solve the issue of having two NavigationHandles in that case. https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1095: // to the current navigation handle. Additionally, coarsely check that: On 2016/11/04 17:30:53, Charlie Reis (OOO til 11-11) wrote: > nit: s/current/given/ Done. https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6997: // Tests that a same-page browser-initiated navigation is properly rported by On 2016/11/04 17:30:53, Charlie Reis (OOO til 11-11) wrote: > nit: reported Done. https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3154: // entry. This is the case for a browser-initiated same-page navigation. On 2016/11/04 17:30:53, Charlie Reis (OOO til 11-11) wrote: > nit: Please add: ", which does not cause a NavigationHandle to be created > because it does not go through DidStartProvisionalLoad." Done. https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3165: return NavigationHandleImpl::Create( On 2016/11/04 17:30:53, Charlie Reis (OOO til 11-11) wrote: > I agree with your reasoning about this case, but it still feels a bit indirect > to assume that params.was_within_same_page means this commit cannot ever match > navigation_handle_. Can we put in some kind of DCHECK or CHECK to verify this > assumption? > > I imagine it's a bit tricky to phrase, since: > 1) There could be either no handle or one for a different navigation. > 2) The URL might match (since a pushState can have the same URL as on ongoing > different-page navigation). > > Maybe something like this? > > // We don't ever expect navigation_handle_ to match, because handles are not > created for same-page navigations. > DCHECK(!navigation_handle_ || !navigation_handle_->IsSamePage()); Done. I didn't add the url check because of the PushState issue (the repro case for the bug is doing a PushState of the url they're navigating to). https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:812: // Returns ownership the NavigationHandle associated with a navigation that On 2016/11/04 17:30:53, Charlie Reis (OOO til 11-11) wrote: > nit: ownership of Done. https://codereview.chromium.org/2475693002/diff/80001/testing/buildbot/filter... File testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter (right): https://codereview.chromium.org/2475693002/diff/80001/testing/buildbot/filter... testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter:9: -NavigationControllerBrowserTest.SamePageBrowserInitiated On 2016/11/04 17:30:53, Charlie Reis (OOO til 11-11) wrote: > Why is this disabled for PlzNavigate? (I'm generally ok with fixing for that > mode later, but it would be helpful to understand why.) Because we don't classify browser-initiated same-page navigations as same-page navigations in PlzNavigate, and we send them to the network. So the NavigationHandle has IsSamePage set to false. https://codereview.chromium.org/2475693002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2475693002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameLoader.cpp:801: // document, since a new document is already loading. On 2016/11/04 17:30:53, Charlie Reis (OOO til 11-11) wrote: > nit: concurrently navigating away Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great-- LGTM. https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... File content/browser/frame_host/interstitial_page_navigator_impl.cc (right): https://codereview.chromium.org/2475693002/diff/80001/content/browser/frame_h... content/browser/frame_host/interstitial_page_navigator_impl.cc:50: if (!navigation_handle->IsSamePage() && navigation_handle_) { On 2016/11/07 14:10:51, clamy wrote: > On 2016/11/04 17:30:53, Charlie Reis (OOO til 11-11) wrote: > > Hmm, I think we should look more closely at this. > > > > The problem happens when navigation_handle != navigation_handle_, in which > case > > we might be calling DidCommitNavigation on the wrong handle. It seems fragile > > to assume that's only true if navigation_handle->IsSamePage() returns false. > If > > we are making that assumption, it's safer to do it as a DCHECK or CHECK, with > > something like: > > if (navigation_handle == navigation_handle_) { > > // existing block > > } else { > > CHECK(navigation_handle->IsSamePage()); > > } > > > > Additionally, in the navigation_handle->IsSamePage() case, aren't we missing > the > > DidCommitNavigation call on it? > > Actually, it is impossible for navigation_handle_ to be navigation_handle. In > the case of interstitials, the NavigationHandle is stored in the > InterstitialPageNavigatorImpl, so a brand new NavigationHandle would be created > in RenderFrameHostImpl ta commit time. > > I've moved the storage of the NavigationHandle to the RenderFrameHost, so this > should solve the issue of having two NavigationHandles in that case. Much better-- thanks! https://codereview.chromium.org/2475693002/diff/80001/testing/buildbot/filter... File testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter (right): https://codereview.chromium.org/2475693002/diff/80001/testing/buildbot/filter... testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter:9: -NavigationControllerBrowserTest.SamePageBrowserInitiated On 2016/11/07 14:10:51, clamy wrote: > On 2016/11/04 17:30:53, Charlie Reis (OOO til 11-11) wrote: > > Why is this disabled for PlzNavigate? (I'm generally ok with fixing for that > > mode later, but it would be helpful to understand why.) > > Because we don't classify browser-initiated same-page navigations as same-page > navigations in PlzNavigate, and we send them to the network. So the > NavigationHandle has IsSamePage set to false. Ah, thanks. Is there a bug on file to fix that?
LGTM with some nits. Overall I like the approach of explicit navigation_handle passed to the Navigator. https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6941: EXPECT_TRUE(main_frame->navigation_handle()); Wouldn't this be true even in PlzNavigate mode? https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6946: "var stateObj = {}; history.pushState(stateObj, \"title 1\", \"" + nit: No need to declare stateObj, just pass {} as the first param to pushState. https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6958: EXPECT_TRUE(main_frame->navigation_handle()); Why not capture the NavigationHandle before the pushState and ensure it is the exact same object here? https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6969: class NavigationHandleCommitObserver : public WebContentsObserver { It might be useful to use this object in the test above to confirm that the commits for both navigation handles - the same page one and the delayed navigation one are properly reporting the expected values. https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1208: // the renderer). Do it now. DCHECK that was_within_same_page is true here? There should be no other way to have null navigation_handle_, right?
Thanks! https://codereview.chromium.org/2475693002/diff/80001/testing/buildbot/filter... File testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter (right): https://codereview.chromium.org/2475693002/diff/80001/testing/buildbot/filter... testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter:9: -NavigationControllerBrowserTest.SamePageBrowserInitiated On 2016/11/08 06:38:53, Charlie Reis (OOO til 11-11) wrote: > On 2016/11/07 14:10:51, clamy wrote: > > On 2016/11/04 17:30:53, Charlie Reis (OOO til 11-11) wrote: > > > Why is this disabled for PlzNavigate? (I'm generally ok with fixing for > that > > > mode later, but it would be helpful to understand why.) > > > > Because we don't classify browser-initiated same-page navigations as same-page > > navigations in PlzNavigate, and we send them to the network. So the > > NavigationHandle has IsSamePage set to false. > > Ah, thanks. Is there a bug on file to fix that? I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=663777. https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6941: EXPECT_TRUE(main_frame->navigation_handle()); On 2016/11/09 01:30:22, nasko wrote: > Wouldn't this be true even in PlzNavigate mode? No because the NavigationHandle is owned by the NavigationRequest at this point. https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6946: "var stateObj = {}; history.pushState(stateObj, \"title 1\", \"" + On 2016/11/09 01:30:22, nasko wrote: > nit: No need to declare stateObj, just pass {} as the first param to pushState. Done. https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6958: EXPECT_TRUE(main_frame->navigation_handle()); On 2016/11/09 01:30:22, nasko wrote: > Why not capture the NavigationHandle before the pushState and ensure it is the > exact same object here? Done. https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6969: class NavigationHandleCommitObserver : public WebContentsObserver { On 2016/11/09 01:30:22, nasko wrote: > It might be useful to use this object in the test above to confirm that the > commits for both navigation handles - the same page one and the delayed > navigation one are properly reporting the expected values. Done. https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2475693002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1208: // the renderer). Do it now. On 2016/11/09 01:30:22, nasko wrote: > DCHECK that was_within_same_page is true here? There should be no other way to > have null navigation_handle_, right? Done. https://codereview.chromium.org/2475693002/diff/160001/content/browser/loader... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/2475693002/diff/160001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:86: SendCheckResultToIOThread(callback, NavigationThrottle::CANCEL); Note that this undoes https://codereview.chromium.org/2481173002/ now that we shouldn't delete the NavigationHandle improperly.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org, estark@chromium.org, creis@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2475693002/#ps160001 (title: "Addressed comments")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2475693002/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2475693002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1213: // the renderer). Do it now. I had to remove the DCHECK(validated_params.was_within_same_page) since it's possible to get here in other cases: about://blank nav & dom serializer. Since I couldn't properly express the DCHECK for the later case I'm removing it. We can try to reintroduce it in a later patch.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, japhet@chromium.org, nasko@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2475693002/#ps200001 (title: "Rebase + removed DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Do not reset NavigationHandle when navigating same-page This CL ensures that the NavigationHandle in the RenderFrameHost is not reset when the frame just navigated same-page. This matches what Blink is doing, where a same-page navigation will not interrupt a cross-document one. BUG=658530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Do not reset NavigationHandle when navigating same-page This CL ensures that the NavigationHandle in the RenderFrameHost is not reset when the frame just navigated same-page. This matches what Blink is doing, where a same-page navigation will not interrupt a cross-document one. BUG=658530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Do not reset NavigationHandle when navigating same-page This CL ensures that the NavigationHandle in the RenderFrameHost is not reset when the frame just navigated same-page. This matches what Blink is doing, where a same-page navigation will not interrupt a cross-document one. BUG=658530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Do not reset NavigationHandle when navigating same-page This CL ensures that the NavigationHandle in the RenderFrameHost is not reset when the frame just navigated same-page. This matches what Blink is doing, where a same-page navigation will not interrupt a cross-document one. BUG=658530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/3bf35e3c0aed8dbfcdfcbc00e15c650e62838d03 Cr-Commit-Position: refs/heads/master@{#431265} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/3bf35e3c0aed8dbfcdfcbc00e15c650e62838d03 Cr-Commit-Position: refs/heads/master@{#431265} |
