Closed Bug 1187470 Opened 4 years ago Closed 4 years ago

service worker scripts treat parser warnings as errors

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: bkelly, Assigned: baku)

References

Details

Attachments

(1 file)

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.
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/
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...
Flags: needinfo?(bkelly)
Its ok to call HandleError() for a real error.  But we need to check for warnings and not call HandleError() in those cases.
Flags: needinfo?(bkelly)
Attached patch error.patchSplinter Review
Attachment #8647028 - Flags: review?(bkelly)
Assignee: nobody → amarchesini
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?
Attachment #8647028 - Flags: review?(bkelly) → review?(ehsan)
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.
Attachment #8647028 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/8c3d49b034c1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Duplicate of this bug: 1195664
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
Attachment #8647028 - Flags: approval-mozilla-aurora?
Comment on attachment 8647028 [details] [diff] [review]
error.patch

We want service workers to work correctly for our developers, taking it.
Attachment #8647028 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.