Changeset 205786 in webkit

Timestamp:

Sep 10, 2016 9:06:05 AM (8 years ago)

Author:

Chris Dumez

Message:

It is possible for Document::m_frame pointer to become stale
​https://bugs.webkit.org/show_bug.cgi?id=161812
<rdar://problem/27745023>

Reviewed by Ryosuke Niwa.

Source/WebCore:

Document::m_frame is supposed to get cleared by Document::prepareForDestruction().
The Frame destructor calls Frame::setView(nullptr) which is supposed to call the
prepareForDestruction() on the Frame's associated document. However,
Frame::setView(nullptr) was calling prepareForDestruction() only if
Document::inPageCache() returned true. This is because, we allow Documents to
stay alive in the PageCache even though they don't have a frame.

The issue is that Document::m_inPageCache flag was set to true right before
firing the pagehide event, so technically before really entering PageCache.
Therefore, we can run into problems if a Frame gets destroyed by a pagehide
EventHandler because ~Frame() will not call Document::prepareForDestruction()
due to Document::m_inPageCache being true. After the frame is destroyed,
Document::m_frame becomes stale and any action on the document will likely
lead to crashes (such as the one in the layout test and the radar which
happens when trying to unregister event listeners from the document).

The solution adopted in this patch is to replace the m_inPageCache boolean
with a m_pageCacheState enumeration that has 3 states:

• NotInPageCache
• AboutToEnterPageCache
• InPageCache

Frame::setView() / Frame::setDocument() were then updated to call
Document::prepareForDestruction() on the associated document whenever
the document's pageCacheState is not InPageCache. This means that we
will now call Document::prepareForDestruction() when the document is
being detached from its frame while firing the pagehide event.

Note that I tried to keep this patch minimal. Therefore, I kept
the Document::inPageCache() getter for now. I plan to switch all its
calls sites to the new Document::pageCacheState() getter in a follow-up
patch so that we can finally drop the confusing Document::inPageCache().

Test: fast/history/pagehide-remove-iframe-crash.html

• dom/Document.cpp:

(WebCore::Document::Document):
(WebCore::Document::~Document):
(WebCore::Document::createRenderTree):
(WebCore::Document::destroyRenderTree):
(WebCore::Document::setFocusedElement):
(WebCore::Document::setPageCacheState):
(WebCore::Document::topDocument):

• dom/Document.h:

(WebCore::Document::pageCacheState):
(WebCore::Document::inPageCache):

• history/CachedFrame.cpp:

(WebCore::CachedFrame::destroy):

• history/PageCache.cpp:

(WebCore::setPageCacheState):
(WebCore::PageCache::addIfCacheable):

• loader/FrameLoader.cpp:

(WebCore::FrameLoader::stopAllLoaders):
(WebCore::FrameLoader::open):

• loader/HistoryController.cpp:

(WebCore::HistoryController::invalidateCurrentItemCachedPage):

• page/Frame.cpp:

(WebCore::Frame::setView):

LayoutTests:

Add layout test that crashes on both Mac and iOS due to using a stale
Document::m_frame pointer.

• fast/history/pagehide-remove-iframe-crash-expected.txt: Added.
• fast/history/pagehide-remove-iframe-crash.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/history/pagehide-remove-iframe-crash-expected.txt (added)
• LayoutTests/fast/history/pagehide-remove-iframe-crash.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.cpp (modified) (8 diffs)
• Source/WebCore/dom/Document.h (modified) (2 diffs)
• Source/WebCore/history/CachedFrame.cpp (modified) (1 diff)
• Source/WebCore/history/PageCache.cpp (modified) (3 diffs)
• Source/WebCore/loader/FrameLoader.cpp (modified) (2 diffs)
• Source/WebCore/loader/HistoryController.cpp (modified) (1 diff)
• Source/WebCore/page/Frame.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2016-09-10  Chris Dumez  <cdumez@apple.com>
+
+        It is possible for Document::m_frame pointer to become stale
+        https://bugs.webkit.org/show_bug.cgi?id=161812
+        <rdar://problem/27745023>
+
+        Reviewed by Ryosuke Niwa.
+
+        Add layout test that crashes on both Mac and iOS due to using a stale
+        Document::m_frame pointer.
+
+        * fast/history/pagehide-remove-iframe-crash-expected.txt: Added.
+        * fast/history/pagehide-remove-iframe-crash.html: Added.
+
 2016-09-10  Gyuyoung Kim  <gyuyoung.kim@webkit.org>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-09-10  Chris Dumez  <cdumez@apple.com>
+
+        It is possible for Document::m_frame pointer to become stale
+        https://bugs.webkit.org/show_bug.cgi?id=161812
+        <rdar://problem/27745023>
+
+        Reviewed by Ryosuke Niwa.
+
+        Document::m_frame is supposed to get cleared by Document::prepareForDestruction().
+        The Frame destructor calls Frame::setView(nullptr) which is supposed to call the
+        prepareForDestruction() on the Frame's associated document. However,
+        Frame::setView(nullptr) was calling prepareForDestruction() only if
+        Document::inPageCache() returned true. This is because, we allow Documents to
+        stay alive in the PageCache even though they don't have a frame.
+
+        The issue is that Document::m_inPageCache flag was set to true right before
+        firing the pagehide event, so technically before really entering PageCache.
+        Therefore, we can run into problems if a Frame gets destroyed by a pagehide
+        EventHandler because ~Frame() will not call Document::prepareForDestruction()
+        due to Document::m_inPageCache being true. After the frame is destroyed,
+        Document::m_frame becomes stale and any action on the document will likely
+        lead to crashes (such as the one in the layout test and the radar which
+        happens when trying to unregister event listeners from the document).
+
+        The solution adopted in this patch is to replace the m_inPageCache boolean
+        with a m_pageCacheState enumeration that has 3 states:
+        - NotInPageCache
+        - AboutToEnterPageCache
+        - InPageCache
+
+        Frame::setView() / Frame::setDocument() were then updated to call
+        Document::prepareForDestruction() on the associated document whenever
+        the document's pageCacheState is not InPageCache. This means that we
+        will now call Document::prepareForDestruction() when the document is
+        being detached from its frame while firing the pagehide event.
+
+        Note that I tried to keep this patch minimal. Therefore, I kept
+        the Document::inPageCache() getter for now. I plan to switch all its
+        calls sites to the new Document::pageCacheState() getter in a follow-up
+        patch so that we can finally drop the confusing Document::inPageCache().
+
+        Test: fast/history/pagehide-remove-iframe-crash.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::Document):
+        (WebCore::Document::~Document):
+        (WebCore::Document::createRenderTree):
+        (WebCore::Document::destroyRenderTree):
+        (WebCore::Document::setFocusedElement):
+        (WebCore::Document::setPageCacheState):
+        (WebCore::Document::topDocument):
+        * dom/Document.h:
+        (WebCore::Document::pageCacheState):
+        (WebCore::Document::inPageCache):
+        * history/CachedFrame.cpp:
+        (WebCore::CachedFrame::destroy):
+        * history/PageCache.cpp:
+        (WebCore::setPageCacheState):
+        (WebCore::PageCache::addIfCacheable):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::stopAllLoaders):
+        (WebCore::FrameLoader::open):
+        * loader/HistoryController.cpp:
+        (WebCore::HistoryController::invalidateCurrentItemCachedPage):
+        * page/Frame.cpp:
+        (WebCore::Frame::setView):
+
 2016-09-10  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -494 +494 @@
 #endif
     , m_createRenderers(true)
-    , m_inPageCache(false)
     , m_accessKeyMapValid(false)
     , m_documentClasses(documentClasses)
@@ -603 +602 @@
 
     ASSERT(!renderView());
-    ASSERT(!m_inPageCache);
+    ASSERT(m_pageCacheState != InPageCache);
     ASSERT(m_ranges.isEmpty());
     ASSERT(!m_parentTreeScope);
