Bug 842126: Avoid deadlock if we can't send an Open message r=mcmanus

content/base/src/nsDOMDataChannel.cpp

@@ -63,6 +63,7 @@ public:
     // one) once we block GC until all the (appropriate) onXxxx handlers
     // are dropped. (See WebRTC spec)
     mDataChannel->SetListener(nullptr, nullptr);
+    mDataChannel->Close();
   }
 
   nsresult Init(nsPIDOMWindow* aDOMWindow);

netwerk/sctp/datachannel/DataChannel.cpp

@@ -961,6 +961,7 @@ DataChannelConnection::SendDeferredMessages()
           // delete the channel.
           mStreamsIn[channel->mStreamIn]   = nullptr;
           mStreamsOut[channel->mStreamOut] = nullptr;
+          channel->mState = CLOSED;
         }
       }
     }
@@ -976,7 +977,8 @@ DataChannelConnection::SendDeferredMessages()
           still_blocked = true;
         } else {
           // Close the channel, inform the user
-          Close(channel);
+          CloseInt(channel);
+          // XXX send error via DataChannelOnMessageAvailable (bug 843625)
         }
       }
     }
@@ -1099,10 +1101,20 @@ DataChannelConnection::OpenResponseFinish(already_AddRefed<DataChannel> aChannel
 
   if (streamOut == INVALID_STREAM) {
     if (!RequestMoreStreamsOut()) {
-      /* XXX: Signal error to the other end. */
+      channel->mState = CLOSED;
+      if (channel->mFlags & DATA_CHANNEL_FLAGS_FINISH_RSP) {
+        // We already returned the channel to the app.
+        NS_ERROR("Failed to request more streams");
+        NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
+                                  DataChannelOnMessageAvailable::ON_CHANNEL_CLOSED, this,
+                                  channel));
+      }
+      // If we weren't deferred, we'll be destroying the channel, but it
+      // never really got set up
+      // Alternative would be to RUN_ON_THREAD(channel.forget(),::Destroy,...) and
+      // Dispatch it to ourselves
       mStreamsIn[channel->mStreamIn] = nullptr;
-      // we can do this with the lock held because mStreamOut is INVALID_STREAM,
-      // so there's no outbound channel to reset
+      /* XXX: Signal error to the other end (and maybe fire onError: bug 843625) */
       return;
     }
     LOG(("Queuing channel %d to finish response", channel->mStreamIn));
@@ -1127,13 +1139,21 @@ DataChannelConnection::OpenResponseFinish(already_AddRefed<DataChannel> aChannel
         channel->mFlags |= DATA_CHANNEL_FLAGS_SEND_RSP;
         StartDefer();
       } else {
+        if (channel->mFlags & DATA_CHANNEL_FLAGS_FINISH_RSP) {
+          // We already returned the channel to the app.
+          NS_ERROR("Failed to send open response");
+          NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
+                                    DataChannelOnMessageAvailable::ON_CHANNEL_CLOSED, this,
+                                    channel));
+        }
+
         /* XXX: Signal error to the other end. */
         mStreamsIn[channel->mStreamIn] = nullptr;
         mStreamsOut[streamOut] = nullptr;
         channel->mStreamOut = INVALID_STREAM;
-        // we can do this with the lock held because mStreamOut is INVALID_STREAM,
-        // so there's no outbound channel to reset (we failed to send on it)
-        return; // paranoia against future changes since we unlocked
+        // we'll be destroying the channel if it wasn't already returned
+        channel->mState = CLOSED;
+        return;
       }
     }
   }
@@ -1741,8 +1761,10 @@ DataChannelConnection::HandleStreamChangeEvent(const struct sctp_stream_change_e
           mStreamsIn[channel->mStreamIn] = nullptr;
         }
         channel->mState = CLOSED;
-        // inform user!
-        // XXX delete channel;
+        NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
+                                  DataChannelOnMessageAvailable::ON_CHANNEL_CLOSED, this,
+                                  channel));
+        // maybe fire onError (bug 843625)
       } else {
         streamOut = FindFreeStreamOut();
         if (streamOut != INVALID_STREAM) {
@@ -1888,14 +1910,18 @@ DataChannelConnection::OpenFinish(already_AddRefed<DataChannel> aChannel)
 
   if (streamOut == INVALID_STREAM) {
     if (!RequestMoreStreamsOut()) {
+      channel->mState = CLOSED;
       if (channel->mFlags & DATA_CHANNEL_FLAGS_FINISH_OPEN) {
-        // We already returned the channel to the app.  Mark it closed
-        channel->mState = CLOSED;
+        // We already returned the channel to the app.
         NS_ERROR("Failed to request more streams");
+        NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
+                                  DataChannelOnMessageAvailable::ON_CHANNEL_CLOSED, this,
+                                  channel));
         return channel.forget();
       }
-      // we can do this with the lock held because mStreamOut is INVALID_STREAM,
-      // so there's no outbound channel to reset
+      // we'll be destroying the channel, but it never really got set up
+      // Alternative would be to RUN_ON_THREAD(channel.forget(),::Destroy,...) and
+      // Dispatch it to ourselves
       return nullptr;
     }
     LOG(("Queuing channel %p to finish open", channel.get()));
@@ -1916,12 +1942,19 @@ DataChannelConnection::OpenFinish(already_AddRefed<DataChannel> aChannel)
       channel->mFlags |= DATA_CHANNEL_FLAGS_SEND_REQ;
       StartDefer();
     } else {
-      // XXX FIX! can't do this if we previously returned it!  Need to internally mark it dead
-      // and file onerror
+      if (channel->mFlags & DATA_CHANNEL_FLAGS_FINISH_OPEN) {
+        // We already returned the channel to the app.
+        NS_ERROR("Failed to send open request");
+        NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
+                                  DataChannelOnMessageAvailable::ON_CHANNEL_CLOSED, this,
+                                  channel));
+      }
+      // If we haven't returned the channel yet, it will get destroyed when we exit
+      // this function.
       mStreamsOut[streamOut] = nullptr;
       channel->mStreamOut = INVALID_STREAM;
-      // we can do this with the lock held because mStreamOut is INVALID_STREAM,
-      // so there's no outbound channel to reset (we didn't sent anything)
+      // we'll be destroying the channel
+      channel->mState = CLOSED;
       return nullptr;
     }
   }
@@ -2094,10 +2127,7 @@ DataChannelConnection::SendMsgCommon(uint16_t stream, const nsACString &aMsg,
   uint32_t len     = aMsg.Length();
   DataChannel *channel;
 
-  if (isBinary)
-    LOG(("Sending to stream %u: %u bytes", stream, len));
-  else
-    LOG(("Sending to stream %u: %s", stream, data));
+  LOG(("Sending %sto stream %u: %u bytes", isBinary ? "binary " : "", stream, len));
   // XXX if we want more efficiency, translate flags once at open time
   channel = mStreamsOut[stream];
   NS_ENSURE_TRUE(channel, 0);
@@ -2109,29 +2139,39 @@ DataChannelConnection::SendMsgCommon(uint16_t stream, const nsACString &aMsg,
 
 void
 DataChannelConnection::Close(DataChannel *aChannel)
+{
+  MutexAutoLock lock(mLock);
+  CloseInt(aChannel);
+}
+
+// So we can call Close() with the lock already held
+// Called from someone who holds a ref via ::Close(), or from ~DataChannel
+void
+DataChannelConnection::CloseInt(DataChannel *aChannel)
 {
   MOZ_ASSERT(aChannel);
   nsRefPtr<DataChannel> channel(aChannel); // make sure it doesn't go away on us
 
-  MutexAutoLock lock(mLock);
+  mLock.AssertCurrentThreadOwns();
   LOG(("Connection %p/Channel %p: Closing stream %d",
-       channel->mConnection.get(), channel.get(), channel->mStreamOut));
-  if (channel->mState == CLOSED || channel->mState == CLOSING) {
-    LOG(("Channel already closing/closed (%d)", channel->mState));
+       aChannel->mConnection.get(), aChannel, aChannel->mStreamOut));
+  // re-test since it may have closed before the lock was grabbed
+  if (aChannel->mState == CLOSED || aChannel->mState == CLOSING) {
+    LOG(("Channel already closing/closed (%d)", aChannel->mState));
     return;
   }
-  channel->mBufferedData.Clear();
-  if (channel->mStreamOut != INVALID_STREAM) {
-    ResetOutgoingStream(channel->mStreamOut);
+  aChannel->mBufferedData.Clear();
+  if (aChannel->mStreamOut != INVALID_STREAM) {
+    ResetOutgoingStream(aChannel->mStreamOut);
     if (mState == CLOSED) { // called from CloseAll()
       // Let resets accumulate then send all at once in CloseAll()
       // we're not going to hang around waiting
-      mStreamsOut[channel->mStreamOut] = nullptr;
+      mStreamsOut[aChannel->mStreamOut] = nullptr;
     } else {
       SendOutgoingStreamReset();
     }
   }
-  channel->mState = CLOSING;
+  aChannel->mState = CLOSING;
   if (mState == CLOSED) {
     // we're not going to hang around waiting
     if (channel->mStreamOut != INVALID_STREAM) {
@@ -2178,7 +2218,10 @@ void DataChannelConnection::CloseAll()
 
 DataChannel::~DataChannel()
 {
-  Close();
+  // NS_ASSERTION since this is more "I think I caught all the cases that
+  // 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

netwerk/sctp/datachannel/DataChannel.h

@@ -141,6 +141,8 @@ public:
                                      nsISupports *aContext);
 
   void Close(DataChannel *aChannel);
+  // CloseInt() must be called with mLock held
+  void CloseInt(DataChannel *aChannel);
   void CloseAll();
 
   int32_t SendMsg(uint16_t stream, const nsACString &aMsg)
@@ -312,7 +314,8 @@ public:
 
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DataChannel)
 
-  // Close this DataChannel.  Can be called multiple times.
+  // 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)