1213077 - Cache API caches.match() with bad cache name could reject with a better message

Status: NEW

(Core :: Storage: Cache API, task)

Description

[Marcos Caceres [:marcosc]]

when you have code like this:

    ev.respondWith(p);
    p.catch(err => console.log(err) );

Getting a: 
"NotFoundError: Node was not found"

Is quite unhelpful - specially when caching a lot of files. Can we please make it so it actually says what was not found.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

What actually produced this promise and what rejected it with that error?

Comment 2

[Marcos Caceres [:marcosc]]

The "images" case below in the "fetch" handler... 

```JS
var CacheTasks = {
  respondFromCache: async(function* (request, cacheName = "", strategy = "") {
    var options = {
      cacheName
    };
    var response = yield caches.match(request, options);
    if (response) {
      return response;
    }
    switch (strategy) {
    case "throw":
      var msg = `Not found in ${cacheName} cache: ${request.url || request}`;
      var err = new Error(msg);
      console.warn(err);
      throw err;
    case "store":
      try {
        var cache = yield caches.open(options);
        response = yield fetch(request.href);
        yield cache.put(request, response);
      } catch (err) {
        var msg = `failed to store ${request} in ${cacheName}.`;
        console.warn(msg, err);
        throw err;
      }
      break;
    //Default passes the request to network using fetch()
    default:
      var url = request.url || request;
      var msg = `Not in cache ${cacheName}. Fetching from network: ${url}`;
      console.warn(msg);
      response = yield fetch(request);
      break;
    }
    return response;
  })
}

self.addEventListener("fetch", (ev) => {
  var key = getSwitchKeyFromURL(ev.request.url);
  var promise;
  switch (key) {
  case "pagethumbs":
    promise = CacheTasks.respondFromCache(ev.request, "thumbs_cache", "throw");
    ev.respondWith(ensureResponse(promise, ev.request.url));
    promise.catch(err => console.log(err));
    break;
  case "images":
    promise = CacheTasks.respondFromCache(ev.request, "tiles_cache", "store");
    ev.respondWith(ensureResponse(promise, ev.request.url));
    promise.catch(err => console.log(err, ev.request.url, remoteURL));
    break;
  default:
    promise = CacheTasks.respondFromCache(ev.request, "skeleton_cache");
    ev.respondWith(ensureResponse(promise, ev.request.url));
  }
});
```

Comment 3

[Marcos Caceres [:marcosc]]

Ben, we are this close (--> <--) to landing stuff that will allow me to more easily show you all this stuff. It's all coming from: https://github.com/mozilla/remote-newtab

Comment 4

[Marcos Caceres [:marcosc]]

Ok, so... first issue:

    if (response) {
      return response;
    }

should be:

    if (response) {
      return response.clone();
    }

Other issue:
```
        response = yield fetch(request.href);
```

Should have been: 

```
        response = yield fetch(request);

```

Still not fixed quite there... but having more context for the Node not Found error would have been helpful, as that's the error I keep getting.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

caches.match() is spec'd to throw NS_ERROR_DOM_NOT_FOUND_ERR if the named cache does not exist yet.  As far as I can tell there is no way for me to provide more information in the error message.

I could possibly log a message when caches.match() rejects with this error.

Comment 6

[Marcos Caceres [:marcosc]]

(In reply to Ben Kelly [:bkelly] from comment #5)
> caches.match() is spec'd to throw NS_ERROR_DOM_NOT_FOUND_ERR if the named
> cache does not exist yet.  As far as I can tell there is no way for me to
> provide more information in the error message.
> 
> I could possibly log a message when caches.match() rejects with this error.

Ah, yeah, that could also help.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> As far as I can tell there is no way for me to provide more information in the error
> message.

It's totally doable.  The spec is not spec'd to throw NS_ERROR_DOM_NOT_FOUND_ERR, which is a Mozilla implementation detail; it's specced to throw (or rather reject with) a NotFoundError exception, and in this case that means DOMException.

At this point you have several options.  If you have the Promise you're trying to reject, you can do:

  nsRefPtr<DOMException> exception = 
    DOMException::Create(NS_ERROR_DOM_NOT_FOUND_ERR, YourMessageHere);
  promise->MaybeReject(exception);

(we'd need to add an overload of MaybeReject that takes DOMException stuff; I thought we had one already...)

If you have an ErrorResult, then you can do the above, then JS-wrap the DOMException and throw it on the ErrorResult, though we should add a way to throw a DOMException directly on ErrorResult too.  If your string were static, you could just add a new nsresult for it, but you want a dynamic string...

If all you have to work with is an nsresult, then of course it's hard to communicate more info, though you could come up with one that you propagate back to where a caller has the promise it's trying to reject _and_ the name involved and then use what I suggest above.

Just logging is simpler, of course, but doesn't allow telemetry in the web page and whatnot.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I have an ErrorResult I believe with support for passing the message over IPC. So I could do the suggestion comment 7.  Sorry for my confusion, I was just looking at other examples of the notfounderror in the tree.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Oh, I also like that better because logging requires the document or do shell I think.  That may not be available from the SW global.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  Passing over IPC might take a bit more care; we'd need a way to serialize the DOMException.  But yes, apart from that it should work.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

I can do it child-side at the cost of some small code complexity to transform the error.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I was thinking about this overnight, and I think the simplest approach is:

1)  Add a way to Throw() or ThrowDOMException with an nsresult and a message on an
    ErrorResult.
2)  Add some storage in the union with mJSException/mMessage, or just reuse mMessage and
    adjust all the asserts, to store the message string.
3)  When serializing, just serialize the error code and message string, easy.
4)  When reporting, use the error code and message string to create a DOMException.

I'm happy to do this work if you'd prefer.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I'm pretty sure ErrorResult already serializes its message:

  https://dxr.mozilla.org/mozilla-central/source/dom/bindings/ErrorIPCUtils.h#45

I think Ehsan implemented that.

I don't know how that interacts with DOMException.

Comment 14

[Boris Zbarsky [:bzbarsky]]

It serializes if IsErrorWithMEssage() which is right now false for all the cases that involve creation of a DOMException.  So we'd need to either make IsErrorWithMessage() true in our new cases or change that serialization code in some other way to detect when we have a message.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Spun off the ErrorResult bits into bug 1213289.