Bug 1111971 - A better life-time management of aListener and aContext in WebSocketChannel. r=smaug
☠☠ backed out by b74c9afa3b4b ☠ ☠
authorAndrea Marchesini <amarchesini@mozilla.com>
Fri, 09 Jan 2015 09:23:13 -0500
changeset 236025 fce8528ac1e2385acd313595e52a4367cfd30ca8
parent 236024 936382a9e919c911ec18daf5452129b7a361a00b
child 236026 a5e88c805f02b20b1e36a43459b3317d443a67f3
push id384
push usermartin.thomson@gmail.com
push dateFri, 09 Jan 2015 21:26:39 +0000
reviewerssmaug
bugs1111971
milestone37.0a1
Bug 1111971 - A better life-time management of aListener and aContext in WebSocketChannel. r=smaug
dom/base/WebSocket.cpp
netwerk/protocol/websocket/WebSocketChannel.cpp
--- a/dom/base/WebSocket.cpp
+++ b/dom/base/WebSocket.cpp
@@ -641,23 +641,24 @@ WebSocketImpl::DoOnMessageAvailable(cons
   }
 
   if (readyState == WebSocket::OPEN) {
     // Dispatch New Message
     nsresult rv = mWebSocket->CreateAndDispatchMessageEvent(aMsg, isBinary);
     if (NS_FAILED(rv)) {
       NS_WARNING("Failed to dispatch the message event");
     }
-  } else {
-    // CLOSING should be the only other state where it's possible to get msgs
-    // from channel: Spec says to drop them.
-    MOZ_ASSERT(readyState == WebSocket::CLOSING,
-               "Received message while CONNECTING or CLOSED");
+
+    return NS_OK;
   }
 
