Bug 1575185 - Subscribe content processes spawning Service Workers to permission updates r=asuth
authorPerry Jiang <perry@mozilla.com>
Wed, 09 Oct 2019 02:23:41 +0000
changeset 496880 5e3149673e9345667919a04dff47f325ce0b1685
parent 496879 bee723fdb043afb5e1485cc0b8e7f658c8b9354b
child 496881 1d1d2599d5052282bbe74b7fa4167e8aa8431b41
push id97516
push userpjiang@mozilla.com
push dateWed, 09 Oct 2019 03:43:49 +0000
treeherderautoland@5e3149673e93 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth
bugs1575185
milestone71.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 1575185 - Subscribe content processes spawning Service Workers to permission updates r=asuth Previously, Service Workers could spawn in a process that isn't subscribed to permission updates, which could happen if that process hadn't loaded any same-origin documents. To address this, parent-process logic for spawning Service Workers would snapshot the permissions state to be sent to a content process. Unfortunately, this approach could lead to outdated, unsynchronized permissions. Note that nsIPermissionManager::SetPermissionsWithKey is only used to initialize permissions for a given key and is a no-op if already called with the same key in a given process. As a result, the following sequence of events could happen: Assume a content process CP that isn't subscribed to permission changes for an origin A: 1) Parent process decides to spawn an origin A Service Worker in CP, snapshotting a value V for permission P. 2) The Service Worker is spawned in CP, setting CP's permission manager's permission P to value V (for origin A). 3) Parent process updates its permission P to a value A', which is not broadcasted to CP (because it's not subscribed). 4) By now, the initial Service Worker has been terminated, and the parent process decides once again to spawn an origin A Service Worker in CP. 5) The Service Worker is spawned in CP, but the call to SetPermissionsWithKey is a no-op, leaving CP1 with a mismatched value for permission P. An additional scenario is if the parent process updates a permission during a remote Service Worker's lifetime. This patch, which would subscribe CP1 to permission updates when the parent process knows a Service Worker would be spawned in CP1, prevents these problems. Differential Revision: https://phabricator.services.mozilla.com/D48620
dom/push/test/mochitest.ini
dom/serviceworkers/ServiceWorkerPrivateImpl.cpp
dom/serviceworkers/ServiceWorkerPrivateImpl.h
dom/workers/remoteworkers/RemoteWorkerChild.cpp
dom/workers/remoteworkers/RemoteWorkerManager.cpp
dom/workers/remoteworkers/RemoteWorkerManager.h
dom/workers/remoteworkers/RemoteWorkerTypes.ipdlh
--- a/dom/push/test/mochitest.ini
+++ b/dom/push/test/mochitest.ini
@@ -6,17 +6,16 @@ support-files =
   webpush.js
   lifetime_worker.js
   test_utils.js
   mockpushserviceparent.js
   error_worker.js
 
 [test_has_permissions.html]
 [test_permissions.html]
-skip-if = fission # bug 1575185
 [test_register.html]
 skip-if = os == "win" # Bug 1373346
 [test_register_key.html]
 [test_multiple_register.html]
 [test_multiple_register_during_service_activation.html]
 skip-if = (os == "win") || (os == "linux") || (os == "mac") #Bug 1274773
 [test_unregister.html]
 [test_multiple_register_different_scope.html]
--- a/dom/serviceworkers/ServiceWorkerPrivateImpl.cpp
+++ b/dom/serviceworkers/ServiceWorkerPrivateImpl.cpp
@@ -17,20 +17,18 @@
 #include "nsIChannel.h"
 #include "nsIHttpChannel.h"
 #include "nsIHttpChannelInternal.h"
 #include "nsIHttpHeaderVisitor.h"
 #include "nsIInputStream.h"
 #include "nsILoadInfo.h"
 #include "nsINetworkInterceptController.h"
 #include "nsIObserverService.h"
-#include "nsIPermissionManager.h"
 #include "nsIURI.h"
 #include "nsIUploadChannel2.h"
-#include "nsPermissionManager.h"
 #include "nsThreadUtils.h"
 
 #include "ServiceWorkerManager.h"
 #include "ServiceWorkerRegistrationInfo.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/ErrorResult.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/ScopeExit.h"
@@ -346,21 +344,17 @@ nsresult ServiceWorkerPrivateImpl::Initi
   mRemoteWorkerData.isSecureContext() = true;
   mRemoteWorkerData.referrerInfo() = MakeAndAddRef<ReferrerInfo>();
   mRemoteWorkerData.storageAccess() = storageAccess;
   mRemoteWorkerData.serviceWorkerData() = std::move(serviceWorkerData);
 
   mRemoteWorkerData.agentClusterId() = regInfo->AgentClusterId();
 
   // This fills in the rest of mRemoteWorkerData.serviceWorkerData().
-  rv = RefreshRemoteWorkerData(regInfo);
-
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
-  }
+  RefreshRemoteWorkerData(regInfo);
 
   return NS_OK;
 }
 
 RefPtr<GenericPromise> ServiceWorkerPrivateImpl::SetSkipWaitingFlag() {
   AssertIsOnMainThread();
   MOZ_ASSERT(mOuter);
   MOZ_ASSERT(mOuter->mInfo);
@@ -383,53 +377,27 @@ RefPtr<GenericPromise> ServiceWorkerPriv
   RefPtr<GenericPromise::Private> promise =
       new GenericPromise::Private(__func__);
 
   regInfo->TryToActivateAsync([promise] { promise->Resolve(true, __func__); });
 
   return promise;
 }
 
