From 802a5d0f266083c8cbe4e8ed9b69a66374965948 Mon Sep 17 00:00:00 2001 From: Andrea Marchesini Date: Tue, 13 Jan 2015 14:03:56 -0500 Subject: [PATCH] Bug 1111971 - A better life-time management of aListener and aContext in WebSocketChannel. r=smaug CLOSED TREE --- dom/base/WebSocket.cpp | 34 +++-- .../websocket/BaseWebSocketChannel.cpp | 22 ++++ .../protocol/websocket/BaseWebSocketChannel.h | 17 ++- .../protocol/websocket/WebSocketChannel.cpp | 118 ++++++++++-------- .../websocket/WebSocketChannelChild.cpp | 31 ++--- 5 files changed, 139 insertions(+), 83 deletions(-) diff --git a/dom/base/WebSocket.cpp b/dom/base/WebSocket.cpp index b0d6df18d48..dea1b9825bc 100644 --- a/dom/base/WebSocket.cpp +++ b/dom/base/WebSocket.cpp @@ -646,13 +646,14 @@ WebSocketImpl::DoOnMessageAvailable(const nsACString& aMsg, bool 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; } @@ -718,14 +719,17 @@ WebSocketImpl::OnStart(nsISupports* aContext) mWebSocket->SetReadyState(WebSocket::OPEN); + // Let's keep the object alive because the webSocket can be CCed in the + // onopen callback. + nsRefPtr 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; } @@ -1596,23 +1600,27 @@ WebSocketImpl::DispatchConnectionCloseEvents() 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 = 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(); } diff --git a/netwerk/protocol/websocket/BaseWebSocketChannel.cpp b/netwerk/protocol/websocket/BaseWebSocketChannel.cpp index e36573c8f51..e3a8241c653 100644 --- a/netwerk/protocol/websocket/BaseWebSocketChannel.cpp +++ b/netwerk/protocol/websocket/BaseWebSocketChannel.cpp @@ -10,6 +10,7 @@ #include "nsILoadGroup.h" #include "nsIInterfaceRequestor.h" #include "nsAutoPtr.h" +#include "nsProxyRelease.h" #include "nsStandardURL.h" #if defined(PR_LOGGING) @@ -284,5 +285,26 @@ BaseWebSocketChannel::RetargetDeliveryTo(nsIEventTarget* aTargetThread) return NS_OK; } +BaseWebSocketChannel::ListenerAndContextContainer::ListenerAndContextContainer( + nsIWebSocketListener* aListener, + nsISupports* aContext) + : mListener(aListener) + , mContext(aContext) +{ + MOZ_ASSERT(NS_IsMainThread()); + MOZ_ASSERT(mListener); +} + +BaseWebSocketChannel::ListenerAndContextContainer::~ListenerAndContextContainer() +{ + MOZ_ASSERT(mListener); + + nsCOMPtr mainThread; + NS_GetMainThread(getter_AddRefs(mainThread)); + + NS_ProxyRelease(mainThread, mListener, false); + NS_ProxyRelease(mainThread, mContext, false); +} + } // namespace net } // namespace mozilla diff --git a/netwerk/protocol/websocket/BaseWebSocketChannel.h b/netwerk/protocol/websocket/BaseWebSocketChannel.h index c324716d06d..e31f06f5bb0 100644 --- a/netwerk/protocol/websocket/BaseWebSocketChannel.h +++ b/netwerk/protocol/websocket/BaseWebSocketChannel.h @@ -57,11 +57,24 @@ class BaseWebSocketChannel : public nsIWebSocketChannel, virtual void GetEffectiveURL(nsAString& aEffectiveURL) const = 0; virtual bool IsEncrypted() const = 0; + class ListenerAndContextContainer MOZ_FINAL + { + public: + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ListenerAndContextContainer) + + ListenerAndContextContainer(nsIWebSocketListener* aListener, + nsISupports* aContext); + + ~ListenerAndContextContainer(); + + nsCOMPtr mListener; + nsCOMPtr mContext; + }; + protected: nsCOMPtr mOriginalURI; nsCOMPtr mURI; - nsCOMPtr mListener; - nsCOMPtr mContext; + nsRefPtr mListenerMT; nsCOMPtr mCallbacks; nsCOMPtr mLoadGroup; nsCOMPtr mLoadInfo; diff --git a/netwerk/protocol/websocket/WebSocketChannel.cpp b/netwerk/protocol/websocket/WebSocketChannel.cpp index 591844250af..78a489ebd1c 100644 --- a/netwerk/protocol/websocket/WebSocketChannel.cpp +++ b/netwerk/protocol/websocket/WebSocketChannel.cpp @@ -555,10 +555,11 @@ 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), + mListenerMT(aChannel->mListenerMT), mData(aData), mLen(aLen) {} @@ -566,19 +567,26 @@ public: { MOZ_ASSERT(mChannel->IsOnTargetThread()); - if (mLen < 0) - mChannel->mListener->OnMessageAvailable(mChannel->mContext, mData); - else - mChannel->mListener->OnBinaryMessageAvailable(mChannel->mContext, mData); + if (mListenerMT) { + if (mLen < 0) { + mListenerMT->mListener->OnMessageAvailable(mListenerMT->mContext, + mData); + } else { + mListenerMT->mListener->OnBinaryMessageAvailable(mListenerMT->mContext, + mData); + } + } + return NS_OK; } private: ~CallOnMessageAvailable() {} - nsRefPtr mChannel; - nsCString mData; - int32_t mLen; + nsRefPtr mChannel; + nsRefPtr mListenerMT; + nsCString mData; + int32_t mLen; }; NS_IMPL_ISUPPORTS(CallOnMessageAvailable, nsIRunnable) @@ -591,10 +599,12 @@ class CallOnStop MOZ_FINAL : public nsIRunnable public: NS_DECL_THREADSAFE_ISUPPORTS - CallOnStop(WebSocketChannel *aChannel, - nsresult aReason) + CallOnStop(WebSocketChannel* aChannel, + nsresult aReason) : mChannel(aChannel), - mReason(aReason) {} + mListenerMT(mChannel->mListenerMT), + mReason(aReason) + {} NS_IMETHOD Run() MOZ_OVERRIDE { @@ -602,19 +612,20 @@ public: nsWSAdmissionManager::OnStopSession(mChannel, mReason); - if (mChannel->mListener) { - mChannel->mListener->OnStop(mChannel->mContext, mReason); - mChannel->mListener = nullptr; - mChannel->mContext = nullptr; + if (mListenerMT) { + mListenerMT->mListener->OnStop(mListenerMT->mContext, mReason); + mChannel->mListenerMT = nullptr; } + return NS_OK; } private: ~CallOnStop() {} - nsRefPtr mChannel; - nsresult mReason; + nsRefPtr mChannel; + nsRefPtr mListenerMT; + nsresult mReason; }; NS_IMPL_ISUPPORTS(CallOnStop, nsIRunnable) @@ -627,10 +638,11 @@ 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), + mListenerMT(mChannel->mListenerMT), mCode(aCode), mReason(aReason) {} @@ -638,16 +650,20 @@ public: { MOZ_ASSERT(mChannel->IsOnTargetThread()); - mChannel->mListener->OnServerClose(mChannel->mContext, mCode, mReason); + if (mListenerMT) { + mListenerMT->mListener->OnServerClose(mListenerMT->mContext, mCode, + mReason); + } return NS_OK; } private: ~CallOnServerClose() {} - nsRefPtr mChannel; - uint16_t mCode; - nsCString mReason; + nsRefPtr mChannel; + nsRefPtr mListenerMT; + uint16_t mCode; + nsCString mReason; }; NS_IMPL_ISUPPORTS(CallOnServerClose, nsIRunnable) @@ -658,9 +674,10 @@ NS_IMPL_ISUPPORTS(CallOnServerClose, nsIRunnable) class CallAcknowledge MOZ_FINAL : public nsCancelableRunnable { public: - CallAcknowledge(WebSocketChannel *aChannel, - uint32_t aSize) + CallAcknowledge(WebSocketChannel* aChannel, + uint32_t aSize) : mChannel(aChannel), + mListenerMT(mChannel->mListenerMT), mSize(aSize) {} NS_IMETHOD Run() @@ -668,15 +685,18 @@ public: MOZ_ASSERT(mChannel->IsOnTargetThread()); LOG(("WebSocketChannel::CallAcknowledge: Size %u\n", mSize)); - mChannel->mListener->OnAcknowledge(mChannel->mContext, mSize); + if (mListenerMT) { + mListenerMT->mListener->OnAcknowledge(mListenerMT->mContext, mSize); + } return NS_OK; } private: ~CallAcknowledge() {} - nsRefPtr mChannel; - uint32_t mSize; + nsRefPtr mChannel; + nsRefPtr mListenerMT; + uint32_t mSize; }; //----------------------------------------------------------------------------- @@ -1174,17 +1194,7 @@ WebSocketChannel::~WebSocketChannel() 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); - } + mListenerMT = nullptr; if (mLoadGroup) { nsILoadGroup *forgettableGroup; @@ -1586,7 +1596,7 @@ WebSocketChannel::ProcessInput(uint8_t *buffer, uint32_t count) LOG(("WebSocketChannel:: %stext frame received\n", isDeflated ? "deflated " : "")); - if (mListener) { + if (mListenerMT) { nsCString utf8Data; if (isDeflated) { @@ -1657,7 +1667,7 @@ WebSocketChannel::ProcessInput(uint8_t *buffer, uint32_t count) mCloseTimer->Cancel(); mCloseTimer = nullptr; } - if (mListener) { + if (mListenerMT) { mTargetThread->Dispatch(new CallOnServerClose(this, mServerCloseCode, mServerCloseReason), NS_DISPATCH_NORMAL); @@ -1696,7 +1706,7 @@ WebSocketChannel::ProcessInput(uint8_t *buffer, uint32_t count) LOG(("WebSocketChannel:: %sbinary frame received\n", isDeflated ? "deflated " : "")); - if (mListener) { + if (mListenerMT) { nsCString binaryData; if (isDeflated) { @@ -2265,8 +2275,11 @@ WebSocketChannel::StopSession(nsresult reason) if (!mCalledOnStop) { mCalledOnStop = 1; - mTargetThread->Dispatch(new CallOnStop(this, reason), - NS_DISPATCH_NORMAL); + + nsWSAdmissionManager::OnStopSession(this, reason); + + nsRefPtr runnable = new CallOnStop(this, reason); + mTargetThread->Dispatch(runnable, NS_DISPATCH_NORMAL); } } @@ -2577,10 +2590,10 @@ WebSocketChannel::StartWebsocketData() nsWSAdmissionManager::OnConnected(this); LOG(("WebSocketChannel::StartWebsocketData Notifying Listener %p\n", - mListener.get())); + mListenerMT ? mListenerMT->mListener.get() : nullptr)); - if (mListener) { - mListener->OnStart(mContext); + if (mListenerMT) { + mListenerMT->mListener->OnStart(mListenerMT->mContext); } // Start keepalive ping timer, if we're using keepalive. @@ -2943,7 +2956,7 @@ WebSocketChannel::AsyncOpen(nsIURI *aURI, return NS_ERROR_UNEXPECTED; } - if (mListener || mWasOpened) + if (mListenerMT || mWasOpened) return NS_ERROR_ALREADY_OPENED; nsresult rv; @@ -3109,8 +3122,7 @@ WebSocketChannel::AsyncOpen(nsIURI *aURI, // Only set these if the open was successful: // mWasOpened = 1; - mListener = aListener; - mContext = aContext; + mListenerMT = new ListenerAndContextContainer(aListener, aContext); IncrementSessionCount(); return rv; diff --git a/netwerk/protocol/websocket/WebSocketChannelChild.cpp b/netwerk/protocol/websocket/WebSocketChannelChild.cpp index 02105f33cb3..1c2bb2db62a 100644 --- a/netwerk/protocol/websocket/WebSocketChannelChild.cpp +++ b/netwerk/protocol/websocket/WebSocketChannelChild.cpp @@ -204,9 +204,9 @@ WebSocketChannelChild::OnStart(const nsCString& aProtocol, mEffectiveURL = aEffectiveURL; mEncrypted = aEncrypted; - if (mListener) { + if (mListenerMT) { AutoEventEnqueuer ensureSerialDispatch(mEventQ); - mListener->OnStart(mContext); + mListenerMT->mListener->OnStart(mListenerMT->mContext); } } @@ -246,9 +246,9 @@ void WebSocketChannelChild::OnStop(const nsresult& aStatusCode) { LOG(("WebSocketChannelChild::RecvOnStop() %p\n", this)); - if (mListener) { + if (mListenerMT) { AutoEventEnqueuer ensureSerialDispatch(mEventQ); - mListener->OnStop(mContext, aStatusCode); + mListenerMT->mListener->OnStop(mListenerMT->mContext, aStatusCode); } } @@ -295,9 +295,9 @@ void WebSocketChannelChild::OnMessageAvailable(const nsCString& aMsg) { LOG(("WebSocketChannelChild::RecvOnMessageAvailable() %p\n", this)); - if (mListener) { + if (mListenerMT) { AutoEventEnqueuer ensureSerialDispatch(mEventQ); - mListener->OnMessageAvailable(mContext, aMsg); + mListenerMT->mListener->OnMessageAvailable(mListenerMT->mContext, aMsg); } } @@ -319,9 +319,10 @@ void WebSocketChannelChild::OnBinaryMessageAvailable(const nsCString& aMsg) { LOG(("WebSocketChannelChild::RecvOnBinaryMessageAvailable() %p\n", this)); - if (mListener) { + if (mListenerMT) { AutoEventEnqueuer ensureSerialDispatch(mEventQ); - mListener->OnBinaryMessageAvailable(mContext, aMsg); + mListenerMT->mListener->OnBinaryMessageAvailable(mListenerMT->mContext, + aMsg); } } @@ -361,9 +362,9 @@ void WebSocketChannelChild::OnAcknowledge(const uint32_t& aSize) { LOG(("WebSocketChannelChild::RecvOnAcknowledge() %p\n", this)); - if (mListener) { + if (mListenerMT) { AutoEventEnqueuer ensureSerialDispatch(mEventQ); - mListener->OnAcknowledge(mContext, aSize); + mListenerMT->mListener->OnAcknowledge(mListenerMT->mContext, aSize); } } @@ -409,9 +410,10 @@ WebSocketChannelChild::OnServerClose(const uint16_t& aCode, const nsCString& aReason) { LOG(("WebSocketChannelChild::RecvOnServerClose() %p\n", this)); - if (mListener) { + if (mListenerMT) { AutoEventEnqueuer ensureSerialDispatch(mEventQ); - mListener->OnServerClose(mContext, aCode, aReason); + mListenerMT->mListener->OnServerClose(mListenerMT->mContext, aCode, + aReason); } } @@ -424,7 +426,7 @@ WebSocketChannelChild::AsyncOpen(nsIURI *aURI, LOG(("WebSocketChannelChild::AsyncOpen() %p\n", this)); NS_ABORT_IF_FALSE(NS_IsMainThread(), "not main thread"); - NS_ABORT_IF_FALSE(aURI && aListener && !mListener, + NS_ABORT_IF_FALSE(aURI && aListener && !mListenerMT, "Invalid state for WebSocketChannelChild::AsyncOpen"); mozilla::dom::TabChild* tabChild = nullptr; @@ -454,8 +456,7 @@ WebSocketChannelChild::AsyncOpen(nsIURI *aURI, mOriginalURI = aURI; mURI = mOriginalURI; - mListener = aListener; - mContext = aContext; + mListenerMT = new ListenerAndContextContainer(aListener, aContext); mOrigin = aOrigin; mWasOpened = 1;