fix(service-worker): handle offline mode and ignore invalid only-if-cached requests

packages/service-worker/worker/src/driver.ts

@@ -94,6 +94,12 @@ export class Driver implements Debuggable, UpdateSource {
    */
   private scheduledNavUpdateCheck: boolean = false;
 
+  /**
+   * Keep track of whether we have logged an invalid `only-if-cached` request.
+   * (See `.onFetch()` for details.)
+   */
+  private loggedInvalidOnlyIfCachedRequest: boolean = false;
+
   /**
    * A scheduler which manages a queue of tasks that need to be executed when the SW is
    * not doing any other work (not processing any other requests).
@@ -156,12 +162,13 @@ export class Driver implements Debuggable, UpdateSource {
    * asynchronous execution that eventually resolves for respondWith() and waitUntil().
    */
   private onFetch(event: FetchEvent): void {
+    const req = event.request;
+
     // The only thing that is served unconditionally is the debug page.
-    if (this.adapter.parseUrl(event.request.url, this.scope.registration.scope).path ===
-        '/ngsw/state') {
+    if (this.adapter.parseUrl(req.url, this.scope.registration.scope).path === '/ngsw/state') {
       // Allow the debugger to handle the request, but don't affect SW state in any
       // other way.
-      event.respondWith(this.debugger.handleFetch(event.request));
+      event.respondWith(this.debugger.handleFetch(req));
       return;
     }
 
@@ -177,6 +184,24 @@ export class Driver implements Debuggable, UpdateSource {
       return;
     }
 
+    // When opening DevTools in Chrome, a request is made for the current URL (and possibly related
+    // resources, e.g. scripts) with `cache: 'only-if-cached'` and `mode: 'no-cors'`. These request
+    // will eventually fail, because `only-if-cached` is only allowed to be used with
+    // `mode: 'same-origin'`.
+    // This is likely a bug in Chrome DevTools. Avoid handling such requests.
+    // (See also https://github.com/angular/angular/issues/22362.)
+    // TODO(gkalpak): Remove once no longer necessary (i.e. fixed in Chrome DevTools).
+    if ((req.cache as string) === 'only-if-cached' && req.mode !== 'same-origin') {
+      // Log the incident only the first time it happens, to avoid spamming the logs.
+      if (!this.loggedInvalidOnlyIfCachedRequest) {
+        this.loggedInvalidOnlyIfCachedRequest = true;
+        this.debugger.log(
+            `Ignoring invalid request: 'only-if-cached' can be set only with 'same-origin' mode`,
+            `Driver.fetch(${req.url}, cache: ${req.cache}, mode: ${req.mode})`);
+      }
+      return;
+    }
+
     // Past this point, the SW commits to handling the request itself. This could still
     // fail (and result in `state` being set to `SAFE_MODE`), but even in that case the
     // SW will still deliver a response.
@@ -607,16 +632,22 @@ export class Driver implements Debuggable, UpdateSource {
 
   /**
    * Retrieve a copy of the latest manifest from the server.
+   * Return `null` if `ignoreOfflineError` is true (default: false) and the server or client are
+   * offline (detected as response status 504).
    */
-  private async fetchLatestManifest(): Promise<Manifest> {
+  private async fetchLatestManifest(ignoreOfflineError?: false): Promise<Manifest>;
+  private async fetchLatestManifest(ignoreOfflineError: true): Promise<Manifest|null>;
+  private async fetchLatestManifest(ignoreOfflineError = false): Promise<Manifest|null> {
     const res =
         await this.safeFetch(this.adapter.newRequest('ngsw.json?ngsw-cache-bust=' + Math.random()));
     if (!res.ok) {
       if (res.status === 404) {
         await this.deleteAllCaches();
         await this.scope.registration.unregister();
+      } else if (res.status === 504 && ignoreOfflineError) {
+        return null;
       }
-      throw new Error('Manifest fetch failed!');
+      throw new Error(`Manifest fetch failed! (status: ${res.status})`);
     }
     this.lastUpdateCheck = this.adapter.time;
     return res.json();
@@ -728,7 +759,15 @@ export class Driver implements Debuggable, UpdateSource {
   async checkForUpdate(): Promise<boolean> {
     let hash: string = '(unknown)';
     try {
-      const manifest = await this.fetchLatestManifest();
+      const manifest = await this.fetchLatestManifest(true);
+
+      if (manifest === null) {
+        // Client or server offline. Unable to check for updates at this time.
+        // Continue to service clients (existing and new).
+        this.debugger.log('Check for update aborted. (Client or server offline.)');
+        return false;
+      }
+
       hash = hashManifest(manifest);
 
       // Check whether this is really an update.
@@ -972,4 +1011,4 @@ function errorToString(error: any): string {
   } else {
     return `${error}`;
   }
-}
+}

packages/service-worker/worker/test/happy_spec.ts

@@ -170,7 +170,11 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
     let driver: Driver;
 
     beforeEach(() => {
-      server.clearRequests();
+      server.reset();
+      serverUpdate.reset();
+      server404.reset();
+      brokenServer.reset();
+
       scope = new SwTestHarnessBuilder().withServerState(server).build();
       driver = new Driver(scope, scope, new CacheDatabase(scope, scope));
     });
@@ -275,39 +279,6 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
       serverUpdate.assertNoOtherRequests();
     });
 
-    async_it('updates to new content when requested', async() => {
-      expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
-      await driver.initialized;
-
-      const client = scope.clients.getMock('default') !;
-      expect(client.messages).toEqual([]);
-
-      scope.updateServerState(serverUpdate);
-      expect(await driver.checkForUpdate()).toEqual(true);
-      serverUpdate.assertSawRequestFor('ngsw.json');
-      serverUpdate.assertSawRequestFor('/foo.txt');
-      serverUpdate.assertSawRequestFor('/redirected.txt');
-      serverUpdate.assertNoOtherRequests();
-
-      expect(client.messages).toEqual([{
-        type: 'UPDATE_AVAILABLE',
-        current: {hash: manifestHash, appData: {version: 'original'}},
-        available: {hash: manifestUpdateHash, appData: {version: 'update'}},
-      }]);
-
-      // Default client is still on the old version of the app.
-      expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
-
-      // Sending a new client id should result in the updated version being returned.
-      expect(await makeRequest(scope, '/foo.txt', 'new')).toEqual('this is foo v2');
-
-      // Of course, the old version should still work.
-      expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
-
-      expect(await makeRequest(scope, '/bar.txt')).toEqual('this is bar');
-      serverUpdate.assertNoOtherRequests();
-    });
-
     async_it('updates a specific client to new content on request', async() => {
       expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
       await driver.initialized;
@@ -403,8 +374,8 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
       serverUpdate.clearRequests();
 
       scope = new SwTestHarnessBuilder()
-                  .withServerState(serverUpdate)
                   .withCacheState(scope.caches.dehydrate())
+                  .withServerState(serverUpdate)
                   .build();
       driver = new Driver(scope, scope, new CacheDatabase(scope, scope));
 
@@ -478,6 +449,10 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
       await driver.initialized;
       serverUpdate.assertNoOtherRequests();
 
+      let keys = await scope.caches.keys();
+      let hasOriginalCaches = keys.some(name => name.startsWith(`ngsw:${manifestHash}:`));
+      expect(hasOriginalCaches).toEqual(true);
+
       scope.clients.remove('default');
 
       scope.advance(12000);
@@ -487,10 +462,9 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
       driver = new Driver(scope, scope, new CacheDatabase(scope, scope));
       expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo v2');
 
-      const oldManifestHash = sha1(JSON.stringify(manifest));
-      const keys = await scope.caches.keys();
-      const hasOldCaches = keys.some(name => name.startsWith(oldManifestHash + ':'));
-      expect(hasOldCaches).toEqual(false);
+      keys = await scope.caches.keys();
+      hasOriginalCaches = keys.some(name => name.startsWith(`ngsw:${manifestHash}:`));
+      expect(hasOriginalCaches).toEqual(false);
     });
 
     async_it('shows notifications for push notifications', async() => {
@@ -542,6 +516,17 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
       expect(await scope.caches.keys()).toEqual([]);
     });
 
+    async_it('does not unregister or change state when offline (i.e. manifest 504s)', async() => {
+      expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
+      await driver.initialized;
+      server.online = false;
+
+      expect(await driver.checkForUpdate()).toEqual(false);
+      expect(driver.state).toEqual(DriverReadyState.NORMAL);
+      expect(scope.unregistered).toBeFalsy();
+      expect(await scope.caches.keys()).not.toEqual([]);
+    });
+
     describe('unhashed requests', () => {
       async_beforeEach(async() => {
         expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
@@ -639,6 +624,7 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
         serverUpdate.assertNoOtherRequests();
       });
     });
+
     describe('routing', () => {
       async_beforeEach(async() => {
         expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
@@ -680,6 +666,7 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
           headers: {
             'Accept': 'text/plain, text/html, text/css',
           },
+          mode: 'navigate',
         })).toBeNull();
         server.assertSawRequestFor('/baz.html');
       });
@@ -721,18 +708,27 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
 
         expect(driver.state).toEqual(DriverReadyState.EXISTING_CLIENTS_ONLY);
       });
+
+      async_it('ignores invalid `only-if-cached` requests ', async() => {
+        const requestFoo = (cache: RequestCache | 'only-if-cached', mode: RequestMode) =>
+            makeRequest(scope, '/foo.txt', undefined, {cache, mode});
+
+        expect(await requestFoo('default', 'no-cors')).toBe('this is foo');
+        expect(await requestFoo('only-if-cached', 'same-origin')).toBe('this is foo');
+        expect(await requestFoo('only-if-cached', 'no-cors')).toBeNull();
+      });
     });
   });
 })();
 
 async function makeRequest(
-    scope: SwTestHarness, url: string, clientId?: string, init?: Object): Promise<string|null> {
-  const [resPromise, done] = scope.handleFetch(new MockRequest(url, init), clientId || 'default');
+    scope: SwTestHarness, url: string, clientId = 'default', init?: Object): Promise<string|null> {
+  const [resPromise, done] = scope.handleFetch(new MockRequest(url, init), clientId);
   await done;
   const res = await resPromise;
-  scope.clients.add(clientId || 'default');
+  scope.clients.add(clientId);
   if (res !== undefined && res.ok) {
     return res.text();
   }
   return null;
-}
+}

