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

NEW
Unassigned

Status

()

Core
DOM
2 years ago
2 years ago

People

(Reporter: marcosc, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected)

Details

(Reporter)

Description

2 years ago
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

2 years ago
What actually produced this promise and what rejected it with that error?
(Reporter)

Comment 2

2 years ago
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));
  }
});
```
(Reporter)

Comment 3

2 years ago
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
(Reporter)

Comment 4

2 years ago
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

2 years ago
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.
Blocks: 1110136
Component: DOM: Service Workers → DOM
Summary: Service Worker respondWith "NotFoundError: Node was not found" is unhelpful → Cache API caches.match() could log bad cache name
(Reporter)

Comment 6

2 years ago
(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.
> 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

2 years ago
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.
Summary: Cache API caches.match() could log bad cache name → Cache API caches.match() with bad cache name could reject with a better message

Comment 9

2 years ago
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.
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

2 years ago
I can do it child-side at the cost of some small code complexity to transform the error.
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

2 years ago
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.
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.
Depends on: 1213289
Spun off the ErrorResult bits into bug 1213289.

Updated

2 years ago
See Also: → bug 1241045
You need to log in before you can comment on or make changes to this bug.