Bug 805251 - Minimal fix for creation refcount and transport release r=ekr,derf
authorRandell Jesup <rjesup@jesup.org>
Sun, 06 Jan 2013 22:01:23 -0500
changeset 126986 ef5b1c8bfd0a36360099a650b6c892678d16a5b6
parent 126985 de7d61c4b11b8edceb9b4837949cd4fe62cbea7e
child 126987 0d16a92e1a4166d7328e6b50837b3215c8d8f8db
push id2151
push userlsblakk@mozilla.com
push dateTue, 19 Feb 2013 18:06:57 +0000
treeherdermozilla-beta@4952e88741ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersekr, derf
bugs805251
milestone20.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 805251 - Minimal fix for creation refcount and transport release r=ekr,derf
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
netwerk/sctp/datachannel/DataChannel.cpp
netwerk/sctp/datachannel/DataChannel.h
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -552,22 +552,22 @@ static void NotifyDataChannel_m(nsRefPtr
   MOZ_ASSERT(NS_IsMainThread());
 
   aObserver->NotifyDataChannel(aChannel);
   NS_DataChannelAppReady(aChannel);
 }
 #endif
 
 void
-PeerConnectionImpl::NotifyDataChannel(mozilla::DataChannel *aChannel)
+PeerConnectionImpl::NotifyDataChannel(already_AddRefed<mozilla::DataChannel> aChannel)
 {
   PC_AUTO_ENTER_API_CALL_NO_CHECK();
-  MOZ_ASSERT(aChannel);
+  MOZ_ASSERT(aChannel.get());
 
-  CSFLogDebugS(logTag, __FUNCTION__ << ": channel: " << static_cast<void*>(aChannel));
+  CSFLogDebugS(logTag, __FUNCTION__ << ": channel: " << static_cast<void*>(aChannel.get()));
 
 #ifdef MOZILLA_INTERNAL_API
    nsCOMPtr<nsIDOMDataChannel> domchannel;
    nsresult rv = NS_NewDOMDataChannel(aChannel, mWindow,
                                       getter_AddRefs(domchannel));
   NS_ENSURE_SUCCESS_VOID(rv);
 
   RUN_ON_THREAD(mThread,
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ -132,17 +132,17 @@ public:
     ccapi_call_event_e aCallEvent,
     CSF::CC_CallPtr aCall,
     CSF::CC_CallInfoPtr aInfo
   );
 
   // DataConnection observers
   void NotifyConnection();
   void NotifyClosedConnection();
-  void NotifyDataChannel(mozilla::DataChannel *aChannel);
+  void NotifyDataChannel(already_AddRefed<mozilla::DataChannel> aChannel);
 
   // Get the media object
   const nsRefPtr<PeerConnectionMedia>& media() const {
     PC_AUTO_ENTER_API_CALL_NO_CHECK();
     return mMedia;
   }
 
   // Handle system to allow weak references to be passed through C code
--- a/netwerk/sctp/datachannel/DataChannel.cpp
+++ b/netwerk/sctp/datachannel/DataChannel.cpp
@@ -22,16 +22,18 @@
 #include "nsIObserver.h"
 #include "mozilla/Services.h"
 #include "nsThreadUtils.h"
 #include "nsAutoPtr.h"
 #include "nsNetUtil.h"
 #ifdef MOZ_PEERCONNECTION
 #include "mtransport/runnable_utils.h"
 #endif
+
+#define DATACHANNEL_LOG(args) LOG(args)
 #include "DataChannel.h"
 #include "DataChannelProtocol.h"
 
 #ifdef PR_LOGGING
 PRLogModuleInfo*
 GetDataChannelLog()
 {
   static PRLogModuleInfo* sLog;
@@ -147,19 +149,22 @@ DataChannelConnection::DataChannelConnec
   mRemotePort = 0;
   mDeferTimeout = 10;
   mTimerRunning = false;
   LOG(("Constructor DataChannelConnection=%p, listener=%p", this, mListener));
 }
 
 DataChannelConnection::~DataChannelConnection()
 {
+  LOG(("Deleting DataChannelConnection %p", (void *) this));
   // This may die on the MainThread, or on the STS thread
   MOZ_ASSERT(mState == CLOSED);
   MOZ_ASSERT(!mMasterSocket);
+  MOZ_ASSERT(mPending.GetSize() == 0);
+  // Already disconnected from sigslot/mTransportFlow
 }
 
 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
   // create a dependant Internal object that would remain around
@@ -172,16 +177,28 @@ DataChannelConnection::Destroy()
   if (mMasterSocket)
     usrsctp_close(mMasterSocket);
 
   mSocket = nullptr;
   mMasterSocket = nullptr;
 
   // We can't get any more new callbacks from the SCTP library
   // All existing callbacks have refs to DataChannelConnection
+
+  // nsDOMDataChannel objects have refs to DataChannels that have refs to us
+
+  if (mTransportFlow) {
+    MOZ_ASSERT(mSTS);
+    MOZ_ASSERT(NS_IsMainThread());
+    RUN_ON_THREAD(mSTS, WrapRunnable(nsRefPtr<DataChannelConnection>(this),
+                                     &DataChannelConnection::disconnect_all),
+                  NS_DISPATCH_NORMAL);
+    // safe to do now from Mainthread per ekr
+    mTransportFlow = nullptr;
+  }
 }
 
 NS_IMPL_THREADSAFE_ISUPPORTS1(DataChannelConnection,
                               nsITimerCallback)
 
 bool
 DataChannelConnection::Init(unsigned short aPort, uint16_t aNumStreams, bool aUsingDtls)
 {
@@ -1502,18 +1519,21 @@ DataChannelConnection::HandleStreamReset
           // 2. We sent our own reset (CLOSING); either they crossed on the
           //    wire, or this is a response to our Reset.
           //    Go to CLOSED
           // 3. We've sent a open but haven't gotten a response yet (OPENING)
           //    I believe this is impossible, as we don't have an input stream yet.
 
           LOG(("Incoming: Channel %d outgoing/%d incoming closed, state %d",
                channel->mStreamOut, channel->mStreamIn, channel->mState));
-          MOZ_ASSERT(channel->mState == OPEN || channel->mState == CLOSING);
-          if (channel->mState == OPEN) {
+          MOZ_ASSERT(channel->mState == DataChannel::OPEN ||
+                     channel->mState == DataChannel::CLOSING ||
+                     channel->mState == DataChannel::WAITING_TO_OPEN);
+          if (channel->mState == DataChannel::OPEN ||
+              channel->mState == DataChannel::WAITING_TO_OPEN) {
             ResetOutgoingStream(channel->mStreamOut);
             NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
                                       DataChannelOnMessageAvailable::ON_CHANNEL_CLOSED, this,
                                       channel));
             mStreamsOut[channel->mStreamOut] = nullptr;
           }
           mStreamsIn[channel->mStreamIn] = nullptr;
 
