Revert to dispatching the popstate event synchronously
authoraestes@apple.com <aestes@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Feb 2016 09:29:44 +0000 (09:29 +0000)
committeraestes@apple.com <aestes@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Feb 2016 09:29:44 +0000 (09:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=153297
rdar://problem/24092294

Reviewed by Brent Fulgham.

Source/WebCore:

r192369 made the popstate event dispatch asynchronously, which matches what the HTML5 spec says to do. However,
due to compatibility regressions we need to revert back to dispatching synchronously. This change reverts
r192369's changes to Document.cpp, but retains the new tests.

Firing popstate synchronously makes both fast/loader/remove-iframe-during-history-navigation-different.html and
fast/loader/remove-iframe-during-history-navigation-same.html crash, because their onpopstate handlers remove
frames from the document that will later be accessed by HistoryController::recursiveGoToItem().

To prevent the crashes, this change does two things:
1. Keep a reference to the current frame inside FrameLoader::loadSameDocumentItem(), since calling
   loadInSameDocument() might otherwise delete it.
2. Handle a null frame when iterating a HistoryItem's child frames in HistoryController::recursiveGoToItem(),
   since calling goToItem() on one frame might cause another frame to be deleted.

Covered by existing tests. fast/loader/stateobjects/popstate-is-asynchronous.html was renamed to
fast/loader/stateobjects/popstate-is-synchronous.html and modified to expect synchronous dispatch.

* dom/Document.cpp:
(WebCore::Document::enqueuePopstateEvent):
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadSameDocumentItem):
* loader/HistoryController.cpp:
(WebCore::HistoryController::recursiveGoToItem):

LayoutTests:

Renamed and modified this test to expect synchronous dispatch.

* fast/loader/stateobjects/popstate-is-synchronous-expected.txt: Renamed from LayoutTests/fast/loader/stateobjects/popstate-is-asynchronous-expected.txt.
* fast/loader/stateobjects/popstate-is-synchronous.html: Renamed from LayoutTests/fast/loader/stateobjects/popstate-is-asynchronous.html.

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

LayoutTests/ChangeLog
LayoutTests/fast/loader/stateobjects/popstate-is-synchronous-expected.txt [moved from LayoutTests/fast/loader/stateobjects/popstate-is-asynchronous-expected.txt with 74% similarity]
LayoutTests/fast/loader/stateobjects/popstate-is-synchronous.html [moved from LayoutTests/fast/loader/stateobjects/popstate-is-asynchronous.html with 59% similarity]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/HistoryController.cpp

index 33b77e5..80eae74 100644 (file)
@@ -1,3 +1,16 @@
+2016-02-18  Andy Estes  <aestes@apple.com>
+
+        Revert to dispatching the popstate event synchronously
+        https://bugs.webkit.org/show_bug.cgi?id=153297
+        rdar://problem/24092294
+
+        Reviewed by Brent Fulgham.
+
+        Renamed and modified this test to expect synchronous dispatch.
+
+        * fast/loader/stateobjects/popstate-is-synchronous-expected.txt: Renamed from LayoutTests/fast/loader/stateobjects/popstate-is-asynchronous-expected.txt.
+        * fast/loader/stateobjects/popstate-is-synchronous.html: Renamed from LayoutTests/fast/loader/stateobjects/popstate-is-asynchronous.html.
+
 2016-02-18  Philippe Normand  <pnormand@igalia.com>
 
         [GStreamer] Bump internal jhbuild versions to 1.6.3
@@ -1,12 +1,12 @@
-Tests that popstate events fire asynchronously.
+Tests that popstate events fire synchronously during fragment navigation.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 onload fired
 popstate fired
 Setting hash to #foo
-Set hash to #foo
 popstate fired
+Set hash to #foo
 PASS successfullyParsed is true
 
 TEST COMPLETE
@@ -6,7 +6,7 @@
 <div id="description"></div>
 <pre id="console"></pre>
 <script>
