1225470 - waitUntil() promise rejection still not reported to console

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

We still have issues where we don't report anything to the console when a waitUntil() promise rejects.  In some cases, like install event, this is a fatal error and blocks the service worker from going forward.  In other cases, like activate and fetch events, there is no negative side effect of the rejection, but the developer should still probably know about it.

We should report these rejections.

I'd like to get this into 44 aurora, but I'm not going to block the release on it.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Report a message to the console when a service worker waitUntil() is rejected. r=baku

Tested locally.  Note, I plan to improve the message in 45, but I can't uplift l10n string changes to 44 any more.

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8689288 [details] [diff] [review]
Report a message to the console when a service worker waitUntil() is rejected. r=baku

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

::: dom/workers/ServiceWorkerEvents.cpp
@@ +759,5 @@
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +
> +  explicit WaitUntilHandler(JSContext* aCx)
> +    : mWorkerPrivate(GetCurrentThreadWorkerPrivate())

This should be received the the callee.

@@ +768,5 @@
> +    mWorkerPrivate->AssertIsOnWorkerThread();
> +
> +    // Save the location of the waitUntil() call itself as a fallback
> +    // in case the rejection value does not contain any location info.
> +    nsJSUtils::GetCallingLocation(aCx, mSourceSpec, &mLine, &mColumn);

this can fail.. in theory.

@@ +847,2 @@
>  {
>    MOZ_ASSERT(!NS_IsMainThread());

Then we are in a worker thread. Can you take WorkerPrivate here, add an assertion, and pass the workerPrivate to the WaitUntilHandler?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini (:baku) from comment #2)
> @@ +768,5 @@
> > +    mWorkerPrivate->AssertIsOnWorkerThread();
> > +
> > +    // Save the location of the waitUntil() call itself as a fallback
> > +    // in case the rejection value does not contain any location info.
> > +    nsJSUtils::GetCallingLocation(aCx, mSourceSpec, &mLine, &mColumn);
> 
> this can fail.. in theory.

If it does, then the location info will be empty.  If the rejection value does not contain any location info either, then the console report is not shown.  I think this is the best we can do.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b082fa04452a

Comment 5

[Pulsebot]

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8689288 [details] [diff] [review]
Report a message to the console when a service worker waitUntil() is rejected. r=baku

Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: This ensures certain script errors are reported to the console for developers trying to use service workers.
[Describe test coverage new/current, TreeHerder]: Local testing.
[Risks and why]: Minimal.  Only affects service workers.
[String/UUID change made/needed]: None.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c18e0cc2b208

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Ben, normally the SW patches you request for uplift have new tests in them. Just wondering, whether you plan to add automated tests for this one later. Thanks!

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8689288 [details] [diff] [review]
Report a message to the console when a service worker waitUntil() is rejected. r=baku

SW is planned for 44, taking it.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3493d5fba50e

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3493d5fba50e

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

I wrote a follow-on bug to add a test.  See bug 1229156.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Ben Kelly [:bkelly] from comment #12)
> I wrote a follow-on bug to add a test.  See bug 1229156.

Nice! That's awesome. Thank you for keeping the quality bar high.