Bug 1615014 - ensure performance storage/counter is set before being read r=dom-workers-and-storage-reviewers,asuth
authorPerry Jiang <perry@mozilla.com>
Thu, 26 Mar 2020 03:22:42 +0000
changeset 520470 52798009e73aa85ba9f2309b7d150c3eea4d7622
parent 520469 c626a363823edf7b8ba37e7915a2c339c6f9318d
child 520471 08ad6f3a8a55d4a3a53c70c662516b146296cd1e
push id37251
push usermalexandru@mozilla.com
push dateThu, 26 Mar 2020 09:33:08 +0000
treeherdermozilla-central@3e5a7430c8d7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdom-workers-and-storage-reviewers, asuth
bugs1615014
milestone76.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 1615014 - ensure performance storage/counter is set before being read r=dom-workers-and-storage-reviewers,asuth WorkerThreadPrimaryRunnable possibly indirectly creates a SendInitBackgroundRunnable (runs on the main thread), which causes a data race with CompileScriptRunnable (runs on the worker thread) by having an unsynchronized read/write of WorkerPrivate::mPerformanceCounter. Differential Revision: https://phabricator.services.mozilla.com/D67434
dom/workers/RuntimeService.cpp
dom/workers/WorkerDebugger.cpp
dom/workers/WorkerPrivate.cpp
--- a/dom/workers/RuntimeService.cpp
+++ b/dom/workers/RuntimeService.cpp
@@ -2217,25 +2217,16 @@ bool LogViolationDetailsRunnable::MainTh
 MOZ_CAN_RUN_SCRIPT_BOUNDARY
 NS_IMETHODIMP
 WorkerThreadPrimaryRunnable::Run() {
   AUTO_PROFILER_LABEL_DYNAMIC_LOSSY_NSSTRING(
       "WorkerThreadPrimaryRunnable::Run", OTHER, mWorkerPrivate->ScriptURL());
 
   using mozilla::ipc::BackgroundChild;
 
-  // Note: GetOrCreateForCurrentThread() must be called prior to
-  //       mWorkerPrivate->SetThread() in order to avoid accidentally consuming
-  //       worker messages here.
-  bool ipcReady = true;
-  if (NS_WARN_IF(!BackgroundChild::GetOrCreateForCurrentThread())) {
-    // Let's report the error only after SetThread().
-    ipcReady = false;
-  }
-
   class MOZ_STACK_CLASS SetThreadHelper final {
     // Raw pointer: this class is on the stack.
     WorkerPrivate* mWorkerPrivate;
     RefPtr<AbstractThread> mAbstractThread;
 
    public:
     SetThreadHelper(WorkerPrivate* aWorkerPrivate, WorkerThread* aThread)
         : mWorkerPrivate(aWorkerPrivate),
@@ -2259,17 +2250,22 @@ WorkerThreadPrimaryRunnable::Run() {
       mWorkerPrivate = nullptr;
     }
   };
 
   SetThreadHelper threadHelper(mWorkerPrivate, mThread);
 
   mWorkerPrivate->AssertIsOnWorkerThread();
 
-  if (!ipcReady) {
+  // These need to be initialized on the worker thread before being used on
+  // the main thread.
+  mWorkerPrivate->EnsurePerformanceStorage();
+  mWorkerPrivate->EnsurePerformanceCounter();
+
+  if (NS_WARN_IF(!BackgroundChild::GetOrCreateForCurrentThread())) {
     WorkerErrorReport::CreateAndDispatchGenericErrorRunnableToParent(
         mWorkerPrivate);
     return NS_ERROR_FAILURE;
   }
 
   {
     nsCycleCollector_startup();
 
--- a/dom/workers/WorkerDebugger.cpp
+++ b/dom/workers/WorkerDebugger.cpp
@@ -87,21 +87,16 @@ class CompileDebuggerScriptRunnable fina
     if (NS_WARN_IF(!aWorkerPrivate->EnsureClientSource())) {
       return false;
     }
 
     if (NS_WARN_IF(!aWorkerPrivate->EnsureCSPEventListener())) {
       return false;
     }
 
-    // Initialize performance state which might be used on the main thread, as
-    // in CompileScriptRunnable. This runnable might execute first.
-    aWorkerPrivate->EnsurePerformanceStorage();
-    aWorkerPrivate->EnsurePerformanceCounter();
-
     JS::Rooted<JSObject*> global(aCx, globalScope->GetWrapper());
 
     ErrorResult rv;
     JSAutoRealm ar(aCx, global);
     workerinternals::LoadMainScript(aWorkerPrivate, nullptr, mScriptURL,
                                     DebuggerScript, rv);
     rv.WouldReportJSException();
     // Explicitly ignore NS_BINDING_ABORTED on rv.  Or more precisely, still
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -334,23 +334,16 @@ class CompileScriptRunnable final : publ
     if (NS_WARN_IF(!aWorkerPrivate->EnsureClientSource())) {
       return false;
     }
 
     if (NS_WARN_IF(!aWorkerPrivate->EnsureCSPEventListener())) {
       return false;
     }
 
-    // PerformanceStorage & PerformanceCounter both need to be initialized
-    // on the worker thread before being used on main-thread.
-    // Let's be sure that it is created before any
-    // content loading.
-    aWorkerPrivate->EnsurePerformanceStorage();
-    aWorkerPrivate->EnsurePerformanceCounter();
-
     ErrorResult rv;
     workerinternals::LoadMainScript(aWorkerPrivate, std::move(mOriginStack),
                                     mScriptURL, WorkerScript, rv);
     rv.WouldReportJSException();
     // Explicitly ignore NS_BINDING_ABORTED on rv.  Or more precisely, still
     // return false and don't SetWorkerScriptExecutedSuccessfully() in that
     // case, but don't throw anything on aCx.  The idea is to not dispatch error
     // events if our load is canceled with that error code.