Use WeakPtr instead of storing raw pointers in WebSocket code
authorachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Mar 2019 23:15:04 +0000 (23:15 +0000)
committerachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Mar 2019 23:15:04 +0000 (23:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196034

Reviewed by Geoff Garen.

This could prevent using freed memory if we forget to reset a pointer somewhere.

* Modules/websockets/WebSocketChannel.cpp:
(WebCore::WebSocketChannel::WebSocketChannel):
(WebCore::WebSocketChannel::connect):
(WebCore::WebSocketChannel::fail):
(WebCore::WebSocketChannel::disconnect):
(WebCore::WebSocketChannel::didOpenSocketStream):
(WebCore::WebSocketChannel::didCloseSocketStream):
(WebCore::WebSocketChannel::didFailSocketStream):
(WebCore::WebSocketChannel::processBuffer):
(WebCore::WebSocketChannel::processFrame):
(WebCore::WebSocketChannel::processOutgoingFrameQueue):
(WebCore::WebSocketChannel::sendFrame):
* Modules/websockets/WebSocketChannel.h:
* Modules/websockets/WebSocketChannelClient.h:
* Modules/websockets/WebSocketHandshake.cpp:
(WebCore::WebSocketHandshake::WebSocketHandshake):
* Modules/websockets/WebSocketHandshake.h:

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

Source/WebCore/ChangeLog
Source/WebCore/Modules/websockets/WebSocketChannel.cpp
Source/WebCore/Modules/websockets/WebSocketChannel.h
Source/WebCore/Modules/websockets/WebSocketChannelClient.h
Source/WebCore/Modules/websockets/WebSocketHandshake.cpp
Source/WebCore/Modules/websockets/WebSocketHandshake.h

index 0ef1587..b8bfa47 100644 (file)
@@ -1,3 +1,30 @@
+2019-03-20  Alex Christensen  <achristensen@webkit.org>
+
+        Use WeakPtr instead of storing raw pointers in WebSocket code
+        https://bugs.webkit.org/show_bug.cgi?id=196034
+
+        Reviewed by Geoff Garen.
+
+        This could prevent using freed memory if we forget to reset a pointer somewhere.
+
+        * Modules/websockets/WebSocketChannel.cpp:
+        (WebCore::WebSocketChannel::WebSocketChannel):
+        (WebCore::WebSocketChannel::connect):
+        (WebCore::WebSocketChannel::fail):
+        (WebCore::WebSocketChannel::disconnect):
+        (WebCore::WebSocketChannel::didOpenSocketStream):
+        (WebCore::WebSocketChannel::didCloseSocketStream):
+        (WebCore::WebSocketChannel::didFailSocketStream):
+        (WebCore::WebSocketChannel::processBuffer):
+        (WebCore::WebSocketChannel::processFrame):
+        (WebCore::WebSocketChannel::processOutgoingFrameQueue):
+        (WebCore::WebSocketChannel::sendFrame):
+        * Modules/websockets/WebSocketChannel.h:
+        * Modules/websockets/WebSocketChannelClient.h:
+        * Modules/websockets/WebSocketHandshake.cpp:
+        (WebCore::WebSocketHandshake::WebSocketHandshake):
+        * Modules/websockets/WebSocketHandshake.h:
+
 2019-03-20  Dean Jackson  <dino@apple.com>
 
         [iOS] Crash in WebCore::Node::renderRect
index 7fca5f0..16f038d 100644 (file)
@@ -63,8 +63,8 @@ namespace WebCore {
 const Seconds TCPMaximumSegmentLifetime { 2_min };
 
 WebSocketChannel::WebSocketChannel(Document& document, WebSocketChannelClient& client, SocketProvider& provider)
-    : m_document(&document)
-    , m_client(&client)
+    : m_document(makeWeakPtr(document))
+    , m_client(makeWeakPtr(client))
     , m_resumeTimer(*this, &WebSocketChannel::resumeTimerFired)
     , m_closingTimer(*this, &WebSocketChannel::closingTimerFired)
     , m_socketProvider(provider)
@@ -112,12 +112,12 @@ void WebSocketChannel::connect(const URL& requestedURL, const String& protocol)
     
     ASSERT(!m_handle);
     ASSERT(!m_suspended);
-    m_handshake = std::make_unique<WebSocketHandshake>(url, protocol, m_document, allowCookies);
+    m_handshake = std::make_unique<WebSocketHandshake>(url, protocol, m_document.get(), allowCookies);
     m_handshake->reset();
     if (m_deflateFramer.canDeflate())
         m_handshake->addExtensionProcessor(m_deflateFramer.createExtensionProcessor());
     if (m_identifier)
-        InspectorInstrumentation::didCreateWebSocket(m_document, m_identifier, url);
+        InspectorInstrumentation::didCreateWebSocket(m_document.get(), m_identifier, url);
 
     if (Frame* frame = m_document->frame()) {
         ref();
@@ -214,7 +214,7 @@ void WebSocketChannel::fail(const String& reason)
     LOG(Network, "WebSocketChannel %p fail() reason='%s'", this, reason.utf8().data());
     ASSERT(!m_suspended);
     if (m_document) {
-        InspectorInstrumentation::didReceiveWebSocketFrameError(m_document, m_identifier, reason);
+        InspectorInstrumentation::didReceiveWebSocketFrameError(m_document.get(), m_identifier, reason);
 
         String consoleMessage;
         if (m_handshake)
@@ -245,7 +245,7 @@ void WebSocketChannel::disconnect()
 {
     LOG(Network, "WebSocketChannel %p disconnect()", this);
     if (m_identifier && m_document)
-        InspectorInstrumentation::didCloseWebSocket(m_document, m_identifier);
+        InspectorInstrumentation::didCloseWebSocket(m_document.get(), m_identifier);
     if (m_handshake)
         m_handshake->clearDocument();
     m_client = nullptr;
@@ -273,7 +273,7 @@ void WebSocketChannel::didOpenSocketStream(SocketStreamHandle& handle)
     if (!m_document)
         return;
     if (m_identifier && UNLIKELY(InspectorInstrumentation::hasFrontends()))
-        InspectorInstrumentation::willSendWebSocketHandshakeRequest(m_document, m_identifier, m_handshake->clientHandshakeRequest());
+        InspectorInstrumentation::willSendWebSocketHandshakeRequest(m_document.get(), m_identifier, m_handshake->clientHandshakeRequest());
     auto handshakeMessage = m_handshake->clientHandshakeMessage();
     auto cookieRequestHeaderFieldProxy = m_handshake->clientHandshakeCookieRequestHeaderFieldProxy();
     handle.sendHandshake(WTFMove(handshakeMessage), WTFMove(cookieRequestHeaderFieldProxy), [this, protectedThis = makeRef(*this)] (bool success, bool didAccessSecureCookies) {
@@ -289,7 +289,7 @@ void WebSocketChannel::didCloseSocketStream(SocketStreamHandle& handle)
 {
     LOG(Network, "WebSocketChannel %p didCloseSocketStream()", this);
     if (m_identifier && m_document)
-        InspectorInstrumentation::didCloseWebSocket(m_document, m_identifier);
+        InspectorInstrumentation::didCloseWebSocket(m_document.get(), m_identifier);
     ASSERT_UNUSED(handle, &handle == m_handle || !m_handle);
     m_closed = true;
     if (m_closingTimer.isActive())
@@ -300,7 +300,7 @@ void WebSocketChannel::didCloseSocketStream(SocketStreamHandle& handle)
         m_unhandledBufferedAmount = m_handle->bufferedAmount();
         if (m_suspended)
             return;
-        WebSocketChannelClient* client = m_client;
+        WebSocketChannelClient* client = m_client.get();
         m_client = nullptr;
         m_document = nullptr;
         m_handle = nullptr;
@@ -363,7 +363,7 @@ void WebSocketChannel::didFailSocketStream(SocketStreamHandle& handle, const Soc
             message = makeString("WebSocket network error: error code ", error.errorCode());
         else
             message = "WebSocket network error: " + error.localizedDescription();
-        InspectorInstrumentation::didReceiveWebSocketFrameError(m_document, m_identifier, message);
+        InspectorInstrumentation::didReceiveWebSocketFrameError(m_document.get(), m_identifier, message);
         m_document->addConsoleMessage(MessageSource::Network, MessageLevel::Error, message);
     }
     m_shouldDiscardReceivedData = true;
@@ -448,7 +448,7 @@ bool WebSocketChannel::processBuffer()
             return false;
         if (m_handshake->mode() == WebSocketHandshake::Connected) {
             if (m_identifier)
-                InspectorInstrumentation::didReceiveWebSocketHandshakeResponse(m_document, m_identifier, m_handshake->serverHandshakeResponse());
+                InspectorInstrumentation::didReceiveWebSocketHandshakeResponse(m_document.get(), m_identifier, m_handshake->serverHandshakeResponse());
             String serverSetCookie = m_handshake->serverSetCookie();
             if (!serverSetCookie.isEmpty()) {
                 if (m_document && m_document->page() && m_document->page()->cookieJar().cookiesEnabled(*m_document))
@@ -582,7 +582,7 @@ bool WebSocketChannel::processFrame()
         return false;
     }
 
-    InspectorInstrumentation::didReceiveWebSocketFrame(m_document, m_identifier, frame);
+    InspectorInstrumentation::didReceiveWebSocketFrame(m_document.get(), m_identifier, frame);
 
     switch (frame.opCode) {
     case WebSocketFrame::OpCodeContinuation:
@@ -768,7 +768,7 @@ void WebSocketChannel::processOutgoingFrameQueue()
                 ASSERT(frame->blobData);
                 m_blobLoader = std::make_unique<FileReaderLoader>(FileReaderLoader::ReadAsArrayBuffer, this);
                 m_blobLoaderStatus = BlobLoaderStarted;
-                m_blobLoader->start(m_document, *frame->blobData);
+                m_blobLoader->start(m_document.get(), *frame->blobData);
                 m_outgoingFrameQueue.prepend(WTFMove(frame));
                 return;
 
@@ -820,7 +820,7 @@ void WebSocketChannel::sendFrame(WebSocketFrame::OpCode opCode, const char* data
     ASSERT(!m_suspended);
 
     WebSocketFrame frame(opCode, true, false, true, data, dataLength);
-    InspectorInstrumentation::didSendWebSocketFrame(m_document, m_identifier, frame);
+    InspectorInstrumentation::didSendWebSocketFrame(m_document.get(), m_identifier, frame);
 
     auto deflateResult = m_deflateFramer.deflate(frame);
     if (!deflateResult->succeeded()) {
index aae66b4..fd44fcb 100644 (file)
@@ -193,8 +193,8 @@ private:
         BlobLoaderFailed
     };
 
-    Document* m_document;
-    WebSocketChannelClient* m_client;
+    WeakPtr<Document> m_document;
+    WeakPtr<WebSocketChannelClient> m_client;
     std::unique_ptr<WebSocketHandshake> m_handshake;
     RefPtr<SocketStreamHandle> m_handle;
     Vector<char> m_buffer;
index d6af8be..8401b5c 100644 (file)
 #pragma once
 
 #include <wtf/Forward.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
 
-class WebSocketChannelClient {
+class WebSocketChannelClient : public CanMakeWeakPtr<WebSocketChannelClient> {
 public:
     virtual ~WebSocketChannelClient() = default;
     virtual void didConnect() = 0;
index 141757c..929f353 100644 (file)
@@ -123,7 +123,7 @@ WebSocketHandshake::WebSocketHandshake(const URL& url, const String& protocol, D
     : m_url(url)
     , m_clientProtocol(protocol)
     , m_secure(m_url.protocolIs("wss"))
-    , m_document(document)
+    , m_document(makeWeakPtr(document))
     , m_mode(Incomplete)
     , m_allowCookies(allowCookies)
 {
index 129f5b4..6a9fa5c 100644 (file)
@@ -35,6 +35,7 @@
 #include "ResourceResponse.h"
 #include "WebSocketExtensionDispatcher.h"
 #include "WebSocketExtensionProcessor.h"
+#include <wtf/WeakPtr.h>
 #include <wtf/text/WTFString.h>
 
 namespace WebCore {
@@ -100,7 +101,7 @@ private:
     URL m_url;
     String m_clientProtocol;
     bool m_secure;
-    Document* m_document;
+    WeakPtr<Document> m_document;
 
     Mode m_mode;
     bool m_allowCookies;