Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694013005:
Cleanup blink-side PlzNavigate logic (Closed)
|
Created:
3 years, 10 months ago by Nate Chapin Modified:
3 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, dglazkov+blink, darin-cc_chromium.org, blink-reviews, kinuko+watch, blink-reviews-api_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCleanup blink-side PlzNavigate logic
Instead of tracking the existence of a PlzNavigate navivation via
FrameLoader::m_isNavigationHandledByClient, track it by creating a
provisional DocumentLoader and doing ~all of the initiatlization steps
except actually calling startLoadingMainResource(). PlzNavigate is
emulating most of these steps anyway. This way we can do the normal
checks for a provisional navigation in the exact same way for both
PlzNavigate and non-PlzNavigate loads. Where PlzNavigate needs special
handling, checking DocumentLoader::didStart() will indicate whether
the real navigation actually began.
Reworked a couple layout tests to have more reliable timings to make
this work.
BUG=
Review-Url: https://codereview.chromium.org/2694013005
Cr-Commit-Position: refs/heads/master@{#454973}
Committed: https://chromium.googlesource.com/chromium/src/+/476af767372c72fd5e71f365e78481af9ce9a1d9
Patch Set 1 #Patch Set 2 : Cleanup blink-side PlzNavigate logic #Patch Set 3 : Test #Patch Set 4 : Test #Patch Set 5 : Fix a test #Patch Set 6 : More fixes #Patch Set 7 : Cleanup blink-side PlzNavigate logic #Patch Set 8 : Cleanup blink-side PlzNavigate logic #Patch Set 9 : Cleanup blink-side PlzNavigate logic #Patch Set 10 : Cleanup blink-side PlzNavigate logic #Patch Set 11 : Cleanup blink-side PlzNavigate logic #Patch Set 12 : Move most of the placeholder handling to startLoad() #Patch Set 13 : Move most of the placeholder handling to startLoad() #
Total comments: 12
Patch Set 14 : merge #
Total comments: 10
Patch Set 15 : Address ananta's comments #
Total comments: 15
Patch Set 16 : Address clamy's comments #Patch Set 17 : Rebase #Messages
Total messages: 75 (57 generated)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
The CQ bit was unchecked by japhet@chromium.org
The CQ bit was checked by japhet@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by japhet@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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 japhet@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by japhet@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by japhet@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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 japhet@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by japhet@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: This issue passed the CQ dry run.
The CQ bit was checked by japhet@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by japhet@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: This issue passed the CQ dry run.
Description was changed from ========== Cleanup blink-side PlzNavigate logic BUG= ========== to ========== Cleanup blink-side PlzNavigate logic Instead of tracking the existence of a PlzNavigate navivation via FrameLoader::m_isNavigationHandledByClient, track it by creating a provisional DocumentLoader and doing ~all of the initiatlization steps except actually calling startLoadingMainResource(). PlzNavigate is emulating most of these steps anyway. This way we can do the normal checks for a provisional navigation in the exact same way for both PlzNavigate and non-PlzNavigate loads. Where PlzNavigate needs special handling, checking DocumentLoader::didStart() will indicate whether the real navigation actually began. BUG= ==========
Description was changed from ========== Cleanup blink-side PlzNavigate logic Instead of tracking the existence of a PlzNavigate navivation via FrameLoader::m_isNavigationHandledByClient, track it by creating a provisional DocumentLoader and doing ~all of the initiatlization steps except actually calling startLoadingMainResource(). PlzNavigate is emulating most of these steps anyway. This way we can do the normal checks for a provisional navigation in the exact same way for both PlzNavigate and non-PlzNavigate loads. Where PlzNavigate needs special handling, checking DocumentLoader::didStart() will indicate whether the real navigation actually began. BUG= ========== to ========== Cleanup blink-side PlzNavigate logic Instead of tracking the existence of a PlzNavigate navivation via FrameLoader::m_isNavigationHandledByClient, track it by creating a provisional DocumentLoader and doing ~all of the initiatlization steps except actually calling startLoadingMainResource(). PlzNavigate is emulating most of these steps anyway. This way we can do the normal checks for a provisional navigation in the exact same way for both PlzNavigate and non-PlzNavigate loads. Where PlzNavigate needs special handling, checking DocumentLoader::didStart() will indicate whether the real navigation actually began. Reworked a couple layout tests to have more reliable timings to make this work. BUG= ==========
japhet@chromium.org changed reviewers: + ananta@chromium.org, clamy@chromium.org
clamy, ananta: how does this look? I think it gets the blink-side navigation logic a lot more similar for PlzNavigate and non-PlzNavigate https://codereview.chromium.org/2694013005/diff/240001/content/renderer/rende... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/2694013005/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:3309: // With PlzNavigate-enabled, this will be called before a DataSource has been willSubmitForm() is always called with a live provisionalDataSource() now. https://codereview.chromium.org/2694013005/diff/240001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2694013005/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:5220: if (request_params.nav_entry_id != 0 || !had_provisional_data_source) { This block guarantees that LoadNavigationErrorPage() is called exactly once, whether directly here, or via didFailProvisionalLoad(). https://codereview.chromium.org/2694013005/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:5309: return blink::WebNavigationPolicyHandledByClientForInitialHistory; The plumbing for this case and the behavior it needs from blink is sufficiently different from PlzNavigate that I split the enum values. I'd like to merge them once I understand this path better (see TODO in FrameLoader). https://codereview.chromium.org/2694013005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left): https://codereview.chromium.org/2694013005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1244: if (m_isNavigationHandledByClient) { Because we know have a dummy DocumentLoader for the client-handled navigation, simply detaching m_provisionalDoucmentLoader will accomplish this. https://codereview.chromium.org/2694013005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1384: if (!m_isNavigationHandledByClient) I'm not sure this was right. If we committed a load, any and all navigations, whether scheduled or actually started, should be preempted. https://codereview.chromium.org/2694013005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1676: if (policy == NavigationPolicyHandledByClient) { All of this case should be handling in the main startLoad() path. https://codereview.chromium.org/2694013005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1725: m_frame->document()->cancelParsing(); Here and below moved in to startLoad(), since they need some special handling if a PlzNavigate load was in progress, and that way we can keep the PlzNavigate-specific logic in fewer functions. https://codereview.chromium.org/2694013005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1779: client()->dispatchDidStartProvisionalLoad(loader); All of this is handled in the main path below, though we obviously leave the DocumentLoader attached as m_provisionalDocumentLoader now. https://codereview.chromium.org/2694013005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1805: if (!m_isNavigationHandledByClient) Remove the need for this if() by teaching progressStarted to filter out repeated calls. https://codereview.chromium.org/2694013005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2694013005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1715: bool hadPlaceholderClientDocumentLoader = I don't like this name. Suggestions welcome if you have something better.
Looks good. https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.h:140: // actually handled in the renderer. Please rephrase to navigation is actually? https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1728: // For PlzNavigate placeholer DocumentLoaders, don't send failure callbacks typo placeholder https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2050: frameloader.loadFailed(frameloader.provisionalDocumentLoader(), error); Is provisionalDocumentLoader() guaranteed to be not null here?. Perhaps a DCHECK?
https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1731: m_provisionalDocumentLoader->setSentDidFinishLoad(); Where do we call client()->dispatchDidStartProvisionalLoad(loader) for PlzNavigate?. We had this to get some layout tests to pass as they expected willStartRequest to happen after didStartProvisionalLoad()
https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.h:140: // actually handled in the renderer. On 2017/03/01 23:13:17, ananta wrote: > Please rephrase to navigation is actually? Done. https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1728: // For PlzNavigate placeholer DocumentLoaders, don't send failure callbacks On 2017/03/01 23:13:17, ananta wrote: > typo placeholder Done. https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1731: m_provisionalDocumentLoader->setSentDidFinishLoad(); On 2017/03/01 23:15:23, ananta wrote: > Where do we call client()->dispatchDidStartProvisionalLoad(loader) for > PlzNavigate?. We had this to get some layout tests to pass as they expected > willStartRequest to happen after didStartProvisionalLoad() With this patch, we'll call it in the same spot as for non-PlzNavigate, in FrameLoader::startLoad(), see comment below. https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1768: client()->dispatchDidStartProvisionalLoad(m_provisionalDocumentLoader); As mentioned above, this will be called for both PlzNavigate and legacy navigation codepaths. https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1771: if (navigationPolicy == NavigationPolicyCurrentTab) { This if() clause is where the PlzNavigate and non-PlzNavigate paths diverge. non-PlzNavigate will go through the if{} clause and call startLoadingMainResource(), where PlzNavigate will go through the else{} and just note that a navigation was scheduled. https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2694013005/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2050: frameloader.loadFailed(frameloader.provisionalDocumentLoader(), error); On 2017/03/01 23:13:17, ananta wrote: > Is provisionalDocumentLoader() guaranteed to be not null here?. Perhaps a > DCHECK? Done.
The CQ bit was checked by japhet@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: This issue passed the CQ dry run.
Thanks for the cleanup, this makes the code easier to read. A few questions below. Also one general question: how does this fit with the goal of killing the provisional DocumentLoader post PlzNavigate? I imagine we could replace the dummy DocumentLoader with a simplified class that just holds info and does not actually do anything and still have it be a WebDataSource, or make it so that eventually code interested in navigation does not depend on a DocumentLoader for the beginning of navigation. https://codereview.chromium.org/2694013005/diff/240001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2694013005/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:5309: return blink::WebNavigationPolicyHandledByClientForInitialHistory; On 2017/02/24 23:42:05, Nate Chapin wrote: > The plumbing for this case and the behavior it needs from blink is sufficiently > different from PlzNavigate that I split the enum values. I'd like to merge them > once I understand this path better (see TODO in FrameLoader). Acknowledged. https://codereview.chromium.org/2694013005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left): https://codereview.chromium.org/2694013005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1244: if (m_isNavigationHandledByClient) { On 2017/02/24 23:42:06, Nate Chapin wrote: > Because we know have a dummy DocumentLoader for the client-handled navigation, > simply detaching m_provisionalDoucmentLoader will accomplish this. Acknowledged. https://codereview.chromium.org/2694013005/diff/280001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2694013005/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:5225: // If we didn't call didFailProvisionalLoad or there wasn't a nit: empty line above. https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.h:137: // Without PlzNavigate, this is only true for a narrow window during Did you mean "Without PlzNavigate, this is only false..."? https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1736: // which can detach this frame. How likely is it that detaching the DocumentLoader here will result in the frame being deleted? This now happens just after we sent the navigation request to the browser while it didn't happen before. IIUC, this also mean that if we're trying to commit a navigation in this frame, it will get cancelled by a new renderer-initiated navigation when previously it wouldn't right? Though this closer to the non-PlzNavigate behavior I imagine. https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1740: m_progressTracker->progressStarted(type); Can we now call this all the time? We had an issue with one of the DevTools layout tests where we would end up signalling the start of teh load twice, which was unexpected. https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.h (right): https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.h:205: NavigationPolicy shouldContinueForNavigationPolicy( Maybe add a comment about which NavigationPolicy should prompt us to continue? https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.h:217: // provisional navigations. nit: I think the comment should be updated. Maybe something like: // PlzNavigate // Note: navigations handled by the client have created a dummy provisional DocumentLoader, so this will return true while the client handles the navigation.
The CQ bit was checked by japhet@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...
On 2017/03/02 14:37:18, clamy wrote: > Also one general question: how does this fit with the goal of killing the > provisional DocumentLoader post PlzNavigate? I imagine we could replace the > dummy DocumentLoader with a simplified class that just holds info and does not > actually do anything and still have it be a WebDataSource, or make it so that > eventually code interested in navigation does not depend on a DocumentLoader for > the beginning of navigation. I think there will be a lot of cleanup opportunities when it's time to kill the provisional DocumentLoader, but I don't have a preference yet. Options include: * A stripped down ProvisionalDocumentLoader class that can then be converted to a DocumentLoader * Keep the dummy DocumentLoader, but teach it to transition to a real DocumentLoader (depending on whether any of its state would be reusuable). * Drop m_provisionalDocumentLoader, go back to the m_isNavigationHandledByClient bit, and just refactor the navigation start logic to not assume existence of a DocumentLoader.
https://codereview.chromium.org/2694013005/diff/280001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2694013005/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:5225: // If we didn't call didFailProvisionalLoad or there wasn't a On 2017/03/02 14:37:18, clamy wrote: > nit: empty line above. Done. https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.h:137: // Without PlzNavigate, this is only true for a narrow window during On 2017/03/02 14:37:18, clamy wrote: > Did you mean "Without PlzNavigate, this is only false..."? Yep. Booleans are hard. https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1736: // which can detach this frame. On 2017/03/02 14:37:18, clamy wrote: > How likely is it that detaching the DocumentLoader here will result in the frame > being deleted? This now happens just after we sent the navigation request to the > browser while it didn't happen before. In my experience, the primary user of this feature is clusterfuzz :) > > IIUC, this also mean that if we're trying to commit a navigation in this frame, > it will get cancelled by a new renderer-initiated navigation when previously it > wouldn't right? Though this closer to the non-PlzNavigate behavior I imagine. I think the detachDocumentLoader() call can't run script in the PlzNavigate-placeholder case, because of the setSentDidFinishLoad() call above. Going through DocumentLoader::detachFromFrame(), stopFetching() will be a noop in a placeholder, loadFailed() won't be called because sentDidFinishLoad() is set to true above, and everything else should never trigger JS. https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1740: m_progressTracker->progressStarted(type); On 2017/03/02 14:37:18, clamy wrote: > Can we now call this all the time? We had an issue with one of the DevTools > layout tests where we would end up signalling the start of teh load twice, which > was unexpected. Yes, see change to ProgressTracker.cpp. Alternately, I could just defend this call with if (!m_frame->isLoading()). https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.h (right): https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.h:205: NavigationPolicy shouldContinueForNavigationPolicy( On 2017/03/02 14:37:18, clamy wrote: > Maybe add a comment about which NavigationPolicy should prompt us to continue? Done. https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.h:217: // provisional navigations. On 2017/03/02 14:37:18, clamy wrote: > nit: I think the comment should be updated. Maybe something like: > // PlzNavigate > // Note: navigations handled by the client have created a dummy provisional > DocumentLoader, so this will return true while the client handles the > navigation. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Just one clarification needed and this should be good for me. https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1736: // which can detach this frame. On 2017/03/02 19:39:18, Nate Chapin wrote: > On 2017/03/02 14:37:18, clamy wrote: > > How likely is it that detaching the DocumentLoader here will result in the > frame > > being deleted? This now happens just after we sent the navigation request to > the > > browser while it didn't happen before. > > In my experience, the primary user of this feature is clusterfuzz :) > > > > > IIUC, this also mean that if we're trying to commit a navigation in this > frame, > > it will get cancelled by a new renderer-initiated navigation when previously > it > > wouldn't right? Though this closer to the non-PlzNavigate behavior I imagine. > > I think the detachDocumentLoader() call can't run script in the > PlzNavigate-placeholder case, because of the setSentDidFinishLoad() call above. > Going through DocumentLoader::detachFromFrame(), stopFetching() will be a noop > in a placeholder, loadFailed() won't be called because sentDidFinishLoad() is > set to true above, and everything else should never trigger JS. My question here was about the brief interval of time where we have a non-placeholder provisional DocumentLoader when trying to commit the navigation. I think it will get cancelled if we start a new renderer-initiated navigation, as we'll try to create a placeholder provisional DocumentLoader and detach the non-placeholder one we just had. Is that correct? https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1740: m_progressTracker->progressStarted(type); On 2017/03/02 19:39:18, Nate Chapin wrote: > On 2017/03/02 14:37:18, clamy wrote: > > Can we now call this all the time? We had an issue with one of the DevTools > > layout tests where we would end up signalling the start of teh load twice, > which > > was unexpected. > > Yes, see change to ProgressTracker.cpp. Alternately, I could just defend this > call with if (!m_frame->isLoading()). Acknowledged.
https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameLoader.cpp:1736: // which can detach this frame. On 2017/03/03 13:45:34, clamy wrote: > On 2017/03/02 19:39:18, Nate Chapin wrote: > > On 2017/03/02 14:37:18, clamy wrote: > > > How likely is it that detaching the DocumentLoader here will result in the > > frame > > > being deleted? This now happens just after we sent the navigation request to > > the > > > browser while it didn't happen before. > > > > In my experience, the primary user of this feature is clusterfuzz :) > > > > > > > > IIUC, this also mean that if we're trying to commit a navigation in this > > frame, > > > it will get cancelled by a new renderer-initiated navigation when previously > > it > > > wouldn't right? Though this closer to the non-PlzNavigate behavior I > imagine. > > > > I think the detachDocumentLoader() call can't run script in the > > PlzNavigate-placeholder case, because of the setSentDidFinishLoad() call > above. > > Going through DocumentLoader::detachFromFrame(), stopFetching() will be a noop > > in a placeholder, loadFailed() won't be called because sentDidFinishLoad() is > > set to true above, and everything else should never trigger JS. > > My question here was about the brief interval of time where we have a > non-placeholder provisional DocumentLoader when trying to commit the navigation. > I think it will get cancelled if we start a new renderer-initiated navigation, > as we'll try to create a placeholder provisional DocumentLoader and detach the > non-placeholder one we just had. Is that correct? I believe that is correct. Though the possibly of a new renderer-initiated navigation cancelling an in-process PlzNavigate commit is more tied to the if() clause around m_frame->navigationScheduler().cancel(); below. That clause causes a placeholder->commit PlzNavigate transition to not cancel scheduled navigations, but anything else will (in effect, treating such a transition as a continuation of a navigation, rather than a new provisional navigation). external/wpt/html/browsers/windows/nested-browsing-contexts/frameElement.html requires this behavior.
On 2017/03/03 20:19:14, Nate Chapin wrote: > https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right): > > https://codereview.chromium.org/2694013005/diff/280001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/FrameLoader.cpp:1736: // which can detach > this frame. > On 2017/03/03 13:45:34, clamy wrote: > > On 2017/03/02 19:39:18, Nate Chapin wrote: > > > On 2017/03/02 14:37:18, clamy wrote: > > > > How likely is it that detaching the DocumentLoader here will result in the > > > frame > > > > being deleted? This now happens just after we sent the navigation request > to > > > the > > > > browser while it didn't happen before. > > > > > > In my experience, the primary user of this feature is clusterfuzz :) > > > > > > > > > > > IIUC, this also mean that if we're trying to commit a navigation in this > > > frame, > > > > it will get cancelled by a new renderer-initiated navigation when > previously > > > it > > > > wouldn't right? Though this closer to the non-PlzNavigate behavior I > > imagine. > > > > > > I think the detachDocumentLoader() call can't run script in the > > > PlzNavigate-placeholder case, because of the setSentDidFinishLoad() call > > above. > > > Going through DocumentLoader::detachFromFrame(), stopFetching() will be a > noop > > > in a placeholder, loadFailed() won't be called because sentDidFinishLoad() > is > > > set to true above, and everything else should never trigger JS. > > > > My question here was about the brief interval of time where we have a > > non-placeholder provisional DocumentLoader when trying to commit the > navigation. > > I think it will get cancelled if we start a new renderer-initiated navigation, > > as we'll try to create a placeholder provisional DocumentLoader and detach the > > non-placeholder one we just had. Is that correct? > > I believe that is correct. Though the possibly of a new renderer-initiated > navigation cancelling an in-process PlzNavigate commit is more tied to the if() > clause around m_frame->navigationScheduler().cancel(); below. That clause causes > a placeholder->commit PlzNavigate transition to not cancel scheduled > navigations, but anything else will (in effect, treating such a transition as a > continuation of a navigation, rather than a new provisional navigation). > external/wpt/html/browsers/windows/nested-browsing-contexts/frameElement.html > requires this behavior. Thanks! Just wanted to check. Otherwise Lgtm.
The CQ bit was checked by japhet@chromium.org
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org Link to the patchset: https://codereview.chromium.org/2694013005/#ps320001 (title: "Rebase")
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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by japhet@chromium.org
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": 320001, "attempt_start_ts": 1488835290267480,
"parent_rev": "8cccf2141e0fbf642d1254943ee6452f73692bea", "commit_rev":
"476af767372c72fd5e71f365e78481af9ce9a1d9"}
Message was sent while issue was closed.
Description was changed from ========== Cleanup blink-side PlzNavigate logic Instead of tracking the existence of a PlzNavigate navivation via FrameLoader::m_isNavigationHandledByClient, track it by creating a provisional DocumentLoader and doing ~all of the initiatlization steps except actually calling startLoadingMainResource(). PlzNavigate is emulating most of these steps anyway. This way we can do the normal checks for a provisional navigation in the exact same way for both PlzNavigate and non-PlzNavigate loads. Where PlzNavigate needs special handling, checking DocumentLoader::didStart() will indicate whether the real navigation actually began. Reworked a couple layout tests to have more reliable timings to make this work. BUG= ========== to ========== Cleanup blink-side PlzNavigate logic Instead of tracking the existence of a PlzNavigate navivation via FrameLoader::m_isNavigationHandledByClient, track it by creating a provisional DocumentLoader and doing ~all of the initiatlization steps except actually calling startLoadingMainResource(). PlzNavigate is emulating most of these steps anyway. This way we can do the normal checks for a provisional navigation in the exact same way for both PlzNavigate and non-PlzNavigate loads. Where PlzNavigate needs special handling, checking DocumentLoader::didStart() will indicate whether the real navigation actually began. Reworked a couple layout tests to have more reliable timings to make this work. BUG= Review-Url: https://codereview.chromium.org/2694013005 Cr-Commit-Position: refs/heads/master@{#454973} Committed: https://chromium.googlesource.com/chromium/src/+/476af767372c72fd5e71f365e784... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/476af767372c72fd5e71f365e784...
Message was sent while issue was closed.
Hi Nate, It seems this patch is causing two layout tests to fail again when PlzNavigate is enabled: inspector-protocol/page/frameAttachedDetached.html and inspector-protocol/page/frameStartedLoading.html. The issue last time was that the Inspector code got notified twice about the start of a renderer-initiated navigation. Could you take a look? Thanks!
Message was sent while issue was closed.
On 2017/03/07 17:06:46, clamy wrote: > Hi Nate, > > It seems this patch is causing two layout tests to fail again when PlzNavigate > is enabled: inspector-protocol/page/frameAttachedDetached.html and > inspector-protocol/page/frameStartedLoading.html. The issue last time was that > the Inspector code got notified twice about the start of a renderer-initiated > navigation. Could you take a look? > > Thanks! Will do. I know those tests were passing earlier, I think I must've messed up my last rebase. :/ |
