Bug 964092: don't let DOM DataChannels get GC'd if they have an active callback r=smaug,jib

MozReview-Commit-ID: 48gjdDpHOM9

dom/base/nsDOMDataChannel.cpp

@@ -23,6 +23,7 @@
 #include "nsContentUtils.h"
 #include "nsCycleCollectionParticipant.h"
 #include "nsIScriptObjectPrincipal.h"
+#include "nsProxyRelease.h"
 
 #include "DataChannel.h"
 #include "DataChannelLog.h"
@@ -44,7 +45,7 @@ nsDOMDataChannel::~nsDOMDataChannel()
   // Don't call us anymore!  Likely isn't an issue (or maybe just less of
   // one) once we block GC until all the (appropriate) onXxxx handlers
   // are dropped. (See WebRTC spec)
-  LOG(("Close()ing %p", mDataChannel.get()));
+  LOG(("%p: Close()ing %p", this, mDataChannel.get()));
   mDataChannel->SetListener(nullptr, nullptr);
   mDataChannel->Close();
 }
@@ -77,6 +78,8 @@ nsDOMDataChannel::nsDOMDataChannel(already_AddRefed<mozilla::DataChannel>& aData
   : DOMEventTargetHelper(aWindow)
   , mDataChannel(aDataChannel)
   , mBinaryType(DC_BINARY_TYPE_BLOB)
+  , mCheckMustKeepAlive(true)
+  , mSentClose(false)
 {
 }
 
@@ -263,6 +266,7 @@ NS_IMETHODIMP
 nsDOMDataChannel::Close()
 {
   mDataChannel->Close();
+  UpdateMustKeepAlive();
   return NS_OK;
 }
 
@@ -471,9 +475,20 @@ nsDOMDataChannel::OnChannelConnected(nsISupports* aContext)
 nsresult
 nsDOMDataChannel::OnChannelClosed(nsISupports* aContext)
 {
-  LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__));
+  nsresult rv;
+  // so we don't have to worry if we're notified from different paths in
+  // the underlying code
+  if (!mSentClose) {
+    LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__));
 
-  return OnSimpleEvent(aContext, NS_LITERAL_STRING("close"));
+    rv = OnSimpleEvent(aContext, NS_LITERAL_STRING("close"));
+    // no more events can happen
+    mSentClose = true;
+  } else {
+    rv = NS_OK;
+  }
+  DontKeepAliveAnyMore();
+  return rv;
 }
 
 nsresult
@@ -484,12 +499,111 @@ nsDOMDataChannel::OnBufferLow(nsISupports* aContext)
   return OnSimpleEvent(aContext, NS_LITERAL_STRING("bufferedamountlow"));
 }
 
+nsresult
+nsDOMDataChannel::NotBuffered(nsISupports* aContext)
+{
+  // In the rare case that we held off GC to let the buffer drain
+  UpdateMustKeepAlive();
+  return NS_OK;
+}
+
 void
 nsDOMDataChannel::AppReady()
 {
   mDataChannel->AppReady();
 }
 
+//-----------------------------------------------------------------------------
+// Methods that keep alive the DataChannel object when:
+//   1. the object has registered event listeners that can be triggered
+//      ("strong event listeners");
+//   2. there are outgoing not sent messages.
+//-----------------------------------------------------------------------------
+
+void
+nsDOMDataChannel::UpdateMustKeepAlive()
+{
+  MOZ_ASSERT(NS_IsMainThread());
+
+  if (!mCheckMustKeepAlive) {
+    return;
+  }
+
+  bool shouldKeepAlive = false;
+  uint16_t readyState = mDataChannel->GetReadyState();
+
+  switch (readyState)
+  {
+    case DataChannel::CONNECTING:
+    case DataChannel::WAITING_TO_OPEN:
+    {
+      if (mListenerManager &&
+          (mListenerManager->HasListenersFor(nsGkAtoms::onopen) ||
+           mListenerManager->HasListenersFor(nsGkAtoms::onmessage) ||
+           mListenerManager->HasListenersFor(nsGkAtoms::onerror) ||
+           mListenerManager->HasListenersFor(nsGkAtoms::onbufferedamountlow) ||
+           mListenerManager->HasListenersFor(nsGkAtoms::onclose))) {
+        shouldKeepAlive = true;
+      }
+    }
+    break;
+
+    case DataChannel::OPEN:
+    case DataChannel::CLOSING:
+    {
+      if (mDataChannel->GetBufferedAmount() != 0 ||
+          (mListenerManager &&
+           (mListenerManager->HasListenersFor(nsGkAtoms::onmessage) ||
+            mListenerManager->HasListenersFor(nsGkAtoms::onerror) ||
+            mListenerManager->HasListenersFor(nsGkAtoms::onbufferedamountlow) ||
+            mListenerManager->HasListenersFor(nsGkAtoms::onclose)))) {
+        shouldKeepAlive = true;
+      }
+    }
+    break;
+
+    case DataChannel::CLOSED:
+    {
+      shouldKeepAlive = false;
+    }
+  }
+
+  if (mSelfRef && !shouldKeepAlive) {
+    // release our self-reference (safely) by putting it in an event (always)
+    NS_ReleaseOnMainThread(mSelfRef.forget(), true);
+  } else if (!mSelfRef && shouldKeepAlive) {
+    mSelfRef = this;
+  }
+}
+
+void
+nsDOMDataChannel::DontKeepAliveAnyMore()
+{
+  MOZ_ASSERT(NS_IsMainThread());
+
+  if (mSelfRef) {
+    // Since we're on MainThread, force an eventloop trip to avoid deleting ourselves.
+    NS_ReleaseOnMainThread(mSelfRef.forget(), true);
+  }
+
+  mCheckMustKeepAlive = false;
+}
+
+void
+nsDOMDataChannel::EventListenerAdded(nsIAtom* aType)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  UpdateMustKeepAlive();
+}
+
+void
+nsDOMDataChannel::EventListenerRemoved(nsIAtom* aType)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  UpdateMustKeepAlive();
+}
+
+
 /* static */
 nsresult
 NS_NewDOMDataChannel(already_AddRefed<mozilla::DataChannel>&& aDataChannel,

dom/base/nsDOMDataChannel.h

@@ -42,6 +42,10 @@ public:
   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(nsDOMDataChannel,
                                            mozilla::DOMEventTargetHelper)
 
+  // EventTarget
+  virtual void EventListenerAdded(nsIAtom* aType) override;
+  virtual void EventListenerRemoved(nsIAtom* aType) override;
+
   virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
     override;
   nsPIDOMWindowInner* GetParentObject() const
@@ -103,9 +107,20 @@ public:
   virtual nsresult
   OnBufferLow(nsISupports* aContext) override;
 
+  virtual nsresult
+  NotBuffered(nsISupports* aContext) override;
+
   virtual void
   AppReady();
 
+  // if there are "strong event listeners" or outgoing not sent messages
+  // then this method keeps the object alive when js doesn't have strong
+  // references to it.
+  void UpdateMustKeepAlive();
+  // ATTENTION, when calling this method the object can be released
+  // (and possibly collected).
+  void DontKeepAliveAnyMore();
+
 protected:
   ~nsDOMDataChannel();
 
@@ -113,6 +128,8 @@ private:
   void Send(nsIInputStream* aMsgStream, const nsACString& aMsgString,
             uint32_t aMsgLength, bool aIsBinary, mozilla::ErrorResult& aRv);
 
+  // to keep us alive while we have listeners
+  RefPtr<nsDOMDataChannel> mSelfRef;
   // Owning reference
   RefPtr<mozilla::DataChannel> mDataChannel;
   nsString  mOrigin;
@@ -121,6 +138,8 @@ private:
     DC_BINARY_TYPE_BLOB,
   };
   DataChannelBinaryType mBinaryType;
+  bool mCheckMustKeepAlive;
+  bool mSentClose;
 };
 
 #endif // nsDOMDataChannel_h

netwerk/sctp/datachannel/DataChannel.cpp

@@ -1140,6 +1140,15 @@ DataChannelConnection::SendDeferredMessages()
                                                 this, channel)));
             was_over_threshold = false;
           }
+          if (buffered_amount == 0) {
+            // buffered-to-not-buffered transition; tell the DOM code in case this makes it
+            // available for GC
+            LOG(("%s: sending NO_LONGER_BUFFERED for %s/%s: %u", __FUNCTION__,
+                 channel->mLabel.get(), channel->mProtocol.get(), channel->mStream));
+            NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable(
+                                                DataChannelOnMessageAvailable::NO_LONGER_BUFFERED,
+                                                this, channel)));
+          }
         }
       }
       if (channel->mBufferedData.IsEmpty())
@@ -2572,6 +2581,9 @@ DataChannel::DestroyLocked()
                 !mConnection->FindChannelByStream(mStream));
   mStream = INVALID_STREAM;
   mState = CLOSED;
+  NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable(
+                                      DataChannelOnMessageAvailable::ON_CHANNEL_CLOSED,
+                                      mConnection, this)));
   mConnection = nullptr;
 }
 

netwerk/sctp/datachannel/DataChannel.h

@@ -456,6 +456,7 @@ public:
     ON_DATA,
     START_DEFER,
     BUFFER_LOW_THRESHOLD,
+    NO_LONGER_BUFFERED,
   };  /* types */
 
   DataChannelOnMessageAvailable(int32_t     aType,
@@ -493,11 +494,17 @@ public:
   NS_IMETHOD Run()
   {
     MOZ_ASSERT(NS_IsMainThread());
+
+    // Note: calling the listeners can indirectly cause the listeners to be
+    // made available for GC (by removing event listeners), especially for
+    // OnChannelClosed().  We hold a ref to the Channel and the listener
+    // while calling this.
     switch (mType) {
       case ON_DATA:
       case ON_CHANNEL_OPEN:
       case ON_CHANNEL_CLOSED:
       case BUFFER_LOW_THRESHOLD:
+      case NO_LONGER_BUFFERED:
         {
           MutexAutoLock lock(mChannel->mListenerLock);
           if (!mChannel->mListener) {
@@ -522,6 +529,9 @@ public:
             case BUFFER_LOW_THRESHOLD:
               mChannel->mListener->OnBufferLow(mChannel->mContext);
               break;
+            case NO_LONGER_BUFFERED:
+              mChannel->mListener->NotBuffered(mChannel->mContext);
+              break;
           }
           break;
         }

netwerk/sctp/datachannel/DataChannelListener.h

@@ -35,6 +35,9 @@ public:
 
   // Called when the BufferedAmount drops below the BufferedAmountLowThreshold
   virtual nsresult OnBufferLow(nsISupports *aContext) = 0;
+
+  // Called when the BufferedAmount drops to 0
+  virtual nsresult NotBuffered(nsISupports *aContext) = 0;
 };
 
 }