@@ -2238 +2237 @@
 {
     ASSERT(!renderView());
-    ASSERT(!m_inPageCache);
+    ASSERT(m_pageCacheState != InPageCache);
     ASSERT(!m_axObjectCache || this != &topDocument());
 
@@ -2302 +2301 @@
 {
     ASSERT(hasLivingRenderTree());
-    ASSERT(!m_inPageCache);
+    ASSERT(m_pageCacheState != InPageCache);
 
     TemporaryChange<bool> change(m_renderTreeBeingDestroyed, true);
@@ -3788 +3787 @@
         return true;
 
-    if (m_inPageCache)
+    if (inPageCache())
         return false;
 
@@ -4716 +4715 @@
 }
 
-void Document::setInPageCache(bool flag)
-{
-    if (m_inPageCache == flag)
-        return;
-
-    m_inPageCache = flag;
+void Document::setPageCacheState(PageCacheState state)
+{
+    if (m_pageCacheState == state)
+        return;
+
+    m_pageCacheState = state;
 
     FrameView* v = view();
     Page* page = this->page();
 
-    if (flag) {
+    switch (state) {
+    case InPageCache:
         if (v) {
             // FIXME: There is some scrolling related work that needs to happen whenever a page goes into the
@@ -4746 +4746 @@
 
         clearSharedObjectPool();
-    } else {
+        break;
+    case NotInPageCache:
         if (childNeedsStyleRecalc())
             scheduleStyleRecalc();
+        break;
+    case AboutToEnterPageCache:
+        break;
     }
 }
@@ -5067 +5071 @@
     // FIXME: This special-casing avoids incorrectly determined top documents during the process
     // of AXObjectCache teardown or notification posting for cached or being-destroyed documents.
-    if (!m_inPageCache && !m_renderTreeBeingDestroyed) {
+    if (!inPageCache() && !m_renderTreeBeingDestroyed) {
         if (!m_frame)
             return const_cast<Document&>(*this);

trunk/Source/WebCore/dom/Document.h

@@ -1002 +1002 @@
     void finishedParsing();
 
-    bool inPageCache() const { return m_inPageCache; }
-    void setInPageCache(bool flag);
+    enum PageCacheState { NotInPageCache, AboutToEnterPageCache, InPageCache };
+
+    PageCacheState pageCacheState() const { return m_pageCacheState; }
+    void setPageCacheState(PageCacheState);
+
+    // FIXME: Update callers to use pageCacheState() instead.
+    bool inPageCache() const { return m_pageCacheState != NotInPageCache; }
 
     // Elements can register themselves for the "suspend()" and
@@ -1594 +1599 @@
 
     bool m_createRenderers;
-    bool m_inPageCache;
+    PageCacheState m_pageCacheState { NotInPageCache };
 
     HashSet<Element*> m_documentSuspensionCallbackElements;

trunk/Source/WebCore/history/CachedFrame.cpp

@@ -265 +265 @@
     m_document->removeAllEventListeners();
 
-    m_document->setInPageCache(false);
+    m_document->setPageCacheState(Document::NotInPageCache);
     m_document->prepareForDestruction();
 

trunk/Source/WebCore/history/PageCache.cpp

@@ -374 +374 @@
 }
 
-static void setInPageCache(Page& page, bool isInPageCache)
+static void setPageCacheState(Page& page, Document::PageCacheState pageCacheState)
 {
     for (Frame* frame = &page.mainFrame(); frame; frame = frame->tree().traverseNext()) {
         if (auto* document = frame->document())
-            document->setInPageCache(isInPageCache);
+            document->setPageCacheState(pageCacheState);
     }
 }
@@ -408 +408 @@
         return;
 
-    // Make sure all the documents know they are being added to the PageCache.
-    setInPageCache(*page, true);
+    setPageCacheState(*page, Document::AboutToEnterPageCache);
 
     // Focus the main frame, defocusing a focused subframe (if we have one). We do this here,
@@ -422 +421 @@
     // could have altered the page in a way that could prevent caching.
     if (!canCache(*page)) {
-        setInPageCache(*page, false);
-        return;
-    }
+        setPageCacheState(*page, Document::NotInPageCache);
+        return;
+    }
+
+    setPageCacheState(*page, Document::InPageCache);
 
     // Make sure we no longer fire any JS events past this point.

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -1597 +1597 @@
 void FrameLoader::stopAllLoaders(ClearProvisionalItemPolicy clearProvisionalItemPolicy)
 {
-    ASSERT(!m_frame.document() || !m_frame.document()->inPageCache());
+    ASSERT(!m_frame.document() || m_frame.document()->pageCacheState() != Document::InPageCache);
     if (m_pageDismissalEventBeingDispatched != PageDismissalType::None)
         return;
@@ -2095 +2095 @@
     clear(document, true, true, cachedFrame.isMainFrame());
 
-    document->setInPageCache(false);
+    document->setPageCacheState(Document::NotInPageCache);
 
     m_needsClear = true;

trunk/Source/WebCore/loader/HistoryController.cpp

@@ -271 +271 @@
     ASSERT(cachedPage->document() == m_frame.document());
     if (cachedPage->document() == m_frame.document()) {
-        cachedPage->document()->setInPageCache(false);
+        cachedPage->document()->setPageCacheState(Document::NotInPageCache);
         cachedPage->clear();
     }

trunk/Source/WebCore/page/Frame.cpp

@@ -247 +247 @@
     // 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->inPageCache())
+    if (!view && m_doc && m_doc->pageCacheState() != Document::InPageCache)
         m_doc->prepareForDestruction();
     
@@ -269 +269 @@
     ASSERT(!newDocument || newDocument->frame() == this);
 
-    if (m_doc && !m_doc->inPageCache())
+    if (m_doc && m_doc->pageCacheState() != Document::InPageCache)
         m_doc->prepareForDestruction();