It is possible for Document::m_frame pointer to become stale
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 10 Sep 2016 16:06:05 +0000 (16:06 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 10 Sep 2016 16:06:05 +0000 (16:06 +0000)
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.

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

LayoutTests/ChangeLog
LayoutTests/fast/history/pagehide-remove-iframe-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/history/pagehide-remove-iframe-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/history/CachedFrame.cpp
Source/WebCore/history/PageCache.cpp
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/HistoryController.cpp
Source/WebCore/page/Frame.cpp

index bde6ba2..cf96e83 100644 (file)
@@ -1,3 +1,17 @@
+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>
 
         [EFL] Mark new media source tests to failure
diff --git a/LayoutTests/fast/history/pagehide-remove-iframe-crash-expected.txt b/LayoutTests/fast/history/pagehide-remove-iframe-crash-expected.txt
new file mode 100644 (file)
index 0000000..d275649
--- /dev/null
@@ -0,0 +1,13 @@
+Tests that we do not crash when deleting a subframe from the pagehide event handler.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+pageshow - not from cache
+pagehide - entering cache
+pageshow - from cache
+PASS Page did enter and was restored from the page cache
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/history/pagehide-remove-iframe-crash.html b/LayoutTests/fast/history/pagehide-remove-iframe-crash.html
new file mode 100644 (file)
index 0000000..c429b36
--- /dev/null
@@ -0,0 +1,50 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<iframe srcdoc="<body></body>"></iframe>
+<script>
+description("Tests that we do not crash when deleting a subframe from the pagehide event handler.");
+jsTestIsAsync = true;
+if (window.testRunner)
+    testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
+
+window.addEventListener("pageshow", function(event) {
+    debug("pageshow - " + (event.persisted ? "" : "not ") + "from cache");
+
+    if (event.persisted) {
+        testPassed("Page did enter and was restored from the page cache");
+        finishJSTest();
+    }
+}, false);
+
+window.addEventListener("pagehide", function(event) {
+    debug("pagehide - " + (event.persisted ? "" : "not ") + "entering cache");
+    if (!event.persisted) {
+        testFailed("Page did not enter the page cache.");
+        finishJSTest();
+    }
+    // Remove subframe in pagehide event handler.
+    var frame = document.getElementsByTagName("iframe")[0];
+    subFrameDoc = frame.contentDocument;
+    document.body.removeChild(frame);
+    frame = null;
+    gc();
+}, false);
+
+onload = function() {
+   var frame = document.getElementsByTagName("iframe")[0];
+   frame.addEventListener("touchstart", function() { });
+   frame.addEventListener("click", function() { });
+   frame.contentDocument.body.addEventListener("touchstart", function() { });
+   frame.contentDocument.body.addEventListener("click", function() { });
+
+   setTimeout(function() {
+       // Force a back navigation back to this page.
+       window.location.href = "resources/page-cache-helper.html";
+   }, 0);
+}
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
index 0100a79..58b303c 100644 (file)
@@ -1,3 +1,70 @@
+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>
 
         Apple.com keynote does not display media controls
index e9141a5..d3193dc 100644 (file)
@@ -493,7 +493,6 @@ Document::Document(Frame* frame, const URL& url, unsigned documentClasses, unsig
     , m_annotatedRegionsDirty(false)
 #endif
     , m_createRenderers(true)
-    , m_inPageCache(false)
     , m_accessKeyMapValid(false)
     , m_documentClasses(documentClasses)
     , m_isSynthesized(constructionFlags & Synthesized)
@@ -602,7 +601,7 @@ Document::~Document()
     allDocuments().remove(this);
 
     ASSERT(!renderView());
-    ASSERT(!m_inPageCache);
+    ASSERT(m_pageCacheState != InPageCache);
     ASSERT(m_ranges.isEmpty());
     ASSERT(!m_parentTreeScope);
     ASSERT(!m_disabledFieldsetElementsCount);
@@ -2237,7 +2236,7 @@ void Document::clearStyleResolver()
 void Document::createRenderTree()
 {
     ASSERT(!renderView());
-    ASSERT(!m_inPageCache);
+    ASSERT(m_pageCacheState != InPageCache);
     ASSERT(!m_axObjectCache || this != &topDocument());
 
     if (m_isNonRenderedPlaceholder)
@@ -2301,7 +2300,7 @@ void Document::disconnectFromFrame()
 void Document::destroyRenderTree()
 {
     ASSERT(hasLivingRenderTree());
-    ASSERT(!m_inPageCache);
+    ASSERT(m_pageCacheState != InPageCache);
 
     TemporaryChange<bool> change(m_renderTreeBeingDestroyed, true);
 
@@ -3787,7 +3786,7 @@ bool Document::setFocusedElement(Element* element, FocusDirection direction, Foc
     if (m_focusedElement == newFocusedElement)
         return true;
 
-    if (m_inPageCache)
+    if (inPageCache())
         return false;
 
     bool focusChangeBlocked = false;
@@ -4715,17 +4714,18 @@ URL Document::completeURL(const String& url) const
     return completeURL(url, m_baseURL);
 }
 
-void Document::setInPageCache(bool flag)
+void Document::setPageCacheState(PageCacheState state)
 {
-    if (m_inPageCache == flag)
+    if (m_pageCacheState == state)
         return;
 
-    m_inPageCache = flag;
+    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
             // page cache and similar work that needs to occur when it comes out. This is where we do the work
@@ -4745,9 +4745,13 @@ void Document::setInPageCache(bool flag)
         m_styleRecalcTimer.stop();
 
         clearSharedObjectPool();
-    } else {
+        break;
+    case NotInPageCache:
         if (childNeedsStyleRecalc())
             scheduleStyleRecalc();
+        break;
+    case AboutToEnterPageCache:
+        break;
     }
 }
 
@@ -5066,7 +5070,7 @@ Document& Document::topDocument() const
 {
     // 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);
         // This should always be non-null.
index 99b95d4..5407f32 100644 (file)
@@ -1001,8 +1001,13 @@ public:
 
     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
     // "resume()" callbacks
@@ -1593,7 +1598,7 @@ private:
     HashMap<String, RefPtr<HTMLCanvasElement>> m_cssCanvasElements;
 
     bool m_createRenderers;
-    bool m_inPageCache;
+    PageCacheState m_pageCacheState { NotInPageCache };
 
     HashSet<Element*> m_documentSuspensionCallbackElements;
     HashSet<Element*> m_mediaVolumeCallbackElements;
index 2cec611..4b686d3 100644 (file)
@@ -264,7 +264,7 @@ void CachedFrame::destroy()
     // fully anyway, because the document won't be able to access its DOMWindow object (due to being frameless).
     m_document->removeAllEventListeners();
 
-    m_document->setInPageCache(false);
+    m_document->setPageCacheState(Document::NotInPageCache);
     m_document->prepareForDestruction();
 
     clear();
index d9c5c7e..6cd5668 100644 (file)
@@ -373,11 +373,11 @@ static String pruningReasonToDiagnosticLoggingKey(PruningReason pruningReason)
     return emptyString();
 }
 
-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);
     }
 }
 
@@ -407,8 +407,7 @@ void PageCache::addIfCacheable(HistoryItem& item, Page* page)
     if (!page || !canCache(*page))
         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,
     // before the page enters the page cache, while we still can dispatch DOM blur/focus events.
@@ -421,10 +420,12 @@ void PageCache::addIfCacheable(HistoryItem& item, Page* page)
     // Check that the page is still page-cacheable after firing the pagehide event. The JS event handlers
     // could have altered the page in a way that could prevent caching.
     if (!canCache(*page)) {
-        setInPageCache(*page, false);
+        setPageCacheState(*page, Document::NotInPageCache);
         return;
     }
 
+    setPageCacheState(*page, Document::InPageCache);
+
     // Make sure we no longer fire any JS events past this point.
     NoEventDispatchAssertion assertNoEventDispatch;
 
index e547938..df952b6 100644 (file)
@@ -1596,7 +1596,7 @@ void FrameLoader::reload(bool endToEndReload, bool contentBlockersEnabled)
 
 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;
 
@@ -2094,7 +2094,7 @@ void FrameLoader::open(CachedFrameBase& cachedFrame)
 
     clear(document, true, true, cachedFrame.isMainFrame());
 
-    document->setInPageCache(false);
+    document->setPageCacheState(Document::NotInPageCache);
 
     m_needsClear = true;
     m_isComplete = false;
index 18efa4f..a2c6234 100644 (file)
@@ -270,7 +270,7 @@ void HistoryController::invalidateCurrentItemCachedPage()
     
     ASSERT(cachedPage->document() == m_frame.document());
     if (cachedPage->document() == m_frame.document()) {
-        cachedPage->document()->setInPageCache(false);
+        cachedPage->document()->setPageCacheState(Document::NotInPageCache);
         cachedPage->clear();
     }
 }
index b1c7cd5..0415388 100644 (file)
@@ -246,7 +246,7 @@ void Frame::setView(RefPtr<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->inPageCache())
+    if (!view && m_doc && m_doc->pageCacheState() != Document::InPageCache)
         m_doc->prepareForDestruction();
     
     if (m_view)
@@ -268,7 +268,7 @@ void Frame::setDocument(RefPtr<Document>&& newDocument)
 {
     ASSERT(!newDocument || newDocument->frame() == this);
 
-    if (m_doc && !m_doc->inPageCache())
+    if (m_doc && m_doc->pageCacheState() != Document::InPageCache)
         m_doc->prepareForDestruction();
 
     m_doc = newDocument.copyRef();