Bug 1735875, ServiceWorkers + bfcache: evict bfcache in certain cases, to pass the existing WPTs, and add a new test for Client.postMessage, r=asuth

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

dom/clients/manager/ClientHandle.cpp

@@ -185,4 +185,11 @@ RefPtr<GenericPromise> ClientHandle::OnDetach() {
   return mDetachPromise;
 }
 
+void ClientHandle::EvictFromBFCache() {
+  ClientEvictBFCacheArgs args;
+  StartOp(
+      std::move(args), [](const ClientOpResult& aResult) {},
+      [](const ClientOpResult& aResult) {});
+}
+
 }  // namespace mozilla::dom

dom/clients/manager/ClientHandle.h

@@ -97,6 +97,13 @@ class ClientHandle final : public ClientThing<ClientHandleChild> {
   //       but the MozPromise lets you Then() to another thread.
   RefPtr<GenericPromise> OnDetach();
 
+  // This is intended to allow the ServiceWorkerManager to evict controlled
+  // clients when their controlling registration changes. This should not be
+  // used by other holders of ClientHandles. This method can probably be removed
+  // when ServiceWorkerManager and ClientManagerService both live on the same
+  // thread.
+  void EvictFromBFCache();
+
   NS_INLINE_DECL_REFCOUNTING(ClientHandle);
 };
 

dom/clients/manager/ClientIPCTypes.ipdlh

@@ -125,6 +125,9 @@ struct ClientOpenWindowArgs
   nsCString baseURL;
 };
 
+struct ClientEvictBFCacheArgs
+{};
+
 union ClientOpConstructorArgs
 {
   ClientControlledArgs;
@@ -135,6 +138,7 @@ union ClientOpConstructorArgs
   ClientClaimArgs;
   ClientGetInfoAndStateArgs;
   ClientOpenWindowArgs;
+  ClientEvictBFCacheArgs;
 };
 
 struct ClientList

dom/clients/manager/ClientManagerService.cpp

