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

third_party/WebKit/Source/core/loader/FrameLoader.cpp

Index: third_party/WebKit/Source/core/loader/FrameLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index f90f72cb79180c0490f90689f16dfc61b73f5469..ebb6205a909b1f5d56629e19d453391c06c4e2f2 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -1499,8 +1499,7 @@ void FrameLoader::loadFailed(DocumentLoader* loader,
     if (loader != m_provisionalDocumentLoader)
       return;
     detachDocumentLoader(m_provisionalDocumentLoader);
-  } else {
-    DCHECK_EQ(loader, m_documentLoader);

[Nate Chapin, 2017/02/02 19:16:48]

I assume this is because tempLoader->detachFromFrame() calls in to
FrameLoader::loadFailed()?

[ananta, 2017/02/02 23:54:02]

Reverted this change

+  } else if (loader == m_documentLoader) {
     if (m_frame->document()->parser())
       m_frame->document()->parser()->stopParsing();
     if (!m_documentLoader->sentDidFinishLoad()) {
@@ -1681,7 +1680,8 @@ bool FrameLoader::shouldContinueForNavigationPolicy(
 bool FrameLoader::checkLoadCanStart(FrameLoadRequest& frameLoadRequest,
                                     FrameLoadType type,
                                     NavigationPolicy navigationPolicy,
-                                    NavigationType navigationType) {
+                                    NavigationType navigationType,
+                                    DocumentLoader* loader) {
   if (m_frame->document()->pageDismissalEventBeingDispatched() !=
       Document::NoDismissal) {
     return false;
@@ -1694,7 +1694,7 @@ bool FrameLoader::checkLoadCanStart(FrameLoadRequest& frameLoadRequest,
   modifyRequestForCSP(resourceRequest, nullptr);
 
   if (!shouldContinueForNavigationPolicy(
-          resourceRequest, frameLoadRequest.substituteData(), nullptr,
+          resourceRequest, frameLoadRequest.substituteData(), loader,
           frameLoadRequest.shouldCheckMainWorldContentSecurityPolicy(),
           navigationType, navigationPolicy, type,
           frameLoadRequest.clientRedirect() ==
@@ -1728,8 +1728,34 @@ void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest,
                                    ? WebURLRequest::FrameTypeTopLevel
                                    : WebURLRequest::FrameTypeNested);
 
+  // In PlzNavigate, the didStartProvisionalLoad() notification happens after
+  // the navigation event, which causes a number of layout tests to fail. We
+  // can call didStartProvisionalLoad() early before a navigation attempt is
+  // sent to the browser. This should work correctly in the non PlzNavigate
+  // case as well. However consumers of didStartProvisionalLoad() rely on the
+  // provisionalLoader being created and save state in it. To workaround this
+  // we create a temporary DocumentLoader instance and pass it to the
+  // didStartProvisionalLoad() function. This is then promoted to be the
+  // provisional loader if navigation is handled in blink.
+  // TODO(ananta)
+  // We should get rid of the dependency on the DocumentLoader in consumers of
+  // the didStartProvisionalLoad() notification.
+  DocumentLoader* tempLoader(client()->createDocumentLoader(

[Nate Chapin, 2017/02/02 19:16:48]

I don't love the name tempLoader. newLoader? Just plain 'loader'? policyLoader
(i.e., what it was called in the WebKit era)?

If you think all of these are worse than tempLoader, feel free to leave it
as-is.

[ananta, 2017/02/02 23:54:02]

Moved this code to a helper function FrameLoader::createDocumentLoader()

+      m_frame, resourceRequest,
+      frameLoadRequest.substituteData().isValid()
+          ? frameLoadRequest.substituteData()
+          : defaultSubstituteDataForURL(resourceRequest.url()),
+      frameLoadRequest.clientRedirect()));
+  tempLoader->setLoadType(type);
+  tempLoader->setNavigationType(navigationType);
+  tempLoader->setReplacesCurrentHistoryItem(type ==
+                                            FrameLoadTypeReplaceCurrentItem);
+
+  client()->dispatchDidStartProvisionalLoad(tempLoader);
+  DCHECK(tempLoader);
+
   if (!checkLoadCanStart(frameLoadRequest, type, navigationPolicy,
-                         navigationType)) {
+                         navigationType, tempLoader)) {
     // PlzNavigate: if the navigation is a commit of a client-handled
     // navigation, record that there is no longer a navigation handled by the
     // client.
@@ -1737,19 +1763,12 @@ void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest,
         !frameLoadRequest.resourceRequest().checkForBrowserSideNavigation()) {
       m_isNavigationHandledByClient = false;
     }
+    tempLoader->setSentDidFinishLoad();
+    tempLoader->detachFromFrame();
     return;
   }
 
-  m_provisionalDocumentLoader = client()->createDocumentLoader(
-      m_frame, resourceRequest,
-      frameLoadRequest.substituteData().isValid()
-          ? frameLoadRequest.substituteData()
-          : defaultSubstituteDataForURL(resourceRequest.url()),
-      frameLoadRequest.clientRedirect());
-  m_provisionalDocumentLoader->setLoadType(type);
-  m_provisionalDocumentLoader->setNavigationType(navigationType);
-  m_provisionalDocumentLoader->setReplacesCurrentHistoryItem(
-      type == FrameLoadTypeReplaceCurrentItem);
+  m_provisionalDocumentLoader = tempLoader;
 
   // PlzNavigate: We need to ensure that script initiated navigations are
   // honored.
@@ -1771,8 +1790,7 @@ void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest,
 
   m_provisionalDocumentLoader->appendRedirect(
       m_provisionalDocumentLoader->getRequest().url());
-  client()->dispatchDidStartProvisionalLoad();
-  DCHECK(m_provisionalDocumentLoader);
+
   m_provisionalDocumentLoader->startLoadingMainResource();
 
   takeObjectSnapshot();