Regression(r182517): WebSocket::suspend() causes error event to be fired
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 16 Apr 2015 19:39:50 +0000 (19:39 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 16 Apr 2015 19:39:50 +0000 (19:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=143806
<rdar://problem/20559812>

Reviewed by Alexey Proskuryakov.

Source/WebCore:

WebSocket::suspend() causes an error event to be fired after r182517.
This is not allowed as firing the event could trigger arbitrary JS
execution, which is no longer allowed at this point.

This patch delays the error event firing until after
WebSocket::resume() is called, similarly to what we already do for
the close event.

Also add assertions in WebSocket::suspend() / WebSocket::resume()
that will be hit if JS events are fired from within these functions.
The pre-existing closed-when-entering-page-cache.html test is hitting
one of these assertions without the fix above.

Tests:
  - http/tests/websocket/tests/hybi/closed-when-entering-page-cache.html
  - http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler.html

* Modules/websockets/WebSocket.cpp:
(WebCore::WebSocket::suspend):
(WebCore::WebSocket::resume):
(WebCore::WebSocket::resumeTimerFired):
(WebCore::WebSocket::stop):
(WebCore::WebSocket::didReceiveMessageError):
(WebCore::WebSocket::didClose):
(WebCore::WebSocket::dispatchOrQueueEvent):
* Modules/websockets/WebSocket.h:

LayoutTests:

* http/tests/websocket/tests/hybi/closed-when-entering-page-cache-expected.txt:
* http/tests/websocket/tests/hybi/closed-when-entering-page-cache.html:
Extend WebSocket PageCache test to make sure that the error event is
fired after restoring the page from the PageCache and before the close
Event is fired.

* http/tests/websocket/tests/hybi/resources/page-cache-websocket.html: Added.
* http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler-expected.txt: Copied from LayoutTests/http/tests/websocket/tests/hybi/closed-when-entering-page-cache-expected.txt.
* http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler.html: Copied from LayoutTests/http/tests/websocket/tests/hybi/closed-when-entering-page-cache.html.
Add layout test to cover the case where WebSocket::stop() is called
while firing the pending events upon restoring the page from PageCache.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/websocket/tests/hybi/closed-when-entering-page-cache-expected.txt
LayoutTests/http/tests/websocket/tests/hybi/closed-when-entering-page-cache.html
LayoutTests/http/tests/websocket/tests/hybi/resources/page-cache-websocket.html [new file with mode: 0644]
LayoutTests/http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/websockets/WebSocket.cpp
Source/WebCore/Modules/websockets/WebSocket.h

index 94ea7d9..89d850b 100644 (file)
@@ -1,3 +1,23 @@
+2015-04-16  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r182517): WebSocket::suspend() causes error event to be fired
+        https://bugs.webkit.org/show_bug.cgi?id=143806
+        <rdar://problem/20559812>
+
+        Reviewed by Alexey Proskuryakov.
+
+        * http/tests/websocket/tests/hybi/closed-when-entering-page-cache-expected.txt:
+        * http/tests/websocket/tests/hybi/closed-when-entering-page-cache.html:
+        Extend WebSocket PageCache test to make sure that the error event is
+        fired after restoring the page from the PageCache and before the close
+        Event is fired.
+
+        * http/tests/websocket/tests/hybi/resources/page-cache-websocket.html: Added.
+        * http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler-expected.txt: Copied from LayoutTests/http/tests/websocket/tests/hybi/closed-when-entering-page-cache-expected.txt.
+        * http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler.html: Copied from LayoutTests/http/tests/websocket/tests/hybi/closed-when-entering-page-cache.html.
+        Add layout test to cover the case where WebSocket::stop() is called
+        while firing the pending events upon restoring the page from PageCache.
+
 2015-04-16  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [iOS] Delete hardcoded font fallback tables
index 16e2e14..c5f3cf8 100644 (file)
@@ -8,8 +8,11 @@ pageshow - not from cache
 pagehide - entering cache
 pageshow - from cache
 PASS Page did enter and was restored from the page cache
-PASS WebSocket was closed
+PASS error event fired
 PASS wasRestoredFromPageCache is true
+PASS close event fired
+PASS wasRestoredFromPageCache is true
+PASS receivedErrorEvent is true
 PASS closeEvent.wasClean is false
 PASS closeEvent.code is 1006
 PASS successfullyParsed is true
index 0429d12..3b38df0 100644 (file)
@@ -10,6 +10,7 @@ if (window.testRunner)
     testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);
 
 var wasRestoredFromPageCache = false;
