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 223213 238f460e7aedc87a4ce3fdff8b9479256d3f3d58
parent 223212 8f3cc2c90893ab18bcb9eda7961ff3fc08022a99
child 223214 0aaec62e668467cc9e90e6959c3feccfcf0652bc
push id10769
push usercbook@mozilla.com
push dateMon, 12 Jan 2015 14:15:52 +0000
treeherderfx-team@0e9765732906 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg
bugs1030372
milestone37.0a1
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]) {