Changeset 214392 in webkit

Timestamp:

Mar 24, 2017 6:13:23 PM (7 years ago)

Author:

dbates@webkit.org

Message:

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:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Document.cpp (modified) (4 diffs)
• dom/Document.h (modified) (1 diff)
• history/CachedPage.cpp (modified) (4 diffs)
• loader/FrameLoader.cpp (modified) (3 diffs)
• loader/FrameLoader.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -2645 +2645 @@
 
     dispatchWindowLoadEvent();
-    enqueuePageshowEvent(PageshowEventNotPersisted);
+    dispatchPageshowEvent(PageshowEventNotPersisted);
     if (m_pendingStateObject)
-        enqueuePopstateEvent(WTFMove(m_pendingStateObject));
+        dispatchPopstateEvent(WTFMove(m_pendingStateObject));
 
     if (f)
@@ -5236 +5236 @@
     // 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);
@@ -5461 +5461 @@
 }
 
-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.
@@ -5472 +5472 @@
 }
 
-void Document::enqueuePopstateEvent(RefPtr<SerializedScriptValue>&& stateObject)
+void Document::dispatchPopstateEvent(RefPtr<SerializedScriptValue>&& stateObject)
 {
     dispatchWindowEvent(PopStateEvent::create(WTFMove(stateObject), m_domWindow ? m_domWindow->history() : nullptr));

trunk/Source/WebCore/dom/Document.h

@@ -1059 +1059 @@
     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; }
 

trunk/Source/WebCore/history/CachedPage.cpp

@@ -31 +31 @@
 #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"
@@ -70 +74 @@
 }
 
+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)
 {
@@ -76 +105 @@
     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.
@@ -117 +152 @@
     }
 
+    firePageShowAndPopStateEvents(page);
+
     clear();
 }

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -87 +87 @@
 #include "MemoryCache.h"
 #include "MemoryRelease.h"
-#include "NoEventDispatchAssertion.h"
 #include "Page.h"
 #include "PageCache.h"
@@ -1867 +1866 @@
         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)
@@ -2110 +2101 @@
         window->setStatus(String());
         window->setDefaultStatus(String());
-    }
-}
-
-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());
     }
 }

trunk/Source/WebCore/loader/FrameLoader.h

@@ -348 +348 @@
     void closeOldDataSources();
     void willRestoreFromCachedPage();
-    void didRestoreFromCachedPage();
 
     bool shouldReloadToHandleUnreachableURL(DocumentLoader*);