media/restore-from-page-cache.html causes NoEventDispatchAssertion::isEventAllowedInM...
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 25 Mar 2017 01:13:23 +0000 (01:13 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 25 Mar 2017 01:13:23 +0000 (01:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=170087
<rdar://problem/31254822>

Reviewed by Simon Fraser.

Reduce the scope of code that should never dispatch DOM events so as to allow updating contents size
after restoring a page from the page cache.

In r214014 we instantiate a NoEventDispatchAssertion in FrameLoader::commitProvisionalLoad()
around the call to CachedPage::restore() to assert when a DOM event is dispatched during
page restoration as such events can cause re-entrancy into the page cache. As it turns out
it is sufficient to ensure that no DOM events are dispatched after restoring all cached frames
as opposed to after CachedPage::restore() returns.

Also rename Document::enqueue{Pageshow, Popstate}Event() to dispatch{Pageshow, Popstate}Event(),
respectively, since they synchronously dispatch events :(. We hope in the future to make them
asynchronously dispatch events.

* dom/Document.cpp:
(WebCore::Document::implicitClose): Update for renaming.
(WebCore::Document::statePopped): Ditto.
(WebCore::Document::dispatchPageshowEvent): Renamed; formerly named enqueuePageshowEvent().
(WebCore::Document::dispatchPopstateEvent): Renamed; formerly named enqueuePopstateEvent().
(WebCore::Document::enqueuePageshowEvent): Deleted.
(WebCore::Document::enqueuePopstateEvent): Deleted.
* dom/Document.h:
* history/CachedPage.cpp:
(WebCore::firePageShowAndPopStateEvents): Moved logic from FrameLoader::didRestoreFromCachedPage() to here.
(WebCore::CachedPage::restore): Modified to call firePageShowAndPopStateEvents().
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::commitProvisionalLoad): Removed use of NoEventDispatchAssertion RAII object. We
will instantiate it in CachedPage::restore() with a smaller scope.
(WebCore::FrameLoader::didRestoreFromCachedPage): Deleted; moved logic from here to WebCore::firePageShowAndPopStateEvents().
* loader/FrameLoader.h:

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

Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/history/CachedPage.cpp
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoader.h

index 89896b8..704ce1a 100644 (file)
@@ -1,3 +1,41 @@
+2017-03-24  Daniel Bates  <dabates@apple.com>
+
+        media/restore-from-page-cache.html causes NoEventDispatchAssertion::isEventAllowedInMainThread() assertion failure
+        https://bugs.webkit.org/show_bug.cgi?id=170087
+        <rdar://problem/31254822>
+
+        Reviewed by Simon Fraser.
+
+        Reduce the scope of code that should never dispatch DOM events so as to allow updating contents size
+        after restoring a page from the page cache.
+
+        In r214014 we instantiate a NoEventDispatchAssertion in FrameLoader::commitProvisionalLoad()
+        around the call to CachedPage::restore() to assert when a DOM event is dispatched during
+        page restoration as such events can cause re-entrancy into the page cache. As it turns out
+        it is sufficient to ensure that no DOM events are dispatched after restoring all cached frames
+        as opposed to after CachedPage::restore() returns.
+
+        Also rename Document::enqueue{Pageshow, Popstate}Event() to dispatch{Pageshow, Popstate}Event(),
+        respectively, since they synchronously dispatch events :(. We hope in the future to make them
+        asynchronously dispatch events.
+
+        * dom/Document.cpp:
+        (WebCore::Document::implicitClose): Update for renaming.
+        (WebCore::Document::statePopped): Ditto.
+        (WebCore::Document::dispatchPageshowEvent): Renamed; formerly named enqueuePageshowEvent().
+        (WebCore::Document::dispatchPopstateEvent): Renamed; formerly named enqueuePopstateEvent().
+        (WebCore::Document::enqueuePageshowEvent): Deleted.
+        (WebCore::Document::enqueuePopstateEvent): Deleted.
+        * dom/Document.h:
+        * history/CachedPage.cpp:
+        (WebCore::firePageShowAndPopStateEvents): Moved logic from FrameLoader::didRestoreFromCachedPage() to here.
+        (WebCore::CachedPage::restore): Modified to call firePageShowAndPopStateEvents().
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::commitProvisionalLoad): Removed use of NoEventDispatchAssertion RAII object. We
+        will instantiate it in CachedPage::restore() with a smaller scope.
+        (WebCore::FrameLoader::didRestoreFromCachedPage): Deleted; moved logic from here to WebCore::firePageShowAndPopStateEvents().
+        * loader/FrameLoader.h:
+
 2017-03-24  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed, rolling out r214361.
index d1a5c3f..62fcd6c 100644 (file)
@@ -2644,9 +2644,9 @@ void Document::implicitClose()
         accessSVGExtensions().dispatchSVGLoadEventToOutermostSVGElements();
 
     dispatchWindowLoadEvent();
-    enqueuePageshowEvent(PageshowEventNotPersisted);
+    dispatchPageshowEvent(PageshowEventNotPersisted);
     if (m_pendingStateObject)
-        enqueuePopstateEvent(WTFMove(m_pendingStateObject));
+        dispatchPopstateEvent(WTFMove(m_pendingStateObject));
 
     if (f)
         f->loader().dispatchOnloadEvents();
@@ -5235,7 +5235,7 @@ void Document::statePopped(Ref<SerializedScriptValue>&& stateObject)
     // Per step 11 of section 6.5.9 (history traversal) of the HTML5 spec, we 
     // defer firing of popstate until we're in the complete state.
     if (m_readyState == Complete)
-        enqueuePopstateEvent(WTFMove(stateObject));
+        dispatchPopstateEvent(WTFMove(stateObject));
     else
         m_pendingStateObject = WTFMove(stateObject);
 }
@@ -5460,7 +5460,7 @@ String Document::displayStringModifiedByEncoding(const String& string) const
     return String { string }.replace('\\', m_decoder->encoding().backslashAsCurrencySymbol());
 }
 
-void Document::enqueuePageshowEvent(PageshowEventPersistence persisted)
+void Document::dispatchPageshowEvent(PageshowEventPersistence persisted)
 {
     // FIXME: https://bugs.webkit.org/show_bug.cgi?id=36334 Pageshow event needs to fire asynchronously.
     dispatchWindowEvent(PageTransitionEvent::create(eventNames().pageshowEvent, persisted), this);
@@ -5471,7 +5471,7 @@ void Document::enqueueHashchangeEvent(const String& oldURL, const String& newURL
     enqueueWindowEvent(HashChangeEvent::create(oldURL, newURL));
 }
 
-void Document::enqueuePopstateEvent(RefPtr<SerializedScriptValue>&& stateObject)
+void Document::dispatchPopstateEvent(RefPtr<SerializedScriptValue>&& stateObject)
 {
     dispatchWindowEvent(PopStateEvent::create(WTFMove(stateObject), m_domWindow ? m_domWindow->history() : nullptr));
 }
index 4c938d7..4b71e6e 100644 (file)
@@ -1058,9 +1058,9 @@ public:
     void enqueueWindowEvent(Ref<Event>&&);
     void enqueueDocumentEvent(Ref<Event>&&);
     void enqueueOverflowEvent(Ref<Event>&&);
-    void enqueuePageshowEvent(PageshowEventPersistence);
+    void dispatchPageshowEvent(PageshowEventPersistence);
     void enqueueHashchangeEvent(const String& oldURL, const String& newURL);
-    void enqueuePopstateEvent(RefPtr<SerializedScriptValue>&& stateObject);
+    void dispatchPopstateEvent(RefPtr<SerializedScriptValue>&& stateObject);
     DocumentEventQueue& eventQueue() const final { return m_eventQueue; }
 
     WEBCORE_EXPORT void addMediaCanStartListener(MediaCanStartListener*);
index 5650abf..3ddd9c1 100644 (file)
 #include "Element.h"
 #include "FocusController.h"
 #include "FrameView.h"
+#include "HistoryController.h"
+#include "HistoryItem.h"
 #include "MainFrame.h"
+#include "NoEventDispatchAssertion.h"
 #include "Node.h"
 #include "Page.h"
+#include "PageTransitionEvent.h"
 #include "Settings.h"
 #include "VisitedLinkState.h"
 #include <wtf/CurrentTime.h>
@@ -69,13 +73,44 @@ CachedPage::~CachedPage()
         m_cachedMainFrame->destroy();
 }
 
