disallow creating nested workers from ServiceWorker

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: bkelly, Assigned: gioyik, Mentored)

Tracking

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment)

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
Posted patch 1137398.patchSplinter Review
Attachment #8570576 - Flags: review?(bkelly)
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.
Attachment #8570576 - Flags: review?(bkelly)
Attachment #8570576 - Flags: review?(amarchesini)
Attachment #8570576 - Flags: review+
Yes I have, could you tell me the computed syntax for this?
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
Comment on attachment 8570576 [details] [diff] [review]
1137398.patch

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

looks good to me!
Attachment #8570576 - Flags: review?(amarchesini) → review+
Ben, check the link to treeherder:

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

Let me know if you need something else
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.)
I have no L3 access, so I will wait for green status flag.

Thanks for the quick review and feedback.
Ben, Try server is almost done, could you check it and see if it could be landed?

Thanks
Flags: needinfo?(bkelly)
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!
Flags: needinfo?(bkelly)
Keywords: checkin-needed
Thank you
https://hg.mozilla.org/mozilla-central/rev/fe48916662d1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.