Bug 1530223 - Guard RemoteWorkerChild's shared data with a mutex r=perry,asuth
authorYaron Tausky <ytausky@mozilla.com>
Thu, 14 Mar 2019 20:19:18 +0000
changeset 464067 e6e7b9953ede5e1435019e9ee80441ba144fab11
parent 464066 4fd7ce83efe1cad3f2ab3686cc9b45671b4cd0d5
child 464068 0ec36abbdb31f9bf12867c6f066ac60ed2e1ba50
push id35707
push userrmaries@mozilla.com
push dateFri, 15 Mar 2019 03:42:43 +0000
treeherdermozilla-central@5ce27c44f79e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersperry, asuth
bugs1530223
milestone67.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 1530223 - Guard RemoteWorkerChild's shared data with a mutex r=perry,asuth Reading and writing data without synchronization from multiple threads is undefined behavior. Note that this commit does not attempt to reason about the during of mutex locking; its purpose is to first establish correctness. Differential Revision: https://phabricator.services.mozilla.com/D22573
dom/workers/remoteworkers/RemoteWorkerChild.cpp
dom/workers/remoteworkers/RemoteWorkerChild.h
--- a/dom/workers/remoteworkers/RemoteWorkerChild.cpp
+++ b/dom/workers/remoteworkers/RemoteWorkerChild.cpp
@@ -184,26 +184,29 @@ class RemoteWorkerChild::InitializeWorke
     mActor->ShutdownOnWorker();
     return WorkerRunnable::Cancel();
   }
 
   RefPtr<RemoteWorkerChild> mActor;
 };
 
 RemoteWorkerChild::RemoteWorkerChild()
-    : mIPCActive(true), mWorkerState(ePending) {
+    : mIPCActive(true), mSharedData("RemoteWorkerChild::mSharedData") {
   MOZ_ASSERT(RemoteWorkerService::Thread()->IsOnCurrentThread());
 }
 
+RemoteWorkerChild::SharedData::SharedData() : mWorkerState(ePending) {}
+
 RemoteWorkerChild::~RemoteWorkerChild() {
   nsCOMPtr<nsIEventTarget> target =
       SystemGroup::EventTargetFor(TaskCategory::Other);
 
+  const auto lock = mSharedData.Lock();
   NS_ProxyRelease("RemoteWorkerChild::mWorkerPrivate", target,
-                  mWorkerPrivate.forget());
+                  lock->mWorkerPrivate.forget());
 }
 
 void RemoteWorkerChild::ActorDestroy(ActorDestroyReason aWhy) {
   MOZ_ACCESS_THREAD_BOUND(mLauncherData, data);
   mIPCActive = false;
   data->mPendingOps.Clear();
 }
 
@@ -308,76 +311,84 @@ nsresult RemoteWorkerChild::ExecWorkerOn
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
 
   AutoJSAPI jsapi;
   jsapi.Init();
 
   ErrorResult error;
