Bug 1382103 - ensure thread-safe access to HttpBackgroundChannelParent::mBackgroundThread. r=mayhemer
authorShih-Chiang Chien <schien@mozilla.com>
Mon, 17 Jul 2017 16:36:30 +0800
changeset 406101 fffa2bcd7e364c1b0e85b9f5580363d9e071963c
parent 406100 4fe0649bd462a3332e4429910a6396c5dd00859e
child 406102 8c927b51ca411f35225282ece3d2271a89b8528d
push idunknown
push userunknown
push dateunknown
reviewersmayhemer
bugs1382103
milestone56.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 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