Null dereference loading Blink layout test http/tests/misc/detach-during-notifyDone...

https://bugs.webkit.org/show_bug.cgi?id=149309
<rdar://problem/22748363>

Reviewed by Brent Fulgham.

Source/WebCore:

A weird order of event execution introduced by the test case will kill the webpage in a
subframe of the page while executing its |frame.loader().checkLoadCompleteForThisFrame()|.
Therefore, any frames comes after the failing subframe will have no page. Check it before
calling to those frames' |frame.loader().checkLoadCompleteForThisFrame()|, otherwise the
assertion in |frame.loader().checkLoadCompleteForThisFrame()| will fail.

Test: http/tests/misc/detach-during-notifyDone.html

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::checkLoadComplete):

Source/WebKit/mac:

* WebView/WebDataSource.mm:
(WebDataSourcePrivate::~WebDataSourcePrivate):
Refine the assertion to treat <rdar://problem/9673866>.

Source/WebKit2:

Callback of bundle clients could kill the documentloader. Therefore, make a copy
of the navigationID before invoking the callback.

* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::dispatchDidChangeLocationWithinPage):
(WebKit::WebFrameLoaderClient::dispatchDidPushStateWithinPage):
(WebKit::WebFrameLoaderClient::dispatchDidReplaceStateWithinPage):
(WebKit::WebFrameLoaderClient::dispatchDidPopStateWithinPage):
(WebKit::WebFrameLoaderClient::dispatchDidFailLoad):
(WebKit::WebFrameLoaderClient::dispatchDidFinishDocumentLoad):
(WebKit::WebFrameLoaderClient::dispatchDidFinishLoad):

LayoutTests:

The test case is from Blink r175601:
https://codereview.chromium.org/317513002
The test case will generate a set of weird ordering events that affects the documentLoader:
1. The subframe finishes loading, and since the frame’s testRunner is not set to wait until
done, WebKitTestRunner stops the load (by calling WKBundlePageStopLoading()).
2. This causes the in-progress XHR to be aborted, which causes its readyState to become DONE
(this bug doesn’t always reproduce because sometimes the XHR has already finished before the
frame finishes loading).
3. The onreadystatechange callback is executed, which sets innerHTML on the parent frame.
4. Setting innerHTML disconnects the subframe, nulling out its DocumentLoader.
5. We return to WebFrameLoaderClient::dispatchDidFinishLoad() from step #1, but now the
FrameLoader’s DocumentLoader is null. And WebKit crashes here.

Note that steps 2-4 happen synchronously inside WebFrameLoaderClient::dispatchDidFinishLoad().

* http/tests/misc/detach-during-notifyDone-expected.txt: Added.
* http/tests/misc/detach-during-notifyDone.html: Added.
* http/tests/misc/resources/detached-frame.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@192844 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index e138071610c63785900da711955f5779e3dc9987..c0064096ed5e905ee3987281fe89118347753bd7 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,30 @@
+2015-11-30  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Null dereference loading Blink layout test http/tests/misc/detach-during-notifyDone.html
+        https://bugs.webkit.org/show_bug.cgi?id=149309
+        <rdar://problem/22748363>
+
+        Reviewed by Brent Fulgham.
+
+        The test case is from Blink r175601:
+        https://codereview.chromium.org/317513002
+        The test case will generate a set of weird ordering events that affects the documentLoader:
+        1. The subframe finishes loading, and since the frame’s testRunner is not set to wait until
+        done, WebKitTestRunner stops the load (by calling WKBundlePageStopLoading()).
+        2. This causes the in-progress XHR to be aborted, which causes its readyState to become DONE
+        (this bug doesn’t always reproduce because sometimes the XHR has already finished before the
+        frame finishes loading).
+        3. The onreadystatechange callback is executed, which sets innerHTML on the parent frame.
+        4. Setting innerHTML disconnects the subframe, nulling out its DocumentLoader.
+        5. We return to WebFrameLoaderClient::dispatchDidFinishLoad() from step #1, but now the
+        FrameLoader’s DocumentLoader is null. And WebKit crashes here.
+
+        Note that steps 2-4 happen synchronously inside WebFrameLoaderClient::dispatchDidFinishLoad().
+
+        * http/tests/misc/detach-during-notifyDone-expected.txt: Added.
+        * http/tests/misc/detach-during-notifyDone.html: Added.
+        * http/tests/misc/resources/detached-frame.html: Added.
+
 2015-11-30  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r192819.

