Bug 1568597 - RemoteWorkerManager::SelectTargetActorForSharedWorker should select an actor that is kept alive. r=asuth

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

dom/workers/remoteworkers/RemoteWorkerManager.cpp

@@ -272,7 +272,7 @@ void RemoteWorkerManager::RegisterActor(RemoteWorkerServiceParent* aActor) {
   mChildActors.AppendElement(aActor);
 
   if (!mPendings.IsEmpty()) {
-    const auto& remoteType = GetRemoteTypeForActor(aActor);
+    const auto& processRemoteType = aActor->GetRemoteType();
     nsTArray<Pending> unlaunched;
 
     // Flush pending launching.
@@ -283,7 +283,7 @@ void RemoteWorkerManager::RegisterActor(RemoteWorkerServiceParent* aActor) {
 
       const auto& workerRemoteType = p.mData.remoteType();
 
-      if (MatchRemoteType(remoteType, workerRemoteType)) {
+      if (MatchRemoteType(processRemoteType, workerRemoteType)) {
         LOG(("RegisterActor - Launch Pending, workerRemoteType=%s",
              workerRemoteType.get()));
         LaunchInternal(p.mController, aActor, p.mData);
@@ -346,11 +346,13 @@ void RemoteWorkerManager::Launch(RemoteWorkerController* aController,
   }
 
   /**
-   * If a target actor for a Service Worker has been selected, the actor has
-   * already been registered with the corresponding ContentParent (see
-   * `SelectTargetActorForServiceWorker()`).
+   * If a target actor for the remote worker has been selected, the actor has
+   * already been registered with the corresponding `ContentParent` and we
+   * should not increment the `mRemoteWorkerActorData`'s `mCount` again (see
+   * `SelectTargetActorForServiceWorker()` /
+   * `SelectTargetActorForSharedWorker()`).
    */
-  LaunchInternal(aController, targetActor, aData, IsServiceWorker(aData));
+  LaunchInternal(aController, targetActor, aData, true);
 }
 
 void RemoteWorkerManager::LaunchInternal(
@@ -388,34 +390,26 @@ void RemoteWorkerManager::AsyncCreationFailed(
   NS_DispatchToCurrentThread(r.forget());
 }
 
-/* static */
-nsCString RemoteWorkerManager::GetRemoteTypeForActor(
-    const RemoteWorkerServiceParent* aActor) {
-  AssertIsInMainProcess();
-  AssertIsOnBackgroundThread();
-
-  MOZ_ASSERT(aActor);
-
-  RefPtr<ContentParent> contentParent =
-      BackgroundParent::GetContentParent(aActor->Manager());
-  auto scopeExit =
-      MakeScopeExit([&] { NS_ReleaseOnMainThread(contentParent.forget()); });
-
-  if (NS_WARN_IF(!contentParent)) {
-    return EmptyCString();
-  }
-
-  nsCString aRemoteType(contentParent->GetRemoteType());
-
-  return aRemoteType;
-}
-
 template <typename Callback>
-void RemoteWorkerManager::ForEachActor(Callback&& aCallback) const {
+void RemoteWorkerManager::ForEachActor(
+    Callback&& aCallback, const nsACString& aRemoteType,
+    Maybe<base::ProcessId> aProcessId) const {
   AssertIsOnBackgroundThread();
 
   const auto length = mChildActors.Length();
-  const auto end = static_cast<uint32_t>(rand()) % length;
+
+  auto end = static_cast<uint32_t>(rand()) % length;
+  if (aProcessId) {
+    // Start from the actor with the given processId instead of starting from
+    // a random index.
+    for (auto j = length - 1; j > 0; j--) {
+      if (mChildActors[j]->OtherPid() == *aProcessId) {
+        end = j;
+        break;
+      }
+    }
+  }
+
   uint32_t i = end;
 
   nsTArray<RefPtr<ContentParent>> proxyReleaseArray;
@@ -424,14 +418,16 @@ void RemoteWorkerManager::ForEachActor(Callback&& aCallback) const {
     MOZ_ASSERT(i < mChildActors.Length());
     RemoteWorkerServiceParent* actor = mChildActors[i];
 
-    RefPtr<ContentParent> contentParent =
-        BackgroundParent::GetContentParent(actor->Manager());
+    if (MatchRemoteType(actor->GetRemoteType(), aRemoteType)) {
+      RefPtr<ContentParent> contentParent =
+          BackgroundParent::GetContentParent(actor->Manager());
 
-    auto scopeExit = MakeScopeExit(
-        [&]() { proxyReleaseArray.AppendElement(std::move(contentParent)); });
+      auto scopeExit = MakeScopeExit(
+          [&]() { proxyReleaseArray.AppendElement(std::move(contentParent)); });
 
-    if (!aCallback(actor, std::move(contentParent))) {
-      break;
+      if (!aCallback(actor, std::move(contentParent))) {
+        break;
+      }
     }
 
     i = (i + 1) % length;
@@ -475,41 +471,55 @@ RemoteWorkerManager::SelectTargetActorForServiceWorker(
 
   const auto& workerRemoteType = aData.remoteType();
 
-  ForEachActor([&](RemoteWorkerServiceParent* aActor,
-                   RefPtr<ContentParent>&& aContentParent) {
-    const auto& remoteType = aContentParent->GetRemoteType();
+  ForEachActor(
+      [&](RemoteWorkerServiceParent* aActor,
+          RefPtr<ContentParent>&& aContentParent) {
+        auto lock = aContentParent->mRemoteWorkerActorData.Lock();
 
-    if (MatchRemoteType(remoteType, workerRemoteType)) {
-      auto lock = aContentParent->mRemoteWorkerActorData.Lock();
+        // Select the first actor that matches the remoteType and it is not
+        // already shutting down.
+        if (lock->mCount || !lock->mShutdownStarted) {
+          ++lock->mCount;
 
-      if (lock->mCount || !lock->mShutdownStarted) {
-        ++lock->mCount;
+          // This won't cause any race conditions because the content process
+          // should wait for the permissions to be received before executing the
+          // Service Worker.
+          nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
+              __func__, [contentParent = std::move(aContentParent),
+                         principalInfo = aData.principalInfo()] {
+                TransmitPermissionsAndBlobURLsForPrincipalInfo(contentParent,
+                                                               principalInfo);
+              });
 
-        // This won't cause any race conditions because the content process
-        // should wait for the permissions to be received before executing the
-        // Service Worker.
-        nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
-            __func__, [contentParent = std::move(aContentParent),
-                       principalInfo = aData.principalInfo()] {
-              TransmitPermissionsAndBlobURLsForPrincipalInfo(contentParent,
-                                                             principalInfo);
-            });
+          MOZ_ALWAYS_SUCCEEDS(
+              SchedulerGroup::Dispatch(TaskCategory::Other, r.forget()));
 
-        MOZ_ALWAYS_SUCCEEDS(
-            SchedulerGroup::Dispatch(TaskCategory::Other, r.forget()));
+          actor = aActor;
+          return false;
+        }
 
-        actor = aActor;
-        return false;
-      }
-    }
-
-    MOZ_ASSERT(!actor);
-    return true;
-  });
+        MOZ_ASSERT(!actor);
+        return true;
+      },
+      workerRemoteType);
 
   return actor;
 }
 
+/**
+ * When Fission is enabled, Shared Workers may have to be spawned into different
+ * child process from the one where it has been registered from, and that child
+ * process may be going to be marked as dead and shutdown.
+ *
+ * To make sure to keep the selected child process alive we can used the same
+ * strategy that is being used by
+ * RemoteWorkerManager::SelectTargetActorForServiceWorker for very similar
+ * reasons and described in more detail in the inline comment right above that
+ * method (in short here on the background thread, while
+ * `ContentParent::mRemoteWorkerActorData` is locked, if `mCount` > 0, we can
+ * register the remote worker actor "early" and guarantee that the corresponding
+ * content process will not shutdown).
+ */
 RemoteWorkerServiceParent*
 RemoteWorkerManager::SelectTargetActorForSharedWorker(
     base::ProcessId aProcessId, const RemoteWorkerData& aData) const {
@@ -518,26 +528,31 @@ RemoteWorkerManager::SelectTargetActorForSharedWorker(
 
   RemoteWorkerServiceParent* actor = nullptr;
 
-  ForEachActor([&](RemoteWorkerServiceParent* aActor,
-                   RefPtr<ContentParent>&& aContentParent) {
-    bool matchRemoteType =
-        MatchRemoteType(aContentParent->GetRemoteType(), aData.remoteType());
+  const auto& workerRemoteType = aData.remoteType();
 
-    if (!matchRemoteType) {
-      return true;
-    }
+  ForEachActor(
+      [&](RemoteWorkerServiceParent* aActor,
+          RefPtr<ContentParent>&& aContentParent) {
+        // Make sure to choose an actor related to a child process that is not
+        // going to shutdown while we are still in the process of launching the
+        // remote worker.
+        //
+        // ForEachActor will start from the child actor coming from the child
+        // process with a pid equal to aProcessId if any, otherwise it would
+        // start from a random actor in the mChildActors array, this guarantees
+        // that we will choose that actor if it does also match the remote type.
+        auto lock = aContentParent->mRemoteWorkerActorData.Lock();
+        if ((lock->mCount || !lock->mShutdownStarted) &&
+            (aActor->OtherPid() == aProcessId || !actor)) {
+          ++lock->mCount;
+          actor = aActor;
+          return false;
+        }
 
-    if (aActor->OtherPid() == aProcessId) {
-      actor = aActor;
-      return false;
-    }
-
-    if (!actor) {
-      actor = aActor;
-    }
-
-    return true;
-  });
+        MOZ_ASSERT(!actor);
+        return true;
+      },
+      workerRemoteType, Some(aProcessId));
 
   return actor;
 }

dom/workers/remoteworkers/RemoteWorkerManager.h

@@ -75,11 +75,10 @@ class RemoteWorkerManager final {
 
   void AsyncCreationFailed(RemoteWorkerController* aController);
 
-  static nsCString GetRemoteTypeForActor(
-      const RemoteWorkerServiceParent* aActor);
-
-  // Iterate through all RemoteWorkerServiceParent actors, starting from a
-  // random index (as if iterating through a circular array).
+  // Iterate through all RemoteWorkerServiceParent actors with the given
+  // remoteType, starting from the actor related to a child process with pid
+  // aProcessId if needed and available or from a random index otherwise (as if
+  // iterating through a circular array).
   //
   // aCallback should be a invokable object with a function signature of
   //   bool (RemoteWorkerServiceParent*, RefPtr<ContentParent>&&)
@@ -91,7 +90,8 @@ class RemoteWorkerManager final {
   // doesn't need to worry about proxy-releasing the ContentParent if it isn't
   // moved out of the parameter.
   template <typename Callback>
-  void ForEachActor(Callback&& aCallback) const;
+  void ForEachActor(Callback&& aCallback, const nsACString& aRemoteType,
+                    Maybe<base::ProcessId> aProcessId = Nothing()) const;
 
   // The list of existing RemoteWorkerServiceParent actors for child processes.
   // Raw pointers because RemoteWorkerServiceParent actors unregister themselves

dom/workers/remoteworkers/RemoteWorkerServiceParent.cpp

@@ -18,8 +18,9 @@ RemoteWorkerServiceParent::RemoteWorkerServiceParent()
 
 RemoteWorkerServiceParent::~RemoteWorkerServiceParent() = default;
 
-void RemoteWorkerServiceParent::Initialize() {
+void RemoteWorkerServiceParent::Initialize(const nsACString& aRemoteType) {
   AssertIsOnBackgroundThread();
+  mRemoteType = aRemoteType;
   mManager->RegisterActor(this);
 }
 

dom/workers/remoteworkers/RemoteWorkerServiceParent.h

@@ -8,6 +8,7 @@
 #define mozilla_dom_RemoteWorkerServiceParent_h
 
 #include "mozilla/dom/PRemoteWorkerServiceParent.h"
+#include "mozilla/dom/RemoteType.h"
 
 namespace mozilla {
 namespace dom {
@@ -21,10 +22,13 @@ class RemoteWorkerServiceParent final : public PRemoteWorkerServiceParent {
 
   void ActorDestroy(mozilla::ipc::IProtocol::ActorDestroyReason) override;
 
-  void Initialize();
+  void Initialize(const nsACString& aRemoteType);
+
+  nsCString GetRemoteType() const { return mRemoteType; }
 
  private:
   RefPtr<RemoteWorkerManager> mManager;
+  nsCString mRemoteType = NOT_REMOTE_TYPE;
 };
 
 }  // namespace dom

ipc/glue/BackgroundParentImpl.cpp

@@ -523,7 +523,15 @@ IPCResult BackgroundParentImpl::RecvPRemoteWorkerServiceConstructor(
     PRemoteWorkerServiceParent* aActor) {
   mozilla::dom::RemoteWorkerServiceParent* actor =
       static_cast<mozilla::dom::RemoteWorkerServiceParent*>(aActor);
-  actor->Initialize();
+
+  RefPtr<ContentParent> parent = BackgroundParent::GetContentParent(this);
+  // If the ContentParent is null we are dealing with a same-process actor.
+  if (!parent) {
+    actor->Initialize(NOT_REMOTE_TYPE);
+  } else {
+    actor->Initialize(parent->GetRemoteType());
+    NS_ReleaseOnMainThread("ContentParent release", parent.forget());
+  }
   return IPC_OK();
 }