1137398 - disallow creating nested workers from ServiceWorker

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

We should disallow creating nested workers in a ServiceWorker for now.  Nesting workers from SW create some interesting footguns:

1) The worker script will not be automatically persisted like the SW script and importScripts().  They would have to manually persist and then use a data URI nested worker.  This is perhaps non-intuitive.

2) The lifecylce of the nested worker would be very hard to predict.  The ServiceWorker can get shutdown by the UA at almost any time.  The dedicated worker would then seemingly get killed randomly as well.

We can fix this by changing Worker.webidl to use:

  Exposed=Window,DedicatedWorker,SharedWorker,System

Comment 1

[Giovanny Gongora [:gioyik]]

Attachment: 1137398.patch

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8570576 [details] [diff] [review]
1137398.patch

Thanks Giovanny!  This looks good to me.

Flagging :baku to review as well, as we need an official DOM peer to sign off on any webidl changes.

Giovanny, do you have permissions to push to try?  We'll need a try build before we can land.

Comment 3

[Giovanny Gongora [:gioyik]]

Yes I have, could you tell me the computed syntax for this?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Sure.  I would use this syntax:

  try: -b do -p all -u all[x64] -t none

This will build on all platforms, but only run tests on the linux64.  Its what we call a "t-shaped" try build.

For future reference you can use the trychooser to generate the syntax:

  http://trychooser.pub.build.mozilla.org/

The checkboxes for "Restrict tests to platform(s)" is the part that lets you only run tests on one platform, etc.

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8570576 [details] [diff] [review]
1137398.patch

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

looks good to me!

Comment 6

[Giovanny Gongora [:gioyik]]

Ben, check the link to treeherder:

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

Let me know if you need something else

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Once that looks green you can add the checkin-needed keyword to the bug.  Then one of the sheriffs will commit it for you.  (Unless you have L3 access and want to push yourself, of course.)

Comment 8

[Giovanny Gongora [:gioyik]]

I have no L3 access, so I will wait for green status flag.

Thanks for the quick review and feedback.

Comment 9

[Giovanny Gongora [:gioyik]]

Ben, Try server is almost done, could you check it and see if it could be landed?

Thanks

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

It looks like the failures you have are either known intermittents or infrastructure failures.  I marked them as such.

I'll add the checkin-needed flag.

Thanks!

Comment 11

[Giovanny Gongora [:gioyik]]

Thank you

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

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

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