Bug 1927247 - Regenerate Client Id for ServiceWorkers on termination. r=dom-worker-reviewers,webidl,smaug

Bug 1544232 changed it so ServiceWorker globals used a ClientInfo and
Client Id created by the ServiceWorkerPrivate rather than creating a
random client id.  This allows the ServiceWorkerManager to reliably map
a ServiceWorker Client Id back to the underlying ServiceWorker.

The problem with this was that ClientManagerService is not okay with
there being multiple ClientSources using the same id and it results in
an IPC_FAIL.  This was not a problem in desktop testing because under
fission the potential race window is incredibly small for a
ServiceWorker and its spawned successor to have a live ClientSource at
the same time because the ClientSource will be torn down by the
ClientManager WorkerRef on the transition to canceling and both SWs
will be spawned in the same process.  But on Android where there is no
fission, SWs spawn randomly with no affinity and so a successor can be
spawned on a different, more responsive process.

The fix here is to regenerate the Client Id whenever we terminate the
SW so we are prepared for the next time we spawn the SW.

This patch adds an additional test case to
browser_sw_lifetime_extension.js that is able to reproduce the crash
case on desktop by artificially blocking the ServiceWorker thread with
a monitor so that the ServiceWorker can't transition to Canceling until
its successor has already been spawned.  This reliably reproduces the
bug (when the fix is not in place).  This required adding some new test
infrastructure to WorkerTestUtils.

The new WorkerTestUtils methods provide 2 ways to hang the worker in a
controlled fashion until an observer notification is notified on the
main thread which use a shared helper class:

1. Use a monitor to completely block the thread until notified.  This
   prevents control runnables from running and thereby prevents worker
   refs from being notified.
2. Acquire a ThreadSafeWorkerRef and hold it until notified.  This lets
   the worker advance to Canceling but prevents progressing to Killing.

I added the WorkerRef mechanism first but it wasn't sufficient, so I
added the monitor mechanism, slightly generalizing the mechanism.

A mechanism to generate an observer notification on the main thread is
also added so that the successor ServiceWorker can notify the
predecessor SW without us needing to involve JSActors or other means
of running arbitrary JS in the process hosting the SWs.  This does mean
that when we are in non-fission mode we do need to limit the browser to
a single process in order to ensure both workers are spawned in the
same process.

Differential Revision: https://phabricator.services.mozilla.com/D227446

dom/serviceworkers/ServiceWorkerPrivate.cpp

@@ -740,6 +740,16 @@ nsresult ServiceWorkerPrivate::Initialize() {
   return NS_OK;
 }
 
