Crash in Web Content Process under ~PDFDocument under clearTouchEventListeners at...
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 26 Jul 2014 18:45:04 +0000 (18:45 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 26 Jul 2014 18:45:04 +0000 (18:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=135319
<rdar://problem/17315168>

Reviewed by Darin Adler and Antti Koivisto.

* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::committedLoad):
Allow data through to WebCore for frames with custom content providers;
the only custom content provider currently implemented is main frame PDF
on iOS, which will end up creating a PDFDocument in WebCore, which drops all
data on the floor immediately, so this won't result in WebCore doing anything
with the data, but makes sure that more of the normal document lifecycle is maintained.

In the future, we might want to consider ensuring that all custom content providers
end up creating a SinkDocument or something similarly generic to ensure that
WebCore doesn't try to do anything with their data, but for now, the only client is covered.

* dom/Document.h:
* dom/Document.cpp:
(WebCore::Document::Document):
(WebCore::Document::prepareForDestruction):
Add a flag on Document, m_hasPreparedForDestruction, which ensures
that each Document only goes through prepareForDestruction() once.
prepareForDestruction() can be called a number of times during teardown,
but it's only necessary to actually execute it once.

This was previously achieved by virtue of all callers of prepareForDestruction()
first checking hasLivingRenderTree, and prepareForDestruction() tearing down
the render tree, but that meant that prepareForDestruction() was not called
for Documents who never had a render tree in the first place.

The only part of prepareForDestruction() that is now predicated on hasLivingRenderTree()
is the call to destroyRenderTree(); the rest of the function has the potential to be relevant
for non-rendered placeholder documents and can safely deal with them in other ways.

It is important to call prepareForDestruction() on non-rendered placeholder documents
because some of the cleanup (like disconnectFromFrame()) is critical to safe destruction.

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::clear):
Call prepareForDestruction() even if we don't have a living render tree.
For the sake of minimizing change, removeFocusedNodeOfSubtree still
depends on having a living render tree before calling prepareForDestruction().

* page/Frame.cpp:
(WebCore::Frame::setView):
(WebCore::Frame::setDocument):
Call prepareForDestruction() even if we don't have a living render tree.

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

Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/page/Frame.cpp
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

index 29cba169b5ca53a81b71234e935658e080f3b228..7671842f514dbb0c2314351598dd5e5098e02d70 100644 (file)
@@ -1,3 +1,43 @@
+2014-07-26  Timothy Horton  <timothy_horton@apple.com>
+
+        Crash in Web Content Process under ~PDFDocument under clearTouchEventListeners at topDocument()
+        https://bugs.webkit.org/show_bug.cgi?id=135319
+        <rdar://problem/17315168>
+
+        Reviewed by Darin Adler and Antti Koivisto.
+
+        * dom/Document.h:
+        * dom/Document.cpp:
+        (WebCore::Document::Document):
+        (WebCore::Document::prepareForDestruction):
+        Add a flag on Document, m_hasPreparedForDestruction, which ensures
+        that each Document only goes through prepareForDestruction() once.
+        prepareForDestruction() can be called a number of times during teardown,
+        but it's only necessary to actually execute it once.
+        
+        This was previously achieved by virtue of all callers of prepareForDestruction()
+        first checking hasLivingRenderTree, and prepareForDestruction() tearing down
+        the render tree, but that meant that prepareForDestruction() was not called
+        for Documents who never had a render tree in the first place.
+
+        The only part of prepareForDestruction() that is now predicated on hasLivingRenderTree()
+        is the call to destroyRenderTree(); the rest of the function has the potential to be relevant
+        for non-rendered placeholder documents and can safely deal with them in other ways.
+
+        It is important to call prepareForDestruction() on non-rendered placeholder documents
+        because some of the cleanup (like disconnectFromFrame()) is critical to safe destruction.
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::clear):
+        Call prepareForDestruction() even if we don't have a living render tree.
+        For the sake of minimizing change, removeFocusedNodeOfSubtree still
+        depends on having a living render tree before calling prepareForDestruction().
+
+        * page/Frame.cpp:
+        (WebCore::Frame::setView):
+        (WebCore::Frame::setDocument):
+        Call prepareForDestruction() even if we don't have a living render tree.
+
 2014-07-25  Filip Pizlo  <fpizlo@apple.com>
 
         Merge r170090, r170092, r170129, r170141, r170161, r170215, r170275, r170375, r170376, r170382, r170383, r170399, r170436, r170489, r170490, r170556 from ftlopt.