-nsresult ServiceWorkerPrivateImpl::RefreshRemoteWorkerData(
+void ServiceWorkerPrivateImpl::RefreshRemoteWorkerData(
     const RefPtr<ServiceWorkerRegistrationInfo>& aRegistration) {
   AssertIsOnMainThread();
   MOZ_ASSERT(mOuter);
   MOZ_ASSERT(mOuter->mInfo);
 
-  nsTArray<KeyAndPermissions> permissions;
-  nsCOMPtr<nsIPermissionManager> permManager = services::GetPermissionManager();
-  nsTArray<nsCString> keys =
-      nsPermissionManager::GetAllKeysForPrincipal(mOuter->mInfo->Principal());
-  MOZ_ASSERT(!keys.IsEmpty());
-
-  for (auto& key : keys) {
-    nsTArray<IPC::Permission> perms;
-    nsresult rv = permManager->GetPermissionsWithKey(key, perms);
-
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      return rv;
-    }
-
-    KeyAndPermissions kp;
-    kp.key() = nsCString(key);
-    kp.permissions() = std::move(perms);
-
-    permissions.AppendElement(std::move(kp));
-  }
-
-  MOZ_ASSERT(!permissions.IsEmpty());
-
   ServiceWorkerData& serviceWorkerData =
       mRemoteWorkerData.serviceWorkerData().get_ServiceWorkerData();
-  serviceWorkerData.permissionsByKey() = std::move(permissions);
   serviceWorkerData.descriptor() = mOuter->mInfo->Descriptor().ToIPC();
   serviceWorkerData.registrationDescriptor() =
       aRegistration->Descriptor().ToIPC();
-
-  return NS_OK;
 }
 
 nsresult ServiceWorkerPrivateImpl::SpawnWorkerIfNeeded() {
   AssertIsOnMainThread();
   MOZ_ASSERT(mOuter);
   MOZ_ASSERT(mOuter->mInfo);
 
   if (mControllerChild) {
@@ -450,21 +418,17 @@ nsresult ServiceWorkerPrivateImpl::Spawn
 
   RefPtr<ServiceWorkerRegistrationInfo> regInfo =
       swm->GetRegistration(mOuter->mInfo->Principal(), mOuter->mInfo->Scope());
 
   if (NS_WARN_IF(!regInfo)) {
     return NS_ERROR_DOM_INVALID_STATE_ERR;
   }
 
-  nsresult rv = RefreshRemoteWorkerData(regInfo);
-
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
-  }
+  RefreshRemoteWorkerData(regInfo);
 
   RefPtr<RemoteWorkerControllerChild> controllerChild =
       new RemoteWorkerControllerChild(this);
 
   if (NS_WARN_IF(!bgChild->SendPRemoteWorkerControllerConstructor(
           controllerChild, mRemoteWorkerData))) {
     return NS_ERROR_DOM_INVALID_STATE_ERR;
   }
--- a/dom/serviceworkers/ServiceWorkerPrivateImpl.h
+++ b/dom/serviceworkers/ServiceWorkerPrivateImpl.h
@@ -111,17 +111,17 @@ class ServiceWorkerPrivateImpl final : p
 
   void CreationSucceeded() override;
 
   void ErrorReceived(const ErrorValue& aError) override;
 
   void Terminated() override;
 
   // Refreshes only the parts of mRemoteWorkerData that may change over time.
-  nsresult RefreshRemoteWorkerData(
+  void RefreshRemoteWorkerData(
       const RefPtr<ServiceWorkerRegistrationInfo>& aRegistration);
 
   nsresult SendPushEventInternal(
       RefPtr<ServiceWorkerRegistrationInfo>&& aRegistration,
       ServiceWorkerPushEventOpArgs&& aArgs);
 
   nsresult SendFetchEventInternal(
       RefPtr<ServiceWorkerRegistrationInfo>&& aRegistration,
--- a/dom/workers/remoteworkers/RemoteWorkerChild.cpp
+++ b/dom/workers/remoteworkers/RemoteWorkerChild.cpp
@@ -411,24 +411,16 @@ nsresult RemoteWorkerChild::ExecWorkerOn
   nsString workerPrivateId = EmptyString();
 
   if (mIsServiceWorker) {
     ServiceWorkerData& data = aData.serviceWorkerData().get_ServiceWorkerData();
 
     MOZ_ASSERT(!data.id().IsEmpty());
     workerPrivateId = std::move(data.id());
 
-    nsCOMPtr<nsIPermissionManager> permissionManager =
-        services::GetPermissionManager();
-
-    for (auto& keyAndPermissions : data.permissionsByKey()) {
-      permissionManager->SetPermissionsWithKey(keyAndPermissions.key(),
-                                               keyAndPermissions.permissions());
-    }
-
     info.mServiceWorkerCacheName = data.cacheName();
     info.mServiceWorkerDescriptor.emplace(data.descriptor());
     info.mServiceWorkerRegistrationDescriptor.emplace(
         data.registrationDescriptor());
     info.mLoadFlags = static_cast<nsLoadFlags>(data.loadFlags());
   } else {
     // Top level workers' main script use the document charset for the script
     // uri encoding.
@@ -466,19 +458,37 @@ nsresult RemoteWorkerChild::ExecWorkerOn
         ThreadSafeWeakPtr<RemoteWorkerChild>(self));
   } else {
     workerPrivate->SetRemoteWorkerController(this);
   }
 
   RefPtr<InitializeWorkerRunnable> runnable =
       new InitializeWorkerRunnable(std::move(workerPrivate), SelfHolder(this));
 
-  if (NS_WARN_IF(!runnable->Dispatch())) {
-    rv = NS_ERROR_FAILURE;
-    return rv;
+  if (mIsServiceWorker) {
+    SelfHolder self = this;
+
+    nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
+        __func__, [initializeWorkerRunnable = std::move(runnable),
+                   self = std::move(self)] {
+          if (NS_WARN_IF(!initializeWorkerRunnable->Dispatch())) {
+            self->TransitionStateToTerminated();
+            self->CreationFailedOnAnyThread();
+          }
+        });
+
+    nsCOMPtr<nsIPermissionManager> permissionManager =
+        services::GetPermissionManager();
+    MOZ_ALWAYS_SUCCEEDS(
+        permissionManager->WhenPermissionsAvailable(principal, r));
+  } else {
+    if (NS_WARN_IF(!runnable->Dispatch())) {
+      rv = NS_ERROR_FAILURE;
+      return rv;
+    }
   }
 
   scopeExit.release();
 
   return NS_OK;
 }
 
 void RemoteWorkerChild::InitializeOnWorker(
--- a/dom/workers/remoteworkers/RemoteWorkerManager.cpp
+++ b/dom/workers/remoteworkers/RemoteWorkerManager.cpp
@@ -9,16 +9,17 @@
 #include <utility>
 
 #include "mozilla/RefPtr.h"
 #include "mozilla/ScopeExit.h"
 #include "mozilla/SystemGroup.h"
 #include "mozilla/dom/ContentParent.h"
 #include "mozilla/dom/RemoteWorkerParent.h"
 #include "mozilla/ipc/BackgroundParent.h"
+#include "mozilla/ipc/BackgroundUtils.h"
 #include "mozilla/ipc/PBackgroundParent.h"
 #include "nsCOMPtr.h"
 #include "nsIXULRuntime.h"
 #include "nsTArray.h"
 #include "nsThreadUtils.h"
 #include "RemoteWorkerServiceParent.h"
 
 namespace mozilla {
@@ -33,16 +34,26 @@ namespace {
 // actors.
 RemoteWorkerManager* sRemoteWorkerManager;
 
 bool IsServiceWorker(const RemoteWorkerData& aData) {
   return aData.serviceWorkerData().type() ==
          OptionalServiceWorkerData::TServiceWorkerData;
 }
 
+void TransmitPermissionsForPrincipalInfo(ContentParent* aContentParent,
+                                         const PrincipalInfo& aPrincipalInfo) {
+  AssertIsOnMainThread();
+  MOZ_ASSERT(aContentParent);
+
+  nsCOMPtr<nsIPrincipal> principal = PrincipalInfoToPrincipal(aPrincipalInfo);
+  MOZ_ALWAYS_SUCCEEDS(
+      aContentParent->TransmitPermissionsForPrincipal(principal));
+}
+
 }  // namespace
 
 /* static */
 already_AddRefed<RemoteWorkerManager> RemoteWorkerManager::GetOrCreate() {
   AssertIsInMainProcess();
   AssertIsOnBackgroundThread();
 
   if (!sRemoteWorkerManager) {
@@ -124,17 +135,17 @@ void RemoteWorkerManager::Launch(RemoteW
     if (mPendings.IsEmpty()) {
       AddRef();
     }
 
     Pending* pending = mPendings.AppendElement();
     pending->mController = aController;
     pending->mData = aData;
 
-    LaunchNewContentProcess();
+    LaunchNewContentProcess(aData);
     return;
   }
 
   /**
    * If a target actor for a Service Worker has been selected, the actor has
    * already been registered with the corresponding ContentParent (see
    * `SelectTargetActorForServiceWorker()`).
    */
@@ -192,20 +203,22 @@ void RemoteWorkerManager::AsyncCreationF
  * worker actors, and we ensure that the check for the number of registered
  * actors and the dispatching of the runnable are atomic. That happens on the
  * main thread, so here on the background thread,  while
  * `ContentParent::mRemoteWorkerActorData` is locked, if `mCount` > 0, we can
  * register a remote worker actor "early" and guarantee that the corresponding
  * content process will not shutdown.
  */
 RemoteWorkerServiceParent*
-RemoteWorkerManager::SelectTargetActorForServiceWorker() const {
+RemoteWorkerManager::SelectTargetActorForServiceWorker(
+    const RemoteWorkerData& aData) const {
   AssertIsInMainProcess();
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(!mChildActors.IsEmpty());
+  MOZ_ASSERT(IsServiceWorker(aData));
 
   nsTArray<RefPtr<ContentParent>> contentParents;
 
   auto scopeExit = MakeScopeExit([&] {
     nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
         __func__,
         [doomed = std::move(contentParents)]() mutable { doomed.Clear(); });
 
@@ -226,16 +239,29 @@ RemoteWorkerManager::SelectTargetActorFo
     auto scopeExit = MakeScopeExit(
         [&] { contentParents.AppendElement(std::move(contentParent)); });
 
     if (IsWebRemoteType(contentParent->GetRemoteType())) {
       auto lock = contentParent->mRemoteWorkerActorData.Lock();
 
       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(contentParent),
+                       principalInfo = aData.principalInfo()] {
+              TransmitPermissionsForPrincipalInfo(contentParent, principalInfo);
+            });
+
+        MOZ_ALWAYS_SUCCEEDS(
+            SystemGroup::Dispatch(TaskCategory::Other, r.forget()));
+
         return actor;
       }
     }
 
     i = (i + 1) % mChildActors.Length();
   } while (i != random);
 
   return nullptr;
@@ -261,41 +287,47 @@ RemoteWorkerServiceParent* RemoteWorkerM
   // We shouldn't have to worry about content-principal parent-process workers.
   MOZ_ASSERT(aProcessId != base::GetCurrentProcId());
 
   if (mChildActors.IsEmpty()) {
     return nullptr;
   }
 
   if (IsServiceWorker(aData)) {
-    return SelectTargetActorForServiceWorker();
+    return SelectTargetActorForServiceWorker(aData);
   }
 
   for (RemoteWorkerServiceParent* actor : mChildActors) {
     // Let's execute the RemoteWorker on the same process.
     if (actor->OtherPid() == aProcessId) {
       return actor;
     }
   }
 
   // Let's choose an actor, randomly.
   uint32_t id = uint32_t(rand()) % mChildActors.Length();
   return mChildActors[id];
 }
 
-void RemoteWorkerManager::LaunchNewContentProcess() {
+void RemoteWorkerManager::LaunchNewContentProcess(
+    const RemoteWorkerData& aData) {
   AssertIsInMainProcess();
   AssertIsOnBackgroundThread();
 
   // This runnable will spawn a new process if it doesn't exist yet.
-  nsCOMPtr<nsIRunnable> r =
-      NS_NewRunnableFunction("LaunchNewContentProcess", []() {
-        RefPtr<ContentParent> unused =
+  nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
+      __func__, [isServiceWorker = IsServiceWorker(aData),
+                 principalInfo = aData.principalInfo()] {
+        RefPtr<ContentParent> contentParent =
             ContentParent::GetNewOrUsedBrowserProcess(
                 nullptr, NS_LITERAL_STRING(DEFAULT_REMOTE_TYPE));
+
+        if (isServiceWorker) {
+          TransmitPermissionsForPrincipalInfo(contentParent, principalInfo);
+        }
       });
 
   nsCOMPtr<nsIEventTarget> target =
       SystemGroup::EventTargetFor(TaskCategory::Other);
   target->Dispatch(r.forget(), NS_DISPATCH_NORMAL);
 }
 
 }  // namespace dom
--- a/dom/workers/remoteworkers/RemoteWorkerManager.h
+++ b/dom/workers/remoteworkers/RemoteWorkerManager.h
@@ -34,24 +34,25 @@ class RemoteWorkerManager final {
 
  private:
   RemoteWorkerManager();
   ~RemoteWorkerManager();
 
   RemoteWorkerServiceParent* SelectTargetActor(const RemoteWorkerData& aData,
                                                base::ProcessId aProcessId);
 
-  RemoteWorkerServiceParent* SelectTargetActorForServiceWorker() const;
+  RemoteWorkerServiceParent* SelectTargetActorForServiceWorker(
+      const RemoteWorkerData& aData) const;
 
   void LaunchInternal(RemoteWorkerController* aController,
                       RemoteWorkerServiceParent* aTargetActor,
                       const RemoteWorkerData& aData,
                       bool aRemoteWorkerAlreadyRegistered = false);
 
-  void LaunchNewContentProcess();
+  void LaunchNewContentProcess(const RemoteWorkerData& aData);
 
   void AsyncCreationFailed(RemoteWorkerController* aController);
 
   // The list of existing RemoteWorkerServiceParent actors for child processes.
   // Raw pointers because RemoteWorkerServiceParent actors unregister themselves
   // when destroyed.
   // XXX For Fission, where we could have a lot of child actors, should we maybe
   // instead keep either a hash table (PID->actor) or perhaps store the actors
--- a/dom/workers/remoteworkers/RemoteWorkerTypes.ipdlh
+++ b/dom/workers/remoteworkers/RemoteWorkerTypes.ipdlh
@@ -5,30 +5,23 @@
 include ClientIPCTypes;
 include IPCServiceWorkerDescriptor;
 include IPCServiceWorkerRegistrationDescriptor;
 include PBackgroundSharedTypes;
 include URIParams;
 include DOMTypes;
 include ProtocolTypes;
 
-using struct IPC::Permission from "mozilla/net/NeckoMessageUtils.h";
 using struct mozilla::void_t from "ipc/IPCMessageUtils.h";
 using mozilla::StorageAccess from "mozilla/dom/ClientIPCUtils.h";
 
 namespace mozilla {
 namespace dom {
 
-struct KeyAndPermissions {
-  nsCString key;
-  Permission[] permissions;
-};
-
 struct ServiceWorkerData {
-  KeyAndPermissions[] permissionsByKey;
   IPCServiceWorkerDescriptor descriptor;
   IPCServiceWorkerRegistrationDescriptor registrationDescriptor;
   nsString cacheName;
   uint32_t loadFlags;
   nsString id;
 };
 
 union OptionalServiceWorkerData {