Fixes a bug with cacheFirst handler + maxAgeSeconds + CORS request (#694

)

* Fixes a bug with cacheFirst handler + maxAgeSeconds

* Keeps the original method signatures for the cacheWillMatch callback.

* Clarifications(?) via comments and restructured return statements.

* Review feedback.

packages/workbox-cache-expiration/src/lib/cache-expiration-plugin.js

@@ -47,14 +47,15 @@ class CacheExpirationPlugin extends CacheExpiration {
    *
    * @private
    * @param {Object} input
+   * @param {string} input.cacheName Name of the cache the responses belong to.
    * @param {Response} input.cachedResponse The `Response` object that's been
    *        read from a cache and whose freshness should be checked.
    * @param {Number} [input.now] A timestamp. Defaults to the current time.
-   * @return {Response|null} Either the `cachedResponse`, if it's fresh, or
-   *          `null` if the `Response` is older than `maxAgeSeconds`.
+   * @return {Response} Either the `cachedResponse`, if it's
+   *         fresh, or `null` if the `Response` is older than `maxAgeSeconds`.
    */
-  cacheWillMatch({cachedResponse, now} = {}) {
-    if (this.isResponseFresh({cachedResponse, now})) {
+  cacheWillMatch({cacheName, cachedResponse, now} = {}) {
+    if (this.isResponseFresh({cacheName, cachedResponse, now})) {
       return cachedResponse;
     }
 

packages/workbox-cache-expiration/src/lib/cache-expiration.js

@@ -126,28 +126,31 @@ class CacheExpiration {
    * `Date` header and the `maxAgeSeconds` parameter passed into the
    * constructor.
    *
+   * The general approach is to default to fresh unless proven otherwise.
+   *
    * If `maxAgeSeconds` or the `Date` header is not set then it will
    * default to returning `true`, i.e. the response is still fresh and should
    * be used.
    *
    * @param {Object} input
+   * @param {string} input.cacheName Name of the cache the responses belong to.
    * @param {Response} input.cachedResponse The `Response` object that's been
    *        read from a cache and whose freshness should be checked.
    * @param {Number} [input.now] A timestamp.
    *
    * Defaults to the current time.
-   * @return {boolean} Either `true` if the response is fresh, or `false` if the
-   * `Response` is older than `maxAgeSeconds` and should no longer be
-   * used.
+   * @return {boolean} Either `true` if the response is fresh, or
+   * `false` if the `Response` is older than `maxAgeSeconds` and should no
+   * longer be used.
    *
    * @example
    * expirationPlugin.isResponseFresh({
    *   cachedResponse: responseFromCache
    * });
    */
-  isResponseFresh({cachedResponse, now} = {}) {
+  isResponseFresh({cacheName, cachedResponse, now} = {}) {
     // Only bother checking for freshness if we have a valid response and if
-    // maxAgeSeconds is set. Otherwise, skip the check and always return true.
+    // maxAgeSeconds is set.
     if (cachedResponse && this.maxAgeSeconds) {
       isInstance({cachedResponse}, Response);
 
@@ -158,16 +161,38 @@ class CacheExpiration {
         }
 
         const parsedDate = new Date(dateHeader);
+        const headerTime = parsedDate.getTime();
         // If the Date header was invalid for some reason, parsedDate.getTime()
-        // will return NaN, and the comparison will always be false. That means
-        // that an invalid date will be treated as if the response is fresh.
-        if ((parsedDate.getTime() + (this.maxAgeSeconds * 1000)) < now) {
-          // Only return false if all the conditions are met.
-          return false;
+        // will return NaN. We want to treat that as a fresh response, since we
+        // assume fresh unless proven otherwise.
+        if (isNaN(headerTime)) {
+          return true;
         }
+
+        // If we have a valid headerTime, then our response is fresh iff the
+        // headerTime plus maxAgeSeconds is greater than the current time.
+        return (headerTime + (this.maxAgeSeconds * 1000)) > now;
+      } else {
+        // TODO (jeffposnick): Change this method interface to be async, and
+        // check for the IDB for the specific URL in order to determine
+        // freshness when Date is not available.
+
+        // If there is no Date header (i.e. if this is a cross-origin response),
+        // then we don't know for sure whether the response is fresh or not.
+        // One thing we can do is trigger cache expiration, which will clean up
+        // any old responses based on IDB timestamps, and ensure that when a
+        // cache-first handler is used, stale responses will eventually be
+        // replaced (though not until the *next* request is made).
+        // See https://github.com/GoogleChrome/workbox/issues/691
+        this.expireEntries({cacheName, now});
+        // Return true, since otherwise a cross-origin cached response without
+        // a Date header would *never* be considered valid.
+        return true;
       }
     }
 
+    // If either cachedResponse or maxAgeSeconds wasn't set, then the response
+    // is "trivially" fresh, so return true.
     return true;
   }
 

packages/workbox-cache-expiration/test/browser/end-to-end.js

@@ -6,6 +6,10 @@ describe(`End to End Test of Cache Expiration`, function() {
 
   beforeEach(() => goog.swUtils.cleanState());
 
+  async function asyncDelay(seconds) {
+    await new Promise((resolve) => setTimeout(resolve, seconds * 1000));
+  }
+
   it(`should work properly when there are many simultaneous requests`, async function() {
     const iframe = await goog.swUtils.controlledBySW(
       '/packages/workbox-cache-expiration/test/static/cache-expiration.js');
@@ -37,4 +41,41 @@ describe(`End to End Test of Cache Expiration`, function() {
 
     expect(keys).to.have.length(EXPECTED_CACHE_SIZE);
   });
+
+  it(`should work properly when a cache-first strategy + maxAgeSeconds is used, and responses lack a Date header`, async function() {
+    // See the comment later about a source of potential flakiness.
+    this.retries(2);
+    this.timeout(6 * 1000);
+
+    const iframe = await goog.swUtils.controlledBySW(
+      '/packages/workbox-cache-expiration/test/static/cache-first-max-age-seconds.js');
+
+    const currentUrl = new URL(location);
+    const nextPort = parseInt(currentUrl.port) + 1;
+    const corsOrigin = `${currentUrl.protocol}//${currentUrl.hostname}:${nextPort}`;
+    const testUrl = new URL('/__echo/cors-no-cache/test', corsOrigin);
+
+    const firstResponse = await iframe.contentWindow.fetch(testUrl);
+    const firstResponseBody = await firstResponse.text();
+
+    // The service worker should be using a maxAgeSeconds of 1, so skip ahead
+    // 1.5 seconds. We can't use sinon's fake clock here, since we need a
+    // consistent clock between the iframe's scope and the service worker scope.
+    await asyncDelay(1.5);
+    const secondResponse = await iframe.contentWindow.fetch(testUrl);
+    const secondResponseBody = await secondResponse.text();
+    // Since we're using a cache-first policy, the expiration happens after the
+    // previous entry had already been read from cache. So the first and second
+    // responses should be the same.
+    expect(firstResponseBody).to.eql(secondResponseBody);
+
+    // POTENTIALLY FLAKY: Wait another 2 seconds, after which point the cache
+    // expiration logic will have hopefully completed.
+    await asyncDelay(2);
+    const thirdResponse = await iframe.contentWindow.fetch(testUrl);
+    const thirdResponseBody = await thirdResponse.text();
+    // By the time the third response is read, the cache expiration will have
+    // completed, so this one should be different.
+    expect(firstResponseBody).not.to.eql(thirdResponseBody);
+  });
 });

packages/workbox-cache-expiration/test/static/cache-first-max-age-seconds.js

@@ -0,0 +1,23 @@
+importScripts('/__test/bundle/workbox-runtime-caching');
+importScripts('/__test/bundle/workbox-cache-expiration');
+
+self.addEventListener('install', (event) => event.waitUntil(self.skipWaiting()));
+self.addEventListener('activate', (event) => event.waitUntil(self.clients.claim()));
+
+const cacheExpirationPlugin= new workbox.cacheExpiration.CacheExpirationPlugin({
+  maxAgeSeconds: 1,
+});
+const requestWrapper = new workbox.runtimeCaching.RequestWrapper({
+  cacheName: 'cache-first-max-age-seconds',
+  plugins: [cacheExpirationPlugin],
+});
+const handler = new workbox.runtimeCaching.CacheFirst({
+  requestWrapper,
+  waitOnCache: true,
+});
+
+self.addEventListener('fetch', (event) => {
+  if (event.request.url.includes('/__echo/cors-no-cache')) {
+    event.respondWith(handler.handle({event}));
+  }
+});

packages/workbox-runtime-caching/src/lib/request-wrapper.js

@@ -175,8 +175,10 @@ class RequestWrapper {
 
     if (this.plugins.has('cacheWillMatch')) {
       const plugin = this.plugins.get('cacheWillMatch')[0];
-      cachedResponse = plugin.cacheWillMatch({request, cache, cachedResponse,
-        matchOptions: this.matchOptions});
+      cachedResponse = plugin.cacheWillMatch({
+        request, cache, cachedResponse,
+        matchOptions: this.matchOptions, cacheName: this.cacheName,
+      });
     }
 
     return cachedResponse;

utils/server-instance.js

@@ -54,6 +54,14 @@ class ServerInstance {
       res.send(`${req.params.file}-${Date.now()}`);
     });
 
+    this._app.all('/__echo/cors-no-cache/:file', function(req, res) {
+      res.setHeader('Access-Control-Allow-Methods',
+        'POST, GET, PUT, DELETE, OPTIONS');
+      res.setHeader('Cache-Control', 'no-cache');
+      res.setHeader('Access-Control-Allow-Origin', '*');
+      res.send(`${req.params.file}-${Date.now()}`);
+    });
+
     this._app.all('/__echo/date-with-cors/:file', function(req, res) {
       res.setHeader('Access-Control-Allow-Methods',
         'POST, GET, PUT, DELETE, OPTIONS');