New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 638900 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

PlzNavigate: When loading subframe, WebFrameClient::didFinishDocumentLoad() and WebFrameClient::didStartProvisionalLoad() callback order inverted

Project Member Reported by blundell@chromium.org, Aug 18 2016

Issue description

When loading a subframe in the non-PlzNavigate case, WebFrameClient receives the following callbacks:

- didStartProvisionalLoad() for the subframe (this occurs while parsing the document)
- didFinishDocumentLoad() (once parsing the document is finished)

However, in the PlzNavigate case the order of these callbacks is inverted. This occurs because during parsing of the document the renderer-initiated navigation to the subframe causes only an IPC to the browser to start the navigation. It is when the browser sends the message back to commit the navigation that the full load in FrameLoader.cpp::load() occurs and results in the callback to didStartProvisionalLoad().

One effect of this is that several layout tests fail under PlzNavigate because the output produced by these callbacks (via WebFrameTestClient) occurs in the opposite order to what they expected. It's unclear whether there are any more serious effects (i.e., whether production code also relies on the order of these callbacks).

 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/321db6cd62126d6a12de04ea8b874d3b7f10adad

commit 321db6cd62126d6a12de04ea8b874d3b7f10adad
Author: blundell <blundell@chromium.org>
Date: Fri Aug 19 10:54:58 2016

PlzNavigate: Triage tests failing due to flipped WebFrameClient callbacks

As described in  crbug.com/638900 , when running in PlzNavigate the
order of the WebFrameClient and didStartProvisionalLoad() and
didFinishDocumentLoad() callbacks is inverted from non-PlzNavigate when
loading a subframe while parsing the document. This inversion causes these
layout tests to fail because they see WebFrameTestClient's output from
these callbacks in the opposite order from what's expected.

BUG= 638900 

Review-Url: https://codereview.chromium.org/2251973004
Cr-Commit-Position: refs/heads/master@{#413102}

[modify] https://crrev.com/321db6cd62126d6a12de04ea8b874d3b7f10adad/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation

Components: Blink>HTML>Frame
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1a023a38bddf1b2d4e64a1f31175b72e1776713d

commit 1a023a38bddf1b2d4e64a1f31175b72e1776713d
Author: ananta <ananta@chromium.org>
Date: Tue Feb 07 19:34:01 2017

PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a navigation early on in FrameLoader::startLoad()

A number of layouttests fail because the didStartProvisionalLoad() notification comes in for PlzNavigate after
didFinishDocumentLoad() notification is called. In the non PlzNavigate case the load is handled in blink and hence the
notifications happen in the correct order.

Approach being tried is to invoke didStartProvisionaLoad() in FrameLoader::startLoad() when we find that a navigation is being
handled by the client. Ideally we would want to invoke this notification early regardless of whether PlzNavigate is on. However
a number of tests and codepaths assume that the current document would be unloaded before we receive the provisional load notification
for the non PlzNavigate case. We create a temporary loader just for the purpose of firing the didStartProvisionalLoad()
notification.

Most of the changes in this patch are boilerplate changes to the didStartProvisionalLoad() function.

BUG= 638900 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2661743002
Cr-Commit-Position: refs/heads/master@{#448695}

[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/chrome/renderer/chrome_render_frame_observer.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/chrome/renderer/chrome_render_frame_observer.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/chrome/renderer/net/net_error_helper.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/chrome/renderer/net/net_error_helper.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/autofill/content/renderer/password_autofill_agent.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/autofill/content/renderer/password_autofill_agent.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/contextual_search/renderer/overlay_js_render_frame_observer.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/contextual_search/renderer/overlay_js_render_frame_observer.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/dom_distiller/content/renderer/distiller_js_render_frame_observer.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/dom_distiller/content/renderer/distiller_js_render_frame_observer.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/printing/renderer/print_web_view_helper.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/printing/renderer/print_web_view_helper.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/subresource_filter/content/renderer/subresource_filter_agent.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/subresource_filter/content/renderer/subresource_filter_agent.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/test_runner/web_frame_test_client.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/test_runner/web_frame_test_client.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/components/test_runner/web_frame_test_proxy.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/content/public/renderer/render_frame_observer.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/content/renderer/render_frame_impl.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/extensions/renderer/extension_frame_helper.cc
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/extensions/renderer/extension_frame_helper.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/third_party/WebKit/Source/core/loader/EmptyClients.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/third_party/WebKit/Source/core/loader/FrameLoader.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/third_party/WebKit/Source/core/loader/FrameLoaderClient.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/third_party/WebKit/Source/web/FrameLoaderClientImpl.h
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/third_party/WebKit/Source/web/tests/WebDocumentSubresourceFilterTest.cpp
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/1a023a38bddf1b2d4e64a1f31175b72e1776713d/third_party/WebKit/public/web/WebFrameClient.h

Owner: ananta@chromium.org
Status: Assigned (was: Available)

Comment 5 by ananta@chromium.org, Feb 16 2017

Status: Fixed (was: Assigned)

Sign in to add a comment