Open Bug 1241045 Opened 4 years ago Updated 22 days ago

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

Categories

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

45 Branch
defect

Tracking

()

People

(Reporter: mcepl, Unassigned)

References

Details

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.
Duplicate of this bug: 1241073
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"
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?
Flags: needinfo?(bkelly)
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
(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?
> 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.
(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
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.
Blocks: 1110136
Flags: needinfo?(bkelly)
See Also: → 1213077
Summary: twice installed resource knocks off ServiceWorker cache ... please at least report in the Error message → Cache API addAll() that rejects due to duplicate entries could provide better message
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
(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.
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?
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
Priority: -- → P3
Component: DOM: Service Workers → Storage: Cache API
No longer blocks: 1110136
You need to log in before you can comment on or make changes to this bug.