Bug 1400563 - More DataChannelConnection shutdown cleanup. r=drno, a=ritu FIREFOX_57b_RELBRANCH
authorRandell Jesup <rjesup@jesup.org>
Thu, 02 Nov 2017 02:40:53 -0400
branchFIREFOX_57b_RELBRANCH
changeset 432923 d1ee467027fdf38f286c6c5753eefb5694e4941e
parent 432922 b7c372f8a26c510c30a0e49b89801508cb2116dc
child 432924 ea733883167184dd414f0227f41d8eaa33193ce3
push id8113
push userryanvm@gmail.com
push dateThu, 02 Nov 2017 13:29:01 +0000
treeherdermozilla-beta@b5ec72ee69fb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdrno, ritu
bugs1400563
milestone57.0
Bug 1400563 - More DataChannelConnection shutdown cleanup. r=drno, a=ritu
netwerk/sctp/datachannel/DataChannel.cpp
netwerk/sctp/datachannel/DataChannel.h
--- a/netwerk/sctp/datachannel/DataChannel.cpp
+++ b/netwerk/sctp/datachannel/DataChannel.cpp
@@ -41,16 +41,17 @@
 #include "mozilla/Sprintf.h"
 #include "nsProxyRelease.h"
 #include "nsThread.h"
 #include "nsThreadUtils.h"
 #include "nsAutoPtr.h"
 #include "nsNetUtil.h"
 #include "nsNetCID.h"
 #include "mozilla/StaticPtr.h"
+#include "mozilla/StaticMutex.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"
@@ -66,25 +67,54 @@ static bool sctp_initialized;
 
 namespace mozilla {
 
 LazyLogModule gDataChannelLog("DataChannel");
 static LazyLogModule gSCTPLog("SCTP");
 
 #define SCTP_LOG(args) MOZ_LOG(mozilla::gSCTPLog, mozilla::LogLevel::Debug, args)
 
+class DataChannelConnectionShutdown : public nsITimerCallback
+{
+public:
+  explicit DataChannelConnectionShutdown(DataChannelConnection* aConnection)
+    : mConnection(aConnection)
+  {
+    nsresult rv;
+    mTimer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv); // we'll crash if this fails
+    mTimer->InitWithCallback(this, 30*1000, nsITimer::TYPE_ONE_SHOT);
+  }
+
+  NS_IMETHODIMP Notify(nsITimer* aTimer) override;
+
+  NS_DECL_THREADSAFE_ISUPPORTS
+
+private:
+  virtual ~DataChannelConnectionShutdown()
+  {
+    mTimer->Cancel();
+  }
+
+  RefPtr<DataChannelConnection> mConnection;
+  nsCOMPtr<nsITimer> mTimer;
+};
+
+class DataChannelShutdown;
+
+StaticRefPtr<DataChannelShutdown> sDataChannelShutdown;
+
 class DataChannelShutdown : public nsIObserver
 {
 public:
-  // This needs to be tied to some form object that is guaranteed to be
+  // This needs to be tied to some object that is guaranteed to be
   // around (singleton likely) unless we want to shutdown sctp whenever
   // we're not using it (and in which case we'd keep a refcnt'd object
   // ref'd by each DataChannelConnection to release the SCTP usrlib via
   // sctp_finish). Right now, the single instance of this class is
-  // owned by the observer service.
+  // owned by the observer service and a StaticRefPtr.
 
   NS_DECL_ISUPPORTS
 
   DataChannelShutdown() {}
 
   void Init()
     {
       nsCOMPtr<nsIObserverService> observerService =
@@ -94,47 +124,88 @@ public:
 
       nsresult rv = observerService->AddObserver(this,
                                                  "xpcom-will-shutdown",
                                                  false);
       MOZ_ASSERT(rv == NS_OK);
       (void) rv;
     }
 
-private:
-  // The only instance of DataChannelShutdown is owned by the observer
-  // service, so there is no need to call RemoveObserver here.
-  virtual ~DataChannelShutdown() = default;
-
-public:
   NS_IMETHOD Observe(nsISupports* aSubject, const char* aTopic,
-                     const char16_t* aData) override {
+                     const char16_t* aData) override
+  {
     // Note: MainThread
     if (strcmp(aTopic, "xpcom-will-shutdown") == 0) {
       LOG(("Shutting down SCTP"));
       if (sctp_initialized) {
         usrsctp_finish();
         sctp_initialized = false;
       }
       nsCOMPtr<nsIObserverService> observerService =
         mozilla::services::GetObserverService();
       if (!observerService)
         return NS_ERROR_FAILURE;
 
       nsresult rv = observerService->RemoveObserver(this,
                                                     "xpcom-will-shutdown");
       MOZ_ASSERT(rv == NS_OK);
       (void) rv;
+
+      {
+        StaticMutexAutoLock lock(sLock);
+        sConnections = nullptr; // clears as well
+      }
+      sDataChannelShutdown = nullptr;
     }
     return NS_OK;
   }
+
+  void CreateConnectionShutdown(DataChannelConnection* aConnection)
+  {
+    StaticMutexAutoLock lock(sLock);
+    if (!sConnections) {
+      sConnections = new nsTArray<RefPtr<DataChannelConnectionShutdown>>();
+    }
+    sConnections->AppendElement(new DataChannelConnectionShutdown(aConnection));
+  }
+
+  void RemoveConnectionShutdown(DataChannelConnectionShutdown* aConnectionShutdown)
+  {
+    StaticMutexAutoLock lock(sLock);
+    if (sConnections) {
+      sConnections->RemoveElement(aConnectionShutdown);
+    }
+  }
+
+private:
+  // The only instance of DataChannelShutdown is owned by the observer
+  // service, so there is no need to call RemoveObserver here.
+  virtual ~DataChannelShutdown() = default;
+
+  // protects sConnections
+  static StaticMutex sLock;
+  static StaticAutoPtr<nsTArray<RefPtr<DataChannelConnectionShutdown>>> sConnections;
 };
 