+void ServiceWorkerPrivate::RegenerateClientInfo() {
+  // inductively, this object can only still be alive after Initialize() if the
+  // mClientInfo was correctly initialized.
+  MOZ_DIAGNOSTIC_ASSERT(mClientInfo.isSome());
+
+  mClientInfo = ClientManager::CreateInfo(
+      ClientType::Serviceworker, mClientInfo->GetPrincipal().unwrap().get());
+  mRemoteWorkerData.clientInfo().ref() = mClientInfo.ref().ToIPC();
+}
+
 nsresult ServiceWorkerPrivate::CheckScriptEvaluation(
     const ServiceWorkerLifetimeExtension& aLifetimeExtension,
     RefPtr<LifeCycleEventCallback> aCallback) {
@@ -1859,9 +1869,16 @@ RefPtr<GenericNonExclusivePromise> ServiceWorkerPrivate::ShutdownInternal(
 
   /**
    * After dispatching a termination operation, no new operations should
-   * be routed through this actor anymore.
+   * be routed through this actor anymore so we can drop the controller
+   * reference.  This also means that the next time SpawnWorkerIfNeeded is
+   * invoked we will spawn a new worker, creating a new mControllerChild.
    */
   mControllerChild = nullptr;
+  // Create a new ClientInfo for the next time we potentially spawn this
+  // ServiceWorker.  We do this now rather than immediately before spawning the
+  // ServiceWorker so it's possible to know what the client id will be before
+  // triggering the next spawn.
+  RegenerateClientInfo();
 
   // Update here, since Evaluation failures directly call ShutdownInternal
   UpdateRunning(-1, mHandlesFetch == Enabled ? -1 : 0);

dom/serviceworkers/ServiceWorkerPrivate.h

@@ -212,6 +212,8 @@ class ServiceWorkerPrivate final : public RemoteWorkerObserver {
 
   nsresult Initialize();
 
+  void RegenerateClientInfo();
+
   /**
    * RemoteWorkerObserver
    */

dom/serviceworkers/test/browser_sw_lifetime_extension.js

@@ -312,3 +312,86 @@ async function test_service_worker_creating_new_registrations() {
   );
 }
 add_task(test_service_worker_creating_new_registrations);
+
+/**
+ * In bug 1927247 a ServiceWorker respawned sufficiently soon after its
+ * termination resulted in a defensive content-process crash when the new SW's
+ * ClientSource was registered with the same Client Id while the old SW's
+ * ClientSource still existed.  We synthetically induce this situation through
+ * use of nsIServiceWorkerInfo::terminateWorker() immediately followed by use of
+ * nsIServiceWorkerInfo::attachDebugger().
+ */
+async function test_respawn_immediately_after_termination() {
+  // Make WorkerTestUtils work in the SW.
+  await SpecialPowers.pushPrefEnv({
+    set: [["dom.workers.testing.enabled", true]],
+  });
+
+  // We need to ensure all ServiceWorkers are spawned consistently in the same
+  // process because of the trick we do with workerrefs and using the observer
+  // service, so force us to only use a single process if fission is not on.
+  if (!Services.appinfo.fissionAutostart) {
+    // Force use of only a single process
+    await SpecialPowers.pushPrefEnv({
+      set: [["dom.ipc.processCount", 1]],
+    });
+  }
+
+  info("## Installing the ServiceWorker we will terminate and respawn");
+  const tSwDesc = {
+    origin: TEST_ORIGIN,
+    scope: "sw-t",
+    script: "sw_inter_sw_postmessage.js?t",
+  };
+
+  // Wipe the origin for cleanup; this will remove the registrations too.
+  registerCleanupFunction(async () => {
+    await clear_qm_origin_group_via_clearData(TEST_ORIGIN);
+  });
+
+  const tReg = await install_sw(tSwDesc);
+  const tSWInfo = tReg.activeWorker;
+
+  info("## Induce the SW to acquire a WorkerRef that prevents shutdown.");
+
+  const { closeHelperTab, broadcastAndWaitFor, postMessageScopeAndWaitFor } =
+    await createMessagingHelperTab(TEST_ORIGIN, "inter-sw-postmessage");
+  registerCleanupFunction(closeHelperTab);
+
+  // Tell the ServiceWorker to block on a monitor that will prevent the worker
+  // from transitioning to the Canceling state and notifying its WorkerRefs,
+  // thereby preventing the ClientManagerChild from beginning teardown of itself
+  // and thereby the ClientSourceChild.  The monitor will be released when we
+  // cause "serviceworker-t-release" to be notified on that process's main
+  // thread.
+  await broadcastAndWaitFor(
+    "t:block:serviceworker-t-release",
+    "t:blocking:serviceworker-t-release"
+  );
+
+  info("## Terminating and respawning the ServiceWorker via attachDebugger");
+  // We must not await the termination if we want to create the lifetime overlap
+  const terminationPromise = tSWInfo.terminateWorker();
+  // Message the ServiceWorker to cause it to spawn, waiting for the newly
+  // spawned ServiceWorker to indicate it is alive and running.
+  await postMessageScopeAndWaitFor(
+    "sw-t",
+    "Hello, the contents of this message don't matter!",
+    "t:received-post-message-from:wc-helper"
+  );
+
+  // Tell the successor to generate an observer notification that will release
+  // the ThreadSafeWorkerRef.  Note that this does assume the ServiceWorker is
+  // placed in the same process as its predecessor.  When isolation is enabled,
+  // like on desktop, this will always be the same process because there will
+  // only be the one possible process.  "browser" tests like this are only run
+  // on desktop, never on Android!  But if we weren't isolating,
+  await broadcastAndWaitFor(
+    "t:notify-observer:serviceworker-t-release",
+    "t:notified-observer:serviceworker-t-release"
+  );
+
+  info("## Awaiting the termination");
+  await terminationPromise;
+}
+add_task(test_respawn_immediately_after_termination);

dom/serviceworkers/test/sw_inter_sw_postmessage.js

@@ -156,6 +156,25 @@ bc.onmessage = async function handle_bc(evt) {
         scope,
       });
       bc.postMessage(`${myId}:registered:${installId}`);
+    } else if (cmd === "workerref-hang") {
+      const topic = pieces[2];
+      globalThis.WorkerTestUtils.holdStrongWorkerRefUntilMainThreadObserverNotified(
+        topic
+      );
+      bc.postMessage(`${myId}:workerref-hung:${topic}`);
+    } else if (cmd === "block") {
+      const topic = pieces[2];
+      globalThis.WorkerTestUtils.blockUntilMainThreadObserverNotified(
+        topic,
+        // This callback is invoked once the observer has been registered.
+        () => {
+          bc.postMessage(`${myId}:blocking:${topic}`);
+        }
+      );
+    } else if (cmd === "notify-observer") {
+      const topic = pieces[2];
+      globalThis.WorkerTestUtils.notifyObserverOnMainThread(topic);
+      bc.postMessage(`${myId}:notified-observer:${topic}`);
     }
   } catch (ex) {
     console.error(ex);

dom/webidl/WorkerTestUtils.webidl

@@ -2,10 +2,37 @@
  * 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/. */
 
+callback WorkerTestCallback = undefined ();
+
 [Exposed=Worker, Pref="dom.workers.testing.enabled"]
 namespace WorkerTestUtils {
   [Throws]
   unsigned long currentTimerNestingLevel();
   [Throws]
   boolean IsRunningInBackground();
+
+  // Mint a (ThreadSafe)StrongWorkerRef and hold it until we see an observer
+  // notification with the given string topic on the main thread.  The
+  // StrongWorkerRef will prevent the worker from moving from the Canceling to
+  // the Killing state and thereby prevent it from shutting down.
+  [Throws]
+  undefined holdStrongWorkerRefUntilMainThreadObserverNotified(UTF8String topic);
+
+  // Create a monitor that blocks the worker entirely until we see an observer
+  // notification with the given string topic on the main thread.  This is a
+  // much more drastic variant of
+  // holdStrongWorkerRefUntilMainThreadObserverNotified for when the goal is to
+  // avoid letting WorkerRefs be notified.  A callback should be passed to be
+  // synchronously invoked once the observer has been registered in order to
+  // communicate that the test can move forward.
+  [Throws]
+  undefined blockUntilMainThreadObserverNotified(UTF8String topic,
+                                                 WorkerTestCallback whenObserving);
+
+  // Dispatch a runnable to the main thread to notify the observer service with
+  // the given topic.  This is primarily intended for use with
+  // `holdStrongWorkerRefUntilMainThreadObserverNotified` but might be useful in
+  // other situations to cut down on test boilerplate.
+  [Throws]
+  undefined notifyObserverOnMainThread(UTF8String topic);
 };

dom/workers/WorkerTestUtils.cpp

@@ -5,8 +5,14 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/ErrorResult.h"
+#include "mozilla/Monitor.h"
+#include "mozilla/RefPtr.h"
 #include "mozilla/dom/WorkerPrivate.h"
+#include "mozilla/dom/WorkerRef.h"
 #include "mozilla/dom/WorkerTestUtils.h"
+#include "mozilla/dom/WorkerTestUtilsBinding.h"
+#include "nsIObserverService.h"
+#include "nsThreadUtils.h"
 
 namespace mozilla::dom {
 
@@ -25,4 +31,145 @@ bool WorkerTestUtils::IsRunningInBackground(const GlobalObject&,
   MOZ_ASSERT(worker);
   return worker->IsRunningInBackground();
 }
+
+namespace {
+
+// Helper for HoldStrongWorkerRefUntilMainThreadObserverNotified that optionally
+// holds a ThreadSafeWorkerRef until the given observer notification is notified
+// and also notifies a monitor.
+class WorkerTestUtilsObserver final : public nsIObserver {
+ public:
+  WorkerTestUtilsObserver(const nsACString& aTopic,
+                          RefPtr<ThreadSafeWorkerRef>&& aWorkerRef)
+      : mMonitor("WorkerTestUtils"),
+        mTopic(aTopic),
+        mWorkerRef(std::move(aWorkerRef)),
+        mRegistered(false),
+        mObserved(false) {}
+
+  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_IMETHOD Observe(nsISupports* aSubject, const char* aTopic,
+                     const char16_t* aData) override {
+    // We only register for one topic so we don't actually need to compare it.
+    nsCOMPtr<nsIObserverService> observerService =
+        services::GetObserverService();
+    MOZ_ALWAYS_SUCCEEDS(observerService->RemoveObserver(this, mTopic.get()));
+
+    // The ThreadSafeWorkerRef is responsible for / knows how to drop the
+    // underlying StrongWorkerRef on the worker.
+    mWorkerRef = nullptr;
+
+    MonitorAutoLock lock(mMonitor);
+    mObserved = true;
+    mMonitor.Notify();
+
+    return NS_OK;
+  }
+
+  void Register() {
+    nsCOMPtr<nsIObserverService> observerService =
+        services::GetObserverService();
+    MOZ_ALWAYS_SUCCEEDS(
+        observerService->AddObserver(this, mTopic.get(), false));
+
+    MonitorAutoLock lock(mMonitor);
+    mRegistered = true;
+    mMonitor.Notify();
+  }
+
+  void WaitOnRegister() {
+    MonitorAutoLock lock(mMonitor);
+    while (!mRegistered) {
+      mMonitor.Wait();
+    }
+  }
+
+  void WaitOnObserver() {
+    MonitorAutoLock lock(mMonitor);
+    while (!mObserved) {
+      mMonitor.Wait();
+    }
+  }
+
+ private:
+  ~WorkerTestUtilsObserver() = default;
+
+  Monitor mMonitor;
+  nsAutoCString mTopic;
+  RefPtr<ThreadSafeWorkerRef> mWorkerRef;
+  bool mRegistered;
+  bool mObserved;
+};
+
+NS_IMPL_ISUPPORTS(WorkerTestUtilsObserver, nsIObserver)
+
+}  // anonymous namespace
+
+void WorkerTestUtils::HoldStrongWorkerRefUntilMainThreadObserverNotified(
+    const GlobalObject&, const nsACString& aTopic, ErrorResult& aErr) {
+  MOZ_ASSERT(!NS_IsMainThread());
+
+  WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
+  MOZ_ASSERT(workerPrivate);
+
+  RefPtr<StrongWorkerRef> strongWorkerRef =
+      StrongWorkerRef::Create(workerPrivate, "WorkerTestUtils");
+  if (NS_WARN_IF(!strongWorkerRef)) {
+    aErr.Throw(NS_ERROR_FAILURE);
+    return;
+  }
+
+  RefPtr<ThreadSafeWorkerRef> tsWorkerRef =
+      new ThreadSafeWorkerRef(strongWorkerRef);
+
+  auto observer =
+      MakeRefPtr<WorkerTestUtilsObserver>(aTopic, std::move(tsWorkerRef));
+
+  aErr = NS_DispatchToMainThread(NewRunnableMethod(
+      "WorkerTestUtils::HoldStrongWorkerRefUntilMainThreadObserverNotified",
+      observer, &WorkerTestUtilsObserver::Register));
+
+  // Wait for the observer to be registered before returning control so that we
+  // can be certain we won't miss an observer notification.
+  observer->WaitOnRegister();
+}
+
+void WorkerTestUtils::BlockUntilMainThreadObserverNotified(
+    const GlobalObject&, const nsACString& aTopic,
+    WorkerTestCallback& aWhenObserving, ErrorResult& aErr) {
+  MOZ_ASSERT(!NS_IsMainThread());
+
+  auto observer = MakeRefPtr<WorkerTestUtilsObserver>(aTopic, nullptr);
+
+  aErr = NS_DispatchToMainThread(
+      NewRunnableMethod("WorkerTestUtils::BlockUntilMainThreadObserverNotified",
+                        observer, &WorkerTestUtilsObserver::Register));
+  if (aErr.Failed()) {
+    return;
+  }
+
+  observer->WaitOnRegister();
+
+  aWhenObserving.Call(aErr);
+  if (aErr.Failed()) {
+    return;
+  }
+
+  observer->WaitOnObserver();
+}
+
+void WorkerTestUtils::NotifyObserverOnMainThread(const GlobalObject&,
+                                                 const nsACString& aTopic,
+                                                 ErrorResult& aErr) {
+  MOZ_ASSERT(!NS_IsMainThread());
+
+  aErr = NS_DispatchToMainThread(NS_NewRunnableFunction(
+      "WorkerTestUtils::NotifyObserverOnMainThread",
+      [topic = nsCString(aTopic)] {
+        nsCOMPtr<nsIObserverService> observerService =
+            services::GetObserverService();
+        observerService->NotifyObservers(nullptr, topic.get(), nullptr);
+      }));
+}
+
 }  // namespace mozilla::dom

dom/workers/WorkerTestUtils.h

@@ -7,12 +7,17 @@
 #ifndef mozilla_dom_WorkerTestUtils__
 #define mozilla_dom_WorkerTestUtils__
 
+#include "nsStringFwd.h"
+
 namespace mozilla {
 
 class ErrorResult;
+class GlobalObject;
 
 namespace dom {
 
+class WorkerTestCallback;
+
 /**
  * dom/webidl/WorkerTestUtils.webidl defines APIs to expose worker's internal
  * status for glass-box testing. The APIs are only exposed to Workers with prefs
@@ -35,6 +40,17 @@ class WorkerTestUtils final {
                                            ErrorResult& aErr);
 
   static bool IsRunningInBackground(const GlobalObject&, ErrorResult& aErr);
+
+  static void HoldStrongWorkerRefUntilMainThreadObserverNotified(
+      const GlobalObject&, const nsACString& aTopic, ErrorResult& aErr);
+
+  MOZ_CAN_RUN_SCRIPT static void BlockUntilMainThreadObserverNotified(
+      const GlobalObject&, const nsACString& aTopic,
+      WorkerTestCallback& aWhenObserving, ErrorResult& aErr);
+
+  static void NotifyObserverOnMainThread(const GlobalObject&,
+                                         const nsACString& aTopic,
+                                         ErrorResult& aErr);
 };
 
 }  // namespace dom