diff --git a/LayoutTests/http/tests/misc/detach-during-notifyDone-expected.txt b/LayoutTests/http/tests/misc/detach-during-notifyDone-expected.txt
new file mode 100644 (file)
index 0000000..3d50174
--- /dev/null
+++ b/LayoutTests/http/tests/misc/detach-during-notifyDone-expected.txt
@@ -0,0 +1 @@
+ PASS. WebKit didn't crash.

diff --git a/LayoutTests/http/tests/misc/detach-during-notifyDone.html b/LayoutTests/http/tests/misc/detach-during-notifyDone.html
new file mode 100644 (file)
index 0000000..7f886e3
--- /dev/null
+++ b/LayoutTests/http/tests/misc/detach-during-notifyDone.html
@@ -0,0 +1,8 @@
+<iframe></iframe><iframe src=resources/detached-frame.html></iframe>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+}
+</script>
+PASS. WebKit didn't crash.

diff --git a/LayoutTests/http/tests/misc/resources/detached-frame.html b/LayoutTests/http/tests/misc/resources/detached-frame.html
new file mode 100644 (file)
index 0000000..e73c8f9
--- /dev/null
+++ b/LayoutTests/http/tests/misc/resources/detached-frame.html
@@ -0,0 +1,15 @@
+<script>
+function test() {
+    var xhr = new XMLHttpRequest;
+    xhr.open("GET", "detached-frame.html");
+    xhr.send();
+    xhr.onreadystatechange = function() {
+        if (xhr.readyState == xhr.DONE) {
+            var parentWindow = window.parent;
+            parentWindow.document.body.innerHTML += "";
+            parentWindow.testRunner.notifyDone();
+        }
+    }
+}
+</script>
+<body onload="test()">
\ No newline at end of file

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index d7f3ad78d0f5b2fee76f49cd5b5d0d13bd074ad4..19671ed1454d7ebdb7f1193c13136fb6fdc19647 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,22 @@
+2015-11-30  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Null dereference loading Blink layout test http/tests/misc/detach-during-notifyDone.html
+        https://bugs.webkit.org/show_bug.cgi?id=149309
+        <rdar://problem/22748363>
+
+        Reviewed by Brent Fulgham.
+
+        A weird order of event execution introduced by the test case will kill the webpage in a
+        subframe of the page while executing its |frame.loader().checkLoadCompleteForThisFrame()|.
+        Therefore, any frames comes after the failing subframe will have no page. Check it before
+        calling to those frames' |frame.loader().checkLoadCompleteForThisFrame()|, otherwise the
+        assertion in |frame.loader().checkLoadCompleteForThisFrame()| will fail.
+
+        Test: http/tests/misc/detach-during-notifyDone.html
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::checkLoadComplete):
+
 2015-11-30  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r192819.

diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
index 921bbf189f92cb89049005496142f0de70ba0e05..523b30d95ba070161167b7f144fdfe66605fa54c 100644 (file)
--- a/Source/WebCore/loader/FrameLoader.cpp
+++ b/Source/WebCore/loader/FrameLoader.cpp
@@ -2431,8 +2431,10 @@ void FrameLoader::checkLoadComplete()
         frames.append(*frame);
 
     // To process children before their parents, iterate the vector backwards.
-    for (unsigned i = frames.size(); i; --i)
-        frames[i - 1]->loader().checkLoadCompleteForThisFrame();
+    for (auto frame = frames.rbegin(); frame != frames.rend(); ++frame) {
+        if ((*frame)->page())
+            (*frame)->loader().checkLoadCompleteForThisFrame();
+    }
 }
 
 int FrameLoader::numPendingOrLoadingRequests(bool recurse) const

diff --git a/Source/WebKit/mac/ChangeLog b/Source/WebKit/mac/ChangeLog
index 3b99f488191ca23061ed20d78e871331d451e460..821ad59e273ed83ffb5a2872b52d1ddaf8f75a49 100644 (file)
--- a/Source/WebKit/mac/ChangeLog
+++ b/Source/WebKit/mac/ChangeLog
@@ -1 +1,13 @@
+2015-11-30  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Null dereference loading Blink layout test http/tests/misc/detach-during-notifyDone.html
+        https://bugs.webkit.org/show_bug.cgi?id=149309
+        <rdar://problem/22748363>
+
+        Reviewed by Brent Fulgham.
+
+        * WebView/WebDataSource.mm:
+        (WebDataSourcePrivate::~WebDataSourcePrivate):
+        Refine the assertion to treat <rdar://problem/9673866>.
+
 == Rolled over to ChangeLog-2015-11-21 ==

diff --git a/Source/WebKit/mac/WebView/WebDataSource.mm b/Source/WebKit/mac/WebView/WebDataSource.mm
index 74d4df52614c378a289434ec52fb455d504bb153..11970915a2b12a420b818b2feee739f9c5648e27 100644 (file)
--- a/Source/WebKit/mac/WebView/WebDataSource.mm
+++ b/Source/WebKit/mac/WebView/WebDataSource.mm
@@ -92,7 +92,12 @@ public:
     ~WebDataSourcePrivate()
     {
         if (loader) {
-            ASSERT(!loader->isLoading());
+            // We might run in to infinite recursion if we're stopping loading as the result of detaching from the frame.
+            // Therefore, DocumentLoader::detachFromFrame() did some smart things to stop the recursion.
+            // As a result of breaking the resursion, DocumentLoader::m_subresourceLoader
+            // and DocumentLoader::m_plugInStreamLoaders might not be empty at this time.
+            // See <rdar://problem/9673866> for more details.
+            ASSERT(!loader->isLoading() || loader->isStopping());
             loader->detachDataSource();
         }
     }

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
index c2300895a2e9f72cb87906d42620813b3853aae4..b119a2c50cba0f8acc26e844fc2b66c393f92c4c 100644 (file)
--- a/Source/WebKit2/ChangeLog
+++ b/Source/WebKit2/ChangeLog
@@ -1,3 +1,23 @@
+2015-11-30  Jiewen Tan  <jiewen_tan@apple.com>
+
+        Null dereference loading Blink layout test http/tests/misc/detach-during-notifyDone.html
+        https://bugs.webkit.org/show_bug.cgi?id=149309
+        <rdar://problem/22748363>
+
+        Reviewed by Brent Fulgham.
+
+        Callback of bundle clients could kill the documentloader. Therefore, make a copy
+        of the navigationID before invoking the callback.
+
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::dispatchDidChangeLocationWithinPage):
+        (WebKit::WebFrameLoaderClient::dispatchDidPushStateWithinPage):
+        (WebKit::WebFrameLoaderClient::dispatchDidReplaceStateWithinPage):
+        (WebKit::WebFrameLoaderClient::dispatchDidPopStateWithinPage):
+        (WebKit::WebFrameLoaderClient::dispatchDidFailLoad):
+        (WebKit::WebFrameLoaderClient::dispatchDidFinishDocumentLoad):
+        (WebKit::WebFrameLoaderClient::dispatchDidFinishLoad):
+
 2015-11-30  Tim Horton  <timothy_horton@apple.com>
 
         Get rid of the !USE(ASYNC_NSTEXTINPUTCLIENT) codepath

diff --git a/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp b/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
index add5a6958cd20695b9c1a0fbbed830ae3efac931..2e744a0f82378b973a58d34a763bf23ef0010b4b 100644 (file)
--- a/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
+++ b/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
@@ -327,12 +327,13 @@ void WebFrameLoaderClient::dispatchDidChangeLocationWithinPage()
 
     RefPtr<API::Object> userData;
 
+    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
+
     // Notify the bundle client.
     webPage->injectedBundleLoaderClient().didSameDocumentNavigationForFrame(webPage, m_frame, SameDocumentNavigationAnchorNavigation, userData);
 
     // Notify the UIProcess.
-    WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
-    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), documentLoader.navigationID(), SameDocumentNavigationAnchorNavigation, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
+    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), navigationID, SameDocumentNavigationAnchorNavigation, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
 }
 
 void WebFrameLoaderClient::dispatchDidPushStateWithinPage()
@@ -343,12 +344,13 @@ void WebFrameLoaderClient::dispatchDidPushStateWithinPage()
 
     RefPtr<API::Object> userData;
 
+    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
+
     // Notify the bundle client.
     webPage->injectedBundleLoaderClient().didSameDocumentNavigationForFrame(webPage, m_frame, SameDocumentNavigationSessionStatePush, userData);
 
     // Notify the UIProcess.