+StaticMutex DataChannelShutdown::sLock;
+StaticAutoPtr<nsTArray<RefPtr<DataChannelConnectionShutdown>>> DataChannelShutdown::sConnections;
+
 NS_IMPL_ISUPPORTS(DataChannelShutdown, nsIObserver);
 
+NS_IMPL_ISUPPORTS(DataChannelConnectionShutdown, nsITimerCallback)
+
+NS_IMETHODIMP
+DataChannelConnectionShutdown::Notify(nsITimer* aTimer)
+{
+  // safely release reference to ourself
+  RefPtr<DataChannelConnectionShutdown> grip(this);
+  sDataChannelShutdown->RemoveConnectionShutdown(this);
+  return NS_OK;
+}
+
 OutgoingMsg::OutgoingMsg(struct sctp_sendv_spa &info, const uint8_t *data,
                          size_t length)
   : mLength(length)
   , mData(data)
 {
   mInfo = &info;
   mPos = 0;
 }
@@ -247,26 +318,22 @@ DataChannelConnection::DataChannelConnec
 
 DataChannelConnection::~DataChannelConnection()
 {
   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);
+  MOZ_ASSERT(!mTransportFlow);
 
   // Already disconnected from sigslot/mTransportFlow
   // TransportFlows must be released from the STS thread
   if (!IsSTSThread()) {
     ASSERT_WEBRTC(NS_IsMainThread());
-    if (mTransportFlow) {
-      ASSERT_WEBRTC(mSTS);
-      NS_ProxyRelease(
-        "DataChannelConnection::mTransportFlow", mSTS, mTransportFlow.forget());
-    }
 
     if (mInternalIOThread) {
       // Avoid spinning the event thread from here (which if we're mainthread
       // is in the event loop already)
       nsCOMPtr<nsIRunnable> r = WrapRunnable(nsCOMPtr<nsIThread>(mInternalIOThread),
                                              &nsIThread::Shutdown);
       Dispatch(r.forget());
     }
@@ -321,16 +388,29 @@ void DataChannelConnection::DestroyOnSTS
     usrsctp_close(aSocket);
   if (aMasterSocket)
     usrsctp_close(aMasterSocket);
 
   usrsctp_deregister_address(static_cast<void *>(this));
   LOG(("Deregistered %p from the SCTP stack.", static_cast<void *>(this)));
 
   disconnect_all();
+
+  // we may have queued packet sends on STS after this; dispatch to ourselves before finishing here
+  // so we can be sure there aren't anymore runnables active that can try to touch the flow.
+  // DON'T use RUN_ON_THREAD, it queue-jumps!
+  mSTS->Dispatch(WrapRunnable(RefPtr<DataChannelConnection>(this),
+                              &DataChannelConnection::DestroyOnSTSFinal),
+                 NS_DISPATCH_NORMAL);
+}
+
+void DataChannelConnection::DestroyOnSTSFinal()
+{
+  mTransportFlow = nullptr;
+  sDataChannelShutdown->CreateConnectionShutdown(this);
 }
 
 bool
 DataChannelConnection::Init(unsigned short aPort, uint16_t aNumStreams, bool aMaxMessageSizeSet,
                             uint64_t aMaxMessageSize)
 {
   struct sctp_initmsg initmsg;
   struct sctp_assoc_value av;
@@ -379,18 +459,18 @@ DataChannelConnection::Init(unsigned sho
       usrsctp_sysctl_set_sctp_ecn_enable(0);
 
       // Enable interleaving messages for different streams (incoming)
       // See: https://tools.ietf.org/html/rfc6458#section-8.1.20
       usrsctp_sysctl_set_sctp_default_frag_interleave(2);
 
       sctp_initialized = true;
 
-      RefPtr<DataChannelShutdown> shutdown = new DataChannelShutdown();
-      shutdown->Init();
+      sDataChannelShutdown = new DataChannelShutdown();
+      sDataChannelShutdown->Init();
     }
   }
 
   // XXX FIX! make this a global we get once
   // Find the STS thread
   nsresult rv;
   mSTS = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
   MOZ_ASSERT(NS_SUCCEEDED(rv));
--- a/netwerk/sctp/datachannel/DataChannel.h
+++ b/netwerk/sctp/datachannel/DataChannel.h
@@ -145,16 +145,17 @@ public:
 
   bool Init(unsigned short aPort, uint16_t aNumStreams, bool aMaxMessageSizeSet,
             uint64_t aMaxMessageSize);
 
   void Destroy(); // So we can spawn refs tied to runnables in shutdown
   // Finish Destroy on STS to avoid SCTP race condition with ABORT from far end
   void DestroyOnSTS(struct socket *aMasterSocket,
                     struct socket *aSocket);
+  void DestroyOnSTSFinal();
 
   void SetMaxMessageSize(bool aMaxMessageSizeSet, uint64_t aMaxMessageSize);
   uint64_t GetMaxMessageSize();
 
 #ifdef ALLOW_DIRECT_SCTP_LISTEN_CONNECT
   // These block; they require something to decide on listener/connector
   // (though you can do simultaneous Connect()).  Do not call these from
   // the main thread!