@@ -2005,16 +2025,20 @@ DataChannelConnection::Close(uint16_t st
 {
   nsRefPtr<DataChannel> channel; // make sure it doesn't go away on us
 
   MutexAutoLock lock(mLock);
   channel = FindChannelByStreamOut(streamOut);
   if (channel) {
     LOG(("Connection %p/Channel %p: Closing stream %d",
          (void *) channel->mConnection.get(), (void *) channel.get(), streamOut));
+    if (channel->mState == CLOSED || channel->mState == CLOSING) {
+      LOG(("Channel already closing/closed (%d)", channel->mState));
+      return;
+    }
     channel->mBufferedData.Clear();
     if (channel->mStreamOut != INVALID_STREAM)
       ResetOutgoingStream(channel->mStreamOut);
     SendOutgoingStreamReset();
     channel->mState = CLOSING;
   } else {
     LOG(("!!!? no channel when closing stream %d?",streamOut));
   }
@@ -2034,30 +2058,34 @@ void DataChannelConnection::CloseAll()
   for (uint32_t i = 0; i < mStreamsOut.Length(); ++i) {
     if (mStreamsOut[i]) {
       mStreamsOut[i]->Close();
     }
   }
 
   // Clean up any pending opens for channels
   nsRefPtr<DataChannel> channel;
-  while (nullptr != (channel = dont_AddRef(static_cast<DataChannel *>(mPending.PopFront()))))
+  while (nullptr != (channel = dont_AddRef(static_cast<DataChannel *>(mPending.PopFront())))) {
+    LOG(("closing pending channel %p, stream %d", channel.get(), channel->mStreamOut));
     channel->Close(); // also releases the ref on each iteration
+  }
 }
 
 DataChannel::~DataChannel()
 {
   if (mConnection)
     Close();
 }
 
 // Used when disconnecting from the DataChannelConnection
 void
 DataChannel::Destroy()
 {
+  ENSURE_DATACONNECTION;
+
   LOG(("Destroying Data channel %d/%d", mStreamOut, mStreamIn));
   MOZ_ASSERT_IF(mStreamOut != INVALID_STREAM,
                 !mConnection->FindChannelByStreamOut(mStreamOut));
   MOZ_ASSERT_IF(mStreamIn != INVALID_STREAM,
                 !mConnection->FindChannelByStreamIn(mStreamIn));
   mStreamIn  = INVALID_STREAM;
   mStreamOut = INVALID_STREAM;
   mState = CLOSED;
@@ -2067,31 +2095,34 @@ DataChannel::Destroy()
 void
 DataChannel::Close()
 {
   if (mState == CLOSING || mState == CLOSED ||
       mStreamOut == INVALID_STREAM) {
     return;
   }
   mState = CLOSING;
+  ENSURE_DATACONNECTION;
   mConnection->Close(mStreamOut);
 }
 
 void
 DataChannel::SetListener(DataChannelListener *aListener, nsISupports *aContext)
 {
   MOZ_ASSERT(!mListener); // only should be set once, avoids races w/o locking
   mContext = aContext;
   mListener = aListener;
 }
 
 // May be called from another (i.e. Main) thread!
 void
 DataChannel::AppReady()
 {
+  ENSURE_DATACONNECTION;
+
   MutexAutoLock lock(mConnection->mLock);
 
   mReady = true;
   if (mState == WAITING_TO_OPEN) {
     mState = OPEN;
     NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
                               DataChannelOnMessageAvailable::ON_CHANNEL_OPEN, mConnection,
                               this));
--- a/netwerk/sctp/datachannel/DataChannel.h
+++ b/netwerk/sctp/datachannel/DataChannel.h
@@ -25,16 +25,20 @@
 #include "DataChannelProtocol.h"
 #ifdef SCTP_DTLS_SUPPORTED
 #include "mtransport/sigslot.h"
 #include "mtransport/transportflow.h"
 #include "mtransport/transportlayer.h"
 #include "mtransport/transportlayerprsock.h"
 #endif
 
+#ifndef DATACHANNEL_LOG
+#define DATACHANNEL_LOG(args)
+#endif
+
 #ifndef EALREADY
 #define EALREADY  WSAEALREADY
 #endif
 
 extern "C" {
   struct socket;
   struct sctp_rcvinfo;
 }
@@ -97,17 +101,17 @@ public:
 
     // Called when a the connection is open
     virtual void NotifyConnection() = 0;
 
     // Called when a the connection is lost/closed
     virtual void NotifyClosedConnection() = 0;
 
     // Called when a new DataChannel has been opened by the other side.
-    virtual void NotifyDataChannel(DataChannel *channel) = 0;
+    virtual void NotifyDataChannel(already_AddRefed<DataChannel> channel) = 0;
   };
 
   DataChannelConnection(DataConnectionListener *listener);
   virtual ~DataChannelConnection();
 
   bool Init(unsigned short aPort, uint16_t aNumStreams, bool aUsingDtls);
   void Destroy(); // So we can spawn refs tied to runnables in shutdown
 
@@ -122,17 +126,17 @@ public:
   bool ConnectDTLS(TransportFlow *aFlow, uint16_t localport, uint16_t remoteport);
 #endif
 
   typedef enum {
     RELIABLE=0,
     PARTIAL_RELIABLE_REXMIT = 1,
     PARTIAL_RELIABLE_TIMED = 2
   } Type;
-    
+
   already_AddRefed<DataChannel> Open(const nsACString& label,
                                      Type type, bool inOrder,
                                      uint32_t prValue,
                                      DataChannelListener *aListener,
                                      nsISupports *aContext);
 
   void Close(uint16_t stream);
   void CloseAll();
@@ -143,18 +147,18 @@ public:
     }
   int32_t SendBinaryMsg(uint16_t stream, const nsACString &aMsg)
     {
       return SendMsgCommon(stream, aMsg, true);
     }
   int32_t SendBlob(uint16_t stream, nsIInputStream *aBlob);
 
   // Called on data reception from the SCTP library
