Service Worker's AbortError lacks details.

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: marcosc, Assigned: khuey)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 unaffected, firefox42 affected, firefox43 fixed)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

When loading [1], the service worker registration rejects with an AbortError, but the abort error lacks a stack. Not having a stack means there is no context to understand what has gone wrong and where. 

Note also that the service worker registration succeeds on "http://localhost". 

[1] https://mozilla.github.io/remote-newtab/
Summary: AbortError lacks stack → Service Worker's AbortError lacks stack
A stack for what?  I see

ServiceWorker registration failed: DOMException [AbortError: "The operation was aborted. "
code: 20
nsresult: 0x80530014] remote-newtab:114:9
<anonymous> remote-newtab:114
throw() self-hosted:675
async/asyncFunction/</step/<() async.js:65

In the devtools.
Component: DOM → DOM: Service Workers
Posted image Guts of AbortError
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> A stack for what?  I see
> 
> ServiceWorker registration failed: DOMException [AbortError: "The operation
> was aborted. "
> code: 20
> nsresult: 0x80530014] remote-newtab:114:9
> <anonymous> remote-newtab:114
> throw() self-hosted:675
> async/asyncFunction/</step/<() async.js:65
> 
> In the devtools.

Hmm... maybe I just want more information then about why the operation aborted. I was expecting the AbortError to maybe indicate what line number it crapped out on, not just that it aborted.
Summary: Service Worker's AbortError lacks stack → Service Worker's AbortError lacks details.
Ok, so, the real question: "Why does the service worker abort? Why u no tell me actual reason?"
Also, I can't see anywhere in the ServiceWorker spec that an AbortError error is thrown. Maybe the spec hasn't caught up to our implementation?
We use NS_BINDING_ABORTED in dom/workers/ScriptLoader.cpp in a number of places.  In devtools network panel I don't see sw.js being downloaded at all.
It seems this:

  importScripts("lib/async.js");

Should be:

  importScripts("js/lib/async.js");

Right?  At least, that's what I have to use to load the async.js in the browser URL bar.
The load fails because https://mozilla.github.io/remote-newtab/sw.js tries to importScripts("lib/async.js") which 404s (it should be js/lib/async.js).  Then MaybeDispatchLoadFailedRunnable grabs the ServiceWorker's load failure runnable and dispatches it which is super bogus.  That code needs to check mIsWorkerScript on the ScriptExecutorRunnable.

Also the fact that none of these attempted loads show up in devtools is suboptimal.  Is that on file?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7)
> The load fails because https://mozilla.github.io/remote-newtab/sw.js tries
> to importScripts("lib/async.js") which 404s (it should be js/lib/async.js).

Correct. Thanks also Ben. 
 
> Then MaybeDispatchLoadFailedRunnable grabs the ServiceWorker's load failure
> runnable and dispatches it which is super bogus.  That code needs to check
> mIsWorkerScript on the ScriptExecutorRunnable.
> 
> Also the fact that none of these attempted loads show up in devtools is
> suboptimal.  Is that on file?

I guess that's what I want this bug to be (i.e., 404 from importScript() as well as other things that can result in "AbortError" should show up in the devtools). I think a lot of people will be caught out by these kinds of silly things.
(In reply to Marcos Caceres [:marcosc] from comment #8)
> I guess that's what I want this bug to be (i.e., 404 from importScript() as
> well as other things that can result in "AbortError" should show up in the
> devtools). I think a lot of people will be caught out by these kinds of
> silly things.

Yea.  We want that too.  Ironically, this is a bug I introduced to prevent the page from hanging when an unexpected error occurs during loading.  Sorry!
Blocks: 1181887
Comment on attachment 8653219 [details] [diff] [review]
0007-Bug-1198982-Don-t-fail-the-SW-load-for-an-importScri.patch

Review of attachment 8653219 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this. r=me
Attachment #8653219 - Flags: review?(bkelly) → review+
That test is bogus, IMO.  The spec clearly states, in the update algorithm, step 4.25.[6-7.1]

6. Invoke Run Service Worker algorithm with worker as the arguement.
7. If an uncaught runtime script error occurs during the above step, then:
   1. Reject p with the error.

importScripts is supposed to fail with a NetworkError on a non-existent script, so that's what the promise should be rejected with.
How do we get comment 14 fixed upstream?
Flags: needinfo?(james)
That test isn't upstream as such since it's in testing/web-platform/mozilla. Our plan is exactly to fix these tests to match the current spec and then upstream them.

However in general one can always just fix a web-platform-test like any other test and the fix will be upstreamed with the next sync.
Flags: needinfo?(james)
Yes, we are trying to fix the tests.  Some of these were already fixed, but not this one because we were doing the AbortError before.  Can you fix it to expect the right error type here?

  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/registration.https.html#192

This one for an uncaught exception may also be wrong:

  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/registration.https.html#163
Posted patch PatchSplinter Review
This makes importScripts throw a NetworkErr when it should, which then makes the SW code throw an AbortErr.  This isn't actually correct, but until https://github.com/slightlyoff/ServiceWorker/issues/740 is fixed this is all pretty poorly specified.  We currently propagate JS errors up but DOM exceptions are swallowed and replaced by AbortErr.
Attachment #8653219 - Attachment is obsolete: true
Attachment #8653621 - Flags: review?(bkelly)
Comment on attachment 8653621 [details] [diff] [review]
Patch

Review of attachment 8653621 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should remove ReportLoadError from WorkerPrivate.cpp.  r=me with that addition.  Thanks!

::: dom/workers/WorkerPrivate.cpp
@@ +4694,5 @@
>                                aIsChromeWorker, InheritLoadGroup,
>                                aWorkerType, stackLoadInfo.ptr());
>      if (NS_FAILED(rv)) {
> +      // XXXkhuey this is weird, why throw again after setting an exception?
> +      scriptloader::ReportLoadError(aCx, rv);

The ReportLoadError() was introduced in bug 587251.  It was originally there to handle errors from creating channels and whatnot.  In bug 643325 all that channel code was replaced with just GetLoadInfo() here.

The aRv.Throw(rv) was added in bug 919885.

It seems at this point we should be able to remove the ReportLoadError.  We report errors for channels in ScriptLoader.cpp and the aRv.Throws() reports errors synchronously back to the caller of the constructor.
Attachment #8653621 - Flags: review?(bkelly) → review+
(In reply to Ben Kelly [:bkelly] from comment #19)
> The ReportLoadError() was introduced in bug 587251.  It was originally there
> to handle errors from creating channels and whatnot.  In bug 643325 all that
> channel code was replaced with just GetLoadInfo() here.
> 
> The aRv.Throw(rv) was added in bug 919885.
> 
> It seems at this point we should be able to remove the ReportLoadError.  We
> report errors for channels in ScriptLoader.cpp and the aRv.Throws() reports
> errors synchronously back to the caller of the constructor.

I decided not to make this change here because I think we still need to do the error code remapping that ReportLoadError does.  I'll file a followup for that.
Flags: needinfo?(khuey)
You need to log in before you can comment on or make changes to this bug.