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 419020 fffa2bcd7e364c1b0e85b9f5580363d9e071963c
parent 419019 4fe0649bd462a3332e4429910a6396c5dd00859e
child 419021 8c927b51ca411f35225282ece3d2271a89b8528d
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
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