-description('Tests that popstate events fire asynchronously.');
+description('Tests that popstate events fire synchronously during fragment navigation.');
 
 window.onload = function()
 {
@@ -21,16 +21,16 @@ window.onpopstate = function()
 {
     debug('popstate fired');
 
-    if (initialPopState) {
-        initialPopState = false;
+    if (!initialPopState) {
+        window.setTimeout(finishJSTest, 0);
+        return;
+    }
 
-        // This should not be re-entrant; there should be no other log lines
-        // between the "Setting..." and "Set..." lines.
-        debug('Setting hash to #foo');
-        location.hash = '#foo';
-        debug('Set hash to #foo');
-    } else
-        finishJSTest();
+    initialPopState = false;
+
+    debug('Setting hash to #foo');
+    location.hash = '#foo';
+    debug('Set hash to #foo');
 }
 
 var successfullyParsed = true;
index 7659d09..2225289 100644 (file)
@@ -1,3 +1,35 @@
+2016-02-18  Andy Estes  <aestes@apple.com>
+
+        Revert to dispatching the popstate event synchronously
+        https://bugs.webkit.org/show_bug.cgi?id=153297
+        rdar://problem/24092294
+
+        Reviewed by Brent Fulgham.
+
+        r192369 made the popstate event dispatch asynchronously, which matches what the HTML5 spec says to do. However,
+        due to compatibility regressions we need to revert back to dispatching synchronously. This change reverts
+        r192369's changes to Document.cpp, but retains the new tests.
+
+        Firing popstate synchronously makes both fast/loader/remove-iframe-during-history-navigation-different.html and
+        fast/loader/remove-iframe-during-history-navigation-same.html crash, because their onpopstate handlers remove
+        frames from the document that will later be accessed by HistoryController::recursiveGoToItem().
+
+        To prevent the crashes, this change does two things:
+        1. Keep a reference to the current frame inside FrameLoader::loadSameDocumentItem(), since calling
+           loadInSameDocument() might otherwise delete it.
+        2. Handle a null frame when iterating a HistoryItem's child frames in HistoryController::recursiveGoToItem(),
+           since calling goToItem() on one frame might cause another frame to be deleted.
+
+        Covered by existing tests. fast/loader/stateobjects/popstate-is-asynchronous.html was renamed to
+        fast/loader/stateobjects/popstate-is-synchronous.html and modified to expect synchronous dispatch.
+
+        * dom/Document.cpp:
+        (WebCore::Document::enqueuePopstateEvent):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadSameDocumentItem):
+        * loader/HistoryController.cpp:
+        (WebCore::HistoryController::recursiveGoToItem):
+
 2016-02-19  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         Unreviewed. Fix GObject DOM bindings API break after r196769.
index ae136c3..90a0cac 100644 (file)
@@ -5511,7 +5511,7 @@ void Document::enqueueHashchangeEvent(const String& oldURL, const String& newURL
 
 void Document::enqueuePopstateEvent(RefPtr<SerializedScriptValue>&& stateObject)
 {
-    enqueueWindowEvent(PopStateEvent::create(WTFMove(stateObject), m_domWindow ? m_domWindow->history() : nullptr));
+    dispatchWindowEvent(PopStateEvent::create(WTFMove(stateObject), m_domWindow ? m_domWindow->history() : nullptr));
 }
 
 void Document::addMediaCanStartListener(MediaCanStartListener* listener)
index b6c2a0e..62359cc 100644 (file)
@@ -3248,6 +3248,8 @@ void FrameLoader::loadSameDocumentItem(HistoryItem& item)
 {
     ASSERT(item.documentSequenceNumber() == history().currentItem()->documentSequenceNumber());
 
+    Ref<Frame> protect(m_frame);
+
     // Save user view state to the current history item here since we don't do a normal load.
     // FIXME: Does form state need to be saved here too?
     history().saveScrollPositionAndViewStateToItem(history().currentItem());
index 53ee120..a33287d 100644 (file)
@@ -754,9 +754,8 @@ void HistoryController::recursiveGoToItem(HistoryItem& item, HistoryItem* fromIt
 
         HistoryItem* fromChildItem = fromItem->childItemWithTarget(childFrameName);
         ASSERT(fromChildItem);
-        Frame* childFrame = m_frame.tree().child(childFrameName);
-        ASSERT(childFrame);
-        childFrame->loader().history().recursiveGoToItem(const_cast<HistoryItem&>(childItem.get()), fromChildItem, type);
+        if (Frame* childFrame = m_frame.tree().child(childFrameName))
+            childFrame->loader().history().recursiveGoToItem(const_cast<HistoryItem&>(childItem.get()), fromChildItem, type);
     }
 }