1198982 - Service Worker's AbortError lacks details.

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Marcos Caceres [:marcosc]]

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/

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 2

[Marcos Caceres [:marcosc]]

Attachment: Guts of AbortError

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 3

[Marcos Caceres [:marcosc]]

Ok, so, the real question: "Why does the service worker abort? Why u no tell me actual reason?"

Comment 4

[Marcos Caceres [:marcosc]]

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?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 8

[Marcos Caceres [:marcosc]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(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!

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: 0007-Bug-1198982-Don-t-fail-the-SW-load-for-an-importScri.patch

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9edb5fe8fb9

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f0729dbb87db for w-p-t failures like https://treeherder.mozilla.org/logviewer.html#?job_id=13350038&repo=mozilla-inbound

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

How do we get comment 14 fixed upstream?

Comment 16

[James Graham [:jgraham]]

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.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

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.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87fdb49dca74

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78acfe74868a

Comment 23

[Ryan VanderMeulen [:RyanVM]]

You have wpt failures as well.
https://treeherder.mozilla.org/logviewer.html#?job_id=13474324&repo=mozilla-inbound

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5969dac170da

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Thanks for fixing that up.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87fdb49dca74
https://hg.mozilla.org/mozilla-central/rev/78acfe74868a
https://hg.mozilla.org/mozilla-central/rev/5969dac170da