Title: [187556] trunk
Revision
187556
Author
[email protected]
Date
2015-07-29 14:08:30 -0700 (Wed, 29 Jul 2015)

Log Message

Crash calling webSocket.close() from onError handler for blocked web socket.
<rdar://problem/21771620> and https://bugs.webkit.org/show_bug.cgi?id=147411

Reviewed by Tim Horton.

Source/WebCore:

Tests: http/tests/security/mixedContent/websocket/insecure-websocket-in-iframe.html
       http/tests/security/mixedContent/websocket/insecure-websocket-in-main-frame.html

This was introduced with http://trac.webkit.org/changeset/185848

* Modules/websockets/WebSocket.cpp:
(WebCore::WebSocket::connect): When blocked because of mixedContent, call dispatchOrQueueErrorEvent().
(WebCore::WebSocket::didReceiveMessageError): Use dispatchOrQueueErrorEvent() instead.
(WebCore::WebSocket::dispatchOrQueueErrorEvent): Dispatch the error event, but don't dispatch one twice!
* Modules/websockets/WebSocket.h:

* Modules/websockets/WebSocketChannel.cpp:
(WebCore::WebSocketChannel::fail): Null-check m_handshake before creating a console message from it.

LayoutTests:

* http/tests/security/mixedContent/resources/frame-with-insecure-websocket.html: Add a call to webSocket.close() inside the onError handler.
* http/tests/security/mixedContent/websocket/insecure-websocket-in-iframe-expected.txt:
* http/tests/security/mixedContent/websocket/insecure-websocket-in-main-frame-expected.txt:

Modified Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (187555 => 187556)


--- trunk/LayoutTests/ChangeLog	2015-07-29 20:30:08 UTC (rev 187555)
+++ trunk/LayoutTests/ChangeLog	2015-07-29 21:08:30 UTC (rev 187556)
@@ -1,3 +1,14 @@
+2015-07-29  Brady Eidson  <[email protected]>
+
+        Crash calling webSocket.close() from onError handler for blocked web socket.
+        <rdar://problem/21771620> and https://bugs.webkit.org/show_bug.cgi?id=147411
+
+        Reviewed by Tim Horton.
+
+        * http/tests/security/mixedContent/resources/frame-with-insecure-websocket.html: Add a call to webSocket.close() inside the onError handler.
+        * http/tests/security/mixedContent/websocket/insecure-websocket-in-iframe-expected.txt:
+        * http/tests/security/mixedContent/websocket/insecure-websocket-in-main-frame-expected.txt:
+
 2015-07-19  Matt Rajca  <[email protected]>
 
         Media Session: test 'Transient Solo' interruptions

Modified: trunk/LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-websocket.html (187555 => 187556)


--- trunk/LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-websocket.html	2015-07-29 20:30:08 UTC (rev 187555)
+++ trunk/LayoutTests/http/tests/security/mixedContent/resources/frame-with-insecure-websocket.html	2015-07-29 21:08:30 UTC (rev 187556)
@@ -2,6 +2,8 @@
 <script>
 window.jsTestIsAsync = true;
 
+var ws;
+
 function onSocketOpened() {
     alert("WebSocket connection opened.");
     finishJSTest();
@@ -9,6 +11,7 @@
 
 function onSocketError() {
     alert("WebSocket connection failed.");
+    ws.close();
     finishJSTest();
 }
 
@@ -18,7 +21,7 @@
 }
 
 try {
-    var ws = new WebSocket("ws://127.0.0.1:8880/websocket/tests/hybi/echo");
+    ws = new WebSocket("ws://127.0.0.1:8880/websocket/tests/hybi/echo");
     ws._onopen_ = onSocketOpened;
     ws._onerror_ = onSocketError;
     ws._onclose_ = onSocketClosed;

Modified: trunk/LayoutTests/http/tests/security/mixedContent/websocket/insecure-websocket-in-iframe-expected.txt (187555 => 187556)


--- trunk/LayoutTests/http/tests/security/mixedContent/websocket/insecure-websocket-in-iframe-expected.txt	2015-07-29 20:30:08 UTC (rev 187555)
+++ trunk/LayoutTests/http/tests/security/mixedContent/websocket/insecure-websocket-in-iframe-expected.txt	2015-07-29 21:08:30 UTC (rev 187556)
@@ -1,6 +1,7 @@
-CONSOLE MESSAGE: line 21: [blocked] The page at https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-websocket.html was not allowed to run insecure content from ws://127.0.0.1:8880/websocket/tests/hybi/echo.
+CONSOLE MESSAGE: line 24: [blocked] The page at https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-websocket.html was not allowed to run insecure content from ws://127.0.0.1:8880/websocket/tests/hybi/echo.
 
 ALERT: WebSocket connection failed.
+CONSOLE MESSAGE: line 14: WebSocket connection failed: WebSocket is closed before the connection is established.
 This test loads an iframe that creates an insecure WebSocket connection. We should block the connection and trigger a mixed content callback because the main frame is HTTPS, but the data sent over the socket could be recorded or controlled by an attacker.
 
 

Modified: trunk/LayoutTests/http/tests/security/mixedContent/websocket/insecure-websocket-in-main-frame-expected.txt (187555 => 187556)


--- trunk/LayoutTests/http/tests/security/mixedContent/websocket/insecure-websocket-in-main-frame-expected.txt	2015-07-29 20:30:08 UTC (rev 187555)
+++ trunk/LayoutTests/http/tests/security/mixedContent/websocket/insecure-websocket-in-main-frame-expected.txt	2015-07-29 21:08:30 UTC (rev 187556)
@@ -1,4 +1,5 @@
-CONSOLE MESSAGE: line 21: [blocked] The page at https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-websocket.html was not allowed to run insecure content from ws://127.0.0.1:8880/websocket/tests/hybi/echo.
+CONSOLE MESSAGE: line 24: [blocked] The page at https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-websocket.html was not allowed to run insecure content from ws://127.0.0.1:8880/websocket/tests/hybi/echo.
 
 ALERT: WebSocket connection failed.
+CONSOLE MESSAGE: line 14: WebSocket connection failed: WebSocket is closed before the connection is established.
 This test opens a window that connects to an insecure ws:// WebSocket. We should block the connection and trigger a mixed content callback because the main frame is HTTPS, but the data sent over the socket could be recorded or controlled by an attacker.

Modified: trunk/Source/WebCore/ChangeLog (187555 => 187556)


--- trunk/Source/WebCore/ChangeLog	2015-07-29 20:30:08 UTC (rev 187555)
+++ trunk/Source/WebCore/ChangeLog	2015-07-29 21:08:30 UTC (rev 187556)
@@ -1,3 +1,24 @@
+2015-07-29  Brady Eidson  <[email protected]>
+
+        Crash calling webSocket.close() from onError handler for blocked web socket.
+        <rdar://problem/21771620> and https://bugs.webkit.org/show_bug.cgi?id=147411
+
+        Reviewed by Tim Horton.
+
+        Tests: http/tests/security/mixedContent/websocket/insecure-websocket-in-iframe.html
+               http/tests/security/mixedContent/websocket/insecure-websocket-in-main-frame.html
+
+        This was introduced with http://trac.webkit.org/changeset/185848
+
+        * Modules/websockets/WebSocket.cpp:
+        (WebCore::WebSocket::connect): When blocked because of mixedContent, call dispatchOrQueueErrorEvent().
+        (WebCore::WebSocket::didReceiveMessageError): Use dispatchOrQueueErrorEvent() instead.
+        (WebCore::WebSocket::dispatchOrQueueErrorEvent): Dispatch the error event, but don't dispatch one twice!
+        * Modules/websockets/WebSocket.h:
+
+        * Modules/websockets/WebSocketChannel.cpp:
+        (WebCore::WebSocketChannel::fail): Null-check m_handshake before creating a console message from it.
+
 2015-07-29  Michael Catanzaro  <[email protected]>
 
         Clean up RefPtrCairo.cpp

Modified: trunk/Source/WebCore/Modules/websockets/WebSocket.cpp (187555 => 187556)


--- trunk/Source/WebCore/Modules/websockets/WebSocket.cpp	2015-07-29 20:30:08 UTC (rev 187555)
+++ trunk/Source/WebCore/Modules/websockets/WebSocket.cpp	2015-07-29 21:08:30 UTC (rev 187556)
@@ -284,12 +284,13 @@
         if (!document.frame()->loader().mixedContentChecker().canRunInsecureContent(document.securityOrigin(), m_url)) {
             // Balanced by the call to ActiveDOMObject::unsetPendingActivity() in WebSocket::stop().
             ActiveDOMObject::setPendingActivity(this);
+
             // We must block this connection. Instead of throwing an exception, we indicate this
             // using the error event. But since this code executes as part of the WebSocket's
             // constructor, we have to wait until the constructor has completed before firing the
             // event; otherwise, users can't connect to the event.
             RunLoop::main().dispatch([this]() {
-                dispatchEvent(Event::create(eventNames().errorEvent, false, false));
+                dispatchOrQueueErrorEvent();
                 stop();
             });
             return;
@@ -587,7 +588,7 @@
     LOG(Network, "WebSocket %p didReceiveErrorMessage()", this);
     m_state = CLOSED;
     ASSERT(scriptExecutionContext());
-    dispatchOrQueueEvent(Event::create(eventNames().errorEvent, false, false));
+    dispatchOrQueueErrorEvent();
 }
 
 void WebSocket::didUpdateBufferedAmount(unsigned long bufferedAmount)
@@ -638,6 +639,15 @@
     return overhead;
 }
 
