Don't stop the documentLoader on navigations.

Source/core/loader/FrameLoader.cpp

Index: Source/core/loader/FrameLoader.cpp
diff --git a/Source/core/loader/FrameLoader.cpp b/Source/core/loader/FrameLoader.cpp
index aeb892669a02f2dce79ca35dbfdf5fe38d5850f0..7a3256f67aba76252c60967a4d70c7ed7ef8e4b0 100644
--- a/Source/core/loader/FrameLoader.cpp
+++ b/Source/core/loader/FrameLoader.cpp
@@ -149,6 +149,7 @@ FrameLoader::FrameLoader(LocalFrame* frame, FrameLoaderClient* client)
     , m_fetchContext(FrameFetchContext::create(frame))
     , m_inStopAllLoaders(false)
     , m_isComplete(false)
+    , m_currentLoaderNeedsStop(false)
     , m_checkTimer(this, &FrameLoader::checkTimerFired)
     , m_shouldCallCheckCompleted(false)
     , m_didAccessInitialDocument(false)
@@ -615,6 +616,27 @@ void FrameLoader::started()
         frame->loader().m_isComplete = false;
 }
 
+void FrameLoader::onResponseReceived()
+{
+    checkCurrentDocumentLoaderNeedsStop();
+}
+
+void FrameLoader::checkCurrentDocumentLoaderNeedsStop()

[Nate Chapin, 2014/03/10 16:12:44]

If this function is only called once, let's merge it into responseReceived()
above.

[mkosiba (inactive), 2014/03/11 23:24:08]

sure, I was keeping it separate because I was experimenting on calling it from
other places too.

+{
+    if (!m_currentLoaderNeedsStop)
+        return;
+
+    for (RefPtr<LocalFrame> child = m_frame->tree().firstChild(); child; child = child->tree().nextSibling())
+        child->loader().stopAllLoaders();
+    if (m_documentLoader)
+        m_documentLoader->stopLoading();
+
+    m_currentLoaderNeedsStop = false;
+
+    if (m_state == FrameStateProvisional)
+        m_progressTracker->progressStarted();

[Nate Chapin, 2014/03/10 16:12:44]

It doesn't seem right to wait to send a progress tracker start notification
until a response is received.

[mkosiba (inactive), 2014/03/11 23:24:08]

this is the meat of the change. The problem is that when you call
loadWithNavigationAction that causes a stopLoading (if a load was in progress)
and an immediate startLoading after that. I'm trying to postpone the stop/start
till we know whether the new loader will not immediately error out.

The alternative to this was adding a new callback (as here:
https://codereview.chromium.org/131363006/).

A third option is to rework the rest of chrome to support overlapping start/stop
notifications, but that's more work so I was hoping to avoid it if possible.

[Nate Chapin, 2014/03/12 19:55:45]

I talked with darin, and our thoughts were:

1. Let's start by seeing how much of a perf impact it would be to just let both
the XHR and the main resource run in parallel. Maybe run some perf measurements
or see what the network stack folks think?

2. If that's going to hurt perf too badly, we could consider using the load
deferral concept in blink. Blink has a way to defer all loading in a page, but
we could use most of that logic to defer just FrameLoader::m_documentLoader
while letting FrameLoader::m_provisionalDocumentLoader proceed normally.

3. If that still causes perf problems, we could look at making load deferral
more aggressively throttle in-progress loads.

Does that seem reasonable?

+}
+
 void FrameLoader::setReferrerForFrameRequest(ResourceRequest& request, ShouldSendReferrer shouldSendReferrer, Document* originDocument)
 {
     if (shouldSendReferrer == NeverSendReferrer) {
@@ -937,6 +959,9 @@ void FrameLoader::checkLoadCompleteForThisFrame()
 {
     ASSERT(m_client->hasWebView());
 
+    if (m_currentLoaderNeedsStop)

[Nate Chapin, 2014/03/10 16:12:44]

Why is this needed?

[mkosiba (inactive), 2014/03/11 23:24:08]

because otherwise we'll call
    m_progressTracker->progressCompleted();
before the delayed
     m_progressTracker->progressStarted();
is called.

+        return;
+
     if (m_state == FrameStateProvisional && m_provisionalDocumentLoader) {
         const ResourceError& error = m_provisionalDocumentLoader->mainDocumentError();
         if (error.isNull())
@@ -1288,7 +1313,7 @@ void FrameLoader::loadWithNavigationAction(const NavigationAction& action, Frame
     else if (m_documentLoader)
         m_policyDocumentLoader->setOverrideEncoding(m_documentLoader->overrideEncoding());
 
-    // stopAllLoaders can detach the LocalFrame, so protect it.
+    // Stopping the provisional loader can detach the Frame, so protect it.
     RefPtr<LocalFrame> protect(m_frame);
     if ((!m_policyDocumentLoader->shouldContinueForNavigationPolicy(request) || !shouldClose()) && m_policyDocumentLoader) {
         m_policyDocumentLoader->detachFromFrame();
@@ -1296,26 +1321,24 @@ void FrameLoader::loadWithNavigationAction(const NavigationAction& action, Frame
         return;
     }
 
-    // A new navigation is in progress, so don't clear the history's provisional item.
-    stopAllLoaders();
-
-    // <rdar://problem/6250856> - In certain circumstances on pages with multiple frames, stopAllLoaders()
-    // might detach the current FrameLoader, in which case we should bail on this newly defunct load.
-    if (!m_frame->page() || !m_policyDocumentLoader)
-        return;
-
     if (isLoadingMainFrame())
         m_frame->page()->inspectorController().resume();
     m_frame->navigationScheduler().cancel();
 
+    if (m_provisionalDocumentLoader)
+        m_provisionalDocumentLoader->stopLoading();
+    if (m_provisionalDocumentLoader)

[Nate Chapin, 2014/03/10 16:12:44]

Merge these 2 ifs? Or can stopLoading() null out the provisional document
loader?

[mkosiba (inactive), 2014/03/11 23:24:08]

merge - sure. call stopLoading() - no, since that stops the documentLoader which
is what I'm trying to prevent.

+        m_provisionalDocumentLoader->detachFromFrame();
+    m_checkTimer.stop();
+
     m_provisionalDocumentLoader = m_policyDocumentLoader.release();
     m_loadType = type;
     m_state = FrameStateProvisional;
+    m_currentLoaderNeedsStop = true;
 
     if (formState)
         m_client->dispatchWillSubmitForm(formState);
 
-    m_progressTracker->progressStarted();
     if (m_provisionalDocumentLoader->isClientRedirect())
         m_provisionalDocumentLoader->appendRedirect(m_frame->document()->url());
     m_provisionalDocumentLoader->appendRedirect(m_provisionalDocumentLoader->request().url());