1187470 - service worker scripts treat parser warnings as errors

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently we call ServiceWorkerManager::HandleError() whenever a WorkerPrivate ReportErrorRunnable is executed.  This is not quite right because these runnables are also fired for warnings.

We should check for the warnings flag here before calling HandleError():

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#1697

You can see this in action by going to Flaki's website here:

  https://flaki.github.io/bpg2jpg/test-sw.html

Its failing because asmjs uses warnings to log that compilation took place, etc.

Comment 1

[Szmozsánszky István [:flaki]]

From IRC:
> <bz> (anonymous namespace)::ReportErrorRunnable::WorkerRun
> <bz> calls
> <bz> mozilla::dom::workers::ServiceWorkerManager::HandleError
> <bz> calls
> <bz> mozilla::dom::workers::ServiceWorkerRegisterJob::Fail> <bz> https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#1696
> <bz> There
> <bz> calling https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#2783
> <bz> Which ignores aFlags

Reproduced error message can be seen here (from a DEBUG build): https://www.irccloud.com/pastebin/ObOIWwTN/

Comment 2

[Andrea Marchesini [:baku]]

Actually we have a test for this: dom/workers/test/serviceworkers/test_strict_mode_error.html
This has been introduced by bug 1170550
But maybe this bug is about something else...

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Its ok to call HandleError() for a real error.  But we need to check for warnings and not call HandleError() in those cases.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: error.patch

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8647028 [details] [diff] [review]
error.patch

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

I'm unsure about this one now.  I think its pretty clear we should not fail on warnings, but its unclear why we previously thought the warning in strict_mode_error.js was actually an error that should stop execution.  Is this from when we thought about making service workers strict only?

Ehsan,  what do you think?  Should we allow service worker scripts with warnings to execute?  And if not, why not?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8647028 [details] [diff] [review]
error.patch

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

After talking to a number of people in the #whatwg channel I'm convinced this is correct.

::: dom/workers/test/serviceworkers/test_strict_mode_error.html
@@ +19,5 @@
>    function runTest() {
>      navigator.serviceWorker
>        .register("strict_mode_error.js", {scope: "strict_mode_error"})
> +      .then(() => {
> +        ok(true, "Registration should not fail for warnings");

Can you rename the files to something about "warning" instead of strict_mode_error?  These files don't actually have any errors in them.

Comment 7

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c3d49b034c1

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c3d49b034c1

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8647028 [details] [diff] [review]
error.patch

Approval Request Comment
[Feature/regressing bug #]: valid javascript code that produces warnings (like asmjs and babel) fails to run in service workers
[User impact if declined]: developers cannot use standard js libraries in service workers while handling push events
[Describe test coverage new/current, TreeHerder]: mochitests and web-platform-tests
[Risks and why]: minimal
[String/UUID change made/needed]: none

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8647028 [details] [diff] [review]
error.patch

We want service workers to work correctly for our developers, taking it.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/10446b876d60