From cb2ca9a66ef54d9f1b3df5ac14962dc3810b4cc1 Mon Sep 17 00:00:00 2001
From: George Kalpakas <kalpakas.g@gmail.com>
Date: Thu, 8 Jul 2021 16:25:16 +0300
Subject: [PATCH] fix(service-worker): correctly handle unrecoverable state
 when a client no longer exists (#42736)

Previously, the ServiceWorker assumed that a client found in
`clientVersionMap` would exist (i.e. it could be retrieved via
`clients.get()`). However, if a browser tab had been closed, the
corresponding client (while present in `clientVersionMap`, which is only
updated on ServiceWorker initialization) would not be retrievable via
`clients.get()`.

This commit fixes it by checking whether the client exists before trying
to notify it about an unrecoverable state.

PR Close #42736
---
 packages/service-worker/worker/src/driver.ts  |  8 ++-
 .../service-worker/worker/test/happy_spec.ts  | 56 +++++++++++++++++++
 2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts
index 542dea8157..a40fde67cc 100644
--- a/packages/service-worker/worker/src/driver.ts
+++ b/packages/service-worker/worker/src/driver.ts
@@ -672,8 +672,10 @@ export class Driver implements Debuggable, UpdateSource {
           }
 
           const client = await this.scope.clients.get(clientId);
+          if (client) {
+            await this.updateClient(client);
+          }
 
-          await this.updateClient(client!);
           appVersion = this.lookupVersionByHash(this.latestHash, 'assignVersion');
         }
 
@@ -1050,7 +1052,9 @@ export class Driver implements Debuggable, UpdateSource {
 
     await Promise.all(affectedClients.map(async clientId => {
       const client = await this.scope.clients.get(clientId);
-      client!.postMessage({type: 'UNRECOVERABLE_STATE', reason});
+      if (client) {
+        client.postMessage({type: 'UNRECOVERABLE_STATE', reason});
+      }
     }));
   }
 
diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts
index 2280add54a..fede0a01a0 100644
--- a/packages/service-worker/worker/test/happy_spec.ts
+++ b/packages/service-worker/worker/test/happy_spec.ts
@@ -2306,6 +2306,62 @@ describe('Driver', () => {
         // latest one.
         expect(driver.state).toEqual(DriverReadyState.EXISTING_CLIENTS_ONLY);
       });
+
+      it('is handled correctly even if some of the clients no longer exist', async () => {
+        const originalFiles = new MockFileSystemBuilder()
+                                  .addFile('/index.html', '<script src="foo.hash.js"></script>')
+                                  .addFile('/foo.hash.js', 'console.log("FOO");')
+                                  .build();
+
+        const updatedFiles = new MockFileSystemBuilder()
+                                 .addFile('/index.html', '<script src="bar.hash.js"></script>')
+                                 .addFile('/bar.hash.js', 'console.log("BAR");')
+                                 .build();
+
+        const {serverState: originalServer, manifest} = generateMockServerState(originalFiles);
+        const {serverState: updatedServer} = generateMockServerState(updatedFiles);
+
+        // Create initial server state and initialize the SW.
+        scope = new SwTestHarnessBuilder().withServerState(originalServer).build();
+        driver = new Driver(scope, scope, new CacheDatabase(scope));
+
+        expect(await makeRequest(scope, '/foo.hash.js', 'client-1')).toBe('console.log("FOO");');
+        expect(await makeRequest(scope, '/foo.hash.js', 'client-2')).toBe('console.log("FOO");');
+        await driver.initialized;
+
+        // Update the server state to emulate deploying a new version (where `foo.hash.js` does not
+        // exist any more). Keep the cache though.
+        scope = new SwTestHarnessBuilder()
+                    .withCacheState(scope.caches.original.dehydrate())
+                    .withServerState(updatedServer)
+                    .build();
+        driver = new Driver(scope, scope, new CacheDatabase(scope));
+
+        // The SW is still able to serve `foo.hash.js` from the cache.
+        expect(await makeRequest(scope, '/foo.hash.js', 'client-1')).toBe('console.log("FOO");');
+        expect(await makeRequest(scope, '/foo.hash.js', 'client-2')).toBe('console.log("FOO");');
+
+        // Remove `foo.hash.js` from the cache to emulate the browser evicting files from the cache.
+        await removeAssetFromCache(scope, manifest, '/foo.hash.js');
+
+        // Remove one of the clients to emulate closing a browser tab.
+        scope.clients.remove('client-1');
+
+        // Retrieve the remaining client to ensure it is notified.
+        const mockClient2 = scope.clients.getMock('client-2')!;
+        expect(mockClient2.messages).toEqual([]);
+
+        // Try to retrieve `foo.hash.js`, which is neither in the cache nor on the server.
+        // This should put the SW in an unrecoverable state and notify clients (even if some of the
+        // previously known clients no longer exist).
+        expect(await makeRequest(scope, '/foo.hash.js', 'client-2')).toBeNull();
+        expect(mockClient2.messages).toEqual([jasmine.objectContaining(
+            {type: 'UNRECOVERABLE_STATE'})]);
+
+        // This should also enter the `SW` into degraded mode, because the broken version was the
+        // latest one.
+        expect(driver.state).toEqual(DriverReadyState.EXISTING_CLIENTS_ONLY);
+      });
     });
 
     describe('backwards compatibility with v5', () => {