-  mWorkerPrivate = WorkerPrivate::Constructor(
+  const auto lock = mSharedData.Lock();
+  lock->mWorkerPrivate = WorkerPrivate::Constructor(
       jsapi.cx(), aData.originalScriptURL(), false,
       aData.isSharedWorker() ? WorkerTypeShared : WorkerTypeService,
       aData.name(), VoidCString(), &info, error);
   if (NS_WARN_IF(error.Failed())) {
     return error.StealNSResult();
   }
 
   RefPtr<InitializeWorkerRunnable> runnable =
-      new InitializeWorkerRunnable(mWorkerPrivate, this);
+      new InitializeWorkerRunnable(lock->mWorkerPrivate, this);
   if (NS_WARN_IF(!runnable->Dispatch())) {
     return NS_ERROR_FAILURE;
   }
 
-  mWorkerPrivate->SetRemoteWorkerController(this);
+  lock->mWorkerPrivate->SetRemoteWorkerController(this);
   return NS_OK;
 }
 
 void RemoteWorkerChild::InitializeOnWorker(WorkerPrivate* aWorkerPrivate) {
   MOZ_ASSERT(aWorkerPrivate);
   aWorkerPrivate->AssertIsOnWorkerThread();
 
   RefPtr<RemoteWorkerChild> self = this;
-  mWorkerRef = WeakWorkerRef::Create(mWorkerPrivate,
-                                     [self]() { self->ShutdownOnWorker(); });
+  {
+    const auto lock = mSharedData.Lock();
+    mWorkerRef = WeakWorkerRef::Create(lock->mWorkerPrivate,
+                                       [self]() { self->ShutdownOnWorker(); });
+  }
 
   if (NS_WARN_IF(!mWorkerRef)) {
     CreationFailedOnAnyThread();
     ShutdownOnWorker();
     return;
   }
 
   CreationSucceededOnAnyThread();
 }
 
 void RemoteWorkerChild::ShutdownOnWorker() {
-  MOZ_ASSERT(mWorkerPrivate);
-  mWorkerPrivate->AssertIsOnWorkerThread();
+  const auto lock = mSharedData.Lock();
+  MOZ_ASSERT(lock->mWorkerPrivate);
+  lock->mWorkerPrivate->AssertIsOnWorkerThread();
 
   // This will release the worker.
   mWorkerRef = nullptr;
 
   nsCOMPtr<nsIEventTarget> target =
       SystemGroup::EventTargetFor(TaskCategory::Other);
 
   NS_ProxyRelease("RemoteWorkerChild::mWorkerPrivate", target,
-                  mWorkerPrivate.forget());
+                  lock->mWorkerPrivate.forget());
 
   RefPtr<RemoteWorkerChild> self = this;
   nsCOMPtr<nsIRunnable> r =
       NS_NewRunnableFunction("RemoteWorkerChild::ShutdownOnWorker",
                              [self]() { self->WorkerTerminated(); });
 
   RemoteWorkerService::Thread()->Dispatch(r.forget(), NS_DISPATCH_NORMAL);
 }
 
 void RemoteWorkerChild::WorkerTerminated() {
   MOZ_ACCESS_THREAD_BOUND(mLauncherData, data);
 
-  mWorkerState = eTerminated;
+  {
+    const auto lock = mSharedData.Lock();
+    lock->mWorkerState = eTerminated;
+  }
   data->mPendingOps.Clear();
 
   if (!mIPCActive) {
     return;
   }
 
   Unused << SendClose();
   mIPCActive = false;
@@ -430,27 +441,28 @@ void RemoteWorkerChild::ErrorPropagation
     return;
   }
 
   Unused << SendError(aValue);
 }
 
 void RemoteWorkerChild::CloseWorkerOnMainThread() {
   MOZ_ASSERT(NS_IsMainThread());
+  const auto lock = mSharedData.Lock();
 
-  if (mWorkerState == ePending) {
-    mWorkerState = ePendingTerminated;
+  if (lock->mWorkerState == ePending) {
+    lock->mWorkerState = ePendingTerminated;
     // Already released.
     return;
   }
 
   // The holder will be notified by this.
-  if (mWorkerState == eRunning) {
-    MOZ_RELEASE_ASSERT(mWorkerPrivate);
-    mWorkerPrivate->Cancel();
+  if (lock->mWorkerState == eRunning) {
+    MOZ_RELEASE_ASSERT(lock->mWorkerPrivate);
+    lock->mWorkerPrivate->Cancel();
   }
 }
 
 void RemoteWorkerChild::FlushReportsOnMainThread(
     nsIConsoleReportCollector* aReporter) {
   MOZ_ASSERT(NS_IsMainThread());
 
   bool reportErrorToBrowserConsole = true;
@@ -467,34 +479,42 @@ void RemoteWorkerChild::FlushReportsOnMa
     aReporter->FlushReportsToConsole(0);
     return;
   }
 
   aReporter->ClearConsoleReports();
 }
 
 IPCResult RemoteWorkerChild::RecvExecOp(const RemoteWorkerOp& aOp) {
+  const auto lock = mSharedData.Lock();
+  return ExecuteOperation(aOp, lock);
+}
+
+template <typename T>
+IPCResult RemoteWorkerChild::ExecuteOperation(const RemoteWorkerOp& aOp,
+                                              const T& aLock) {
   MOZ_ACCESS_THREAD_BOUND(mLauncherData, data);
 
   if (!mIPCActive) {
     return IPC_OK();
   }
 
   // The worker is not ready yet.
-  if (mWorkerState == ePending) {
+  if (aLock->mWorkerState == ePending) {
     data->mPendingOps.AppendElement(aOp);
     return IPC_OK();
   }
 
-  if (mWorkerState == eTerminated || mWorkerState == ePendingTerminated) {
+  if (aLock->mWorkerState == eTerminated ||
+      aLock->mWorkerState == ePendingTerminated) {
     // No op.
     return IPC_OK();
   }
 
-  MOZ_ASSERT(mWorkerState == eRunning);
+  MOZ_ASSERT(aLock->mWorkerState == eRunning);
 
   // Main-thread operations
   if (aOp.type() == RemoteWorkerOp::TRemoteWorkerSuspendOp ||
       aOp.type() == RemoteWorkerOp::TRemoteWorkerResumeOp ||
       aOp.type() == RemoteWorkerOp::TRemoteWorkerFreezeOp ||
       aOp.type() == RemoteWorkerOp::TRemoteWorkerThawOp ||
       aOp.type() == RemoteWorkerOp::TRemoteWorkerTerminateOp ||
       aOp.type() == RemoteWorkerOp::TRemoteWorkerAddWindowIDOp ||
@@ -509,58 +529,62 @@ IPCResult RemoteWorkerChild::RecvExecOp(
     target->Dispatch(r.forget(), NS_DISPATCH_NORMAL);
     return IPC_OK();
   }
 
   if (aOp.type() == RemoteWorkerOp::TRemoteWorkerPortIdentifierOp) {
     const RemoteWorkerPortIdentifierOp& op =
         aOp.get_RemoteWorkerPortIdentifierOp();
     RefPtr<MessagePortIdentifierRunnable> runnable =
-        new MessagePortIdentifierRunnable(mWorkerPrivate, this,
+        new MessagePortIdentifierRunnable(aLock->mWorkerPrivate, this,
                                           op.portIdentifier());
     if (NS_WARN_IF(!runnable->Dispatch())) {
       ErrorPropagation(NS_ERROR_FAILURE);
     }
     return IPC_OK();
   }
 
   MOZ_CRASH("Unknown operation.");
 
   return IPC_OK();
 }
 
 void RemoteWorkerChild::RecvExecOpOnMainThread(const RemoteWorkerOp& aOp) {
   MOZ_ASSERT(NS_IsMainThread());
 
-  if (aOp.type() == RemoteWorkerOp::TRemoteWorkerSuspendOp) {
-    if (mWorkerPrivate) {
-      mWorkerPrivate->ParentWindowPaused();
-    }
-    return;
-  }
+  {
+    const auto lock = mSharedData.Lock();
 
-  if (aOp.type() == RemoteWorkerOp::TRemoteWorkerResumeOp) {
-    if (mWorkerPrivate) {
-      mWorkerPrivate->ParentWindowResumed();
+    if (aOp.type() == RemoteWorkerOp::TRemoteWorkerSuspendOp) {
+      if (lock->mWorkerPrivate) {
+        lock->mWorkerPrivate->ParentWindowPaused();
+      }
+      return;
     }
-    return;
-  }
 
-  if (aOp.type() == RemoteWorkerOp::TRemoteWorkerFreezeOp) {
-    if (mWorkerPrivate) {
-      mWorkerPrivate->Freeze(nullptr);
+    if (aOp.type() == RemoteWorkerOp::TRemoteWorkerResumeOp) {
+      if (lock->mWorkerPrivate) {
+        lock->mWorkerPrivate->ParentWindowResumed();
+      }
+      return;
     }
-    return;
-  }
 
-  if (aOp.type() == RemoteWorkerOp::TRemoteWorkerThawOp) {
-    if (mWorkerPrivate) {
-      mWorkerPrivate->Thaw(nullptr);
+    if (aOp.type() == RemoteWorkerOp::TRemoteWorkerFreezeOp) {
+      if (lock->mWorkerPrivate) {
+        lock->mWorkerPrivate->Freeze(nullptr);
+      }
+      return;
     }
-    return;
+
+    if (aOp.type() == RemoteWorkerOp::TRemoteWorkerThawOp) {
+      if (lock->mWorkerPrivate) {
+        lock->mWorkerPrivate->Thaw(nullptr);
+      }
+      return;
+    }
   }
 
   if (aOp.type() == RemoteWorkerOp::TRemoteWorkerTerminateOp) {
     CloseWorkerOnMainThread();
     return;
   }
 
   if (aOp.type() == RemoteWorkerOp::TRemoteWorkerAddWindowIDOp) {
@@ -592,36 +616,37 @@ void RemoteWorkerChild::CreationSucceede
 
   RemoteWorkerService::Thread()->Dispatch(r.forget(), NS_DISPATCH_NORMAL);
 }
 
 void RemoteWorkerChild::CreationSucceeded() {
   MOZ_ACCESS_THREAD_BOUND(mLauncherData, data);
 
   // The worker is created but we need to terminate it already.
-  if (mWorkerState == ePendingTerminated) {
+  const auto lock = mSharedData.Lock();
+  if (lock->mWorkerState == ePendingTerminated) {
     RefPtr<RemoteWorkerChild> self = this;
     nsCOMPtr<nsIRunnable> r =
         NS_NewRunnableFunction("RemoteWorkerChild::CreationSucceeded",
                                [self]() { self->CloseWorkerOnMainThread(); });
 
     nsCOMPtr<nsIEventTarget> target =
         SystemGroup::EventTargetFor(TaskCategory::Other);
     target->Dispatch(r.forget(), NS_DISPATCH_NORMAL);
     return;
   }
 
-  mWorkerState = eRunning;
+  lock->mWorkerState = eRunning;
 
   if (!mIPCActive) {
     return;
   }
 
   for (const RemoteWorkerOp& op : data->mPendingOps) {
-    RecvExecOp(op);
+    ExecuteOperation(op, lock);
   }
 
   data->mPendingOps.Clear();
 
   Unused << SendCreated(true);
 }
 
 void RemoteWorkerChild::CreationFailedOnAnyThread() {
@@ -631,17 +656,18 @@ void RemoteWorkerChild::CreationFailedOn
                              [self]() { self->CreationFailed(); });
 
   RemoteWorkerService::Thread()->Dispatch(r.forget(), NS_DISPATCH_NORMAL);
 }
 
 void RemoteWorkerChild::CreationFailed() {
   MOZ_ACCESS_THREAD_BOUND(mLauncherData, data);
 
-  mWorkerState = eTerminated;
+  const auto lock = mSharedData.Lock();
+  lock->mWorkerState = eTerminated;
   data->mPendingOps.Clear();
 
   if (!mIPCActive) {
     return;
   }
 
   Unused << SendCreated(false);
 }
--- a/dom/workers/remoteworkers/RemoteWorkerChild.h
+++ b/dom/workers/remoteworkers/RemoteWorkerChild.h
@@ -3,16 +3,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * 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_dom_RemoteWorkerChild_h
 #define mozilla_dom_RemoteWorkerChild_h
 
 #include "mozilla/dom/PRemoteWorkerChild.h"
+#include "mozilla/DataMutex.h"
 #include "mozilla/ThreadBound.h"
 #include "mozilla/UniquePtr.h"
 #include "nsISupportsImpl.h"
 
 class nsIConsoleReportCollector;
 
 namespace mozilla {
 namespace dom {
@@ -51,16 +52,23 @@ class RemoteWorkerChild final : public P
   class InitializeWorkerRunnable;
 
   ~RemoteWorkerChild();
 
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
   mozilla::ipc::IPCResult RecvExecOp(const RemoteWorkerOp& aOp);
 
+  // This member is a function template because DataMutex<SharedData>::AutoLock
+  // is private, yet it must be passed by const reference into ExecuteOperation.
+  // There should only be one instantiation of this template.
+  template <typename T>
+  mozilla::ipc::IPCResult ExecuteOperation(const RemoteWorkerOp&,
+                                           const T& aLock);
+
   void RecvExecOpOnMainThread(const RemoteWorkerOp& aOp);
 
   nsresult ExecWorkerOnMainThread(const RemoteWorkerData& aData);
 
   void ErrorPropagation(const ErrorValue& aValue);
 
   void ErrorPropagationDispatch(nsresult aError);
 
@@ -72,17 +80,16 @@ class RemoteWorkerChild final : public P
 
   void CreationFailed();
 
   void WorkerTerminated();
 
   // Touched on main-thread only.
   nsTArray<uint64_t> mWindowIDs;
 
-  RefPtr<WorkerPrivate> mWorkerPrivate;
   RefPtr<WeakWorkerRef> mWorkerRef;
   bool mIPCActive;
 
   enum WorkerState {
     // CreationSucceeded/CreationFailed not called yet.
     ePending,
 
     // The worker is not created yet, but we want to terminate as soon as
@@ -91,18 +98,24 @@ class RemoteWorkerChild final : public P
 
     // Worker up and running.
     eRunning,
 
     // Worker terminated.
     eTerminated,
   };
 
-  // Touched only on the owning thread (Worker Launcher).
-  WorkerState mWorkerState;
+  struct SharedData {
+    SharedData();
+
+    RefPtr<WorkerPrivate> mWorkerPrivate;
+    WorkerState mWorkerState;
+  };
+
+  DataMutex<SharedData> mSharedData;
 
   // Touched only on the owning thread (Worker Launcher).
   struct LauncherBoundData {
     nsTArray<RemoteWorkerOp> mPendingOps;
   };
 
   ThreadBound<LauncherBoundData> mLauncherData;
 };