Diff
Modified: branches/safari-602-branch/LayoutTests/ChangeLog (207206 => 207207)
--- branches/safari-602-branch/LayoutTests/ChangeLog 2016-10-12 08:41:35 UTC (rev 207206)
+++ branches/safari-602-branch/LayoutTests/ChangeLog 2016-10-12 08:41:40 UTC (rev 207207)
@@ -1,5 +1,23 @@
2016-10-12 Matthew Hanson <matthew_han...@apple.com>
+ Merge r205786. rdar://problem/28476956
+
+ 2016-09-10 Chris Dumez <cdu...@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-10-12 Matthew Hanson <matthew_han...@apple.com>
+
Merge r205190. rdar://problem/28545010
2016-08-30 Youenn Fablet <you...@apple.com>
Added: branches/safari-602-branch/LayoutTests/fast/history/pagehide-remove-iframe-crash-expected.txt (0 => 207207)
--- branches/safari-602-branch/LayoutTests/fast/history/pagehide-remove-iframe-crash-expected.txt (rev 0)
+++ branches/safari-602-branch/LayoutTests/fast/history/pagehide-remove-iframe-crash-expected.txt 2016-10-12 08:41:40 UTC (rev 207207)
@@ -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
+
Added: branches/safari-602-branch/LayoutTests/fast/history/pagehide-remove-iframe-crash.html (0 => 207207)
--- branches/safari-602-branch/LayoutTests/fast/history/pagehide-remove-iframe-crash.html (rev 0)
+++ branches/safari-602-branch/LayoutTests/fast/history/pagehide-remove-iframe-crash.html 2016-10-12 08:41:40 UTC (rev 207207)
@@ -0,0 +1,50 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<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 = ""
+ }, 0);
+}
+</script>
+<script src=""
+</body>
+</html>
Modified: branches/safari-602-branch/Source/WebCore/ChangeLog (207206 => 207207)
--- branches/safari-602-branch/Source/WebCore/ChangeLog 2016-10-12 08:41:35 UTC (rev 207206)
+++ branches/safari-602-branch/Source/WebCore/ChangeLog 2016-10-12 08:41:40 UTC (rev 207207)
@@ -1,5 +1,76 @@
2016-10-12 Matthew Hanson <matthew_han...@apple.com>
+ Merge r205786. rdar://problem/28476956
+
+ 2016-09-10 Chris Dumez <cdu...@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-10-12 Matthew Hanson <matthew_han...@apple.com>
+
Merge r205197. rdar://problem/28481424
2016-08-30 Brent Fulgham <bfulg...@apple.com>
Modified: branches/safari-602-branch/Source/WebCore/dom/Document.cpp (207206 => 207207)
--- branches/safari-602-branch/Source/WebCore/dom/Document.cpp 2016-10-12 08:41:35 UTC (rev 207206)
+++ branches/safari-602-branch/Source/WebCore/dom/Document.cpp 2016-10-12 08:41:40 UTC (rev 207207)
@@ -492,7 +492,6 @@
, m_annotatedRegionsDirty(false)
#endif
, m_createRenderers(true)
- , m_inPageCache(false)
, m_accessKeyMapValid(false)
, m_documentClasses(documentClasses)
, m_isSynthesized(constructionFlags & Synthesized)
@@ -601,7 +600,7 @@
allDocuments().remove(this);
ASSERT(!renderView());
- ASSERT(!m_inPageCache);
+ ASSERT(m_pageCacheState != InPageCache);
ASSERT(m_ranges.isEmpty());
ASSERT(!m_parentTreeScope);
ASSERT(!m_disabledFieldsetElementsCount);
@@ -2240,7 +2239,7 @@
void Document::createRenderTree()
{
ASSERT(!renderView());
- ASSERT(!m_inPageCache);
+ ASSERT(m_pageCacheState != InPageCache);
ASSERT(!m_axObjectCache || this != &topDocument());
if (m_isNonRenderedPlaceholder)
@@ -2304,7 +2303,7 @@
void Document::destroyRenderTree()
{
ASSERT(hasLivingRenderTree());
- ASSERT(!m_inPageCache);
+ ASSERT(m_pageCacheState != InPageCache);
TemporaryChange<bool> change(m_renderTreeBeingDestroyed, true);
@@ -3791,7 +3790,7 @@
if (m_focusedElement == newFocusedElement)
return true;
- if (m_inPageCache)
+ if (inPageCache())
return false;
bool focusChangeBlocked = false;
@@ -4710,17 +4709,18 @@
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
@@ -4740,9 +4740,13 @@
m_styleRecalcTimer.stop();
clearSharedObjectPool();
- } else {
+ break;
+ case NotInPageCache:
if (childNeedsStyleRecalc())
scheduleStyleRecalc();
+ break;
+ case AboutToEnterPageCache:
+ break;
}
}
@@ -5044,7 +5048,7 @@
{
// 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.
Modified: branches/safari-602-branch/Source/WebCore/dom/Document.h (207206 => 207207)
--- branches/safari-602-branch/Source/WebCore/dom/Document.h 2016-10-12 08:41:35 UTC (rev 207206)
+++ branches/safari-602-branch/Source/WebCore/dom/Document.h 2016-10-12 08:41:40 UTC (rev 207207)
@@ -1001,9 +1001,14 @@
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
void registerForDocumentSuspensionCallbacks(Element*);
@@ -1599,7 +1604,7 @@
HashMap<String, RefPtr<HTMLCanvasElement>> m_cssCanvasElements;
bool m_createRenderers;
- bool m_inPageCache;
+ PageCacheState m_pageCacheState { NotInPageCache };
HashSet<Element*> m_documentSuspensionCallbackElements;
HashSet<Element*> m_mediaVolumeCallbackElements;
Modified: branches/safari-602-branch/Source/WebCore/history/CachedFrame.cpp (207206 => 207207)
--- branches/safari-602-branch/Source/WebCore/history/CachedFrame.cpp 2016-10-12 08:41:35 UTC (rev 207206)
+++ branches/safari-602-branch/Source/WebCore/history/CachedFrame.cpp 2016-10-12 08:41:40 UTC (rev 207207)
@@ -264,7 +264,7 @@
// 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();
Modified: branches/safari-602-branch/Source/WebCore/history/PageCache.cpp (207206 => 207207)
--- branches/safari-602-branch/Source/WebCore/history/PageCache.cpp 2016-10-12 08:41:35 UTC (rev 207206)
+++ branches/safari-602-branch/Source/WebCore/history/PageCache.cpp 2016-10-12 08:41:40 UTC (rev 207207)
@@ -373,11 +373,11 @@
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 @@
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 @@
// 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;
Modified: branches/safari-602-branch/Source/WebCore/loader/FrameLoader.cpp (207206 => 207207)
--- branches/safari-602-branch/Source/WebCore/loader/FrameLoader.cpp 2016-10-12 08:41:35 UTC (rev 207206)
+++ branches/safari-602-branch/Source/WebCore/loader/FrameLoader.cpp 2016-10-12 08:41:40 UTC (rev 207207)
@@ -1600,7 +1600,7 @@
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;
@@ -2100,7 +2100,7 @@
clear(document, true, true, cachedFrame.isMainFrame());
- document->setInPageCache(false);
+ document->setPageCacheState(Document::NotInPageCache);
m_needsClear = true;
m_isComplete = false;
Modified: branches/safari-602-branch/Source/WebCore/loader/HistoryController.cpp (207206 => 207207)
--- branches/safari-602-branch/Source/WebCore/loader/HistoryController.cpp 2016-10-12 08:41:35 UTC (rev 207206)
+++ branches/safari-602-branch/Source/WebCore/loader/HistoryController.cpp 2016-10-12 08:41:40 UTC (rev 207207)
@@ -270,7 +270,7 @@
ASSERT(cachedPage->document() == m_frame.document());
if (cachedPage->document() == m_frame.document()) {
- cachedPage->document()->setInPageCache(false);
+ cachedPage->document()->setPageCacheState(Document::NotInPageCache);
cachedPage->clear();
}
}
Modified: branches/safari-602-branch/Source/WebCore/page/Frame.cpp (207206 => 207207)
--- branches/safari-602-branch/Source/WebCore/page/Frame.cpp 2016-10-12 08:41:35 UTC (rev 207206)
+++ branches/safari-602-branch/Source/WebCore/page/Frame.cpp 2016-10-12 08:41:40 UTC (rev 207207)
@@ -245,7 +245,7 @@
// 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)
@@ -267,7 +267,7 @@
{
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();
