Bug 1030372: use explicit runnable and Release for ReadBlob to avoid wrong-thread release assert r=bsmedberg
--- 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]) {