+void WebSocket::dispatchOrQueueErrorEvent()
+{
+    if (m_dispatchedErrorEvent)
+        return;
+
+    m_dispatchedErrorEvent = true;
+    dispatchOrQueueEvent(Event::create(eventNames().errorEvent, false, false));
+}
+
 void WebSocket::dispatchOrQueueEvent(Ref<Event>&& event)
 {
     if (m_shouldDelayEventFiring)

Modified: trunk/Source/WebCore/Modules/websockets/WebSocket.h (187555 => 187556)


--- trunk/Source/WebCore/Modules/websockets/WebSocket.h	2015-07-29 20:30:08 UTC (rev 187555)
+++ trunk/Source/WebCore/Modules/websockets/WebSocket.h	2015-07-29 21:08:30 UTC (rev 187556)
@@ -110,6 +110,7 @@
     explicit WebSocket(ScriptExecutionContext&);
 
     void resumeTimerFired();
+    void dispatchOrQueueErrorEvent();
     void dispatchOrQueueEvent(Ref<Event>&&);
 
     // ActiveDOMObject API.
@@ -143,6 +144,7 @@
     Timer m_resumeTimer;
     bool m_shouldDelayEventFiring { false };
     Deque<Ref<Event>> m_pendingEvents;
+    bool m_dispatchedErrorEvent { false };
 };
 
 } // namespace WebCore

Modified: trunk/Source/WebCore/Modules/websockets/WebSocketChannel.cpp (187555 => 187556)


--- trunk/Source/WebCore/Modules/websockets/WebSocketChannel.cpp	2015-07-29 20:30:08 UTC (rev 187555)
+++ trunk/Source/WebCore/Modules/websockets/WebSocketChannel.cpp	2015-07-29 21:08:30 UTC (rev 187556)
@@ -201,7 +201,14 @@
     ASSERT(!m_suspended);
     if (m_document) {
         InspectorInstrumentation::didReceiveWebSocketFrameError(m_document, m_identifier, reason);
-        m_document->addConsoleMessage(MessageSource::Network, MessageLevel::Error, "WebSocket connection to '" + m_handshake->url().stringCenterEllipsizedToLength() + "' failed: " + reason);
+
+        String consoleMessage;
+        if (m_handshake)
+            consoleMessage = makeString("WebSocket connection to '", m_handshake->url().stringCenterEllipsizedToLength(), "' failed: ", reason);
+        else
+            consoleMessage = makeString("WebSocket connection failed: ", reason);
+
+        m_document->addConsoleMessage(MessageSource::Network, MessageLevel::Error, consoleMessage);
     }
 
     // Hybi-10 specification explicitly states we must not continue to handle incoming data
@@ -218,7 +225,8 @@
     if (m_handle && !m_closed)
         m_handle->disconnect(); // Will call didClose().
 
-    ASSERT(m_closed);
+    // We should be closed by now, but if we never got a handshake then we never even opened.
+    ASSERT(m_closed || !m_handshake);
 }
 
 void WebSocketChannel::disconnect()
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to