Changeset 92439 in webkit

Timestamp:

Aug 4, 2011 6:32:24 PM (13 years ago)

Author:

commit-queue@webkit.org

Message:

Bad interaction between document destruction and unload events
​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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/loader/document-destruction-within-unload-expected.txt (added)
• LayoutTests/loader/document-destruction-within-unload.html (added)
• LayoutTests/loader/resources/document-destruction-within-unload-iframe.html (added)
• LayoutTests/loader/resources/document-destruction-within-unload.svg (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.cpp (modified) (1 diff)
• Source/WebCore/loader/FrameLoader.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -1996 +1996 @@
     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);
 

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -1835 +1835 @@
         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)
@@ -2333 +2337 @@
 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();
 }