Bug 1382103 - ensure thread-safe access to HttpBackgroundChannelParent::mBackgroundThread. r?mayhemer draft
authorShih-Chiang Chien <schien@mozilla.com>
Mon, 17 Jul 2017 16:36:30 +0800
changeset 611000 33a1361f9c118d3ffe7ca57c4e8d21934b80df63
parent 610973 1b065ffd8a535a0ad4c39a912af18e948e6a42c1
child 638036 49d1dc88af8dbb42ec10d055ba40efe684e6e2c1
push id69087
push userschien@mozilla.com
push dateWed, 19 Jul 2017 03:25:57 +0000
reviewersmayhemer
bugs1382103
milestone56.0a1
Bug 1382103 - ensure thread-safe access to HttpBackgroundChannelParent::mBackgroundThread. r?mayhemer mBackgroundThread is an nsCOMPtr write on PBackground thread but read on main thread. We need to use Mutex to ensure memory sync between multiple threads. MozReview-Commit-ID: 2CJ359ivhQh
netwerk/protocol/http/HttpBackgroundChannelParent.cpp
netwerk/protocol/http/HttpBackgroundChannelParent.h
--- a/netwerk/protocol/http/HttpBackgroundChannelParent.cpp
+++ b/netwerk/protocol/http/HttpBackgroundChannelParent.cpp
@@ -63,20 +63,25 @@ public:
 
 private:
   RefPtr<HttpBackgroundChannelParent> mActor;
   const uint64_t mChannelId;
 };
 
 HttpBackgroundChannelParent::HttpBackgroundChannelParent()
   : mIPCOpened(true)
-  , mBackgroundThread(NS_GetCurrentThread())
+  , mBgThreadMutex("HttpBackgroundChannelParent::BgThreadMutex")
 {
   AssertIsInMainProcess();
   AssertIsOnBackgroundThread();
+
+  {
+    MutexAutoLock lock(mBgThreadMutex);
+    mBackgroundThread = NS_GetCurrentThread();
+  }
 }
 
 HttpBackgroundChannelParent::~HttpBackgroundChannelParent()
 {
   MOZ_ASSERT(NS_IsMainThread() || IsOnBackgroundThread());
   MOZ_ASSERT(!mIPCOpened);
 }
 
@@ -117,47 +122,51 @@ HttpBackgroundChannelParent::OnChannelCl
   MOZ_ASSERT(NS_IsMainThread());
 
   if (!mIPCOpened) {
     return;
   }
 
   nsresult rv;
 