-    WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
-    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), documentLoader.navigationID(), SameDocumentNavigationSessionStatePush, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
+    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), navigationID, SameDocumentNavigationSessionStatePush, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
 }
 
 void WebFrameLoaderClient::dispatchDidReplaceStateWithinPage()
@@ -359,12 +361,13 @@ void WebFrameLoaderClient::dispatchDidReplaceStateWithinPage()
 
     RefPtr<API::Object> userData;
 
+    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
+
     // Notify the bundle client.
     webPage->injectedBundleLoaderClient().didSameDocumentNavigationForFrame(webPage, m_frame, SameDocumentNavigationSessionStateReplace, userData);
 
     // Notify the UIProcess.
-    WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
-    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), documentLoader.navigationID(), SameDocumentNavigationSessionStateReplace, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
+    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), navigationID, SameDocumentNavigationSessionStateReplace, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
 }
 
 void WebFrameLoaderClient::dispatchDidPopStateWithinPage()
@@ -375,12 +378,13 @@ void WebFrameLoaderClient::dispatchDidPopStateWithinPage()
 
     RefPtr<API::Object> userData;
 
+    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
+
     // Notify the bundle client.
     webPage->injectedBundleLoaderClient().didSameDocumentNavigationForFrame(webPage, m_frame, SameDocumentNavigationSessionStatePop, userData);
 
     // Notify the UIProcess.
-    WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
-    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), documentLoader.navigationID(), SameDocumentNavigationSessionStatePop, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
+    webPage->send(Messages::WebPageProxy::DidSameDocumentNavigationForFrame(m_frame->frameID(), navigationID, SameDocumentNavigationSessionStatePop, m_frame->coreFrame()->document()->url().string(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
 }
 
 void WebFrameLoaderClient::dispatchWillClose()
@@ -504,12 +508,13 @@ void WebFrameLoaderClient::dispatchDidFailLoad(const ResourceError& error)
 
     RefPtr<API::Object> userData;
 
+    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
+
     // Notify the bundle client.
     webPage->injectedBundleLoaderClient().didFailLoadWithErrorForFrame(webPage, m_frame, error, userData);
 
     // Notify the UIProcess.
-    WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
-    webPage->send(Messages::WebPageProxy::DidFailLoadForFrame(m_frame->frameID(), documentLoader.navigationID(), error, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
+    webPage->send(Messages::WebPageProxy::DidFailLoadForFrame(m_frame->frameID(), navigationID, error, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
 
     // If we have a load listener, notify it.
     if (WebFrame::LoadListener* loadListener = m_frame->loadListener())
@@ -524,13 +529,13 @@ void WebFrameLoaderClient::dispatchDidFinishDocumentLoad()
 
     RefPtr<API::Object> userData;
 
+    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
+
     // Notify the bundle client.
     webPage->injectedBundleLoaderClient().didFinishDocumentLoadForFrame(webPage, m_frame, userData);
 
-    WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
-
     // Notify the UIProcess.
-    webPage->send(Messages::WebPageProxy::DidFinishDocumentLoadForFrame(m_frame->frameID(), documentLoader.navigationID(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
+    webPage->send(Messages::WebPageProxy::DidFinishDocumentLoadForFrame(m_frame->frameID(), navigationID, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
 }
 
 void WebFrameLoaderClient::dispatchDidFinishLoad()
@@ -541,13 +546,13 @@ void WebFrameLoaderClient::dispatchDidFinishLoad()
 
     RefPtr<API::Object> userData;
 
+    auto navigationID = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader()).navigationID();
+
     // Notify the bundle client.
     webPage->injectedBundleLoaderClient().didFinishLoadForFrame(webPage, m_frame, userData);
 
-    WebDocumentLoader& documentLoader = static_cast<WebDocumentLoader&>(*m_frame->coreFrame()->loader().documentLoader());
-
     // Notify the UIProcess.
-    webPage->send(Messages::WebPageProxy::DidFinishLoadForFrame(m_frame->frameID(), documentLoader.navigationID(), UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
+    webPage->send(Messages::WebPageProxy::DidFinishLoadForFrame(m_frame->frameID(), navigationID, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())));
 
     // If we have a load listener, notify it.
     if (WebFrame::LoadListener* loadListener = m_frame->loadListener())