@@ -204,9 +204,8 @@ ClientSourceParent* ClientManagerService::FindExistingSource(
 
   ClientSourceParent* source = MaybeUnwrapAsExistingSource(entry.Data());
 
-  if (!source || source->IsFrozen() ||
-      NS_WARN_IF(!ClientMatchPrincipalInfo(source->Info().PrincipalInfo(),
-                                           aPrincipalInfo))) {
+  if (!source || NS_WARN_IF(!ClientMatchPrincipalInfo(
+                     source->Info().PrincipalInfo(), aPrincipalInfo))) {
     return nullptr;
   }
   return source;
@@ -377,8 +376,7 @@ RefPtr<SourcePromise> ClientManagerService::FindSource(
   }
 
   ClientSourceParent* source = entry.Data().as<ClientSourceParent*>();
-  if (source->IsFrozen() ||
-      NS_WARN_IF(!ClientMatchPrincipalInfo(source->Info().PrincipalInfo(),
+  if (NS_WARN_IF(!ClientMatchPrincipalInfo(source->Info().PrincipalInfo(),
                                            aPrincipalInfo))) {
     CopyableErrorResult rv;
     rv.ThrowInvalidStateError("Unknown client.");
@@ -634,7 +632,7 @@ RefPtr<ClientOpPromise> ClientManagerService::Claim(
   for (const auto& entry : mSourceTable) {
     ClientSourceParent* source = MaybeUnwrapAsExistingSource(entry.GetData());
 
-    if (!source || source->IsFrozen()) {
+    if (!source) {
       continue;
     }
 
@@ -660,6 +658,11 @@ RefPtr<ClientOpPromise> ClientManagerService::Claim(
       continue;
     }
 
+    if (source->IsFrozen()) {
+      Unused << source->SendEvictFromBFCache();
+      continue;
+    }
+
     promiseList->AddPromise(ClaimOnMainThread(
         source->Info(), ServiceWorkerDescriptor(serviceWorker)));
   }

dom/clients/manager/ClientSource.cpp

@@ -357,6 +357,19 @@ void ClientSource::Thaw() {
   MaybeExecute([](PClientSourceChild* aActor) { aActor->SendThaw(); });
 }
 
+void ClientSource::EvictFromBFCache() {
+  if (nsCOMPtr<nsPIDOMWindowInner> win = GetInnerWindow()) {
+    win->RemoveFromBFCacheSync();
+  } else if (WorkerPrivate* vp = GetWorkerPrivate()) {
+    vp->EvictFromBFCache();
+  }
+}
+
+RefPtr<ClientOpPromise> ClientSource::EvictFromBFCacheOp() {
+  EvictFromBFCache();
+  return ClientOpPromise::CreateAndResolve(CopyableErrorResult(), __func__);
+}
+
 const ClientInfo& ClientSource::Info() const { return mClientInfo; }
 
 void ClientSource::WorkerSyncPing(WorkerPrivate* aWorkerPrivate) {
@@ -567,6 +580,10 @@ RefPtr<ClientOpPromise> ClientSource::PostMessage(
   if (nsPIDOMWindowInner* const window = GetInnerWindow()) {
     const RefPtr<ServiceWorkerContainer> container =
         window->Navigator()->ServiceWorker();
+
+    // Note, EvictFromBFCache() may delete the ClientSource object
+    // when bfcache lives in the child process.
+    EvictFromBFCache();
     container->ReceiveMessage(aArgs);
     return ClientOpPromise::CreateAndResolve(CopyableErrorResult(), __func__);
   }

dom/clients/manager/ClientSource.h

@@ -104,6 +104,10 @@ class ClientSource final : public ClientThing<ClientSourceChild> {
 
   void Thaw();
 
+  void EvictFromBFCache();
+
+  RefPtr<ClientOpPromise> EvictFromBFCacheOp();
+
   const ClientInfo& Info() const;
 
   // Trigger a synchronous IPC ping to the parent process to confirm that

dom/clients/manager/ClientSourceChild.cpp

@@ -43,6 +43,14 @@ IPCResult ClientSourceChild::RecvPClientSourceOpConstructor(
   return IPC_OK();
 }
 
+mozilla::ipc::IPCResult ClientSourceChild::RecvEvictFromBFCache() {
+  if (mSource) {
+    mSource->EvictFromBFCache();
+  }
+
+  return IPC_OK();
+}
+
 ClientSourceChild::ClientSourceChild(const ClientSourceConstructorArgs& aArgs)
     : mSource(nullptr), mTeardownStarted(false) {}
 

dom/clients/manager/ClientSourceChild.h

@@ -31,6 +31,8 @@ class ClientSourceChild final : public PClientSourceChild {
       PClientSourceOpChild* aActor,
       const ClientOpConstructorArgs& aArgs) override;
 
+  mozilla::ipc::IPCResult RecvEvictFromBFCache() override;
+
  public:
   explicit ClientSourceChild(const ClientSourceConstructorArgs& aArgs);
 

dom/clients/manager/ClientSourceOpChild.cpp

@@ -18,8 +18,8 @@ ClientSource* ClientSourceOpChild::GetSource() const {
   return actor->GetSource();
 }
 
-template <typename Method, typename Args>
-void ClientSourceOpChild::DoSourceOp(Method aMethod, const Args& aArgs) {
+template <typename Method, typename... Args>
+void ClientSourceOpChild::DoSourceOp(Method aMethod, Args&&... aArgs) {
   RefPtr<ClientOpPromise> promise;
   nsCOMPtr<nsISerialEventTarget> target;
 
@@ -40,7 +40,7 @@ void ClientSourceOpChild::DoSourceOp(Method aMethod, const Args& aArgs) {
 
     // This may cause the ClientSource object to be destroyed.  Do not
     // use the source variable after this call.
-    promise = (source->*aMethod)(aArgs);
+    promise = (source->*aMethod)(std::forward<Args>(aArgs)...);
   }
 
   // The ClientSource methods are required to always return a promise.  If
@@ -97,6 +97,10 @@ void ClientSourceOpChild::Init(const ClientOpConstructorArgs& aArgs) {
                  aArgs.get_ClientGetInfoAndStateArgs());
       break;
     }
+    case ClientOpConstructorArgs::TClientEvictBFCacheArgs: {
+      DoSourceOp(&ClientSource::EvictFromBFCacheOp);
+      break;
+    }
     default: {
       MOZ_ASSERT_UNREACHABLE("unknown client operation!");
       break;

dom/clients/manager/ClientSourceOpChild.h

@@ -29,8 +29,8 @@ class ClientSourceOpChild final : public PClientSourceOpChild {
 
   ClientSource* GetSource() const;
 
-  template <typename Method, typename Args>
-  void DoSourceOp(Method aMethod, const Args& aArgs);
+  template <typename Method, typename... Args>
+  void DoSourceOp(Method aMethod, Args&&... aArgs);
 
   // PClientSourceOpChild interface
   void ActorDestroy(ActorDestroyReason aReason) override;

dom/clients/manager/ClientSourceParent.cpp

@@ -119,12 +119,6 @@ IPCResult ClientSourceParent::RecvFreeze() {
   MOZ_DIAGNOSTIC_ASSERT(!mFrozen);
   mFrozen = true;
 
-  // Frozen clients should not be observable.  Act as if the client has
-  // been destroyed.
-  for (ClientHandleParent* handle : mHandleList.Clone()) {
-    Unused << ClientHandleParent::Send__delete__(handle);
-  }
-
   return IPC_OK();
 }
 
@@ -255,7 +249,6 @@ void ClientSourceParent::ClearController() { mController.reset(); }
 
 void ClientSourceParent::AttachHandle(ClientHandleParent* aClientHandle) {
   MOZ_DIAGNOSTIC_ASSERT(aClientHandle);
-  MOZ_DIAGNOSTIC_ASSERT(!mFrozen);
   MOZ_ASSERT(!mHandleList.Contains(aClientHandle));
   mHandleList.AppendElement(aClientHandle);
 }

dom/clients/manager/PClientSource.ipdl

@@ -28,6 +28,8 @@ parent:
 child:
   async PClientSourceOp(ClientOpConstructorArgs aArgs);
 
+  async EvictFromBFCache();
+
   async __delete__();
 };
 

dom/serviceworkers/ServiceWorkerManager.cpp

@@ -2663,6 +2663,16 @@ void ServiceWorkerManager::UpdateClientControllers(
   }
 }
 
+void ServiceWorkerManager::EvictFromBFCache(
+    ServiceWorkerRegistrationInfo* aRegistration) {
+  MOZ_ASSERT(NS_IsMainThread());
+  for (const auto& client : mControlledClients.Values()) {
+    if (client->mRegistrationInfo == aRegistration) {
+      client->mClientHandle->EvictFromBFCache();
+    }
+  }
+}
+
 already_AddRefed<ServiceWorkerRegistrationInfo>
 ServiceWorkerManager::GetRegistration(nsIPrincipal* aPrincipal,
                                       const nsACString& aScope) const {

dom/serviceworkers/ServiceWorkerManager.h

@@ -277,6 +277,8 @@ class ServiceWorkerManager final : public nsIServiceWorkerManager,
   // RecordTelemetryGap() and Accumulate them.
   void RecordTelemetry(uint32_t aNumber, uint32_t aFetch);
 
+  void EvictFromBFCache(ServiceWorkerRegistrationInfo* aRegistration);
+
  private:
   struct RegistrationDataPerPrincipal;
 

dom/serviceworkers/ServiceWorkerUnregisterJob.cpp

@@ -114,6 +114,8 @@ void ServiceWorkerUnregisterJob::Unregister() {
     swm->MaybeSendUnregister(mPrincipal, mScope);
   }
 
+  swm->EvictFromBFCache(registration);
+
   // "Remove scope to registration map[job's scope url]."
   swm->RemoveRegistration(registration);
   MOZ_ASSERT(registration->IsUnregistered());

dom/workers/WorkerPrivate.cpp

@@ -1359,6 +1359,29 @@ nsPIDOMWindowInner* WorkerPrivate::GetAncestorWindow() const {
   return nullptr;
 }
 
+class EvictFromBFCacheRunnable final : public WorkerProxyToMainThreadRunnable {
+ public:
+  void RunOnMainThread(WorkerPrivate* aWorkerPrivate) override {
+    MOZ_ASSERT(aWorkerPrivate);
+    AssertIsOnMainThread();
+    if (nsCOMPtr<nsPIDOMWindowInner> win =
+            aWorkerPrivate->GetAncestorWindow()) {
+      win->RemoveFromBFCacheSync();
+    }
+  }
+
+  void RunBackOnWorkerThreadForCleanup(WorkerPrivate* aWorkerPrivate) override {
+    MOZ_ASSERT(aWorkerPrivate);
+    aWorkerPrivate->AssertIsOnWorkerThread();
+  }
+};
+
+void WorkerPrivate::EvictFromBFCache() {
+  AssertIsOnWorkerThread();
+  RefPtr<EvictFromBFCacheRunnable> runnable = new EvictFromBFCacheRunnable();
+  runnable->Dispatch(this);
+}
+
 void WorkerPrivate::SetCSP(nsIContentSecurityPolicy* aCSP) {
   AssertIsOnMainThread();
   if (!aCSP) {

dom/workers/WorkerPrivate.h

@@ -791,6 +791,8 @@ class WorkerPrivate final
 
   nsPIDOMWindowInner* GetAncestorWindow() const;
 
+  void EvictFromBFCache();
+
   nsIContentSecurityPolicy* GetCSP() const {
     AssertIsOnMainThread();
     return mLoadInfo.mCSP;

testing/web-platform/meta/html/browsers/browsing-the-web/back-forward-cache/service-worker-clients-claim.https.html.ini

@@ -1,3 +0,0 @@
-[service-worker-clients-claim.https.html]
-  [Clients.claim() evicts pages that would be affected from BFCache]
-    expected: FAIL

testing/web-platform/meta/html/browsers/browsing-the-web/back-forward-cache/service-worker-controlled-after-restore.https.html.ini

@@ -1,3 +0,0 @@
-[service-worker-controlled-after-restore.https.html]
-  [Pages should remain controlled after restored from BFCache]
-    expected: FAIL

testing/web-platform/meta/html/browsers/browsing-the-web/back-forward-cache/service-worker-unregister.https.html.ini

@@ -1,3 +0,0 @@
-[service-worker-unregister.https.html]
-  [Unregister service worker while a controlled page is in BFCache]
-    expected: FAIL

testing/web-platform/tests/html/browsers/browsing-the-web/back-forward-cache/resources/helper.sub.js

@@ -197,6 +197,33 @@ async function claim(t, worker) {
       resolve();
     });
   });
-  worker.postMessage({port: channel.port2}, [channel.port2]);
+  worker.postMessage({type: "claim", port: channel.port2}, [channel.port2]);
+  await saw_message;
+}
+
+// Assigns the current client to a local variable on the service worker.
+async function storeClients(t, worker) {
+  const channel = new MessageChannel();
+  const saw_message = new Promise(function(resolve) {
+    channel.port1.onmessage = t.step_func(function(e) {
+      assert_equals(e.data, 'PASS', 'storeClients');
+      resolve();
+    });
+  });
+  worker.postMessage({type: "storeClients", port: channel.port2}, [channel.port2]);
+  await saw_message;
+}
+
+// Call storedClients.postMessage("") on the service worker
+async function postMessageToStoredClients(t, worker) {
+  const channel = new MessageChannel();
+  const saw_message = new Promise(function(resolve) {
+    channel.port1.onmessage = t.step_func(function(e) {
+      assert_equals(e.data, 'PASS', 'postMessageToStoredClients');
+      resolve();
+    });
+  });
+  worker.postMessage({type: "postMessageToStoredClients",
+                      port: channel.port2}, [channel.port2]);
   await saw_message;
 }

testing/web-platform/tests/html/browsers/browsing-the-web/back-forward-cache/resources/service-worker.js

@@ -1,16 +1,29 @@
 self.addEventListener('message', function(event) {
-    self.clients.claim()
-      .then(function(result) {
-          if (result !== undefined) {
-              event.data.port.postMessage(
-                  'FAIL: claim() should be resolved with undefined');
-              return;
-          }
-          event.data.port.postMessage('PASS');
-        })
-      .catch(function(error) {
-          event.data.port.postMessage('FAIL: exception: ' + error.name);
+    if (event.data.type == "claim") {
+      self.clients.claim()
+        .then(function(result) {
+            if (result !== undefined) {
+                event.data.port.postMessage(
+                    'FAIL: claim() should be resolved with undefined');
+                return;
+            }
+            event.data.port.postMessage('PASS');
+          })
+        .catch(function(error) {
+            event.data.port.postMessage('FAIL: exception: ' + error.name);
+          });
+    } else if (event.data.type == "storeClients") {
+      self.clients.matchAll()
+        .then(function(result) {
+          self.storedClients = result;
+          event.data.port.postMessage("PASS");
         });
+    } else if (event.data.type == "postMessageToStoredClients") {
+      for (let client of self.storedClients) {
+        client.postMessage("dummyValue");
+      }
+      event.data.port.postMessage("PASS");
+    }
   });
 
 self.addEventListener('fetch', e => {

testing/web-platform/tests/html/browsers/browsing-the-web/back-forward-cache/service-worker-client-postmessage.https.html

@@ -0,0 +1,71 @@
+<!doctype html>
+<meta name="timeout" content="long">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/common/utils.js"></script>
+<script src="/common/dispatcher/dispatcher.js"></script>
+<script src="resources/helper.sub.js"></script>
+<script src="/service-workers/service-worker/resources/test-helpers.sub.js"></script>
+<script>
+// When a service worker is unregistered when a controlled page is in BFCache,
+// the page can be still restored from BFCache and remain controlled by the
+// service worker.
+promise_test(async t => {
+  // Register a service worker and make this page controlled.
+  const workerUrl =
+      'resources/service-worker.js?pipe=header(Service-Worker-Allowed,../)';
+  const registration =
+      await service_worker_unregister_and_register(t, workerUrl, './');
+  t.add_cleanup(_ => registration.unregister());
+  await wait_for_state(t, registration.installing, 'activated');
+  const controllerChanged = new Promise(
+      resolve => navigator.serviceWorker.oncontrollerchange = resolve);
+  await claim(t, registration.active);
+  await controllerChanged;
+
+  const pageA = new RemoteContext(token());
+  const pageB = new RemoteContext(token());
+
+  const urlA = location.origin + executorPath + pageA.context_id;
+  const urlB = originCrossSite + executorPath + pageB.context_id;
+
+  // Open `urlA`.
+  window.open(urlA, '_blank', 'noopener');
+  await pageA.execute_script(waitForPageShow);
+
+  assert_true(
+    await pageA.execute_script(
+        () => (navigator.serviceWorker.controller !== null)),
+    'pageA should be controlled before navigation');
+
+  await storeClients(t, registration.active);
+
+  // Navigate to `urlB`.
+  await pageA.execute_script(
+    (url) => prepareNavigation(() => {
+      location.href = url;
+    }),
+    [urlB]);
+  await pageB.execute_script(waitForPageShow);
+
+  // Posting a message to a client should evict it from the bfcache.
+  await postMessageToStoredClients(t, registration.active);
+
+  // Back navigate and check whether the page is restored from BFCache.
+  await pageB.execute_script(
+    () => {
+      prepareNavigation(() => { history.back(); });
+    }
+  );
+  await pageA.execute_script(waitForPageShow);
+  await assert_not_bfcached(pageA);
+
+  await pageA.execute_script(() => navigator.serviceWorker.ready);
+
+  assert_true(
+    await pageA.execute_script(
+        () => (navigator.serviceWorker.controller !== null)),
+    'pageA should be controlled after history navigation');
+
+}, 'Client.postMessage while a controlled page is in BFCache');
+</script>