index 8d666c301315ab307177fc4891df50a594f7b04d..964562fc452c307f79e2e2179c997254c765bade 100644 (file)
@@ -515,6 +515,7 @@ Document::Document(Frame* frame, const URL& url, unsigned documentClasses, unsig
     , m_disabledFieldsetElementsCount(0)
     , m_hasInjectedPlugInsScript(false)
     , m_renderTreeBeingDestroyed(false)
+    , m_hasPreparedForDestruction(false)
     , m_hasStyleWithViewportUnits(false)
 {
     allDocuments().add(this);
@@ -2047,6 +2048,9 @@ void Document::destroyRenderTree()
 
 void Document::prepareForDestruction()
 {
+    if (m_hasPreparedForDestruction)
+        return;
+
 #if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS)
     clearTouchEventListeners();
 #endif
@@ -2055,7 +2059,8 @@ void Document::prepareForDestruction()
     if (m_domWindow && m_frame)
         m_domWindow->willDetachDocumentFromFrame();
 
-    destroyRenderTree();
+    if (hasLivingRenderTree())
+        destroyRenderTree();
 
     if (isPluginDocument())
         toPluginDocument(this)->detachFromPluginElement();
@@ -2087,6 +2092,8 @@ void Document::prepareForDestruction()
         m_mediaQueryMatcher->documentDestroyed();
 
     disconnectFromFrame();
+
+    m_hasPreparedForDestruction = true;
 }
 
 void Document::removeAllEventListeners()
index 794ccd78c037c58762e79139a7d14013f2a1e7ab..0e1ada62ca18203767f98542e2b1aca3e8e7b494 100644 (file)
@@ -1695,6 +1695,7 @@ private:
 
     bool m_hasInjectedPlugInsScript;
     bool m_renderTreeBeingDestroyed;
+    bool m_hasPreparedForDestruction;
 
     bool m_hasStyleWithViewportUnits;
 };
index bbf7add8512c262ad1cc877d6b938662e9fb46ca..e5c8da29743ae9cc29bbbddf596a67477da5008b 100644 (file)
@@ -613,10 +613,10 @@ void FrameLoader::clear(Document* newDocument, bool clearWindowProperties, bool
     if (!m_frame.document()->inPageCache()) {
         m_frame.document()->cancelParsing();
         m_frame.document()->stopActiveDOMObjects();
-        if (m_frame.document()->hasLivingRenderTree()) {
-            m_frame.document()->prepareForDestruction();
+        bool hadLivingRenderTree = m_frame.document()->hasLivingRenderTree();
+        m_frame.document()->prepareForDestruction();
+        if (hadLivingRenderTree)
             m_frame.document()->removeFocusedNodeOfSubtree(m_frame.document());
-        }
     }
 
     // Do this after detaching the document so that the unload event works.
index e09244d42c65aa5db2481c2a0f7670f5b04c417e..fe26dd1e3b7adc64e3a8eb15a2c7684c4f3996d7 100644 (file)
@@ -251,7 +251,7 @@ void Frame::setView(PassRefPtr<FrameView> view)
     // Prepare for destruction now, so any unload event handlers get run and the DOMWindow is
     // notified. If we wait until the view is destroyed, then things won't be hooked up enough for
     // these calls to work.
-    if (!view && m_doc && m_doc->hasLivingRenderTree() && !m_doc->inPageCache())
+    if (!view && m_doc && !m_doc->inPageCache())
         m_doc->prepareForDestruction();
     
     if (m_view)
@@ -271,7 +271,7 @@ void Frame::setDocument(PassRefPtr<Document> newDocument)
 {
     ASSERT(!newDocument || newDocument->frame() == this);
 
-    if (m_doc && m_doc->hasLivingRenderTree() && !m_doc->inPageCache())
+    if (m_doc && !m_doc->inPageCache())
         m_doc->prepareForDestruction();
 
     m_doc = newDocument.get();
index 8e96a7a7bad119c06f2966560d82e5224164f276..7b249b488a98e1d320ac3796d78434c34c297ab0 100644 (file)
@@ -1,3 +1,23 @@
+2014-07-26  Timothy Horton  <timothy_horton@apple.com>
+
+        Crash in Web Content Process under ~PDFDocument under clearTouchEventListeners at topDocument()
+        https://bugs.webkit.org/show_bug.cgi?id=135319
+        <rdar://problem/17315168>
+
+        Reviewed by Darin Adler and Antti Koivisto.
+
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::committedLoad):
+        Allow data through to WebCore for frames with custom content providers;
+        the only custom content provider currently implemented is main frame PDF
+        on iOS, which will end up creating a PDFDocument in WebCore, which drops all
+        data on the floor immediately, so this won't result in WebCore doing anything
+        with the data, but makes sure that more of the normal document lifecycle is maintained.
+
+        In the future, we might want to consider ensuring that all custom content providers
+        end up creating a SinkDocument or something similarly generic to ensure that
+        WebCore doesn't try to do anything with their data, but for now, the only client is covered.
+
 2014-07-25  Jeremy Jones  <jeremyj@apple.com>
 
         Parent fullscreen from window instead of view
index 6dc288c53eed6942796a6b9c8c9c52d4c1cf09cd..bb89358bf367e8378569eccb3f34456141fd09fb 100644 (file)
@@ -887,10 +887,6 @@ void WebFrameLoaderClient::didChangeTitle(DocumentLoader*)
 
 void WebFrameLoaderClient::committedLoad(DocumentLoader* loader, const char* data, int length)
 {
-    // If we're loading a custom representation, we don't want to hand off the data to WebCore.
-    if (m_frameHasCustomContentProvider)
-        return;
-
     if (!m_pluginView)
         loader->commitData(data, length);