From 2bd767c4a69860762c89b9e16d6c417e52aeacd6 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Thu, 20 Sep 2018 21:04:37 +0300 Subject: [PATCH] fix(service-worker): do not blow up when caches are unwritable (#26042) In some cases, example when the user clears the caches in DevTools but the SW remains active on another tab and keeps references to the deleted caches, trying to write to the cache throws errors (e.g. `Entry was not found`). When this happens, the SW can no longer work correctly and should enter a degraded mode allowing requests to be served from the network. Possibly related: - https://github.com/GoogleChrome/workbox/issues/792 - https://bugs.chromium.org/p/chromium/issues/detail?id=639034 This commits remedies this situation, by ensuring the SW can enter the degraded `EXISTING_CLIENTS_ONLY` mode and forward requests to the network. PR Close #26042 --- packages/service-worker/worker/src/assets.ts | 39 ++++++++++++------- packages/service-worker/worker/src/driver.ts | 25 ++++++------ packages/service-worker/worker/src/error.ts | 8 ++++ .../service-worker/worker/test/happy_spec.ts | 23 +++++++++++ .../service-worker/worker/testing/cache.ts | 17 ++++++++ 5 files changed, 84 insertions(+), 28 deletions(-) diff --git a/packages/service-worker/worker/src/assets.ts b/packages/service-worker/worker/src/assets.ts index b5dc2a9f04..dedbceff3a 100644 --- a/packages/service-worker/worker/src/assets.ts +++ b/packages/service-worker/worker/src/assets.ts @@ -9,7 +9,7 @@ import {Adapter, Context} from './adapter'; import {CacheState, UpdateCacheStatus, UpdateSource, UrlMetadata} from './api'; import {Database, Table} from './database'; -import {SwCriticalError} from './error'; +import {SwCriticalError, errorToString} from './error'; import {IdleScheduler} from './idle'; import {AssetGroupConfig} from './manifest'; import {sha1Binary} from './sha1'; @@ -317,24 +317,33 @@ export abstract class AssetGroup { `Response not Ok (fetchAndCacheOnce): request for ${req.url} returned response ${res.status} ${res.statusText}`); } - // This response is safe to cache (as long as it's cloned). Wait until the cache operation - // is complete. - const cache = await this.scope.caches.open(`${this.prefix}:${this.config.name}:cache`); - await cache.put(req, res.clone()); + try { + // This response is safe to cache (as long as it's cloned). Wait until the cache operation + // is complete. + const cache = await this.scope.caches.open(`${this.prefix}:${this.config.name}:cache`); + await cache.put(req, res.clone()); - // If the request is not hashed, update its metadata, especially the timestamp. This is needed - // for future determination of whether this cached response is stale or not. - if (!this.hashes.has(req.url)) { - // Metadata is tracked for requests that are unhashed. - const meta: UrlMetadata = {ts: this.adapter.time, used}; - const metaTable = await this.metadata; - await metaTable.write(req.url, meta); + // If the request is not hashed, update its metadata, especially the timestamp. This is + // needed for future determination of whether this cached response is stale or not. + if (!this.hashes.has(req.url)) { + // Metadata is tracked for requests that are unhashed. + const meta: UrlMetadata = {ts: this.adapter.time, used}; + const metaTable = await this.metadata; + await metaTable.write(req.url, meta); + } + + return res; + } catch (err) { + // Among other cases, this can happen when the user clears all data through the DevTools, + // but the SW is still running and serving another tab. In that case, trying to write to the + // caches throws an `Entry was not found` error. + // If this happens the SW can no longer work correctly. This situation is unrecoverable. + throw new SwCriticalError( + `Failed to update the caches for request to '${req.url}' (fetchAndCacheOnce): ${errorToString(err)}`); } - - return res; } finally { // Finally, it can be removed from `inFlightRequests`. This might result in a double-remove - // if some other chain was already making this request too, but that won't hurt anything. + // if some other chain was already making this request too, but that won't hurt anything. this.inFlightRequests.delete(req.url); } } diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts index 1dc99259b3..86afc43d86 100644 --- a/packages/service-worker/worker/src/driver.ts +++ b/packages/service-worker/worker/src/driver.ts @@ -6,12 +6,12 @@ * found in the LICENSE file at https://angular.io/license */ -import {Adapter, Context} from './adapter'; +import {Adapter} from './adapter'; import {CacheState, DebugIdleState, DebugState, DebugVersion, Debuggable, UpdateCacheStatus, UpdateSource} from './api'; import {AppVersion} from './app-version'; -import {Database, Table} from './database'; +import {Database} from './database'; import {DebugHandler} from './debug'; -import {SwCriticalError} from './error'; +import {errorToString} from './error'; import {IdleScheduler} from './idle'; import {Manifest, ManifestHash, hashManifest} from './manifest'; import {MsgAny, isMsgActivateUpdate, isMsgCheckForUpdates} from './msg'; @@ -709,7 +709,7 @@ export class Driver implements Debuggable, UpdateSource { // network, but caches continue to be valid for previous versions. This is // unfortunate but unavoidable. this.state = DriverReadyState.EXISTING_CLIENTS_ONLY; - this.stateMessage = `Degraded due to failed initialization: ${errorToString(err)}`; + this.stateMessage = `Degraded due to: ${errorToString(err)}`; // Cancel the binding for these clients. Array.from(this.clientVersionMap.keys()) @@ -724,7 +724,14 @@ export class Driver implements Debuggable, UpdateSource { // Push the affected clients onto the latest version. affectedClients.forEach(clientId => this.clientVersionMap.set(clientId, this.latestHash !)); } - await this.sync(); + + try { + await this.sync(); + } catch (err2) { + // We are already in a bad state. No need to make things worse. + // Just log the error and move on. + this.debugger.log(err2, `Driver.versionFailed(${err.message || err})`); + } } private async setupUpdate(manifest: Manifest, hash: string): Promise { @@ -999,11 +1006,3 @@ export class Driver implements Debuggable, UpdateSource { } } } - -function errorToString(error: any): string { - if (error instanceof Error) { - return `${error.message}\n${error.stack}`; - } else { - return `${error}`; - } -} diff --git a/packages/service-worker/worker/src/error.ts b/packages/service-worker/worker/src/error.ts index fa445acabd..25ecdadc0d 100644 --- a/packages/service-worker/worker/src/error.ts +++ b/packages/service-worker/worker/src/error.ts @@ -7,3 +7,11 @@ */ export class SwCriticalError extends Error { readonly isCritical: boolean = true; } + +export function errorToString(error: any): string { + if (error instanceof Error) { + return `${error.message}\n${error.stack}`; + } else { + return `${error}`; + } +} diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts index 4ca3969b4a..0e43108da0 100644 --- a/packages/service-worker/worker/test/happy_spec.ts +++ b/packages/service-worker/worker/test/happy_spec.ts @@ -11,6 +11,7 @@ import {CacheDatabase} from '../src/db-cache'; import {Driver, DriverReadyState} from '../src/driver'; import {Manifest} from '../src/manifest'; import {sha1} from '../src/sha1'; +import {MockCache, clearAllCaches} from '../testing/cache'; import {MockRequest} from '../testing/fetch'; import {MockFileSystemBuilder, MockServerStateBuilder, tmpHashTableForFs} from '../testing/mock'; import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; @@ -851,6 +852,28 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate)); expect(driver.state).toEqual(DriverReadyState.EXISTING_CLIENTS_ONLY); }); + async_it('enters degraded mode when failing to write to cache', async() => { + // Initialize the SW. + await makeRequest(scope, '/foo.txt'); + await driver.initialized; + expect(driver.state).toBe(DriverReadyState.NORMAL); + + server.clearRequests(); + + // Operate normally. + expect(await makeRequest(scope, '/foo.txt')).toBe('this is foo'); + server.assertNoOtherRequests(); + + // Clear the caches and make them unwritable. + await clearAllCaches(scope.caches); + spyOn(MockCache.prototype, 'put').and.throwError('Can\'t touch this'); + + // Enter degraded mode and serve from network. + expect(await makeRequest(scope, '/foo.txt')).toBe('this is foo'); + expect(driver.state).toBe(DriverReadyState.EXISTING_CLIENTS_ONLY); + server.assertSawRequestFor('/foo.txt'); + }); + 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}); diff --git a/packages/service-worker/worker/testing/cache.ts b/packages/service-worker/worker/testing/cache.ts index b24147e2f6..5d9f3c2fee 100644 --- a/packages/service-worker/worker/testing/cache.ts +++ b/packages/service-worker/worker/testing/cache.ts @@ -164,3 +164,20 @@ export class MockCache { return dehydrated; } } + +// This can be used to simulate a situation (bug?), where the user clears the caches from DevTools, +// while the SW is still running (e.g. serving another tab) and keeps references to the deleted +// caches. +export async function clearAllCaches(caches: CacheStorage): Promise { + const cacheNames = await caches.keys(); + const cacheInstances = await Promise.all(cacheNames.map(name => caches.open(name))); + + // Delete all cache instances from `CacheStorage`. + await Promise.all(cacheNames.map(name => caches.delete(name))); + + // Delete all entries from each cache instance. + await Promise.all(cacheInstances.map(async cache => { + const keys = await cache.keys(); + await Promise.all(keys.map(key => cache.delete(key))); + })); +}