Bug 1517601, part 3 - WebSocketEvent subclasses should not hold strong references to the channel. r=mayhemer
authorAndrew McCreight <continuation@gmail.com>
Wed, 13 Feb 2019 16:58:31 +0000
changeset 458931 56f7fee6d139
parent 458930 57206fca017d
child 458932 414b2c238839
push id35551
push usershindli@mozilla.com
push dateWed, 13 Feb 2019 21:34:09 +0000
treeherdermozilla-central@08f794a4928e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs1517601
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1517601, part 3 - WebSocketEvent subclasses should not hold strong references to the channel. r=mayhemer This patch moves the channel pointer from the WebSocketEvents into the classes that wrap them (EventTargetDispatcher and WrappedWebSocketEvent) to fix leaks. EventTargetDispatcher uses a weak pointer. This is needed to prevent a leak if the ChannelEvent dispatch fails, because it would create a cycle of strong references between the WebSocketEvent, the channel, the channel event queue, and EventTargetDispatcher. It is safe because the ChannelEventQueue ensures that the channel remains alive. WrappedWebSocketEvent uses a strong pointer. This won't create a leak because the runnable is not owned by the channel. It is needed for safety because it can't rely on the ChannelEventQueue mechanism for keeping the channel alive. The WPT whitelisting moves them into two subdirectories that still seem to leak sometimes, but the top level websockets/ directory seems okay now. Depends on D19586 Differential Revision: https://phabricator.services.mozilla.com/D19587
netwerk/protocol/websocket/WebSocketChannelChild.cpp
testing/web-platform/meta/websockets/__dir__.ini
testing/web-platform/meta/websockets/binary/__dir__.ini
testing/web-platform/meta/websockets/constructor/__dir__.ini
--- a/netwerk/protocol/websocket/WebSocketChannelChild.cpp
+++ b/netwerk/protocol/websocket/WebSocketChannelChild.cpp
@@ -118,93 +118,98 @@ void WebSocketChannelChild::GetEffective
 }
 
 bool WebSocketChannelChild::IsEncrypted() const { return mEncrypted; }
 
 class WebSocketEvent {
  public:
   WebSocketEvent() { MOZ_COUNT_CTOR(WebSocketEvent); }
   virtual ~WebSocketEvent() { MOZ_COUNT_DTOR(WebSocketEvent); }
-  virtual void Run() = 0;
+  virtual void Run(WebSocketChannelChild* aChild) = 0;
 };
 
 class WrappedWebSocketEvent : public Runnable {
  public:
-  explicit WrappedWebSocketEvent(WebSocketEvent* aWebSocketEvent)
+  WrappedWebSocketEvent(WebSocketChannelChild* aChild,
+                        WebSocketEvent* aWebSocketEvent)
       : Runnable("net::WrappedWebSocketEvent"),
+        mChild(aChild),
         mWebSocketEvent(aWebSocketEvent) {
     MOZ_RELEASE_ASSERT(aWebSocketEvent);
   }
   NS_IMETHOD Run() override {
-    mWebSocketEvent->Run();
+    mWebSocketEvent->Run(mChild);
     return NS_OK;
   }
 
  private:
+  RefPtr<WebSocketChannelChild> mChild;
   nsAutoPtr<WebSocketEvent> mWebSocketEvent;
 };
 
 class EventTargetDispatcher : public ChannelEvent {
  public:
-  EventTargetDispatcher(WebSocketEvent* aWebSocketEvent,
+  EventTargetDispatcher(WebSocketChannelChild* aChild,
+                        WebSocketEvent* aWebSocketEvent,
                         nsIEventTarget* aEventTarget)
-      : mWebSocketEvent(aWebSocketEvent), mEventTarget(aEventTarget) {}
+      : mChild(aChild),
+        mWebSocketEvent(aWebSocketEvent),
+        mEventTarget(aEventTarget) {}
 
   void Run() override {
     if (mEventTarget) {
       mEventTarget->Dispatch(
-          new WrappedWebSocketEvent(mWebSocketEvent.forget()),
+          new WrappedWebSocketEvent(mChild, mWebSocketEvent.forget()),
           NS_DISPATCH_NORMAL);
       return;
     }
 
-    mWebSocketEvent->Run();
+    mWebSocketEvent->Run(mChild);
   }
 
   already_AddRefed<nsIEventTarget> GetEventTarget() override {
     nsCOMPtr<nsIEventTarget> target = mEventTarget;
     if (!target) {
       target = GetMainThreadEventTarget();
     }
     return target.forget();
   }
 
  private:
+  // The lifetime of the child is ensured by ChannelEventQueue.
+  WebSocketChannelChild* mChild;
   nsAutoPtr<WebSocketEvent> mWebSocketEvent;
   nsCOMPtr<nsIEventTarget> mEventTarget;
 };
 
 class StartEvent : public WebSocketEvent {
  public:
-  StartEvent(WebSocketChannelChild* aChild, const nsCString& aProtocol,
-             const nsCString& aExtensions, const nsString& aEffectiveURL,
-             bool aEncrypted)
-      : mChild(aChild),
-        mProtocol(aProtocol),
+  StartEvent(const nsCString& aProtocol, const nsCString& aExtensions,
+             const nsString& aEffectiveURL, bool aEncrypted)
+      : mProtocol(aProtocol),
         mExtensions(aExtensions),
         mEffectiveURL(aEffectiveURL),
         mEncrypted(aEncrypted) {}
 
-  void Run() override {
-    mChild->OnStart(mProtocol, mExtensions, mEffectiveURL, mEncrypted);
+  void Run(WebSocketChannelChild* aChild) override {
+    aChild->OnStart(mProtocol, mExtensions, mEffectiveURL, mEncrypted);
   }
 
  private:
-  RefPtr<WebSocketChannelChild> mChild;
   nsCString mProtocol;
   nsCString mExtensions;
   nsString mEffectiveURL;
   bool mEncrypted;
 };
 
 mozilla::ipc::IPCResult WebSocketChannelChild::RecvOnStart(
     const nsCString& aProtocol, const nsCString& aExtensions,
     const nsString& aEffectiveURL, const bool& aEncrypted) {
   mEventQ->RunOrEnqueue(new EventTargetDispatcher(
-      new StartEvent(this, aProtocol, aExtensions, aEffectiveURL, aEncrypted),
+      this, new StartEvent(aProtocol, aExtensions, aEffectiveURL, aEncrypted),
       mTargetThread));
 
   return IPC_OK();
 }
 
 void WebSocketChannelChild::OnStart(const nsCString& aProtocol,
                                     const nsCString& aExtensions,
                                     const nsString& aEffectiveURL,
@@ -224,30 +229,30 @@ void WebSocketChannelChild::OnStart(cons
            "mListenerMT->mListener->OnStart() failed with error 0x%08" PRIx32,
            static_cast<uint32_t>(rv)));
     }
   }
 }
 
 class StopEvent : public WebSocketEvent {
  public:
-  StopEvent(WebSocketChannelChild* aChild, const nsresult& aStatusCode)
-      : mChild(aChild), mStatusCode(aStatusCode) {}
+  explicit StopEvent(const nsresult& aStatusCode) : mStatusCode(aStatusCode) {}
 
-  void Run() override { mChild->OnStop(mStatusCode); }
+  void Run(WebSocketChannelChild* aChild) override {
+    aChild->OnStop(mStatusCode);
+  }
 
  private:
-  RefPtr<WebSocketChannelChild> mChild;
   nsresult mStatusCode;
 };
 
 mozilla::ipc::IPCResult WebSocketChannelChild::RecvOnStop(
     const nsresult& aStatusCode) {
   mEventQ->RunOrEnqueue(new EventTargetDispatcher(
-      new StopEvent(this, aStatusCode), mTargetThread));
+      this, new StopEvent(aStatusCode), mTargetThread));
 
   return IPC_OK();
 }
 
 void WebSocketChannelChild::OnStop(const nsresult& aStatusCode) {
   LOG(("WebSocketChannelChild::RecvOnStop() %p\n", this));
   if (mListenerMT) {
     AutoEventEnqueuer ensureSerialDispatch(mEventQ);
@@ -259,38 +264,36 @@ void WebSocketChannelChild::OnStop(const
            "mListenerMT->mListener->OnStop() failed with error 0x%08" PRIx32,
            static_cast<uint32_t>(rv)));
     }
   }
 }
 
 class MessageEvent : public WebSocketEvent {
  public:
-  MessageEvent(WebSocketChannelChild* aChild, const nsCString& aMessage,
-               bool aBinary)
-      : mChild(aChild), mMessage(aMessage), mBinary(aBinary) {}
+  MessageEvent(const nsCString& aMessage, bool aBinary)
+      : mMessage(aMessage), mBinary(aBinary) {}
 
-  void Run() override {
+  void Run(WebSocketChannelChild* aChild) override {
     if (!mBinary) {
-      mChild->OnMessageAvailable(mMessage);
+      aChild->OnMessageAvailable(mMessage);
     } else {
-      mChild->OnBinaryMessageAvailable(mMessage);
+      aChild->OnBinaryMessageAvailable(mMessage);
     }
   }
 
  private:
-  RefPtr<WebSocketChannelChild> mChild;
   nsCString mMessage;
   bool mBinary;
 };
 
 mozilla::ipc::IPCResult WebSocketChannelChild::RecvOnMessageAvailable(
     const nsCString& aMsg) {
   mEventQ->RunOrEnqueue(new EventTargetDispatcher(
-      new MessageEvent(this, aMsg, false), mTargetThread));
+      this, new MessageEvent(aMsg, false), mTargetThread));
 
   return IPC_OK();
 }
 
 void WebSocketChannelChild::OnMessageAvailable(const nsCString& aMsg) {
   LOG(("WebSocketChannelChild::RecvOnMessageAvailable() %p\n", this));
   if (mListenerMT) {
     AutoEventEnqueuer ensureSerialDispatch(mEventQ);
@@ -304,17 +307,17 @@ void WebSocketChannelChild::OnMessageAva
            static_cast<uint32_t>(rv)));
     }
   }
 }
 
 mozilla::ipc::IPCResult WebSocketChannelChild::RecvOnBinaryMessageAvailable(
     const nsCString& aMsg) {
   mEventQ->RunOrEnqueue(new EventTargetDispatcher(
-      new MessageEvent(this, aMsg, true), mTargetThread));
+      this, new MessageEvent(aMsg, true), mTargetThread));
 
   return IPC_OK();
 }
 
 void WebSocketChannelChild::OnBinaryMessageAvailable(const nsCString& aMsg) {
   LOG(("WebSocketChannelChild::RecvOnBinaryMessageAvailable() %p\n", this));
   if (mListenerMT) {
     AutoEventEnqueuer ensureSerialDispatch(mEventQ);
@@ -327,30 +330,30 @@ void WebSocketChannelChild::OnBinaryMess
            "failed with error 0x%08" PRIx32,
            static_cast<uint32_t>(rv)));
     }
   }
 }
 
 class AcknowledgeEvent : public WebSocketEvent {
  public:
-  AcknowledgeEvent(WebSocketChannelChild* aChild, const uint32_t& aSize)
-      : mChild(aChild), mSize(aSize) {}
+  explicit AcknowledgeEvent(const uint32_t& aSize) : mSize(aSize) {}
 
-  void Run() override { mChild->OnAcknowledge(mSize); }
+  void Run(WebSocketChannelChild* aChild) override {
+    aChild->OnAcknowledge(mSize);
+  }
 
  private:
-  RefPtr<WebSocketChannelChild> mChild;
   uint32_t mSize;
 };
 
 mozilla::ipc::IPCResult WebSocketChannelChild::RecvOnAcknowledge(
     const uint32_t& aSize) {
   mEventQ->RunOrEnqueue(new EventTargetDispatcher(
-      new AcknowledgeEvent(this, aSize), mTargetThread));
+      this, new AcknowledgeEvent(aSize), mTargetThread));
 
   return IPC_OK();
 }
 
 void WebSocketChannelChild::OnAcknowledge(const uint32_t& aSize) {
   LOG(("WebSocketChannelChild::RecvOnAcknowledge() %p\n", this));
   if (mListenerMT) {
     AutoEventEnqueuer ensureSerialDispatch(mEventQ);
@@ -363,32 +366,32 @@ void WebSocketChannelChild::OnAcknowledg
            "failed with error 0x%08" PRIx32,
            static_cast<uint32_t>(rv)));
     }
   }
 }
 
 class ServerCloseEvent : public WebSocketEvent {
  public:
-  ServerCloseEvent(WebSocketChannelChild* aChild, const uint16_t aCode,
-                   const nsCString& aReason)
-      : mChild(aChild), mCode(aCode), mReason(aReason) {}
+  ServerCloseEvent(const uint16_t aCode, const nsCString& aReason)
+      : mCode(aCode), mReason(aReason) {}
 
-  void Run() override { mChild->OnServerClose(mCode, mReason); }
+  void Run(WebSocketChannelChild* aChild) override {
+    aChild->OnServerClose(mCode, mReason);
+  }
 
  private:
-  RefPtr<WebSocketChannelChild> mChild;
   uint16_t mCode;
   nsCString mReason;
 };
 
 mozilla::ipc::IPCResult WebSocketChannelChild::RecvOnServerClose(
     const uint16_t& aCode, const nsCString& aReason) {
   mEventQ->RunOrEnqueue(new EventTargetDispatcher(
-      new ServerCloseEvent(this, aCode, aReason), mTargetThread));
+      this, new ServerCloseEvent(aCode, aReason), mTargetThread));
 
   return IPC_OK();
 }
 
 void WebSocketChannelChild::OnServerClose(const uint16_t& aCode,
                                           const nsCString& aReason) {
   LOG(("WebSocketChannelChild::RecvOnServerClose() %p\n", this));
   if (mListenerMT) {
--- a/testing/web-platform/meta/websockets/__dir__.ini
+++ b/testing/web-platform/meta/websockets/__dir__.ini
@@ -1,3 +1,1 @@
-lsan-allowed: [Alloc, Create, Malloc, NewPage, PLDHashTable::Add, PLDHashTable::ChangeTable, Realloc, RecvOnAcknowledge, RecvOnStop, mozilla::BasePrincipal::CreateCodebasePrincipal, mozilla::SchedulerGroup::CreateEventTargetFor, mozilla::ThrottledEventQueue::Create, mozilla::WeakPtr, mozilla::dom::ScriptElement::MaybeProcessScript, mozilla::dom::WebSocket::WebSocket, mozilla::dom::WorkerCSPEventListener::Create, mozilla::dom::nsIContentChild::GetConstructedEventTarget, mozilla::net::WebSocketChannelChild::RecvOnServerClose, mozilla::net::nsStandardURL::TemplatedMutator, nsAtomTable::Atomize, nsDocShell::Create]
-lsan-max-stack-depth: 7
-leak-threshold: [default:51200, tab:51200]
+leak-threshold: [default:51200]
--- a/testing/web-platform/meta/websockets/binary/__dir__.ini
+++ b/testing/web-platform/meta/websockets/binary/__dir__.ini
@@ -1,1 +1,3 @@
-lsan-allowed: [MakeUnique, mozilla::dom::WebSocket::ConstructorCommon, mozilla::net::BaseWebSocketChannel::InitLoadInfo, mozilla::net::WebSocketChannelChild::AsyncOpen, mozilla::net::WebSocketEventService::GetOrCreate]
+lsan-allowed: [MakeUnique, mozilla::dom::WebSocket::ConstructorCommon, mozilla::net::BaseWebSocketChannel::InitLoadInfo, mozilla::net::WebSocketChannelChild::AsyncOpen, mozilla::net::WebSocketEventService::GetOrCreate, Alloc, Create, Malloc, NewPage, PLDHashTable::Add, PLDHashTable::ChangeTable, Realloc, RecvOnAcknowledge, RecvOnStop, mozilla::BasePrincipal::CreateCodebasePrincipal, mozilla::SchedulerGroup::CreateEventTargetFor, mozilla::ThrottledEventQueue::Create, mozilla::WeakPtr, mozilla::dom::ScriptElement::MaybeProcessScript, mozilla::dom::WebSocket::WebSocket, mozilla::dom::WorkerCSPEventListener::Create, mozilla::dom::nsIContentChild::GetConstructedEventTarget, mozilla::net::WebSocketChannelChild::RecvOnServerClose, mozilla::net::nsStandardURL::TemplatedMutator, nsAtomTable::Atomize, nsDocShell::Create]
+lsan-max-stack-depth: 7
+leak-threshold: [tab:51200]
--- a/testing/web-platform/meta/websockets/constructor/__dir__.ini
+++ b/testing/web-platform/meta/websockets/constructor/__dir__.ini
@@ -1,1 +1,3 @@
-lsan-allowed: [MakeUnique, mozilla::dom::WebSocket::ConstructorCommon, mozilla::net::BaseWebSocketChannel::InitLoadInfo, mozilla::net::WebSocketChannelChild::AsyncOpen, mozilla::net::WebSocketEventService::GetOrCreate]
+lsan-allowed: [MakeUnique, mozilla::dom::WebSocket::ConstructorCommon, mozilla::net::BaseWebSocketChannel::InitLoadInfo, mozilla::net::WebSocketChannelChild::AsyncOpen, mozilla::net::WebSocketEventService::GetOrCreate, Alloc, Create, Malloc, NewPage, PLDHashTable::Add, PLDHashTable::ChangeTable, Realloc, RecvOnAcknowledge, RecvOnStop, mozilla::BasePrincipal::CreateCodebasePrincipal, mozilla::SchedulerGroup::CreateEventTargetFor, mozilla::ThrottledEventQueue::Create, mozilla::WeakPtr, mozilla::dom::ScriptElement::MaybeProcessScript, mozilla::dom::WebSocket::WebSocket, mozilla::dom::WorkerCSPEventListener::Create, mozilla::dom::nsIContentChild::GetConstructedEventTarget, mozilla::net::WebSocketChannelChild::RecvOnServerClose, mozilla::net::nsStandardURL::TemplatedMutator, nsAtomTable::Atomize, nsDocShell::Create]
+lsan-max-stack-depth: 7
+leak-threshold: [tab:51200]