Bug 1425975 P7 Use the mControlledClients list to drive controller start and stop logic. r=asuth
☠☠ backed out by 2e0db1b48499 ☠ ☠
authorBen Kelly <ben@wanderview.com>
Fri, 22 Dec 2017 21:09:19 -0500
changeset 449128 6f520ab76d6a1fe6eca397c1ef72c7abcb7fe16c
parent 449127 f091f5e182c442289c53ca0b68955e0bdbab44b7
child 449129 4ce186c6d8f5eb88ef10dd8602bf11382839b497
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth
bugs1425975
milestone59.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 1425975 P7 Use the mControlledClients list to drive controller start and stop logic. r=asuth
docshell/base/nsDocShell.cpp
dom/interfaces/base/nsIServiceWorkerManager.idl
dom/workers/ServiceWorkerManager.cpp
dom/workers/ServiceWorkerManager.h
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -150,16 +150,17 @@
 #include "nsIStructuredCloneContainer.h"
 #include "nsISupportsPrimitives.h"
 #ifdef MOZ_PLACES
 #include "nsIFaviconService.h"
 #include "mozIPlacesPendingOperation.h"
 #include "mozIAsyncFavicons.h"
 #endif
 #include "nsINetworkPredictor.h"
+#include "nsIServiceWorkerManager.h"
 
 // Editor-related
 #include "nsIEditingSession.h"
 
 #include "nsPIDOMWindow.h"
 #include "nsGlobalWindow.h"
 #include "nsPIWindowRoot.h"
 #include "nsICachingChannel.h"
@@ -3459,23 +3460,37 @@ nsDocShell::MaybeCreateInitialClientSour
     return;
   }
 
   Maybe<ServiceWorkerDescriptor> controller(parentInner->GetController());
   if (controller.isNothing()) {
     return;
   }
 
+  nsCOMPtr<nsIServiceWorkerManager> swm = mozilla::services::GetServiceWorkerManager();
+  if (!swm) {
+    return;
+  }
+
   // If the parent is controlled then propagate that controller to the
   // initial about:blank client as well.  This will set the controller
   // in the ClientManagerService in the parent.
-  RefPtr<ClientHandle> handle =
-    ClientManager::CreateHandle(mInitialClientSource->Info(),
-                                parentInner->EventTargetFor(TaskCategory::Other));
-  handle->Control(controller.ref());
+  //
+  // Note: If the registration is missing from the SWM we avoid setting
+  //       the controller on the client.  We can do this synchronously
+  //       for now since SWM is in the child process.  In the future
+  //       when SWM is in the parent process we will probably have to
+  //       always set the initial client source and then somehow clear
+  //       it if we find the registration is acutally gone.  Its also
+  //       possible this race only occurs in cases where the resulting
+  //       window is no longer exposed.  For example, in theory the SW
+  //       should not go away if our parent window is controlled.
+  if (!swm->StartControlling(mInitialClientSource->Info(), controller.ref())) {
+    return;
+  }
 
   // Also mark the ClientSource as controlled directly in case script
   // immediately accesses navigator.serviceWorker.controller.
   mInitialClientSource->SetController(controller.ref());
 }
 
 Maybe<ClientInfo>
 nsDocShell::GetInitialClientInfo() const
--- a/dom/interfaces/base/nsIServiceWorkerManager.idl
+++ b/dom/interfaces/base/nsIServiceWorkerManager.idl
@@ -10,16 +10,27 @@ interface mozIDOMWindow;
 interface nsPIDOMWindowInner;
 interface mozIDOMWindowProxy;
 interface nsIArray;
 interface nsIDocument;
 interface nsIInterceptedChannel;
 interface nsIPrincipal;
 interface nsIRunnable;
 interface nsIURI;
+%{C++
+namespace mozilla {
+namespace dom {
+class ClientInfo;
+class ServiceWorkerDescriptor;
+} // namespace dom
+} // namespace mozilla
+%}
+
+[ref] native const_ClientInfoRef(const mozilla::dom::ClientInfo);
+[ref] native const_ServiceWorkerDescriptorRef(const mozilla::dom::ServiceWorkerDescriptor);
 
 [scriptable, uuid(52ee2c9d-ee87-4caf-9588-23ae77ff8798)]
 interface nsIServiceWorkerUnregisterCallback : nsISupports
 {
   // aState is true if the unregistration succeded.
   // It's false if this ServiceWorkerRegistration doesn't exist.
   void unregisterSucceeded(in bool aState);
   void unregisterFailed();
@@ -145,16 +156,19 @@ interface nsIServiceWorkerManager : nsIS
   /**
    * Call this to request that document `aDoc` be controlled by a ServiceWorker
    * if a registration exists for it's scope.
    *
    * This MUST only be called once per document!
    */
   [notxpcom,nostdcall] void MaybeStartControlling(in nsIDocument aDoc);
 
+  [notxpcom, nostdcall] bool StartControlling(in const_ClientInfoRef aClientInfo,
+                                              in const_ServiceWorkerDescriptorRef aServiceWorker);
+
   /**
    * Documents that have called MaybeStartControlling() should call this when
    * they are destroyed. This function may be called multiple times, and is
    * idempotent.
    */
   [notxpcom,nostdcall] void MaybeStopControlling(in nsIDocument aDoc);
 
   /*
--- a/dom/workers/ServiceWorkerManager.cpp
+++ b/dom/workers/ServiceWorkerManager.cpp
@@ -308,49 +308,83 @@ ServiceWorkerManager::Init(ServiceWorker
   if (!actor) {
     MaybeStartShutdown();
     return;
   }
 
   mActor = static_cast<ServiceWorkerManagerChild*>(actor);
 }
 
-void
+RefPtr<GenericPromise>
 ServiceWorkerManager::StartControllingClient(const ClientInfo& aClientInfo,
                                              ServiceWorkerRegistrationInfo* aRegistrationInfo)
 {
-  RefPtr<ClientHandle> clientHandle =
-    ClientManager::CreateHandle(aClientInfo,
-                                SystemGroup::EventTargetFor(TaskCategory::Other));
+  MOZ_DIAGNOSTIC_ASSERT(aRegistrationInfo->GetActive());
+
+  RefPtr<GenericPromise> ref;
+
+  const ServiceWorkerDescriptor& active =
+    aRegistrationInfo->GetActive()->Descriptor();
 
   auto entry = mControlledClients.LookupForAdd(aClientInfo.Id());
   if (entry) {
+    RefPtr<ServiceWorkerRegistrationInfo> old =
+      entry.Data()->mRegistrationInfo.forget();
+
+    ref = Move(entry.Data()->mClientHandle->Control(active));
     entry.Data()->mRegistrationInfo = aRegistrationInfo;
-  } else {
-    entry.OrInsert([&] {
-      return new ControlledClientData(clientHandle, aRegistrationInfo);
+
+    if (old != aRegistrationInfo) {
+      StopControllingRegistration(old);
+      aRegistrationInfo->StartControllingClient();
+    }
+
+    Telemetry::Accumulate(Telemetry::SERVICE_WORKER_CONTROLLED_DOCUMENTS, 1);
+
+    return Move(ref);
+  }
+
+  RefPtr<ClientHandle> clientHandle =
+    ClientManager::CreateHandle(aClientInfo,
+                                SystemGroup::EventTargetFor(TaskCategory::Other));
+
+  ref = Move(clientHandle->Control(active));
+
+  aRegistrationInfo->StartControllingClient();
+
+  entry.OrInsert([&] {
+    return new ControlledClientData(clientHandle, aRegistrationInfo);
+  });
+
+  RefPtr<ServiceWorkerManager> self(this);
+  clientHandle->OnDetach()->Then(
+    SystemGroup::EventTargetFor(TaskCategory::Other), __func__,
+    [self = Move(self), aClientInfo] {
+      self->StopControllingClient(aClientInfo);
     });
 
-    RefPtr<ServiceWorkerManager> self(this);
-    clientHandle->OnDetach()->Then(
-      SystemGroup::EventTargetFor(TaskCategory::Other), __func__,
-      [self = Move(self), aClientInfo] {
-        self->StopControllingClient(aClientInfo);
-      });
-  }
+  Telemetry::Accumulate(Telemetry::SERVICE_WORKER_CONTROLLED_DOCUMENTS, 1);
+
+  return Move(ref);
 }
 
 void
 ServiceWorkerManager::StopControllingClient(const ClientInfo& aClientInfo)
 {
   auto entry = mControlledClients.Lookup(aClientInfo.Id());
   if (!entry) {
     return;
   }
+
+  RefPtr<ServiceWorkerRegistrationInfo> reg =
+    entry.Data()->mRegistrationInfo.forget();
+
   entry.Remove();
+
+  StopControllingRegistration(reg);
 }
 
 void
 ServiceWorkerManager::MaybeStartShutdown()
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   if (mShuttingDown) {
@@ -2304,28 +2338,37 @@ ServiceWorkerManager::RemoveScopeAndRegi
     return;
   }
 
   if (auto entry = data->mUpdateTimers.Lookup(aRegistration->mScope)) {
     entry.Data()->Cancel();
     entry.Remove();
   }
 
-  // Verify there are no controlled documents for the purged registration.
-#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
-  for (auto iter = swm->mControlledDocuments.Iter(); !iter.Done(); iter.Next()) {
-    ServiceWorkerRegistrationInfo* reg = iter.UserData();
+  // Verify there are no controlled clients for the purged registration.
+  for (auto iter = swm->mControlledClients.Iter(); !iter.Done(); iter.Next()) {
+    auto& reg = iter.UserData()->mRegistrationInfo;
     if (reg->mScope.Equals(aRegistration->mScope)) {
       MOZ_DIAGNOSTIC_ASSERT(false,
-                            "controlled document when removing registration");
+                            "controlled client when removing registration");
       iter.Remove();
       break;
     }
   }
-#endif
+
+  // Registration lifecycle is managed via mControlledClients now.  Do not
+  // assert on on mControlledDocuments as races may cause this to still be
+  // set when the registration is destroyed.
+  for (auto iter = swm->mControlledDocuments.Iter(); !iter.Done(); iter.Next()) {
+    ServiceWorkerRegistrationInfo* reg = iter.UserData();
+    if (reg->mScope.Equals(aRegistration->mScope)) {
+      iter.Remove();
+      break;
+    }
+  }
 
   RefPtr<ServiceWorkerRegistrationInfo> info;
   data->mInfos.Remove(aRegistration->mScope, getter_AddRefs(info));
   data->mOrderedScopes.RemoveElement(aRegistration->mScope);
   swm->NotifyListenersOnUnregister(info);
 
   swm->MaybeRemoveRegistrationInfo(scopeKey);
   swm->NotifyServiceWorkerRegistrationRemoved(aRegistration);
@@ -2344,35 +2387,53 @@ ServiceWorkerManager::MaybeRemoveRegistr
 
 void
 ServiceWorkerManager::MaybeStartControlling(nsIDocument* aDoc)
 {
   AssertIsOnMainThread();
   MOZ_ASSERT(aDoc);
   RefPtr<ServiceWorkerRegistrationInfo> registration =
     GetServiceWorkerRegistrationInfo(aDoc);
-  if (registration) {
+  if (registration && registration->GetActive() &&
+      aDoc->GetSandboxFlags() == 0) {
     MOZ_ASSERT(!mControlledDocuments.Contains(aDoc));
     StartControllingADocument(registration, aDoc);
   }
 }
 
+bool
+ServiceWorkerManager::StartControlling(const ClientInfo& aClientInfo,
+                                       const ServiceWorkerDescriptor& aServiceWorker)
+{
+  AssertIsOnMainThread();
+
+  nsCOMPtr<nsIPrincipal> principal =
+    PrincipalInfoToPrincipal(aServiceWorker.PrincipalInfo());
+  NS_ENSURE_TRUE(principal, false);
+
+  nsCOMPtr<nsIURI> scope;
+  nsresult rv =
+    NS_NewURI(getter_AddRefs(scope), aServiceWorker.Scope(), nullptr, nullptr);
+  NS_ENSURE_SUCCESS(rv, false);
+
+  RefPtr<ServiceWorkerRegistrationInfo> registration =
+    GetServiceWorkerRegistrationInfo(principal, scope);
+  NS_ENSURE_TRUE(registration, false);
+
+  StartControllingClient(aClientInfo, registration);
+
+  return true;
+}
+
 void
 ServiceWorkerManager::MaybeStopControlling(nsIDocument* aDoc)
 {
   AssertIsOnMainThread();
   MOZ_ASSERT(aDoc);
-  RefPtr<ServiceWorkerRegistrationInfo> registration;
-  mControlledDocuments.Remove(aDoc, getter_AddRefs(registration));
-  // A document which was uncontrolled does not maintain that state itself, so
-  // it will always call MaybeStopControlling() even if there isn't an
-  // associated registration. So this check is required.
-  if (registration) {
-    StopControllingRegistration(registration);
-  }
+  mControlledDocuments.Remove(aDoc);
 }
 
 void
 ServiceWorkerManager::MaybeCheckNavigationUpdate(nsIDocument* aDoc)
 {
   AssertIsOnMainThread();
   MOZ_ASSERT(aDoc);
   // We perform these success path navigation update steps when the
@@ -2386,60 +2447,24 @@ ServiceWorkerManager::MaybeCheckNavigati
   //    algorithm.
   RefPtr<ServiceWorkerRegistrationInfo> registration;
   mControlledDocuments.Get(aDoc, getter_AddRefs(registration));
   if (registration) {
     registration->MaybeScheduleUpdate();
   }
 }
 
-RefPtr<GenericPromise>
+void
 ServiceWorkerManager::StartControllingADocument(ServiceWorkerRegistrationInfo* aRegistration,
                                                 nsIDocument* aDoc)
 {
   MOZ_ASSERT(aRegistration);
   MOZ_ASSERT(aDoc);
 
-#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
-  auto storageAllowed = nsContentUtils::StorageAllowedForDocument(aDoc);
-  MOZ_DIAGNOSTIC_ASSERT(storageAllowed == nsContentUtils::StorageAccess::eAllow);
-#endif // MOZ_DIAGNOSTIC_ASSERT_ENABLED
-
-  RefPtr<GenericPromise> ref;
-
-  ServiceWorkerInfo* activeWorker = aRegistration->GetActive();
-  if (NS_WARN_IF(!activeWorker)) {
-    ref = GenericPromise::CreateAndReject(NS_ERROR_DOM_INVALID_STATE_ERR,
-                                          __func__);
-    return ref.forget();
-  }
-
-  Maybe<ClientInfo> clientInfo = aDoc->GetClientInfo();
-  if (NS_WARN_IF(clientInfo.isNothing())) {
-    ref = GenericPromise::CreateAndReject(NS_ERROR_DOM_INVALID_STATE_ERR,
-                                          __func__);
-    return ref.forget();
-  }
-
-  aRegistration->StartControllingClient();
   mControlledDocuments.Put(aDoc, aRegistration);
-
-  StartControllingClient(clientInfo.ref(), aRegistration);
-
-  // Mark the document's ClientSource as controlled using the ClientHandle
-  // interface.  While we could get at the ClientSource directly from the
-  // document here, our goal is to move ServiceWorkerManager to a separate
-  // process.  Using the ClientHandle supports this remote operation.
-  RefPtr<ClientHandle> clientHandle =
-    ClientManager::CreateHandle(clientInfo.ref(),
-                                SystemGroup::EventTargetFor(TaskCategory::Other));
-  ref = Move(clientHandle->Control(activeWorker->Descriptor()));
-
-  Telemetry::Accumulate(Telemetry::SERVICE_WORKER_CONTROLLED_DOCUMENTS, 1);
-  return Move(ref);
 }
 
 void
 ServiceWorkerManager::StopControllingRegistration(ServiceWorkerRegistrationInfo* aRegistration)
 {
   aRegistration->StopControllingClient();
   if (aRegistration->IsControllingClients() || !aRegistration->IsIdle()) {
     return;
@@ -2766,20 +2791,17 @@ ServiceWorkerManager::DispatchFetchEvent
         //       initial about:blank global.  See bug 1419620 and the spec
         //       issue here: https://github.com/w3c/ServiceWorker/issues/1232
       }
 
       if (clientInfo.isSome()) {
         // First, attempt to mark the reserved client controlled directly.  This
         // will update the controlled status in the ClientManagerService in the
         // parent.  It will also eventually propagate back to the ClientSource.
-        RefPtr<ClientHandle> clientHandle =
-          ClientManager::CreateHandle(clientInfo.ref(),
-                                      SystemGroup::EventTargetFor(TaskCategory::Other));
-        clientHandle->Control(serviceWorker->Descriptor());
+        StartControllingClient(clientInfo.ref(), registration);
       }
 
       // But we also note the reserved state on the LoadInfo.  This allows the
       // ClientSource to be updated immediately after the nsIChannel starts.
       // This is necessary to have the correct controller in place for immediate
       // follow-on requests.
       loadInfo->SetController(serviceWorker->Descriptor());
     }
@@ -3240,21 +3262,18 @@ ServiceWorkerManager::MaybeClaimClient(n
                         getter_AddRefs(controllingRegistration));
 
   if (aWorkerRegistration != matchingRegistration ||
       aWorkerRegistration == controllingRegistration) {
     ref = GenericPromise::CreateAndResolve(true, __func__);
     return ref.forget();
   }
 
-  if (controllingRegistration) {
-    StopControllingRegistration(controllingRegistration);
-  }
-
-  ref = StartControllingADocument(aWorkerRegistration, aDocument);
+  StartControllingADocument(aWorkerRegistration, aDocument);
+  ref = StartControllingClient(clientInfo.ref(), aWorkerRegistration);
   return ref.forget();
 }
 
 already_AddRefed<GenericPromise>
 ServiceWorkerManager::MaybeClaimClient(nsIDocument* aDoc,
                                        const ServiceWorkerDescriptor& aServiceWorker)
 {
   RefPtr<GenericPromise> ref;
--- a/dom/workers/ServiceWorkerManager.h
+++ b/dom/workers/ServiceWorkerManager.h
@@ -339,17 +339,17 @@ public:
 
 private:
   ServiceWorkerManager();
   ~ServiceWorkerManager();
 
   void
   Init(ServiceWorkerRegistrar* aRegistrar);
 
-  void
+  RefPtr<GenericPromise>
   StartControllingClient(const ClientInfo& aClientInfo,
                          ServiceWorkerRegistrationInfo* aRegistrationInfo);
 
   void
   StopControllingClient(const ClientInfo& aClientInfo);
 
   void
   MaybeStartShutdown();
@@ -393,17 +393,17 @@ private:
                                             WhichServiceWorker aWhichOne);
   void
   InvalidateServiceWorkerRegistrationWorker(ServiceWorkerRegistrationInfo* aRegistration,
                                             WhichServiceWorker aWhichOnes);
 
   void
   NotifyServiceWorkerRegistrationRemoved(ServiceWorkerRegistrationInfo* aRegistration);
 
-  RefPtr<GenericPromise>
+  void
   StartControllingADocument(ServiceWorkerRegistrationInfo* aRegistration,
                             nsIDocument* aDoc);
 
   void
   StopControllingRegistration(ServiceWorkerRegistrationInfo* aRegistration);
 
   already_AddRefed<ServiceWorkerRegistrationInfo>
   GetServiceWorkerRegistrationInfo(nsPIDOMWindowInner* aWindow);