+static void firePageShowAndPopStateEvents(Page& page)
+{
+    // Dispatching JavaScript events can cause frame destruction.
+    auto& mainFrame = page.mainFrame();
+    Vector<Ref<Frame>> childFrames;
+    for (auto* child = mainFrame.tree().traverseNextInPostOrderWithWrap(true); child; child = child->tree().traverseNextInPostOrderWithWrap(false))
+        childFrames.append(*child);
+
+    for (auto& child : childFrames) {
+        if (!child->tree().isDescendantOf(&mainFrame))
+            continue;
+        auto* document = child->document();
+        if (!document)
+            continue;
+
+        // FIXME: Update Page Visibility state here.
+        // https://bugs.webkit.org/show_bug.cgi?id=116770
+        document->dispatchPageshowEvent(PageshowEventPersisted);
+
+        auto* historyItem = child->loader().history().currentItem();
+        if (historyItem && historyItem->stateObject())
+            document->dispatchPopstateEvent(historyItem->stateObject());
+    }
+}
+
 void CachedPage::restore(Page& page)
 {
     ASSERT(m_cachedMainFrame);
     ASSERT(m_cachedMainFrame->view()->frame().isMainFrame());
     ASSERT(!page.subframeCount());
 
-    m_cachedMainFrame->open();
+    {
+        // Do not dispatch DOM events as their JavaScript listeners could cause the page to be put
+        // into the page cache before we have finished restoring it from the page cache.
+        NoEventDispatchAssertion noEventDispatchAssertion;
+
+        m_cachedMainFrame->open();
+    }
     
     // Restore the focus appearance for the focused element.
     // FIXME: Right now we don't support pages w/ frames in the b/f cache.  This may need to be tweaked when we add support for that.
@@ -116,6 +151,8 @@ void CachedPage::restore(Page& page)
             frameView->updateContentsSize();
     }
 
+    firePageShowAndPopStateEvents(page);
+
     clear();
 }
 
index e04ea50..bd1e99f 100644 (file)
@@ -86,7 +86,6 @@
 #include "MainFrame.h"
 #include "MemoryCache.h"
 #include "MemoryRelease.h"
-#include "NoEventDispatchAssertion.h"
 #include "Page.h"
 #include "PageCache.h"
 #include "PageTransitionEvent.h"
@@ -1866,19 +1865,11 @@ void FrameLoader::commitProvisionalLoad()
 
         std::optional<HasInsecureContent> hasInsecureContent = cachedPage->cachedMainFrame()->hasInsecureContent();
 
-        {
-            // Do not dispatch DOM events as their JavaScript listeners could cause the page to be put
-            // into the page cache before we have finished restoring it from the page cache.
-            NoEventDispatchAssertion assertNoEventDispatch;
-
-            // FIXME: This API should be turned around so that we ground CachedPage into the Page.
-            cachedPage->restore(*m_frame.page());
-        }
+        // FIXME: This API should be turned around so that we ground CachedPage into the Page.
+        cachedPage->restore(*m_frame.page());
 
         dispatchDidCommitLoad(hasInsecureContent);
 
-        didRestoreFromCachedPage();
-
 #if PLATFORM(IOS)
         m_frame.page()->chrome().setDispatchViewportDataDidChangeSuppressed(false);
         m_frame.page()->chrome().dispatchViewportPropertiesDidChange(m_frame.page()->viewportArguments());
@@ -2112,31 +2103,6 @@ void FrameLoader::willRestoreFromCachedPage()
     }
 }
 
-void FrameLoader::didRestoreFromCachedPage()
-{
-    // Dispatching JavaScript events can cause frame destruction.
-    auto& mainFrame = m_frame.page()->mainFrame();
-    Vector<Ref<Frame>> childFrames;
-    for (auto* child = mainFrame.tree().traverseNextInPostOrderWithWrap(true); child; child = child->tree().traverseNextInPostOrderWithWrap(false))
-        childFrames.append(*child);
-
-    for (auto& child : childFrames) {
-        if (!child->tree().isDescendantOf(&mainFrame))
-            continue;
-        auto* document = child->document();
-        if (!document)
-            continue;
-
-        // FIXME: Update Page Visibility state here.
-        // https://bugs.webkit.org/show_bug.cgi?id=116770
-        document->enqueuePageshowEvent(PageshowEventPersisted);
-
-        auto* historyItem = child->loader().history().currentItem();
-        if (historyItem && historyItem->stateObject())
-            document->enqueuePopstateEvent(historyItem->stateObject());
-    }
-}
-
 void FrameLoader::open(CachedFrameBase& cachedFrame)
 {
     m_isComplete = false;
index fd1bb15..7b47f94 100644 (file)
@@ -347,7 +347,6 @@ private:
 
     void closeOldDataSources();
     void willRestoreFromCachedPage();
-    void didRestoreFromCachedPage();
 
     bool shouldReloadToHandleUnreachableURL(DocumentLoader*);