1241045 - Cache API addAll() that rejects due to duplicate entries could provide better message

Status: NEW

(Core :: Storage: Cache API, enhancement, P3)

Description

[Matěj Cepl]

I have this simple ServiceWorker-based replacement for appcache:

var version = 'v34 - 2016-01-20';
var toCache = [
  '/',
  'index.html',
  'activePage.js',
  'activePage.js.map',
  'config.js',
  'config.js.map',
  'require.js',
  'hesla.js',
  'hesla.js.map',
  'hesla.webapp',
  'favicon.ico',
  'icon-128.png',
  'icon-30.png',
  'icon-60.png',
  'index_de.html',
  'require.js',
  'screen.css'
];

self.addEventListener('install', event => {
    event.waitUntil(
      caches.open('static' + version)
      .then(
        c => c.addAll(toCache)
          .then(v => console.log('Yay!'), e => console.log('Error'))
      )
    );
});

self.addEventListener('activate', event => {
  event.waitUntil(caches.keys().then(
        keys => Promise.all(
          keys.filter(key => key != 'static' + version).
            map(key => caches.delete(key))
        )
    )
  );
});

self.addEventListener('fetch', function(event) {
  event.respondWith(caches.match(event.request));
});

Unfortunately, 'require.js' resource (RequireJS, of course) is enlisted twice in toCache array. Apparently, it is correct according to spec to break 'install' event handler in such situation. However, it would be nice from you to at least make this clear in the error message, otherwise this is horribly undebuggable problem.

Comment 2

[Marco Castelluccio [:marco]]

A very simple snippet that reproduces the problem:
> caches.open('cache')
> .then(cache => cache.addAll(['index.html', 'index.html']))
> .catch(e => console.log(e));

If you run it, the following message is printed to the console:
> InvalidStateError: "An attempt was made to use an object that is not, or is no longer, usable"

Comment 3

[Salvador de la Puente González [:salva]]

Actually, I can not find in the spec [1] that addAll() must fail when overwriting an already added resource is overwriting a resource though the MDN says it is expected [2].

[1] https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#cache-addAll
[2] https://developer.mozilla.org/en-US/docs/Web/API/Cache/addAll

Ben, is this an expected behavior?

Comment 4

[Marco Castelluccio [:marco]]

There's definitely something wrong happening, I was building a new site from scratch at the same URL.
I've unregistered the old service worker, cleared the cache. When I reload the page of my new site, the old mcepl's site was loaded...

I think there's something wrong with the cache. In about:serviceworkers, the cache name has a weird character at the end: http://imgur.com/Pv6e9LP

Comment 5

[Matěj Cepl]

(In reply to Marco Castelluccio [:marco] from comment #4)
> There's definitely something wrong happening, I was building a new site from
> scratch at the same URL.
> I've unregistered the old service worker, cleared the cache. When I reload
> the page of my new site, the old mcepl's site was loaded...

Actually, there is something wrong (with Aurora, at least). When you unregister a service worker in about:serviceworkers and restart Firefox, it is back there. What am I missing?

Comment 6

[Salvador de la Puente González [:salva]]

> Actually, there is something wrong (with Aurora, at least). When you unregister a service worker in
> about:serviceworkers and restart Firefox, it is back there. What am I missing?

Let's file another bug for this, please.

Comment 7

[Matěj Cepl]

(In reply to Salvador de la Puente González [:salva] from comment #6)
> > Actually, there is something wrong (with Aurora, at least). When you unregister a service worker in
> > about:serviceworkers and restart Firefox, it is back there. What am I missing?
> 
> Let's file another bug for this, please.

done in bug 1241108

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

The behavior in comment 0 is correct per spec.  The addAll() call is supposed to fail if you end up writing two entries to the same matching request.  See step 2.3.2.3 in Batch Cache Operations:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#batch-cache-operations-algorithm

I agree the message could be better.

Comment 9

[Salvador de la Puente González [:salva]]

Wow. Thank you, I suppose this is due to step 9.2.1 of QueryCache algorithm [1] which I did not read carefully enough. Sorry, my bad. Thanks for the clarification.

[1] https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#query-cache-algorithm

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Salvador de la Puente González [:salva] from comment #9)
> Wow. Thank you, I suppose this is due to step 9.2.1 of QueryCache algorithm
> [1] which I did not read carefully enough. Sorry, my bad. Thanks for the
> clarification.

That step is about vary header processing.  That doesn't have to do with this issue.

The step I reference in comment 8 is rather opaquely written, but its effect is to look for a "match" on any request previous in the addAll() list.  If it gets such a match within the addAll() list of requests, then it rejects with InvalidStateError.

Comment 11

[Salvador de la Puente González [:salva]]

Ben, the point you are referencing says:

> If the result of running Query Cache algorithm passing operation.request, operation.options, and addedRecords as the arguments is not an empty array, throw an "InvalidStateError" exception.

So, I'm looking in Query Cache algorithm where it is appending something to the resultArray (as it starts empty in step 3). The only two points where something is added to that array are steps 9.2.1 or 9.6 Both steps are related with the `Vary` header. So, if this is not related, where is the part where you check that it will be overwriting a previously added record?

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

I reference the step in comment 8.  Step 2.3.2.3 in Batch Cache Operations:

"If the result of running Query Cache algorithm passing operation.request, operation.options, and addedRecords as the arguments is not an empty array, throw an "InvalidStateError" exception."

So if there is a match (i.e. duplicate) in the operation's list of addedRecords, then throw InvalidStateError.

Comment 13

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2024-10-06)]

c = await caches.open('static1')
c.addAll(['/', '/'])

With the above, Chrome throws "DOMException: Failed to execute 'addAll' on 'Cache': Cache.addAll(): duplicate requests (https://example.com/)" which is more readable.

Comment 14

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2024-10-06)]

Our error is coming from https://searchfox.org/mozilla-central/rev/d31f750ad9e5b6c8f7b62d387dbea3f804044350/dom/cache/AutoUtils.cpp#261 which just uses NS_ERROR_DOM_INVALID_STATE_ERR which we should really stop using.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

It's interesting that they put that in the actual error message; thanks for checking. This does seem like a case where it's safe to provide the detail in the rejection that's surfaced to content, but in general we do need to be concerned about side-channel leaks, so it could also be fine for us to log a console message, but we do have the plumbing to propagate a more detailed message in the ErrorResult.