+  // CLOSING should be the only other state where it's possible to get msgs
+  // from channel: Spec says to drop them.
+  MOZ_ASSERT(readyState == WebSocket::CLOSING,
+             "Received message while CONNECTING or CLOSED");
   return NS_OK;
 }
 
 NS_IMETHODIMP
 WebSocketImpl::OnMessageAvailable(nsISupports* aContext,
                                   const nsACString& aMsg)
 {
   AssertIsOnTargetThread();
@@ -713,24 +714,27 @@ WebSocketImpl::OnStart(nsISupports* aCon
     mChannel->GetProtocol(mWebSocket->mEstablishedProtocol);
   }
 
   mChannel->GetExtensions(mWebSocket->mEstablishedExtensions);
   UpdateURI();
 
   mWebSocket->SetReadyState(WebSocket::OPEN);
 
+  // Let's keep the object alive because the webSocket can be CCed in the
+  // onopen callback.
+  nsRefPtr<WebSocket> webSocket = mWebSocket;
+
   // Call 'onopen'
-  rv = mWebSocket->CreateAndDispatchSimpleEvent(NS_LITERAL_STRING("open"));
+  rv = webSocket->CreateAndDispatchSimpleEvent(NS_LITERAL_STRING("open"));
   if (NS_FAILED(rv)) {
     NS_WARNING("Failed to dispatch the open event");
   }
 
-  mWebSocket->UpdateMustKeepAlive();
-
+  webSocket->UpdateMustKeepAlive();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 WebSocketImpl::OnStop(nsISupports* aContext, nsresult aStatusCode)
 {
   AssertIsOnTargetThread();
 
@@ -1591,33 +1595,37 @@ WebSocketImpl::DispatchConnectionCloseEv
   AssertIsOnTargetThread();
 
   if (mDisconnectingOrDisconnected) {
     return;
   }
 
   mWebSocket->SetReadyState(WebSocket::CLOSED);
 
+  // Let's keep the object alive because the webSocket can be CCed in the
+  // onerror or in the onclose callback.
+  nsRefPtr<WebSocket> webSocket = mWebSocket;
+
   // Call 'onerror' if needed
   if (mFailed) {
     nsresult rv =
-      mWebSocket->CreateAndDispatchSimpleEvent(NS_LITERAL_STRING("error"));
+      webSocket->CreateAndDispatchSimpleEvent(NS_LITERAL_STRING("error"));
     if (NS_FAILED(rv)) {
       NS_WARNING("Failed to dispatch the error event");
     }
   }
 
-  nsresult rv = mWebSocket->CreateAndDispatchCloseEvent(mCloseEventWasClean,
-                                                        mCloseEventCode,
-                                                        mCloseEventReason);
+  nsresult rv = webSocket->CreateAndDispatchCloseEvent(mCloseEventWasClean,
+                                                       mCloseEventCode,
+                                                       mCloseEventReason);
   if (NS_FAILED(rv)) {
     NS_WARNING("Failed to dispatch the close event");
   }
 
-  mWebSocket->UpdateMustKeepAlive();
+  webSocket->UpdateMustKeepAlive();
   Disconnect();
 }
 
 nsresult
 WebSocket::CreateAndDispatchSimpleEvent(const nsAString& aName)
 {
   MOZ_ASSERT(mImpl);
   AssertIsOnTargetThread();
--- a/netwerk/protocol/websocket/WebSocketChannel.cpp
+++ b/netwerk/protocol/websocket/WebSocketChannel.cpp
@@ -67,16 +67,32 @@
 // dupe one constant we need from it
 #define CLOSE_GOING_AWAY 1001
 
 extern PRThread *gSocketThread;
 
 using namespace mozilla;
 using namespace mozilla::net;
 
+static void
+ReleaseListenerAndContext(nsCOMPtr<nsIWebSocketListener>& aListener,
+                          nsCOMPtr<nsISupports>& aContext)
+{
+  nsCOMPtr<nsIThread> mainThread;
+  NS_GetMainThread(getter_AddRefs(mainThread));
+
+  if (aListener) {
+    NS_ProxyRelease(mainThread, aListener, false);
+  }
+
+  if (aContext) {
+    NS_ProxyRelease(mainThread, aContext, false);
+  }
+}
+
 namespace mozilla {
 namespace net {
 
 NS_IMPL_ISUPPORTS(WebSocketChannel,
                   nsIWebSocketChannel,
                   nsIHttpUpgradeListener,
                   nsIRequestObserver,
                   nsIStreamListener,
@@ -550,31 +566,35 @@ StaticMutex           nsWSAdmissionManag
 // CallOnMessageAvailable
 //-----------------------------------------------------------------------------
 
 class CallOnMessageAvailable MOZ_FINAL : public nsIRunnable
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
 
-  CallOnMessageAvailable(WebSocketChannel *aChannel,
-                         nsCString        &aData,
-                         int32_t           aLen)
+  CallOnMessageAvailable(WebSocketChannel* aChannel,
+                         nsACString& aData,
+                         int32_t aLen)
     : mChannel(aChannel),
       mData(aData),
       mLen(aLen) {}
 
   NS_IMETHOD Run() MOZ_OVERRIDE
   {
     MOZ_ASSERT(mChannel->IsOnTargetThread());
 
-    if (mLen < 0)
-      mChannel->mListener->OnMessageAvailable(mChannel->mContext, mData);
-    else
-      mChannel->mListener->OnBinaryMessageAvailable(mChannel->mContext, mData);
+    if (mChannel->mListener) {
+      if (mLen < 0) {
+        mChannel->mListener->OnMessageAvailable(mChannel->mContext, mData);
+      } else {
+        mChannel->mListener->OnBinaryMessageAvailable(mChannel->mContext, mData);
+      }
+    }
+
     return NS_OK;
   }
 
 private:
   ~CallOnMessageAvailable() {}
 
   nsRefPtr<WebSocketChannel>        mChannel;
   nsCString                         mData;
@@ -586,64 +606,74 @@ NS_IMPL_ISUPPORTS(CallOnMessageAvailable
 // CallOnStop
 //-----------------------------------------------------------------------------
 
 class CallOnStop MOZ_FINAL : public nsIRunnable
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
 
-  CallOnStop(WebSocketChannel *aChannel,
-             nsresult          aReason)
+  // CallOnStop steals mListener and mContext from the aChannel in order to
+  // release them on the main thread when needed.
+  CallOnStop(WebSocketChannel* aChannel,
+             nsresult aReason)
     : mChannel(aChannel),
-      mReason(aReason) {}
+      mReason(aReason)
+  {
+    mListener.swap(mChannel->mListener);
+    mContext.swap(mChannel->mContext);
+  }
 
   NS_IMETHOD Run() MOZ_OVERRIDE
   {
     MOZ_ASSERT(mChannel->IsOnTargetThread());
 
     nsWSAdmissionManager::OnStopSession(mChannel, mReason);
 
-    if (mChannel->mListener) {
-      mChannel->mListener->OnStop(mChannel->mContext, mReason);
-      mChannel->mListener = nullptr;
-      mChannel->mContext = nullptr;
+    if (mListener) {
+      mListener->OnStop(mContext, mReason);
+      ReleaseListenerAndContext(mListener, mContext);
     }
+
     return NS_OK;
   }
 
 private:
   ~CallOnStop() {}
 
   nsRefPtr<WebSocketChannel>        mChannel;
+  nsCOMPtr<nsIWebSocketListener>    mListener;
+  nsCOMPtr<nsISupports>             mContext;
   nsresult                          mReason;
 };
 NS_IMPL_ISUPPORTS(CallOnStop, nsIRunnable)
 
 //-----------------------------------------------------------------------------
 // CallOnServerClose
 //-----------------------------------------------------------------------------
 
 class CallOnServerClose MOZ_FINAL : public nsIRunnable
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
 
-  CallOnServerClose(WebSocketChannel *aChannel,
-                    uint16_t          aCode,
-                    nsCString        &aReason)
+  CallOnServerClose(WebSocketChannel* aChannel,
+                    uint16_t aCode,
+                    nsACString& aReason)
     : mChannel(aChannel),
       mCode(aCode),
       mReason(aReason) {}
 
   NS_IMETHOD Run() MOZ_OVERRIDE
   {
     MOZ_ASSERT(mChannel->IsOnTargetThread());
 
-    mChannel->mListener->OnServerClose(mChannel->mContext, mCode, mReason);
+    if (mChannel->mListener) {
+      mChannel->mListener->OnServerClose(mChannel->mContext, mCode, mReason);
+    }
     return NS_OK;
   }
 
 private:
   ~CallOnServerClose() {}
 
   nsRefPtr<WebSocketChannel>        mChannel;
   uint16_t                          mCode;
@@ -653,27 +683,29 @@ NS_IMPL_ISUPPORTS(CallOnServerClose, nsI
 
 //-----------------------------------------------------------------------------
 // CallAcknowledge
 //-----------------------------------------------------------------------------
 
 class CallAcknowledge MOZ_FINAL : public nsCancelableRunnable
 {
 public:
-  CallAcknowledge(WebSocketChannel *aChannel,
-                  uint32_t          aSize)
+  CallAcknowledge(WebSocketChannel* aChannel,
+                  uint32_t aSize)
     : mChannel(aChannel),
       mSize(aSize) {}
 
   NS_IMETHOD Run()
   {
     MOZ_ASSERT(mChannel->IsOnTargetThread());
 
     LOG(("WebSocketChannel::CallAcknowledge: Size %u\n", mSize));
-    mChannel->mListener->OnAcknowledge(mChannel->mContext, mSize);
+    if (mChannel->mListener) {
+      mChannel->mListener->OnAcknowledge(mChannel->mContext, mSize);
+    }
     return NS_OK;
   }
 
 private:
   ~CallAcknowledge() {}
 
   nsRefPtr<WebSocketChannel>        mChannel;
   uint32_t                          mSize;
@@ -1169,27 +1201,17 @@ WebSocketChannel::~WebSocketChannel()
     NS_ProxyRelease(mainThread, forgettable, false);
   }
 
   if (mOriginalURI) {
     mOriginalURI.forget(&forgettable);
     NS_ProxyRelease(mainThread, forgettable, false);
   }
 
-  if (mListener) {
-    nsIWebSocketListener *forgettableListener;
-    mListener.forget(&forgettableListener);
-    NS_ProxyRelease(mainThread, forgettableListener, false);
-  }
-
-  if (mContext) {
-    nsISupports *forgettableContext;
-    mContext.forget(&forgettableContext);
-    NS_ProxyRelease(mainThread, forgettableContext, false);
-  }
+  ReleaseListenerAndContext(mListener, mContext);
 
   if (mLoadGroup) {
     nsILoadGroup *forgettableGroup;
     mLoadGroup.forget(&forgettableGroup);
     NS_ProxyRelease(mainThread, forgettableGroup, false);
   }
 
   if (mLoadInfo) {
@@ -2260,18 +2282,27 @@ WebSocketChannel::StopSession(nsresult r
     mCancelable->Cancel(NS_ERROR_UNEXPECTED);
     mCancelable = nullptr;
   }
 
   mPMCECompressor = nullptr;
 
   if (!mCalledOnStop) {
     mCalledOnStop = 1;
-    mTargetThread->Dispatch(new CallOnStop(this, reason),
-                            NS_DISPATCH_NORMAL);
+
+    nsWSAdmissionManager::OnStopSession(this, reason);
+
+    nsRefPtr<CallOnStop> runnable = new CallOnStop(this, reason);
+
+    // CallOnStop steals mListener and mContext in order to release them on the
+    // main thread when needed.
+    MOZ_ASSERT(!mListener);
+    MOZ_ASSERT(!mContext);
+
+    mTargetThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
   }
 }
 
 void
 WebSocketChannel::AbortSession(nsresult reason)
 {
   LOG(("WebSocketChannel::AbortSession() %p [reason %x] stopped = %d\n",
        this, reason, mStopped));