Bug 1030372: use explicit runnable and Release for ReadBlob to avoid wrong-thread release assert r=bsmedberg relanding on a CLOSED TREE
authorRandell Jesup <rjesup@jesup.org>
Sun, 11 Jan 2015 00:28:34 -0500
changeset 223155 238f460e7aedc87a4ce3fdff8b9479256d3f3d58
parent 223154 8f3cc2c90893ab18bcb9eda7961ff3fc08022a99
child 223156 0aaec62e668467cc9e90e6959c3feccfcf0652bc
push id53838
push userkwierso@gmail.com
push dateSun, 11 Jan 2015 20:28:05 +0000
treeherdermozilla-inbound@238f460e7aed [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg
bugs1030372
milestone37.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 1030372: use explicit runnable and Release for ReadBlob to avoid wrong-thread release assert r=bsmedberg relanding on a CLOSED TREE
media/mtransport/runnable_utils.h
netwerk/sctp/datachannel/DataChannel.cpp
--- a/media/mtransport/runnable_utils.h
+++ b/media/mtransport/runnable_utils.h
@@ -31,17 +31,20 @@ RunOnThreadInternal(nsIEventTarget *thre
     nsresult rv;
     rv = thread->IsOnCurrentThread(&on);
 
     // If the target thread has already shut down, we don't want to assert.
     if (rv != NS_ERROR_NOT_INITIALIZED) {
       MOZ_ASSERT(NS_SUCCEEDED(rv));
     }
 
-    NS_ENSURE_SUCCESS(rv, rv);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      // we're going to destroy the runnable on this thread!
+      return rv;
+    }
     if (!on) {
       return thread->Dispatch(runnable_ref, flags);
     }
   }
   return runnable_ref->Run();
 }
 
 template<RunnableResult result>
--- a/netwerk/sctp/datachannel/DataChannel.cpp
+++ b/netwerk/sctp/datachannel/DataChannel.cpp
@@ -36,16 +36,17 @@
 #include "nsIObserver.h"
 #include "mozilla/Services.h"
 #include "nsProxyRelease.h"
 #include "nsThread.h"
 #include "nsThreadUtils.h"
 #include "nsAutoPtr.h"
 #include "nsNetUtil.h"
 #include "mozilla/StaticPtr.h"
+#include "mozilla/unused.h"
 #ifdef MOZ_PEERCONNECTION
 #include "mtransport/runnable_utils.h"
 #endif
 
 #define DATACHANNEL_LOG(args) LOG(args)
 #include "DataChannel.h"
 #include "DataChannelProtocol.h"
 
@@ -2305,17 +2306,17 @@ DataChannelConnection::SendBinary(DataCh
 
 class ReadBlobRunnable : public nsRunnable {
 public:
   ReadBlobRunnable(DataChannelConnection* aConnection, uint16_t aStream,
     nsIInputStream* aBlob) :
     mConnection(aConnection),
     mStream(aStream),
     mBlob(aBlob)
-  { }
+  {}
 
   NS_IMETHODIMP Run() {
     // ReadBlob() is responsible to releasing the reference
     DataChannelConnection *self = mConnection;
     self->ReadBlob(mConnection.forget(), mStream, mBlob);
     return NS_OK;
   }
 
@@ -2344,49 +2345,89 @@ DataChannelConnection::SendBlob(uint16_t
     }
   }
 
   nsCOMPtr<nsIRunnable> runnable = new ReadBlobRunnable(this, stream, aBlob);
   mInternalIOThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
   return 0;
 }
 
+class DataChannelBlobSendRunnable : public nsRunnable
+{
+public:
+  DataChannelBlobSendRunnable(already_AddRefed<DataChannelConnection>& aConnection,
+                              uint16_t aStream)
+    : mConnection(aConnection)
+    , mStream(aStream) {}
+
+  ~DataChannelBlobSendRunnable()
+  {
+    if (!NS_IsMainThread() && mConnection) {
+      MOZ_ASSERT(false);
+      // explicitly leak the connection if destroyed off mainthread
+      unused << mConnection.forget().take();
+    }
+  }
+
+  NS_IMETHODIMP Run()
+  {
+    ASSERT_WEBRTC(NS_IsMainThread());
+
+    mConnection->SendBinaryMsg(mStream, mData);
+    mConnection = nullptr;
+    return NS_OK;
+  }
+
+  // explicitly public so we can avoid allocating twice and copying
+  nsCString mData;
+
+private:
+  // Note: we can be destroyed off the target thread, so be careful not to let this
+  // get Released()ed on the temp thread!
+  nsRefPtr<DataChannelConnection> mConnection;
+  uint16_t mStream;
+};
+
 void
 DataChannelConnection::ReadBlob(already_AddRefed<DataChannelConnection> aThis,
                                 uint16_t aStream, nsIInputStream* aBlob)
 {
   // 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
+  // output socket.  We need a sender thread(s?) to enqueue 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;
   nsCOMPtr<nsIThread> mainThread;
   NS_GetMainThread(getter_AddRefs(mainThread));
 
+  // Must not let Dispatching it cause the DataChannelConnection to get
+  // released on the wrong thread.  Using WrapRunnable(nsRefPtr<DataChannelConnection>(aThis),...
+  // will occasionally cause aThis to get released on this thread.  Also, an explicit Runnable
+  // lets us avoid copying the blob data an extra time.
+  nsRefPtr<DataChannelBlobSendRunnable> runnable = new DataChannelBlobSendRunnable(aThis,
+                                                                                   aStream);
+  // avoid copying the blob data by passing the mData from the runnable
   if (NS_FAILED(aBlob->Available(&len)) ||
-      NS_FAILED(NS_ReadInputStreamToString(aBlob, temp, len))) {
+      NS_FAILED(NS_ReadInputStreamToString(aBlob, runnable->mData, len))) {
     // Bug 966602:  Doesn't return an error to the caller via onerror.
     // We must release DataChannelConnection on MainThread to avoid issues (bug 876167)
-    NS_ProxyRelease(mainThread, aThis.take());
+    // aThis is now owned by the runnable; release it there
+    NS_ProxyRelease(mainThread, runnable);
     return;
   }
   aBlob->Close();
-  RUN_ON_THREAD(mainThread, WrapRunnable(nsRefPtr<DataChannelConnection>(aThis),
-                               &DataChannelConnection::SendBinaryMsg,
-                               aStream, temp),
-                NS_DISPATCH_NORMAL);
+  NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL);
 }
 
 void
 DataChannelConnection::GetStreamIds(std::vector<uint16_t>* aStreamList)
 {
   ASSERT_WEBRTC(NS_IsMainThread());
   for (uint32_t i = 0; i < mStreams.Length(); ++i) {
     if (mStreams[i]) {