-  // must(?) be public so my c->c++ tramploine can call it
-  int ReceiveCallback(struct socket* sock, void *data, size_t datalen, 
+  // must(?) be public so my c->c++ trampoline can call it
+  int ReceiveCallback(struct socket* sock, void *data, size_t datalen,
                       struct sctp_rcvinfo rcv, int32_t flags);
 
   // Find out state
   enum {
     CONNECTING = 0U,
     OPEN = 1U,
     CLOSING = 2U,
     CLOSED = 3U
@@ -253,28 +257,34 @@ private:
   nsCOMPtr<nsITimer> mDeferredTimer;
   uint32_t mDeferTimeout; // in ms
   bool mTimerRunning;
 
   // Thread used for connections
   nsCOMPtr<nsIThread> mConnectThread;
 };
 
+#define ENSURE_DATACONNECTION \
+  do { if (!mConnection) { DATACHANNEL_LOG(("%s: %p no connection!",__FUNCTION__, this)); return; } } while (0)
+
+#define ENSURE_DATACONNECTION_RET(x) \
+  do { if (!mConnection) { DATACHANNEL_LOG(("%s: %p no connection!",__FUNCTION__, this)); return (x); } } while (0)
+
 class DataChannel {
 public:
   enum {
     CONNECTING = 0U,
     OPEN = 1U,
     CLOSING = 2U,
     CLOSED = 3U,
     WAITING_TO_OPEN = 4U
   };
 
   DataChannel(DataChannelConnection *connection,
-              uint16_t streamOut, uint16_t streamIn, 
+              uint16_t streamOut, uint16_t streamIn,
               uint16_t state,
               const nsACString& label,
               uint16_t policy, uint32_t value,
               uint32_t flags,
               DataChannelListener *aListener,
               nsISupports *aContext)
     : mListener(aListener)
     , mConnection(connection)
@@ -301,34 +311,40 @@ public:
 
   // Set the listener (especially for channels created from the other side)
   // Note: The Listener and Context should only be set once
   void SetListener(DataChannelListener *aListener, nsISupports *aContext);
 
   // Send a string
   bool SendMsg(const nsACString &aMsg)
     {
+      ENSURE_DATACONNECTION_RET(false);
+
       if (mStreamOut != INVALID_STREAM)
         return (mConnection->SendMsg(mStreamOut, aMsg) > 0);
       else
         return false;
     }
 
   // Send a binary message (TypedArray)
   bool SendBinaryMsg(const nsACString &aMsg)
     {
+      ENSURE_DATACONNECTION_RET(false);
+
       if (mStreamOut != INVALID_STREAM)
         return (mConnection->SendBinaryMsg(mStreamOut, aMsg) > 0);
       else
         return false;
     }
 
   // Send a binary blob
   bool SendBinaryStream(nsIInputStream *aBlob, uint32_t msgLen)
     {
+      ENSURE_DATACONNECTION_RET(false);
+
       if (mStreamOut != INVALID_STREAM)
         return (mConnection->SendBlob(mStreamOut, aBlob) > 0);
       else
         return false;
     }
 
   uint16_t GetType() { return mPrPolicy; }
 
@@ -396,17 +412,17 @@ public:
 
   DataChannelOnMessageAvailable(int32_t     aType,
                                 DataChannelConnection *aConnection,
                                 DataChannel *aChannel,
                                 nsCString   &aData,  // XXX this causes inefficiency
                                 int32_t     aLen)
     : mType(aType),
       mChannel(aChannel),
-      mConnection(aConnection), 
+      mConnection(aConnection),
       mData(aData),
       mLen(aLen) {}
 
   DataChannelOnMessageAvailable(int32_t     aType,
                                 DataChannel *aChannel)
     : mType(aType),
       mChannel(aChannel) {}
   // XXX is it safe to leave mData/mLen uninitialized?  This should only be
@@ -456,17 +472,18 @@ public:
         break;
       case ON_CHANNEL_OPEN:
         mChannel->mListener->OnChannelConnected(mChannel->mContext);
         break;
       case ON_CHANNEL_CLOSED:
         mChannel->mListener->OnChannelClosed(mChannel->mContext);
         break;
       case ON_CHANNEL_CREATED:
-        mConnection->mListener->NotifyDataChannel(mChannel);
+        // important to give it an already_AddRefed pointer!
+        mConnection->mListener->NotifyDataChannel(mChannel.forget());
         break;
       case ON_CONNECTION:
         if (mResult) {
           mConnection->mListener->NotifyConnection();
         }
         mConnection->mConnectThread = nullptr; // kill the connection thread
         break;
       case ON_DISCONNECTED: