Better distinguish didFinishLoad and didStopLoading

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

Index: third_party/WebKit/Source/core/loader/DocumentLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/DocumentLoader.cpp b/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
index ee3a2fc1c211c1dea5a8419646de66dcb93ce90a..5ade613746bb48c12d98373888f46146c2cf22f3 100644
--- a/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
@@ -293,16 +293,10 @@ void DocumentLoader::finishedLoading(double finishTime)
 
     endWriting(m_writer.get());
 
-    if (!m_mainDocumentError.isNull())
+    if (!m_mainDocumentError.isNull() || !m_frame)

[dcheng, 2015/10/08 05:34:35]

How can m_frame be null now?

[Nate Chapin, 2015/10/08 21:03:59]

It could be null when the DocumentLoader is synchronously detached inside
endWriting() before this change, too. It's m_mainDocumentError.isNull() that
changed.

Before:
* iframe completes, sends didFinishLoad, fires parent's load event.
* Parent's load event detached iframe, which calls DocumentLoader::stopLoading()
* DocumentLoader::finishedLoading() hasn't yet set m_state to MainResourceDone,
so it cancels the main resource load, which makes m_mainDocumentError non-null
as well as nulling m_frame and m_applicationCacheHost.
* Successful early exit.

After:
* iframe completes, sends didFinishLoad, ***sets DocumentLoader::m_state to
SentDidFinishLoad***, fires parent's load event.
* Parent's load event detached iframe, which calls DocumentLoader::stopLoading()
* DocumentLoader::isLoading() returns false because m_state is not in the right
range, main resource is not cancelled, m_mainDocumentError remains null.
* m_applicationCacheHost and m_frame were still both nulled, so we crash when we
unconditionally derefernce m_applicationCacheHost.

Moving m_applicationCacheHost->finishedLoadingMainResource() before
endWriting(), so that it is guaranteed non-null. The rest of the function is
benign when detached.

         return;
-    m_state = MainResourceDone;
-
-    // If the document specified an application cache manifest, it violates the author's intent if we store it in the memory cache
-    // and deny the appcache the chance to intercept it in the future, so remove from the memory cache.
-    if (m_frame) {
-        if (m_mainResource && m_frame->document()->hasAppCacheManifest())

[Nate Chapin, 2015/10/06 18:35:36]

This code should have been removed when we blinked, it's irrelevant because we
never store main resources in MemoryCache.

-            memoryCache()->remove(m_mainResource.get());
-    }
+    if (m_state < MainResourceDone)
+        m_state = MainResourceDone;
     m_applicationCacheHost->finishedLoadingMainResource();
     clearMainResourceHandle();
 }