Bug 892630: Fix for SendBlob() IO thread races and event loop spinning r=smaug
☠☠ backed out by 2cd1363497cc ☠ ☠
authorRandell Jesup <rjesup@jesup.org>
Sat, 01 Feb 2014 08:09:00 -0500
changeset 182527 a9930b656941c2f7917dcb6d329d951af912d2ce
parent 182526 0c194a7651a53aa67438b02f2cbbb74a2f6de61f
child 182528 2cd1363497cc5e8b19f1f4fcccf16ca96bd7e78a
push id3343
push userffxbld
push dateMon, 17 Mar 2014 21:55:32 +0000
treeherdermozilla-beta@2f7d3415f79f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs892630
milestone29.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 892630: Fix for SendBlob() IO thread races and event loop spinning r=smaug
netwerk/sctp/datachannel/DataChannel.cpp
netwerk/sctp/datachannel/DataChannel.h
--- a/netwerk/sctp/datachannel/DataChannel.cpp
+++ b/netwerk/sctp/datachannel/DataChannel.cpp
@@ -30,16 +30,17 @@
 #endif
 
 #include "DataChannelLog.h"
 
 #include "nsServiceManagerUtils.h"
 #include "nsIObserverService.h"
 #include "nsIObserver.h"
 #include "mozilla/Services.h"
+#include "nsThread.h"
 #include "nsThreadUtils.h"
 #include "nsAutoPtr.h"
 #include "nsNetUtil.h"
 #include "mozilla/StaticPtr.h"
 #ifdef MOZ_PEERCONNECTION
 #include "mtransport/runnable_utils.h"
 #endif
 
@@ -210,23 +211,33 @@ DataChannelConnection::~DataChannelConne
   LOG(("Deleting DataChannelConnection %p", (void *) this));
   // This may die on the MainThread, or on the STS thread
   ASSERT_WEBRTC(mState == CLOSED);
   MOZ_ASSERT(!mMasterSocket);
   MOZ_ASSERT(mPending.GetSize() == 0);
 
   // Already disconnected from sigslot/mTransportFlow
   // TransportFlows must be released from the STS thread
-  if (mTransportFlow && !IsSTSThread()) {
-    ASSERT_WEBRTC(mSTS);
-    RUN_ON_THREAD(mSTS, WrapRunnableNM(ReleaseTransportFlow, mTransportFlow.forget()),
-                  NS_DISPATCH_NORMAL);
-  }
+  if (!IsSTSThread()) {
+    ASSERT_WEBRTC(NS_IsMainThread());
+    if (mTransportFlow) {
+      ASSERT_WEBRTC(mSTS);
+      RUN_ON_THREAD(mSTS, WrapRunnableNM(ReleaseTransportFlow, mTransportFlow.forget()),
+                    NS_DISPATCH_NORMAL);
+    }
 
-  if (mInternalIOThread) {
+    if (mInternalIOThread) {
+      // Avoid spinning the event thread from here (which if we're mainthread
+      // is in the event loop already)
+      NS_DispatchToMainThread(WrapRunnable(nsCOMPtr<nsIThread>(mInternalIOThread),
+                                           &nsIThread::Shutdown),
+                              NS_DISPATCH_NORMAL);
+    }
+  } else {
+    // on STS, safe to call shutdown
     mInternalIOThread->Shutdown();
   }
 }
 
 void
 DataChannelConnection::Destroy()
 {
   // Though it's probably ok to do this and close the sockets;
@@ -2262,25 +2273,29 @@ public:
   ReadBlobRunnable(DataChannelConnection* aConnection, uint16_t aStream,
     nsIInputStream* aBlob) :
     mConnection(aConnection),
     mStream(aStream),
     mBlob(aBlob)
   { }
 
   NS_IMETHODIMP Run() {
-    mConnection->ReadBlob(mStream, mBlob);
+    // ReadBlob() is responsible to releasing the reference
+    DataChannelConnection *self = mConnection;
+    self->ReadBlob(mConnection.forget(), mStream, mBlob);
     return NS_OK;
   }
 
 private:
-  // The lifetime of DataChannelConnection will be longer than ReadBlobRunnable.
-  // In ~DataChannelConnection(), it calls mInternalIOThread::Shutdown to make
-  // sure all the jobs are ran.
-  DataChannelConnection* mConnection;
+  // Make sure the Connection doesn't die while there are jobs outstanding.
+  // Let it die (if released by PeerConnectionImpl while we're running)
+  // when we send our runnable back to MainThread.  Then ~DataChannelConnection
+  // can send the IOThread to MainThread to die in a runnable, avoiding
+  // unsafe event loop recursion.  Evil.
+  nsRefPtr<DataChannelConnection> mConnection;
   uint16_t mStream;
   // Use RefCount for preventing the object is deleted when SendBlob returns.
   nsRefPtr<nsIInputStream> mBlob;
 };
 
 int32_t
 DataChannelConnection::SendBlob(uint16_t stream, nsIInputStream *aBlob)
 {
@@ -2294,43 +2309,50 @@ DataChannelConnection::SendBlob(uint16_t
     }
   }
 
   nsCOMPtr<nsIRunnable> runnable = new ReadBlobRunnable(this, stream, aBlob);
   mInternalIOThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
   return 0;
 }
 