+var receivedErrorEvent = false;
 
 window.addEventListener("pageshow", function(event) {
     debug("pageshow - " + (event.persisted ? "" : "not ") + "from cache");
@@ -30,9 +31,15 @@ window.addEventListener("pagehide", function(event) {
 
 window.addEventListener('load', function() {
     ws = new WebSocket("ws://127.0.0.1:8880/websocket/tests/hybi/echo");
+    ws.onerror = function(ev) {
+        receivedErrorEvent = true;
+        testPassed("error event fired");
+        shouldBeTrue("wasRestoredFromPageCache");
+    }
     ws.onclose = function(ev) {
-        testPassed("WebSocket was closed");
+        testPassed("close event fired");
         shouldBeTrue("wasRestoredFromPageCache");
+        shouldBeTrue("receivedErrorEvent");
         closeEvent = ev;
         shouldBeFalse("closeEvent.wasClean");
         shouldBe("closeEvent.code", "1006");
diff --git a/LayoutTests/http/tests/websocket/tests/hybi/resources/page-cache-websocket.html b/LayoutTests/http/tests/websocket/tests/hybi/resources/page-cache-websocket.html
new file mode 100644 (file)
index 0000000..f841794
--- /dev/null
@@ -0,0 +1,8 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script>
+ws = new WebSocket("ws://127.0.0.1:8880/websocket/tests/hybi/echo");
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler-expected.txt b/LayoutTests/http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler-expected.txt
new file mode 100644 (file)
index 0000000..b598db6
--- /dev/null
@@ -0,0 +1,16 @@
+CONSOLE MESSAGE: WebSocket connection to 'ws://127.0.0.1:8880/websocket/tests/hybi/echo' failed: WebSocket is closed due to suspension.
+Tests that WebSocket correctly handles being stopped in the error event handler upon restoring from PageCache.
+
+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
+error event fired
+PASS wasRestoredFromPageCache is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler.html b/LayoutTests/http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler.html
new file mode 100644 (file)
index 0000000..7f338cc
--- /dev/null
@@ -0,0 +1,63 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="/js-test-resources/js-test-pre.js"></script>
+<script>
+description("Tests that WebSocket correctly handles being stopped in the error event handler upon restoring from PageCache.");
+
+var wasRestoredFromPageCache = false;
+
+window.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");
+        wasRestoredFromPageCache = true;
+    }
+}, 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();
+    }
+}, false);
+
+var totalLoaded = 0;
+function loaded() {
+    totalLoaded++;
+    if (totalLoaded < 2)
+        return;
+
+    testFrame = document.getElementById("testFrame");
+    ws = testFrame.contentWindow.ws;
+    ws.onerror = function(ev) {
+        debug("error event fired");
+        shouldBeTrue("wasRestoredFromPageCache");
+        testFrame.remove();
+        setTimeout(function() {
+             finishJSTest();
+        }, 500);
+    }
+    ws.onclose = function(ev) {
+        testFailed("Close event should not have fired.");
+    }
+
+    setTimeout(function() {
+        // Force a back navigation back to this page.
+        window.location.href = "/navigation/resources/page-cache-helper.html";
+    }, 0);
+}
+
+window.onload = loaded;
+
+</script>
+<iframe id="testFrame" src="resources/page-cache-websocket.html" onload="loaded()"></iframe>
+<script src="/js-test-resources/js-test-post.js"></script>
+</body>
+</html>
index 5d8a503..a41d7bc 100644 (file)
@@ -1,3 +1,38 @@
+2015-04-16  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r182517): WebSocket::suspend() causes error event to be fired
+        https://bugs.webkit.org/show_bug.cgi?id=143806
+        <rdar://problem/20559812>
+
+        Reviewed by Alexey Proskuryakov.
+
+        WebSocket::suspend() causes an error event to be fired after r182517.
+        This is not allowed as firing the event could trigger arbitrary JS
+        execution, which is no longer allowed at this point.
+
+        This patch delays the error event firing until after
+        WebSocket::resume() is called, similarly to what we already do for
+        the close event.
+
+        Also add assertions in WebSocket::suspend() / WebSocket::resume()
+        that will be hit if JS events are fired from within these functions.
+        The pre-existing closed-when-entering-page-cache.html test is hitting
+        one of these assertions without the fix above.
+
+        Tests:
+          - http/tests/websocket/tests/hybi/closed-when-entering-page-cache.html
+          - http/tests/websocket/tests/hybi/stop-on-resume-in-error-handler.html
+
+        * Modules/websockets/WebSocket.cpp:
+        (WebCore::WebSocket::suspend):
+        (WebCore::WebSocket::resume):
+        (WebCore::WebSocket::resumeTimerFired):
+        (WebCore::WebSocket::stop):
+        (WebCore::WebSocket::didReceiveMessageError):
+        (WebCore::WebSocket::didClose):
+        (WebCore::WebSocket::dispatchOrQueueEvent):
+        * Modules/websockets/WebSocket.h:
+
 2015-04-15  Roger Fong  <roger_fong@apple.com>
 
         Adjustments to button graphics for media controls.
index e425448..94a03ee 100644 (file)
@@ -468,13 +468,15 @@ bool WebSocket::canSuspendForPageCache() const
 
 void WebSocket::suspend(ReasonForSuspension reason)
 {
+    NoEventDispatchAssertion assertNoEventDispatch;
+
     if (m_resumeTimer.isActive())
         m_resumeTimer.stop();
 
+    m_shouldDelayEventFiring = true;
+
     if (m_channel) {
         if (reason == ActiveDOMObject::PageCache) {
-            // The page will enter the page cache, close the channel but only fire the close event after resuming.
-            m_shouldDelayCloseEvent = true;
             // This will cause didClose() to be called.
             m_channel->fail("WebSocket is closed due to suspension.");
         } else
@@ -484,18 +486,28 @@ void WebSocket::suspend(ReasonForSuspension reason)
 
 void WebSocket::resume()
 {
+    NoEventDispatchAssertion assertNoEventDispatch;
+
     if (m_channel)
         m_channel->resume();
-    else if (m_pendingCloseEvent && !m_resumeTimer.isActive()) {
-        // Fire the close event in a timer as we are not allowed to execute arbitrary JS from resume().
+    else if (!m_pendingEvents.isEmpty() && !m_resumeTimer.isActive()) {
+        // Fire the pending events in a timer as we are not allowed to execute arbitrary JS from resume().
         m_resumeTimer.startOneShot(0);
     }
+
+    m_shouldDelayEventFiring = false;
 }
 
 void WebSocket::resumeTimerFired()
 {
-    ASSERT(m_pendingCloseEvent);
-    dispatchEvent(WTF::move(m_pendingCloseEvent));
+    Ref<WebSocket> protect(*this);
+
+    ASSERT(!m_pendingEvents.isEmpty());
+
+    // Check m_shouldDelayEventFiring when iterating in case firing an event causes
+    // suspend() to be called.
+    while (!m_pendingEvents.isEmpty() && !m_shouldDelayEventFiring)
+        dispatchEvent(m_pendingEvents.takeFirst());
 }
 
 void WebSocket::stop()
@@ -505,6 +517,7 @@ void WebSocket::stop()
         m_channel->disconnect();
     m_channel = 0;
     m_state = CLOSED;
+    m_pendingEvents.clear();
     ActiveDOMObject::stop();
     if (pending)
         ActiveDOMObject::unsetPendingActivity(this);
@@ -560,7 +573,7 @@ void WebSocket::didReceiveMessageError()
     LOG(Network, "WebSocket %p didReceiveErrorMessage()", this);
     m_state = CLOSED;
     ASSERT(scriptExecutionContext());
-    dispatchEvent(Event::create(eventNames().errorEvent, false, false));
+    dispatchOrQueueEvent(Event::create(eventNames().errorEvent, false, false));
 }
 
 void WebSocket::didUpdateBufferedAmount(unsigned long bufferedAmount)
@@ -587,12 +600,7 @@ void WebSocket::didClose(unsigned long unhandledBufferedAmount, ClosingHandshake
     m_bufferedAmount = unhandledBufferedAmount;
     ASSERT(scriptExecutionContext());
 
-    RefPtr<CloseEvent> event = CloseEvent::create(wasClean, code, reason);
-    if (m_shouldDelayCloseEvent) {
-        m_pendingCloseEvent = WTF::move(event);
-        m_shouldDelayCloseEvent = false;
-    } else
-        dispatchEvent(event);
+    dispatchOrQueueEvent(CloseEvent::create(wasClean, code, reason));
 
     if (m_channel) {
         m_channel->disconnect();
@@ -616,6 +624,14 @@ size_t WebSocket::getFramingOverhead(size_t payloadSize)
     return overhead;
 }
 
+void WebSocket::dispatchOrQueueEvent(Ref<Event>&& event)
+{
+    if (m_shouldDelayEventFiring)
+        m_pendingEvents.append(WTF::move(event));
+    else
+        dispatchEvent(WTF::move(event));
+}
+
 }  // namespace WebCore
 
 #endif
index 9dfc6ee..2dc5a89 100644 (file)
@@ -110,6 +110,7 @@ private:
     explicit WebSocket(ScriptExecutionContext&);
 
     void resumeTimerFired();
+    void dispatchOrQueueEvent(Ref<Event>&&);
 
     // ActiveDOMObject API.
     void contextDestroyed() override;
@@ -140,8 +141,8 @@ private:
     String m_extensions;
 
     Timer m_resumeTimer;
-    bool m_shouldDelayCloseEvent { false };
-    RefPtr<CloseEvent> m_pendingCloseEvent;
+    bool m_shouldDelayEventFiring { false };
+    Deque<Ref<Event>> m_pendingEvents;
 };
 
 } // namespace WebCore