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..1730efe2502ec34df17c6a8cfafbf611d0748b5e 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -1737,19 +1737,35 @@ void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest,
         !frameLoadRequest.resourceRequest().checkForBrowserSideNavigation()) {
       m_isNavigationHandledByClient = false;
     }
+    if (m_isNavigationHandledByClient) {
+      DocumentLoader* loader = createDocumentLoader(
+          resourceRequest, frameLoadRequest, type, navigationType);
+      // PlzNavigate: If the navigation is handled by the client, then the
+      // didFinishDocumentLoad() event occurs before the
+      // didStartProvisionalLoad() notification which occurs after the
+      // navigation
+      // is committed. This causes a number of layout tests to fail. We
+      // workaround this by invoking the didStartProvisionalLoad() notification
+      // here. Consumers of the didStartProvisionalLoad() notification rely on
+      // the provisional loader and save navigation state in it. We want to
+      // avoid
+      // this dependency on the provisional loader. For now we create a
+      // temporary
+      // loader and pass it to the didStartProvisionalLoad() function.
+      // TODO(ananta)
+      // We should get rid of the dependency on the DocumentLoader in consumers
+      // of
+      // the didStartProvisionalLoad() notification.
+      client()->dispatchDidStartProvisionalLoad(loader);
+      DCHECK(loader);
+      loader->setSentDidFinishLoad();
+      loader->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 = createDocumentLoader(
+      resourceRequest, frameLoadRequest, type, navigationType);
 
   // PlzNavigate: We need to ensure that script initiated navigations are
   // honored.
@@ -1771,8 +1787,12 @@ void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest,
 
   m_provisionalDocumentLoader->appendRedirect(
       m_provisionalDocumentLoader->getRequest().url());
-  client()->dispatchDidStartProvisionalLoad();
+  // TODO(ananta)
+  // We should get rid of the dependency on the DocumentLoader in consumers of
+  // the didStartProvisionalLoad() notification.
+  client()->dispatchDidStartProvisionalLoad(m_provisionalDocumentLoader);

[Nate Chapin, 2017/02/03 19:35:22]

Why did this move back here? No objections, just curious :)

[ananta, 2017/02/03 20:42:00]

Sending the provisional load notification early causes a number of tests to
break in the non plznavigate case. Turns out that there are assumptions that the
current page unload handlers would be called before subsequent load
notifications etc.

   DCHECK(m_provisionalDocumentLoader);
+
   m_provisionalDocumentLoader->startLoadingMainResource();
 
   takeObjectSnapshot();
@@ -1963,4 +1983,22 @@ inline void FrameLoader::takeObjectSnapshot() const {
                                       toTracedValue());
 }
 
+DocumentLoader* FrameLoader::createDocumentLoader(
+    const ResourceRequest& request,
+    const FrameLoadRequest& frameLoadRequest,
+    FrameLoadType loadType,
+    NavigationType navigationType) {
+  DocumentLoader* loader = client()->createDocumentLoader(
+      m_frame, request, frameLoadRequest.substituteData().isValid()
+                            ? frameLoadRequest.substituteData()
+                            : defaultSubstituteDataForURL(request.url()),
+      frameLoadRequest.clientRedirect());
+
+  loader->setLoadType(loadType);
+  loader->setNavigationType(navigationType);
+  loader->setReplacesCurrentHistoryItem(loadType ==
+                                        FrameLoadTypeReplaceCurrentItem);
+  return loader;
+}
+
 }  // namespace blink