-  RefPtr<HttpBackgroundChannelParent> self = this;
-  rv = mBackgroundThread->Dispatch(
-    NS_NewRunnableFunction(
-      "net::HttpBackgroundChannelParent::OnChannelClosed",
-      [self]() {
-        LOG(("HttpBackgroundChannelParent::DeleteRunnable [this=%p]\n",
-             self.get()));
-        AssertIsOnBackgroundThread();
+  {
+    MutexAutoLock lock(mBgThreadMutex);
+    RefPtr<HttpBackgroundChannelParent> self = this;
+    rv = mBackgroundThread->Dispatch(
+      NS_NewRunnableFunction(
+        "net::HttpBackgroundChannelParent::OnChannelClosed",
+        [self]() {
+          LOG(("HttpBackgroundChannelParent::DeleteRunnable [this=%p]\n",
+               self.get()));
+          AssertIsOnBackgroundThread();
 
-        if (!self->mIPCOpened.compareExchange(true, false)) {
-          return;
-        }
+          if (!self->mIPCOpened.compareExchange(true, false)) {
+            return;
+          }
 
-        Unused << self->Send__delete__(self);
-      }),
-    NS_DISPATCH_NORMAL);
+          Unused << self->Send__delete__(self);
+        }),
+      NS_DISPATCH_NORMAL);
+  }
 
   Unused << NS_WARN_IF(NS_FAILED(rv));
 }
 
 bool
 HttpBackgroundChannelParent::OnStartRequestSent()
 {
   LOG(("HttpBackgroundChannelParent::OnStartRequestSent [this=%p]\n", this));
   AssertIsInMainProcess();
 
   if (NS_WARN_IF(!mIPCOpened)) {
     return false;
   }
 
   if (!IsOnBackgroundThread()) {
+    MutexAutoLock lock(mBgThreadMutex);
     nsresult rv = mBackgroundThread->Dispatch(
       NewRunnableMethod("net::HttpBackgroundChannelParent::OnStartRequestSent",
                         this,
                         &HttpBackgroundChannelParent::OnStartRequestSent),
       NS_DISPATCH_NORMAL);
 
     MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
 
@@ -178,16 +187,17 @@ HttpBackgroundChannelParent::OnTransport
   LOG(("HttpBackgroundChannelParent::OnTransportAndData [this=%p]\n", this));
   AssertIsInMainProcess();
 
   if (NS_WARN_IF(!mIPCOpened)) {
     return false;
   }
 
   if (!IsOnBackgroundThread()) {
+    MutexAutoLock lock(mBgThreadMutex);
     nsresult rv = mBackgroundThread->Dispatch(
       NewRunnableMethod<const nsresult,
                         const nsresult,
                         const uint64_t,
                         const uint32_t,
                         const nsCString>(
         "net::HttpBackgroundChannelParent::OnTransportAndData",
         this,
@@ -216,16 +226,17 @@ HttpBackgroundChannelParent::OnStopReque
         "status=%" PRIx32 "]\n", this, static_cast<uint32_t>(aChannelStatus)));
   AssertIsInMainProcess();
 
   if (NS_WARN_IF(!mIPCOpened)) {
     return false;
   }
 
   if (!IsOnBackgroundThread()) {
+    MutexAutoLock lock(mBgThreadMutex);
     nsresult rv = mBackgroundThread->Dispatch(
       NewRunnableMethod<const nsresult, const ResourceTimingStruct>(
         "net::HttpBackgroundChannelParent::OnStopRequest",
         this,
         &HttpBackgroundChannelParent::OnStopRequest,
         aChannelStatus,
         aTiming),
       NS_DISPATCH_NORMAL);
@@ -246,16 +257,17 @@ HttpBackgroundChannelParent::OnProgress(
        " max=%" PRId64 "]\n", this, aProgress, aProgressMax));
   AssertIsInMainProcess();
 
   if (NS_WARN_IF(!mIPCOpened)) {
     return false;
   }
 
   if (!IsOnBackgroundThread()) {
+    MutexAutoLock lock(mBgThreadMutex);
     nsresult rv = mBackgroundThread->Dispatch(
       NewRunnableMethod<const int64_t, const int64_t>(
         "net::HttpBackgroundChannelParent::OnProgress",
         this,
         &HttpBackgroundChannelParent::OnProgress,
         aProgress,
         aProgressMax),
       NS_DISPATCH_NORMAL);
@@ -275,16 +287,17 @@ HttpBackgroundChannelParent::OnStatus(co
        "]\n", this, static_cast<uint32_t>(aStatus)));
   AssertIsInMainProcess();
 
   if (NS_WARN_IF(!mIPCOpened)) {
     return false;
   }
 
   if (!IsOnBackgroundThread()) {
+    MutexAutoLock lock(mBgThreadMutex);
     nsresult rv = mBackgroundThread->Dispatch(
       NewRunnableMethod<const nsresult>(
         "net::HttpBackgroundChannelParent::OnStatus",
         this,
         &HttpBackgroundChannelParent::OnStatus,
         aStatus),
       NS_DISPATCH_NORMAL);
 
@@ -302,16 +315,17 @@ HttpBackgroundChannelParent::OnDiversion
   LOG(("HttpBackgroundChannelParent::OnDiversion [this=%p]\n", this));
   AssertIsInMainProcess();
 
   if (NS_WARN_IF(!mIPCOpened)) {
     return false;
   }
 
   if (!IsOnBackgroundThread()) {
+    MutexAutoLock lock(mBgThreadMutex);
     nsresult rv = mBackgroundThread->Dispatch(
       NewRunnableMethod("net::HttpBackgroundChannelParent::OnDiversion",
                         this,
                         &HttpBackgroundChannelParent::OnDiversion),
       NS_DISPATCH_NORMAL);
 
     MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
 
@@ -337,16 +351,17 @@ HttpBackgroundChannelParent::OnNotifyTra
   LOG(("HttpBackgroundChannelParent::OnNotifyTrackingProtectionDisabled [this=%p]\n", this));
   AssertIsInMainProcess();
 
   if (NS_WARN_IF(!mIPCOpened)) {
     return false;
   }
 
   if (!IsOnBackgroundThread()) {
+    MutexAutoLock lock(mBgThreadMutex);
     nsresult rv = mBackgroundThread->Dispatch(
       NewRunnableMethod(
         "net::HttpBackgroundChannelParent::OnNotifyTrackingProtectionDisabled",
         this,
         &HttpBackgroundChannelParent::OnNotifyTrackingProtectionDisabled),
       NS_DISPATCH_NORMAL);
 
     MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
@@ -363,16 +378,17 @@ HttpBackgroundChannelParent::OnNotifyTra
   LOG(("HttpBackgroundChannelParent::OnNotifyTrackingResource [this=%p]\n", this));
   AssertIsInMainProcess();
 
   if (NS_WARN_IF(!mIPCOpened)) {
     return false;
   }
 
   if (!IsOnBackgroundThread()) {
+    MutexAutoLock lock(mBgThreadMutex);
     nsresult rv = mBackgroundThread->Dispatch(
       NewRunnableMethod(
         "net::HttpBackgroundChannelParent::OnNotifyTrackingResource",
         this,
         &HttpBackgroundChannelParent::OnNotifyTrackingResource),
       NS_DISPATCH_NORMAL);
 
     MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
@@ -392,16 +408,17 @@ HttpBackgroundChannelParent::OnSetClassi
   LOG(("HttpBackgroundChannelParent::OnSetClassifierMatchedInfo [this=%p]\n", this));
   AssertIsInMainProcess();
 
   if (NS_WARN_IF(!mIPCOpened)) {
     return false;
   }
 
   if (!IsOnBackgroundThread()) {
+    MutexAutoLock lock(mBgThreadMutex);
     nsresult rv = mBackgroundThread->Dispatch(
       NewRunnableMethod<const nsCString, const nsCString, const nsCString>(
         "net::HttpBackgroundChannelParent::OnSetClassifierMatchedInfo",
         this,
         &HttpBackgroundChannelParent::OnSetClassifierMatchedInfo,
         aList,
         aProvider,
         aPrefix),
--- a/netwerk/protocol/http/HttpBackgroundChannelParent.h
+++ b/netwerk/protocol/http/HttpBackgroundChannelParent.h
@@ -5,16 +5,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_net_HttpBackgroundChannelParent_h
 #define mozilla_net_HttpBackgroundChannelParent_h
 
 #include "mozilla/net/PHttpBackgroundChannelParent.h"
 #include "mozilla/Atomics.h"
+#include "mozilla/Mutex.h"
 #include "nsID.h"
 #include "nsISupportsImpl.h"
 
 class nsIEventTarget;
 
 namespace mozilla {
 namespace net {
 
@@ -78,16 +79,19 @@ public:
 protected:
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
 private:
   virtual ~HttpBackgroundChannelParent();
 
   Atomic<bool> mIPCOpened;
 
+  // Used to ensure atomicity of mBackgroundThread
+  Mutex mBgThreadMutex;
+
   nsCOMPtr<nsIEventTarget> mBackgroundThread;
 
   // associated HttpChannelParent for generating the channel events
   RefPtr<HttpChannelParent> mChannelParent;
 };
 
 } // namespace net
 } // namespace mozilla