packages/service-worker/worker/testing/fetch.ts

@@ -110,6 +110,9 @@ export class MockRequest extends MockBody implements Request {
         Object.keys(headers).forEach(header => { this.headers.set(header, headers[header]); });
       }
     }
+    if (init.cache !== undefined) {
+      this.cache = init.cache;
+    }
     if (init.mode !== undefined) {
       this.mode = init.mode;
     }
@@ -170,4 +173,4 @@ export class MockResponse extends MockBody implements Response {
     return new MockResponse(
         this._body, {status: this.status, statusText: this.statusText, headers: this.headers});
   }
-}
+}

packages/service-worker/worker/testing/mock.ts

@@ -96,6 +96,7 @@ export class MockServerState {
   private gate: Promise<void> = Promise.resolve();
   private resolve: Function|null = null;
   private resolveNextRequest: Function;
+  online = true;
   nextRequest: Promise<Request>;
 
   constructor(private resources: Map<string, Response>, private errors: Set<string>) {
@@ -108,6 +109,10 @@ export class MockServerState {
 
     await this.gate;
 
+    if (!this.online) {
+      throw new Error('Offline.');
+    }
+
     if (req.credentials === 'include') {
       return new MockResponse(null, {status: 0, statusText: '', type: 'opaque'});
     }
@@ -171,6 +176,7 @@ export class MockServerState {
     this.nextRequest = new Promise(resolve => { this.resolveNextRequest = resolve; });
     this.gate = Promise.resolve();
     this.resolve = null;
+    this.online = true;
   }
 }