Bug 1469873 Make ClientSource::Control() fail if storage access is not available and make claim() respect the result. r=mrbkap

dom/clients/manager/ClientManagerService.cpp

@@ -475,6 +475,8 @@ ClaimOnMainThread(const ClientInfo& aClientInfo,
         }, [promise] (nsresult aRv) {
           promise->Reject(aRv, __func__);
         });
+
+      scopeExit.release();
     });
 
   MOZ_ALWAYS_SUCCEEDS(SystemGroup::Dispatch(TaskCategory::Other, r.forget()));

dom/clients/manager/ClientSource.cpp

@@ -124,6 +124,28 @@ ClientSource::GetDocShell() const
   return mOwner.as<nsCOMPtr<nsIDocShell>>();
 }
 
+nsIGlobalObject*
+ClientSource::GetGlobal() const
+{
+  NS_ASSERT_OWNINGTHREAD(ClientSource);
+  nsPIDOMWindowInner* win = GetInnerWindow();
+  if (win) {
+    return win->AsGlobal();
+  }
+
+  WorkerPrivate* wp = GetWorkerPrivate();
+  if (wp) {
+    return wp->GlobalScope();
+  }
+
+  // Note, ClientSource objects attached to docshell for conceptual
+  // initial about:blank will get nullptr here.  The caller should
+  // use MaybeCreateIntitialDocument() to create the window before
+  // GetGlobal() if it wants this before.
+
+  return nullptr;
+}
+
 void
 ClientSource::MaybeCreateInitialDocument()
 {
@@ -431,10 +453,47 @@ ClientSource::Control(const ClientControlledArgs& aArgs)
 {
   NS_ASSERT_OWNINGTHREAD(ClientSource);
 
+  // Determine if the client is allowed to be controlled.  Currently we
+  // prevent service workers from controlling clients that cannot access
+  // storage.  We exempt this restriction for local URL clients, like about:blank
+  // and blob:, since access to service workers is dictated by their parent.
+  //
+  // Note, we default to allowing the client to be controlled in the case
+  // where we are not execution ready yet.  This can only happen if the
+  // the non-subresource load is intercepted by a service worker.  Since
+  // ServiceWorkerInterceptController() uses StorageAllowedForChannel()
+  // it should be fine to accept these control messages.
+  //
+  // Its also fine to default to allowing ClientSource attached to a docshell
+  // to be controlled.  These clients represent inital about:blank windows
+  // that do not have an inner window created yet.  We explicitly allow initial
+  // about:blank.
+  bool controlAllowed = true;
+  if (GetInnerWindow()) {
+
+    // Local URL windows and windows with access to storage can be controlled.
+    controlAllowed = Info().URL().LowerCaseEqualsLiteral("about:blank") ||
+                     StringBeginsWith(Info().URL(), NS_LITERAL_CSTRING("blob:")) ||
+                     nsContentUtils::StorageAllowedForWindow(GetInnerWindow()) ==
+                      nsContentUtils::StorageAccess::eAllow;
+  } else if (GetWorkerPrivate()) {
+    // Local URL workers and workers with access to storage cna be controlled.
+    controlAllowed = GetWorkerPrivate()->IsStorageAllowed() ||
+                     StringBeginsWith(GetWorkerPrivate()->ScriptURL(),
+                                      NS_LITERAL_STRING("blob:"));
+  }
+
+  RefPtr<ClientOpPromise> ref;
+
+  if (NS_WARN_IF(!controlAllowed)) {
+    ref = ClientOpPromise::CreateAndReject(NS_ERROR_DOM_INVALID_STATE_ERR,
+                                           __func__);
+    return ref.forget();
+  }
+
   SetController(ServiceWorkerDescriptor(aArgs.serviceWorker()));
 
-  RefPtr<ClientOpPromise> ref =
-    ClientOpPromise::CreateAndResolve(NS_OK, __func__);
+  ref = ClientOpPromise::CreateAndResolve(NS_OK, __func__);
   return ref.forget();
 }
 
@@ -670,35 +729,55 @@ ClientSource::PostMessage(const ClientPostMessageArgs& aArgs)
 RefPtr<ClientOpPromise>
 ClientSource::Claim(const ClientClaimArgs& aArgs)
 {
+  // The ClientSource::Claim method is only needed in the legacy
+  // mode where the ServiceWorkerManager is run in each child-process.
+  // In parent-process mode this method should not be called.
+  MOZ_DIAGNOSTIC_ASSERT(!ServiceWorkerParentInterceptEnabled());
+
   RefPtr<ClientOpPromise> ref;
 
+  nsIGlobalObject* global = GetGlobal();
+  if (NS_WARN_IF(!global)) {
+    ref = ClientOpPromise::CreateAndReject(NS_ERROR_DOM_INVALID_STATE_ERR,
+                                           __func__);
+    return ref.forget();
+  }
+
+  // Note, we cannot just mark the ClientSource controlled.  We must go through
+  // the SWM so that it can keep track of which clients are controlled by each
+  // registration.  We must tell the child-process SWM in legacy child-process
+  // mode.  In parent-process service worker mode the SWM is notified in the
+  // parent-process in ClientManagerService::Claim().
+
+  RefPtr<GenericPromise::Private> innerPromise =
+    new GenericPromise::Private(__func__);
   ServiceWorkerDescriptor swd(aArgs.serviceWorker());
 
-  // Today the ServiceWorkerManager maintains its own list of
-  // nsIDocument objects controlled by each service worker.  We
-  // need to try to update that data structure for now.  If we
-  // can't, however, then simply mark the Client as controlled.
-  // In the future this will be enough for the SWM as well since
-  // it will eventually hold ClientHandle objects instead of
-  // nsIDocuments.
-  nsPIDOMWindowInner* innerWindow = GetInnerWindow();
-  nsIDocument* doc = innerWindow ? innerWindow->GetExtantDoc() : nullptr;
-  RefPtr<ServiceWorkerManager> swm = doc ? ServiceWorkerManager::GetInstance()
-                                         : nullptr;
-  if (!swm || !doc) {
-    SetController(swd);
-    ref = ClientOpPromise::CreateAndResolve(NS_OK, __func__);
-    return ref.forget();
+  nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
+    "ClientSource::Claim",
+    [innerPromise, clientInfo = mClientInfo, swd] () mutable {
+      RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
+      if (NS_WARN_IF(!swm)) {
+        innerPromise->Reject(NS_ERROR_DOM_INVALID_STATE_ERR, __func__);
+        return;
+      }
+
+      RefPtr<GenericPromise> p = swm->MaybeClaimClient(clientInfo, swd);
+      p->ChainTo(innerPromise.forget(), __func__);
+    });
+
+  if (NS_IsMainThread()) {
+    r->Run();
+  } else {
+    MOZ_ALWAYS_SUCCEEDS(SystemGroup::Dispatch(TaskCategory::Other, r.forget()));
   }
 
   RefPtr<ClientOpPromise::Private> outerPromise =
     new ClientOpPromise::Private(__func__);
 
-  auto holder =
-    MakeRefPtr<DOMMozPromiseRequestHolder<GenericPromise>>(innerWindow->AsGlobal());
+  auto holder = MakeRefPtr<DOMMozPromiseRequestHolder<GenericPromise>>(global);
 
-  RefPtr<GenericPromise> p = swm->MaybeClaimClient(mClientInfo, swd);
-  p->Then(mEventTarget, __func__,
+  innerPromise->Then(mEventTarget, __func__,
     [outerPromise, holder] (bool aResult) {
       holder->Complete();
       outerPromise->Resolve(NS_OK, __func__);

dom/clients/manager/ClientSource.h

@@ -17,6 +17,7 @@
 #endif
 
 class nsIDocShell;
+class nsIGlobalObject;
 class nsISerialEventTarget;
 class nsPIDOMWindowInner;
 
@@ -78,6 +79,9 @@ class ClientSource final : public ClientThing<ClientSourceChild>
   nsIDocShell*
   GetDocShell() const;
 
+  nsIGlobalObject*
+  GetGlobal() const;
+
   void
   MaybeCreateInitialDocument();
 

dom/clients/manager/ClientSourceOpParent.cpp

@@ -6,6 +6,8 @@
 
 #include "ClientSourceOpParent.h"
 
+#include "ClientSourceParent.h"
+
 namespace mozilla {
 namespace dom {
 
@@ -25,10 +27,22 @@ ClientSourceOpParent::Recv__delete__(const ClientOpResult& aResult)
 {
   if (aResult.type() == ClientOpResult::Tnsresult &&
       NS_FAILED(aResult.get_nsresult())) {
+
+    // If a control message fails then clear the controller from
+    // the ClientSourceParent.  We eagerly marked it controlled at
+    // the start of the operation.
+    if (mArgs.type() == ClientOpConstructorArgs::TClientControlledArgs) {
+      auto source = static_cast<ClientSourceParent*>(Manager());
+      if (source) {
+        source->ClearController();
+      }
+    }
+
     mPromise->Reject(aResult.get_nsresult(), __func__);
     mPromise = nullptr;
     return IPC_OK();
   }
+
   mPromise->Resolve(aResult, __func__);
   mPromise = nullptr;
   return IPC_OK();
@@ -36,7 +50,8 @@ ClientSourceOpParent::Recv__delete__(const ClientOpResult& aResult)
 
 ClientSourceOpParent::ClientSourceOpParent(const ClientOpConstructorArgs& aArgs,
                                            ClientOpPromise::Private* aPromise)
-  : mPromise(aPromise)
+  : mArgs(aArgs)
+  , mPromise(aPromise)
 {
   MOZ_DIAGNOSTIC_ASSERT(mPromise);
 }

dom/clients/manager/ClientSourceOpParent.h

@@ -14,6 +14,7 @@ namespace dom {
 
 class ClientSourceOpParent final : public PClientSourceOpParent
 {
+  const ClientOpConstructorArgs mArgs;
   RefPtr<ClientOpPromise::Private> mPromise;
 
   // PClientSourceOpParent interface

dom/clients/manager/ClientSourceParent.cpp

@@ -274,6 +274,12 @@ ClientSourceParent::GetController() const
   return mController;
 }
 
+void
+ClientSourceParent::ClearController()
+{
+  mController.reset();
+}
+
 void
 ClientSourceParent::AttachHandle(ClientHandleParent* aClientHandle)
 {
@@ -298,7 +304,11 @@ ClientSourceParent::StartOp(const ClientOpConstructorArgs& aArgs)
     new ClientOpPromise::Private(__func__);
 
   // If we are being controlled, remember that data before propagating
-  // on to the ClientSource.
+  // on to the ClientSource.  This must be set prior to triggering
+  // the controllerchange event from the ClientSource since some tests
+  // expect matchAll() to find the controlled client immediately after.
+  // If the control operation fails, then we reset the controller value
+  // to reflect the final state.
   if (aArgs.type() == ClientOpConstructorArgs::TClientControlledArgs) {
     mController.reset();
     mController.emplace(aArgs.get_ClientControlledArgs().serviceWorker());

dom/clients/manager/ClientSourceParent.h

@@ -79,6 +79,9 @@ public:
   const Maybe<ServiceWorkerDescriptor>&
   GetController() const;
 
+  void
+  ClearController();
+
   void
   AttachHandle(ClientHandleParent* aClientSource);
 

dom/serviceworkers/ServiceWorkerManager.cpp

@@ -317,6 +317,7 @@ ServiceWorkerManager::StartControllingClient(const ClientInfo& aClientInfo,
   MOZ_DIAGNOSTIC_ASSERT(aRegistrationInfo->GetActive());
 
   RefPtr<GenericPromise> ref;
+  RefPtr<ServiceWorkerManager> self(this);
 
   const ServiceWorkerDescriptor& active =
     aRegistrationInfo->GetActive()->Descriptor();
@@ -326,7 +327,12 @@ ServiceWorkerManager::StartControllingClient(const ClientInfo& aClientInfo,
     RefPtr<ServiceWorkerRegistrationInfo> old =
       entry.Data()->mRegistrationInfo.forget();
 
+    if (aControlClientHandle) {
       ref = entry.Data()->mClientHandle->Control(active);
+    } else {
+      ref = GenericPromise::CreateAndResolve(false, __func__);
+    }
+
     entry.Data()->mRegistrationInfo = aRegistrationInfo;
 
     if (old != aRegistrationInfo) {
@@ -336,6 +342,17 @@ ServiceWorkerManager::StartControllingClient(const ClientInfo& aClientInfo,
 
     Telemetry::Accumulate(Telemetry::SERVICE_WORKER_CONTROLLED_DOCUMENTS, 1);
 
+    // Always check to see if we failed to actually control the client.  In
+    // that case removed the client from our list of controlled clients.
+    ref->Then(
+      SystemGroup::EventTargetFor(TaskCategory::Other), __func__,
+      [] (bool) {
+        // do nothing on success
+      }, [self, aClientInfo] (nsresult aRv) {
+        // failed to control, forget about this client
+        self->StopControllingClient(aClientInfo);
+      });
+
     return ref;
   }
 
@@ -355,15 +372,25 @@ ServiceWorkerManager::StartControllingClient(const ClientInfo& aClientInfo,
     return new ControlledClientData(clientHandle, aRegistrationInfo);
   });
 
-  RefPtr<ServiceWorkerManager> self(this);
   clientHandle->OnDetach()->Then(
     SystemGroup::EventTargetFor(TaskCategory::Other), __func__,
-    [self = std::move(self), aClientInfo] {
+    [self, aClientInfo] {
       self->StopControllingClient(aClientInfo);
     });
 
   Telemetry::Accumulate(Telemetry::SERVICE_WORKER_CONTROLLED_DOCUMENTS, 1);
 
+  // Always check to see if we failed to actually control the client.  In
+  // that case removed the client from our list of controlled clients.
+  ref->Then(
+    SystemGroup::EventTargetFor(TaskCategory::Other), __func__,
+    [] (bool) {
+      // do nothing on success
+    }, [self, aClientInfo] (nsresult aRv) {
+      // failed to control, forget about this client
+      self->StopControllingClient(aClientInfo);
+    });
+
   return ref;
 }
 
@@ -2695,7 +2722,20 @@ ServiceWorkerManager::UpdateClientControllers(ServiceWorkerRegistrationInfo* aRe
   // Fire event after iterating mControlledClients is done to prevent
   // modification by reentering from the event handlers during iteration.
   for (auto& handle : handleList) {
-    handle->Control(activeWorker->Descriptor());
+    RefPtr<GenericPromise> p = handle->Control(activeWorker->Descriptor());
+
+    RefPtr<ServiceWorkerManager> self = this;
+
+    // If we fail to control the client, then automatically remove it
+    // from our list of controlled clients.
+    p->Then(
+      SystemGroup::EventTargetFor(TaskCategory::Other), __func__,
+      [] (bool) {
+        // do nothing on success
+      }, [self, clientInfo = handle->Info()] (nsresult aRv) {
+        // failed to control, forget about this client
+        self->StopControllingClient(clientInfo);
+      });
   }
 }