Closed Bug 1137398 Opened 10 years ago Closed 10 years ago

disallow creating nested workers from ServiceWorker

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bkelly, Assigned: gioyik, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file)

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
Attached patch 1137398.patch (deleted) — Splinter 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
Blocks: 1137816
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
Status: NEW → RESOLVED
Closed: 10 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.

Attachment

General

Created:
Updated:
Size: