Bug 892630 - Fix for SendBlob() IO thread races and event loop spinning. r=smaug, r=ekr, a=sledru
authorRandell Jesup <rjesup@jesup.org>
Sat, 01 Feb 2014 08:09:00 -0500
changeset 176398 eb3b0ef6c34d005bdefda5c99311525e7fe8195e
parent 176397 8934420cbdba0435c2259e8f6c8651ef2c32f4c3
child 176399 72efe9bb86a9a0f8a70befb122ea32b20133ab91
push id445
push userffxbld
push dateMon, 10 Mar 2014 22:05:19 +0000
treeherdermozilla-release@dc38b741b04e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug, ekr, sledru
bugs892630
milestone28.0
Bug 892630 - Fix for SendBlob() IO thread races and event loop spinning. r=smaug, r=ekr, a=sledru
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,24 +211,36 @@ 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) {
-    mInternalIOThread->Shutdown();
+    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
+    if (mInternalIOThread) {
+      mInternalIOThread->Shutdown();
+    }
   }
 }
 
 void
 DataChannelConnection::Destroy()
 {
   // Though it's probably ok to do this and close the sockets;
   // if we really want it to do true clean shutdowns it can
@@ -2262,25 +2275,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 +2311,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: