Bad interaction between document destruction and unload events
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Aug 2011 01:32:24 +0000 (01:32 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Aug 2011 01:32:24 +0000 (01:32 +0000)
https://bugs.webkit.org/show_bug.cgi?id=64741

Patch by Scott Graham <scottmg@chromium.org> on 2011-08-04
Reviewed by Adam Barth.

Source/WebCore:

Three different errors triggered by this test case. The case to
consider is a subdocument with an onunload on an element, that
destroys the parent document during the onunload. One fix was a
lifetime issue fixed by a protecting RefPtr, and another was an
additional cancel of event triggers. The main fix was that during the
transition to commited state, the documentLoader is being replaced by
the provisionalDocumentLoader. But, because during firing events in
the subdocument the parent is destroyed, that subevent caused the
provisionalDocumentLoader to be detached from its frame. By marking
the page as being in committed state before the parent documentLoader
is set, this is avoided.

Test: loader/document-destruction-within-unload.html

* dom/Document.cpp:
(WebCore::Document::implicitOpen):
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::transitionToCommitted):
(WebCore::FrameLoader::detachChildren):

LayoutTests:

* loader/document-destruction-within-unload-expected.txt: Added.
* loader/document-destruction-within-unload.html: Added.
* loader/resources/document-destruction-within-unload-iframe.html: Added.
* loader/resources/document-destruction-within-unload.svg: Added.

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

LayoutTests/ChangeLog
LayoutTests/loader/document-destruction-within-unload-expected.txt [new file with mode: 0644]
LayoutTests/loader/document-destruction-within-unload.html [new file with mode: 0755]
LayoutTests/loader/resources/document-destruction-within-unload-iframe.html [new file with mode: 0755]
LayoutTests/loader/resources/document-destruction-within-unload.svg [new file with mode: 0755]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/loader/FrameLoader.cpp

index fc1e2bf..c651400 100644 (file)
@@ -1,3 +1,15 @@
+2011-08-04  Scott Graham  <scottmg@chromium.org>
+
+        Bad interaction between document destruction and unload events
+        https://bugs.webkit.org/show_bug.cgi?id=64741
+
+        Reviewed by Adam Barth.
+
+        * loader/document-destruction-within-unload-expected.txt: Added.
+        * loader/document-destruction-within-unload.html: Added.
+        * loader/resources/document-destruction-within-unload-iframe.html: Added.
+        * loader/resources/document-destruction-within-unload.svg: Added.
+
 2011-08-04  Kent Tamura  <tkent@chromium.org>
 
         Move <input type=week> tests to fast/forms/week/
diff --git a/LayoutTests/loader/document-destruction-within-unload-expected.txt b/LayoutTests/loader/document-destruction-within-unload-expected.txt
new file mode 100644 (file)
index 0000000..b6077a5
--- /dev/null
@@ -0,0 +1,2 @@
+
+For the test to pass there should be no crash.
diff --git a/LayoutTests/loader/document-destruction-within-unload.html b/LayoutTests/loader/document-destruction-within-unload.html
new file mode 100755 (executable)
index 0000000..b0564f2
--- /dev/null
@@ -0,0 +1,20 @@
+<html>
+    <body>
+        <script>
+            if (window.layoutTestController) {
+                layoutTestController.dumpAsText();
+                layoutTestController.waitUntilDone();
+            }
+        </script>
+        <iframe src="resources/document-destruction-within-unload-iframe.html">
+        </iframe>
+        <p>For the test to pass there should be no crash.</p>
+        <script>
+            function done() {
+                if (window.layoutTestController)
+                    layoutTestController.notifyDone();
+            }
+        </script>
+
+    </body>
+</html>
diff --git a/LayoutTests/loader/resources/document-destruction-within-unload-iframe.html b/LayoutTests/loader/resources/document-destruction-within-unload-iframe.html
new file mode 100755 (executable)
index 0000000..6536449
--- /dev/null
@@ -0,0 +1,16 @@
+<html>
+    <body onload="window.done()">
+        <script>
+            function runTest() {
+                var test = document.getElementById('root').contentDocument;
+                test.firstChild.setAttribute('onunload', "parent.clearUs();");
+                location.reload();
+            }
+            function clearUs() {
+                document.write();
+            }
+        </script>
+        <object data="does_not_exist"></object>
+        <object data="document-destruction-within-unload.svg" id="root" onload="runTest();"></object>
+    </body>
+</html>
diff --git a/LayoutTests/loader/resources/document-destruction-within-unload.svg b/LayoutTests/loader/resources/document-destruction-within-unload.svg
new file mode 100755 (executable)
index 0000000..e0af766
--- /dev/null
@@ -0,0 +1,2 @@
+<svg xmlns="http://www.w3.org/2000/svg">
+</svg>
index 0fcf535..8096674 100644 (file)
@@ -1,3 +1,30 @@
+2011-08-04  Scott Graham  <scottmg@chromium.org>
+
+        Bad interaction between document destruction and unload events
+        https://bugs.webkit.org/show_bug.cgi?id=64741
+
+        Reviewed by Adam Barth.
+
+        Three different errors triggered by this test case. The case to
+        consider is a subdocument with an onunload on an element, that
+        destroys the parent document during the onunload. One fix was a
+        lifetime issue fixed by a protecting RefPtr, and another was an
+        additional cancel of event triggers. The main fix was that during the
+        transition to commited state, the documentLoader is being replaced by
+        the provisionalDocumentLoader. But, because during firing events in
+        the subdocument the parent is destroyed, that subevent caused the
+        provisionalDocumentLoader to be detached from its frame. By marking
+        the page as being in committed state before the parent documentLoader
+        is set, this is avoided.
+
+        Test: loader/document-destruction-within-unload.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::implicitOpen):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::transitionToCommitted):
+        (WebCore::FrameLoader::detachChildren):
+
 2011-08-04  Simon Fraser  <simon.fraser@apple.com>
 
         Add code to determine whether a Range in inside fixed position content
index fd2886e..dd2aa46 100644 (file)
@@ -1995,6 +1995,10 @@ void Document::implicitOpen()
 
     removeChildren();
 
+    // cancel again, as removeChildren can cause event triggers to be added
+    // again, which we don't want triggered on the old document.
+    cancelParsing();
+
     setCompatibilityMode(NoQuirksMode);
 
     m_parser = createParser();
index 6a49e46..5d7b792 100644 (file)
@@ -1834,9 +1834,13 @@ void FrameLoader::transitionToCommitted(PassRefPtr<CachedPage> cachedPage)
     if (m_documentLoader)
         m_documentLoader->stopLoadingPlugIns();
 
+    // State must be set before setting m_documentLoader to avoid
+    // m_provisionalDocumentLoader getting detached from the frame via a sub
+    // frame. See https://bugs.webkit.org/show_bug.cgi?id=64741 for more
+    // discussion.
+    setState(FrameStateCommittedPage);
     setDocumentLoader(m_provisionalDocumentLoader.get());
     setProvisionalDocumentLoader(0);
-    setState(FrameStateCommittedPage);
 
 #if ENABLE(TOUCH_EVENTS)
     if (isLoadingMainFrame())
@@ -2332,12 +2336,14 @@ void FrameLoader::frameLoadCompleted()
 
 void FrameLoader::detachChildren()
 {
+    typedef Vector<RefPtr<Frame> > FrameVector;
+    FrameVector protect;
+
     // FIXME: Is it really necessary to do this in reverse order?
-    Frame* previous;
-    for (Frame* child = m_frame->tree()->lastChild(); child; child = previous) {
-        previous = child->tree()->previousSibling();
-        child->loader()->detachFromParent();
-    }
+    for (Frame* child = m_frame->tree()->lastChild(); child; child = child->tree()->previousSibling())
+        protect.append(child);
+    for (FrameVector::iterator it = protect.begin(); it != protect.end(); ++it)
+        (*it)->loader()->detachFromParent();
 }
 
 void FrameLoader::closeAndRemoveChild(Frame* child)