Bug 1280041: more complete fix for issues surrounding mConnection clearing r=drno
authorRandell Jesup <rjesup@jesup.org>
Thu, 20 Oct 2016 01:03:44 -0400
changeset 318720 d4532c93e46c015e8fd91f07b1c0101dd8c18828
parent 318719 642ee7eb70bbe1d512a06cf6478910bd2d0db273
child 318721 a8e964a086a0da826239b9089af8c9f3175ef24c
push id20727
push usercbook@mozilla.com
push dateThu, 20 Oct 2016 14:51:54 +0000
treeherderfx-team@11ae641575dc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdrno
bugs1280041
milestone52.0a1
Bug 1280041: more complete fix for issues surrounding mConnection clearing r=drno
dom/base/nsDOMDataChannel.cpp
netwerk/sctp/datachannel/DataChannel.cpp
netwerk/sctp/datachannel/DataChannel.h
--- a/dom/base/nsDOMDataChannel.cpp
+++ b/dom/base/nsDOMDataChannel.cpp
@@ -111,16 +111,18 @@ nsDOMDataChannel::Init(nsPIDOMWindowInne
   return rv;
 }
 
 NS_IMPL_EVENT_HANDLER(nsDOMDataChannel, open)
 NS_IMPL_EVENT_HANDLER(nsDOMDataChannel, error)
 NS_IMPL_EVENT_HANDLER(nsDOMDataChannel, close)
 NS_IMPL_EVENT_HANDLER(nsDOMDataChannel, message)
 
+// Most of the GetFoo()/SetFoo()s don't need to touch shared resources and
+// are safe after Close()
 NS_IMETHODIMP
 nsDOMDataChannel::GetLabel(nsAString& aLabel)
 {
   mDataChannel->GetLabel(aLabel);
   return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -175,17 +177,21 @@ nsDOMDataChannel::ReadyState() const
 {
   return static_cast<RTCDataChannelState>(mDataChannel->GetReadyState());
 }
 
 
 NS_IMETHODIMP
 nsDOMDataChannel::GetReadyState(nsAString& aReadyState)
 {
-  uint16_t readyState = mDataChannel->GetReadyState();
+  // mState is handled on multiple threads and needs locking
+  uint16_t readyState = mozilla::DataChannel::CLOSED;
+  if (!mSentClose) {
+    readyState = mDataChannel->GetReadyState();
+  }
   // From the WebRTC spec
   const char * stateName[] = {
     "connecting",
     "open",
     "closing",
     "closed"
   };
   MOZ_ASSERT(/*readyState >= mozilla::DataChannel::CONNECTING && */ // Always true due to datatypes
@@ -193,17 +199,20 @@ nsDOMDataChannel::GetReadyState(nsAStrin
   aReadyState.AssignASCII(stateName[readyState]);
 
   return NS_OK;
 }
 
 uint32_t
 nsDOMDataChannel::BufferedAmount() const
 {
-  return mDataChannel->GetBufferedAmount();
+  if (!mSentClose) {
+    return mDataChannel->GetBufferedAmount();
+  }
+  return 0;
 }
 
 uint32_t
 nsDOMDataChannel::BufferedAmountLowThreshold() const
 {
   return mDataChannel->GetBufferedAmountLowThreshold();
 }
 
@@ -323,17 +332,20 @@ nsDOMDataChannel::Send(const ArrayBuffer
 void
 nsDOMDataChannel::Send(nsIInputStream* aMsgStream,
                        const nsACString& aMsgString,
                        uint32_t aMsgLength,
                        bool aIsBinary,
                        ErrorResult& aRv)
 {
   MOZ_ASSERT(NS_IsMainThread());
-  uint16_t state = mDataChannel->GetReadyState();
+  uint16_t state = mozilla::DataChannel::CLOSED;
+  if (!mSentClose) {
+    state = mDataChannel->GetReadyState();
+  }
 
   // In reality, the DataChannel protocol allows this, but we want it to
   // look like WebSockets
   if (state == mozilla::DataChannel::CONNECTING) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return;
   }
 
@@ -460,16 +472,18 @@ nsDOMDataChannel::OnChannelConnected(nsI
 
 nsresult
 nsDOMDataChannel::OnChannelClosed(nsISupports* aContext)
 {
   nsresult rv;
   // so we don't have to worry if we're notified from different paths in
   // the underlying code
   if (!mSentClose) {
+    // Ok, we're done with it.
+    mDataChannel->ReleaseConnection();
     LOG(("%p(%p): %s - Dispatching\n",this,(void*)mDataChannel,__FUNCTION__));
 
     rv = OnSimpleEvent(aContext, NS_LITERAL_STRING("close"));
     // no more events can happen
     mSentClose = true;
   } else {
     rv = NS_OK;
   }
@@ -491,17 +505,19 @@ nsDOMDataChannel::NotBuffered(nsISupport
   // In the rare case that we held off GC to let the buffer drain
   UpdateMustKeepAlive();
   return NS_OK;
 }
 
 void
 nsDOMDataChannel::AppReady()
 {
-  mDataChannel->AppReady();
+  if (!mSentClose) { // may not be possible, simpler to just test anyways
+    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.
 //-----------------------------------------------------------------------------
--- a/netwerk/sctp/datachannel/DataChannel.cpp
+++ b/netwerk/sctp/datachannel/DataChannel.cpp
@@ -1743,25 +1743,22 @@ DataChannelConnection::HandleStreamReset
                         channel->mState == DataChannel::CLOSING ||
                         channel->mState == DataChannel::CONNECTING ||
                         channel->mState == DataChannel::WAITING_TO_OPEN);
           if (channel->mState == DataChannel::OPEN ||
               channel->mState == DataChannel::WAITING_TO_OPEN) {
             // Mark the stream for reset (the reset is sent below)
             ResetOutgoingStream(channel->mStream);
           }
-          NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable(
-                                    DataChannelOnMessageAvailable::ON_CHANNEL_CLOSED, this,
-                                    channel)));
           mStreams[channel->mStream] = nullptr;
 
           LOG(("Disconnected DataChannel %p from connection %p",
                (void *) channel.get(), (void *) channel->mConnection.get()));
-          channel->DestroyLocked();
-          // At this point when we leave here, the object is a zombie held alive only by the DOM object
+          // This sends ON_CHANNEL_CLOSED to mainthread
+          channel->StreamClosedLocked();
         } else {
           LOG(("Can't find incoming channel %d",i));
         }
       }
     }
   }
 
   // Process any pending resets now:
@@ -2487,17 +2484,17 @@ DataChannelConnection::CloseInt(DataChan
       mStreams[channel->mStream] = nullptr;
     } else {
       SendOutgoingStreamReset();
     }
   }
   aChannel->mState = CLOSING;
   if (mState == CLOSED) {
     // we're not going to hang around waiting
-    channel->DestroyLocked();
+    channel->StreamClosedLocked();
   }
   // At this point when we leave here, the object is a zombie held alive only by the DOM object
 }
 
 void DataChannelConnection::CloseAll()
 {
   LOG(("Closing all channels (connection %p)", (void*) this));
   // Don't need to lock here
@@ -2540,36 +2537,45 @@ DataChannel::~DataChannel()
   // can cause this" than a true kill-the-program assertion.  If this is
   // wrong, nothing bad happens.  A worst it's a leak.
   NS_ASSERTION(mState == CLOSED || mState == CLOSING, "unexpected state in ~DataChannel");
 }
 
 void
 DataChannel::Close()
 {
-  ENSURE_DATACONNECTION;
-  RefPtr<DataChannelConnection> connection(mConnection);
-  connection->Close(this);
+  if (mConnection) {
+    // ensure we don't get deleted
+    RefPtr<DataChannelConnection> connection(mConnection);
+    connection->Close(this);
+  }
 }
 
 // Used when disconnecting from the DataChannelConnection
 void
-DataChannel::DestroyLocked()
+DataChannel::StreamClosedLocked()
 {
   mConnection->mLock.AssertCurrentThreadOwns();
   ENSURE_DATACONNECTION;
 
   LOG(("Destroying Data channel %u", mStream));
   MOZ_ASSERT_IF(mStream != INVALID_STREAM,
                 !mConnection->FindChannelByStream(mStream));
   mStream = INVALID_STREAM;
   mState = CLOSED;
   NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable(
                                       DataChannelOnMessageAvailable::ON_CHANNEL_CLOSED,
                                       mConnection, this)));
+  // We leave mConnection live until the DOM releases us, to avoid races
+}
+
+void
+DataChannel::ReleaseConnection()
+{
+  ASSERT_WEBRTC(NS_IsMainThread());
   mConnection = nullptr;
 }
 
 void
 DataChannel::SetListener(DataChannelListener *aListener, nsISupports *aContext)
 {
   MutexAutoLock mLock(mListenerLock);
   mContext = aContext;
--- a/netwerk/sctp/datachannel/DataChannel.h
+++ b/netwerk/sctp/datachannel/DataChannel.h
@@ -280,20 +280,20 @@ private:
   uint16_t mLocalPort; // Accessed from connect thread
   uint16_t mRemotePort;
   bool mUsingDtls;
 
   nsCOMPtr<nsIThread> mInternalIOThread;
 };
 
 #define ENSURE_DATACONNECTION \
-  do { if (!mConnection) { DATACHANNEL_LOG(("%s: %p no connection!",__FUNCTION__, this)); return; } } while (0)
+  do { MOZ_ASSERT(mConnection); if (!mConnection) { return; } } while (0)
 
 #define ENSURE_DATACONNECTION_RET(x) \
-  do { if (!mConnection) { DATACHANNEL_LOG(("%s: %p no connection!",__FUNCTION__, this)); return (x); } } while (0)
+  do { MOZ_ASSERT(mConnection); if (!mConnection) { return (x); } } while (0)
 
 class DataChannel {
 public:
   enum {
     CONNECTING = 0U,
     OPEN = 1U,
     CLOSING = 2U,
     CLOSED = 3U,
@@ -329,17 +329,22 @@ public:
 
 private:
   ~DataChannel();
 
 public:
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DataChannel)
 
   // when we disconnect from the connection after stream RESET
-  void DestroyLocked();
+  void StreamClosedLocked();
+
+  // Complete dropping of the link between DataChannel and the connection.
+  // After this, except for a few methods below listed to be safe, you can't
+  // call into DataChannel.
+  void ReleaseConnection();
 
   // Close this DataChannel.  Can be called multiple times.  MUST be called
   // before destroying the DataChannel (state must be CLOSED or CLOSING).
   void Close();
 
   // Set the listener (especially for channels created from the other side)
   void SetListener(DataChannelListener *aListener, nsISupports *aContext);