-int32_t
-DataChannelConnection::ReadBlob(uint16_t aStream, nsIInputStream* aBlob)
+void
+DataChannelConnection::ReadBlob(already_AddRefed<DataChannelConnection> aThis,
+                                uint16_t aStream, nsIInputStream* aBlob)
 {
-  DataChannel *channel = mStreams[aStream];
-  NS_ENSURE_TRUE(channel, 0);
+  // NOTE: 'aThis' has been forgotten by the caller to avoid releasing
+  // it off mainthread; if PeerConnectionImpl has released then we want
+  // ~DataChannelConnection() to run on MainThread
+
   // XXX to do this safely, we must enqueue these atomically onto the
   // output socket.  We need a sender thread(s?) to enque data into the
   // socket and to avoid main-thread IO that might block.  Even on a
   // background thread, we may not want to block on one stream's data.
   // I.e. run non-blocking and service multiple channels.
 
   // For now as a hack, send as a single blast of queued packets which may
   // be deferred until buffer space is available.
   nsCString temp;
   uint64_t len;
   aBlob->Available(&len);
   nsresult rv = NS_ReadInputStreamToString(aBlob, temp, len);
-
-  NS_ENSURE_SUCCESS(rv, 0);
+  if (NS_FAILED(rv)) {
+    // Bug 966602:  Doesn't return an error to the caller via onerror.
+    // Let aThis (aka this) be released when we exit out of paranoia
+    // instead of calling Release()
+    nsRefPtr<DataChannelConnection> self(aThis);
+    return;
+  }
   aBlob->Close();
   nsCOMPtr<nsIThread> mainThread;
   NS_GetMainThread(getter_AddRefs(mainThread));
-  RUN_ON_THREAD(mainThread, WrapRunnable(nsRefPtr<DataChannelConnection>(this),
+  RUN_ON_THREAD(mainThread, WrapRunnable(nsRefPtr<DataChannelConnection>(aThis),
                                &DataChannelConnection::SendBinaryMsg,
                                aStream, temp),
                 NS_DISPATCH_NORMAL);
-  return 0;
 }
 
 int32_t
 DataChannelConnection::SendMsgCommon(uint16_t stream, const nsACString &aMsg,
                                      bool isBinary)
 {
   ASSERT_WEBRTC(NS_IsMainThread());
   // We really could allow this from other threads, so long as we deal with
--- a/netwerk/sctp/datachannel/DataChannel.h
+++ b/netwerk/sctp/datachannel/DataChannel.h
@@ -184,17 +184,17 @@ public:
     CLOSING = 2U,
     CLOSED = 3U
   };
   uint16_t GetReadyState() { MutexAutoLock lock(mLock); return mState; }
 
   friend class DataChannel;
   Mutex  mLock;
 
-  int32_t ReadBlob(uint16_t aStream, nsIInputStream* aBlob);
+  void ReadBlob(already_AddRefed<DataChannelConnection> aThis, uint16_t aStream, nsIInputStream* aBlob);
 
 protected:
   friend class DataChannelOnMessageAvailable;
   // Avoid cycles with PeerConnectionImpl
   // Use from main thread only as WeakPtr is not threadsafe
   WeakPtr<DataConnectionListener> mListener;
 
 private: