fix(service-worker): properly handle invalid hashes in all scenarios (#21288)

alxhub · alexeagle · commit 51eb3d418e7e · 2018-01-10T12:18:55.000-08:00
When the SW fetches URLs listed in a manifest with hashes, it checks the content hash against the manifest to make sure it has the correct version of the URL. In the event of a mismatch, the SW is supposed to consider the manifest invalid, and avoid using it. There are 3 cases to consider by which this can happen. Case 1: during the initial SW installation, a manifest is activated without waiting for every URL to be fully loaded. In the background, every prefetch URL listed by the manifest is requested and cached. One such prefetch request could fail the hash test, and cause the manifest to be treated as invalid. In such a case, the SW should enter a state of EXISTING_CLIENTS_ONLY, as the latest manifest is invalid. This case works today. Case 2: during the initial SW installation, as in Case 1, a manifest is activated without waiting for each URL to fully load. However, it's possible that the application could request a URL with a bad hash before background initialization tries to load that URL. This happens if, for example, the application has a broken index.html. In this case, the SW should enter a state of EXISTING_CLIENTS_ONLY, and serve the request from the network instead. What happens today is that the internal error escapes the SW and is returned as a rejected Promise to respondWith(), causing a browser-level error that the site cannot be loaded, breaking the site. This change allows the SW to detect the error and enter the correct state, falling back on the network if needed. Case 3: during checkForUpdate(), the SW will try to fully cache the new update before making it the latest version. Failure here is complicated - if the page fails to load due to transient network conditions (timeouts, 500s, etc), then it makes sense to continue serving the existing cached version, and attempt to activate the update on the next cycle. If the page fails due to non-transient conditions though (400 error, hash mismatch, etc), then the SW should consider the updated manifest invalid, and enter a state of EXISTING_CLIENTS_ONLY. Currently, all errors are treated as transient. This change causes the SW to treat all errors during updates as non-transient, which can cause the SW to unnecessarily enter a safe mode. A future change can allow the SW to remain in normal mode if the error is provably transient. PR Close #21288
diff --git a/packages/service-worker/worker/src/assets.ts b/packages/service-worker/worker/src/assets.ts
@@ -9,6 +9,7 @@
 import {Adapter, Context} from './adapter';
 import {CacheState, UpdateCacheStatus, UpdateSource, UrlMetadata} from './api';
 import {Database, Table} from './database';
+import {SwCriticalError} from './error';
 import {IdleScheduler} from './idle';
 import {AssetGroupConfig} from './manifest';
 import {sha1Binary} from './sha1';
@@ -345,7 +346,7 @@ export abstract class AssetGroup {
     if ((res as any)['redirected'] && !!res.url) {
       // If the redirect limit is exhausted, fail with an error.
       if (redirectLimit === 0) {
-        throw new Error(
+        throw new SwCriticalError(
             `Response hit redirect limit (fetchFromNetwork): request redirected too many times, next is ${res.url}`);
       }
 
@@ -409,7 +410,7 @@ export abstract class AssetGroup {
 
         // If the response was unsuccessful, there's nothing more that can be done.
         if (!cacheBustedResult.ok) {
-          throw new Error(
+          throw new SwCriticalError(
               `Response not Ok (cacheBustedFetchFromNetwork): cache busted request for ${req.url} returned response ${cacheBustedResult.status} ${cacheBustedResult.statusText}`);
         }
 
@@ -419,7 +420,7 @@ export abstract class AssetGroup {
         // If the cache-busted version doesn't match, then the manifest is not an accurate
         // representation of the server's current set of files, and the SW should give up.
         if (canonicalHash !== cacheBustedHash) {
-          throw new Error(
+          throw new SwCriticalError(
               `Hash mismatch (cacheBustedFetchFromNetwork): ${req.url}: expected ${canonicalHash}, got ${cacheBustedHash} (after cache busting)`);
         }
 
diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts
@@ -11,6 +11,7 @@ import {CacheState, DebugIdleState, DebugState, DebugVersion, Debuggable, Update
 import {AppVersion} from './app-version';
 import {Database, Table} from './database';
 import {DebugHandler} from './debug';
+import {SwCriticalError} from './error';
 import {IdleScheduler} from './idle';
 import {Manifest, ManifestHash, hashManifest} from './manifest';
 import {MsgAny, isMsgActivateUpdate, isMsgCheckForUpdates} from './msg';
@@ -38,7 +39,7 @@ interface LatestEntry {
   latest: string;
 }
 
-enum DriverReadyState {
+export enum DriverReadyState {
   // The SW is operating in a normal mode, responding to all traffic.
   NORMAL,
 
@@ -57,7 +58,7 @@ export class Driver implements Debuggable, UpdateSource {
    * Tracks the current readiness condition under which the SW is operating. This controls
    * whether the SW attempts to respond to some or all requests.
    */
-  private state: DriverReadyState = DriverReadyState.NORMAL;
+  state: DriverReadyState = DriverReadyState.NORMAL;
   private stateMessage: string = '(nominal)';
 
   /**
@@ -351,8 +352,22 @@ export class Driver implements Debuggable, UpdateSource {
       return this.safeFetch(event.request);
     }
 
-    // Handle the request. First try the AppVersion. If that doesn't work, fall back on the network.
-    const res = await appVersion.handleFetch(event.request, event);
+    let res: Response|null = null;
+    try {
+      // Handle the request. First try the AppVersion. If that doesn't work, fall back on the
+      // network.
+      res = await appVersion.handleFetch(event.request, event);
+    } catch (err) {
+      if (err.isCritical) {
+        // Something went wrong with the activation of this version.
+        await this.versionFailed(appVersion, err, this.latestHash === appVersion.manifestHash);
+
+        event.waitUntil(this.idle.trigger());
+        return this.safeFetch(event.request);
+      }
+      throw err;
+    }
+
 
     // The AppVersion will only return null if the manifest doesn't specify what to do about this
     // request. In that case, just fall back on the network.
@@ -482,7 +497,7 @@ export class Driver implements Debuggable, UpdateSource {
         // Attempt to schedule or initialize this version. If this operation is
         // successful, then initialization either succeeded or was scheduled. If
         // it fails, then full initialization was attempted and failed.
-        await this.scheduleInitialization(this.versions.get(hash) !);
+        await this.scheduleInitialization(this.versions.get(hash) !, this.latestHash === hash);
       } catch (err) {
         this.debugger.log(err, `initialize: schedule init of ${hash}`);
         return false;
@@ -623,13 +638,13 @@ export class Driver implements Debuggable, UpdateSource {
    * when the SW is not busy and has connectivity. This returns a Promise which must be
    * awaited, as under some conditions the AppVersion might be initialized immediately.
    */
-  private async scheduleInitialization(appVersion: AppVersion): Promise<void> {
+  private async scheduleInitialization(appVersion: AppVersion, latest: boolean): Promise<void> {
     const initialize = async() => {
       try {
         await appVersion.initializeFully();
       } catch (err) {
         this.debugger.log(err, `initializeFully for ${appVersion.manifestHash}`);
-        await this.versionFailed(appVersion, err);
+        await this.versionFailed(appVersion, err, latest);
       }
     };
     // TODO: better logic for detecting localhost.
@@ -639,7 +654,7 @@ export class Driver implements Debuggable, UpdateSource {
     this.idle.schedule(`initialization(${appVersion.manifestHash})`, initialize);
   }
 
-  private async versionFailed(appVersion: AppVersion, err: Error): Promise<void> {
+  private async versionFailed(appVersion: AppVersion, err: Error, latest: boolean): Promise<void> {
     // This particular AppVersion is broken. First, find the manifest hash.
     const broken =
         Array.from(this.versions.entries()).find(([hash, version]) => version === appVersion);
@@ -653,7 +668,7 @@ export class Driver implements Debuggable, UpdateSource {
 
     // The action taken depends on whether the broken manifest is the active (latest) or not.
     // If so, the SW cannot accept new clients, but can continue to service old ones.
-    if (this.latestHash === brokenHash) {
+    if (this.latestHash === brokenHash || latest) {
       // The latest manifest is broken. This means that new clients are at the mercy of the
       // network, but caches continue to be valid for previous versions. This is
       // unfortunate but unavoidable.
@@ -711,9 +726,10 @@ export class Driver implements Debuggable, UpdateSource {
   }
 
   async checkForUpdate(): Promise<boolean> {
+    let hash: string = '(unknown)';
     try {
       const manifest = await this.fetchLatestManifest();
-      const hash = hashManifest(manifest);
+      hash = hashManifest(manifest);
 
       // Check whether this is really an update.
       if (this.versions.has(hash)) {
@@ -723,7 +739,12 @@ export class Driver implements Debuggable, UpdateSource {
       await this.setupUpdate(manifest, hash);
 
       return true;
-    } catch (_) {
+    } catch (err) {
+      this.debugger.log(err, `Error occurred while updating to manifest ${hash}`);
+
+      this.state = DriverReadyState.EXISTING_CLIENTS_ONLY;
+      this.stateMessage = `Degraded due to failed initialization: ${errorToString(err)}`;
+
       return false;
     }
   }
diff --git a/packages/service-worker/worker/src/error.ts b/packages/service-worker/worker/src/error.ts
@@ -0,0 +1,9 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+export class SwCriticalError extends Error { readonly isCritical: boolean = true; }
diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts
@@ -7,7 +7,7 @@
  */
 
 import {CacheDatabase} from '../src/db-cache';
-import {Driver} from '../src/driver';
+import {Driver, DriverReadyState} from '../src/driver';
 import {Manifest} from '../src/manifest';
 import {sha1} from '../src/sha1';
 import {MockRequest} from '../testing/fetch';
@@ -36,6 +36,24 @@ const distUpdate =
         .addUnhashedFile('/unhashed/a.txt', 'this is unhashed v2', {'Cache-Control': 'max-age=10'})
         .build();
 
+const brokenFs = new MockFileSystemBuilder().addFile('/foo.txt', 'this is foo').build();
+
+const brokenManifest: Manifest = {
+  configVersion: 1,
+  index: '/foo.txt',
+  assetGroups: [{
+    name: 'assets',
+    installMode: 'prefetch',
+    updateMode: 'prefetch',
+    urls: [
+      '/foo.txt',
+    ],
+    patterns: [],
+  }],
+  dataGroups: [],
+  hashTable: tmpHashTableForFs(brokenFs, {'/foo.txt': true}),
+};
+
 const manifest: Manifest = {
   configVersion: 1,
   appData: {
@@ -132,6 +150,9 @@ const serverUpdate =
         .withRedirect('/redirected.txt', '/redirect-target.txt', 'this was a redirect')
         .build();
 
+const brokenServer =
+    new MockServerStateBuilder().withStaticFiles(brokenFs).withManifest(brokenManifest).build();
+
 const server404 = new MockServerStateBuilder().withStaticFiles(dist).build();
 
 const scope = new SwTestHarnessBuilder().withServerState(server).build();
@@ -673,6 +694,34 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
         server.assertSawRequestFor('/baz');
       });
     });
+
+    describe('bugs', () => {
+      async_it('does not crash with bad index hash', async() => {
+        scope = new SwTestHarnessBuilder().withServerState(brokenServer).build();
+        (scope.registration as any).scope = 'http://site.com';
+        driver = new Driver(scope, scope, new CacheDatabase(scope, scope));
+
+        expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
+      });
+
+      async_it('enters degraded mode when update has a bad index', async() => {
+        expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
+        await driver.initialized;
+        server.clearRequests();
+
+        scope = new SwTestHarnessBuilder()
+                    .withCacheState(scope.caches.dehydrate())
+                    .withServerState(brokenServer)
+                    .build();
+        driver = new Driver(scope, scope, new CacheDatabase(scope, scope));
+        await driver.checkForUpdate();
+
+        scope.advance(12000);
+        await driver.idle.empty;
+
+        expect(driver.state).toEqual(DriverReadyState.EXISTING_CLIENTS_ONLY);
+      });
+    });
   });
 })();
 
diff --git a/packages/service-worker/worker/testing/mock.ts b/packages/service-worker/worker/testing/mock.ts
@@ -194,12 +194,16 @@ export function tmpManifestSingleAssetGroup(fs: MockFileSystem): Manifest {
   };
 }
 
-export function tmpHashTableForFs(fs: MockFileSystem): {[url: string]: string} {
+export function tmpHashTableForFs(
+    fs: MockFileSystem, breakHashes: {[url: string]: boolean} = {}): {[url: string]: string} {
   const table: {[url: string]: string} = {};
   fs.list().forEach(path => {
     const file = fs.lookup(path) !;
     if (file.hashThisFile) {
       table[path] = file.hash;
+      if (breakHashes[path]) {
+        table[path] = table[path].split('').reverse().join('');
+      }
     }
   });
   return table;
