Closed Bug 1137398 Opened 6 years ago Closed 6 years ago

disallow creating nested workers from ServiceWorker


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

Not set



Tracking Status
firefox39 --- fixed


(Reporter: bkelly, Assigned: gioyik, Mentored)



(Whiteboard: [good first bug])


(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:

Attached patch 1137398.patchSplinter Review
Attachment #8570576 - Flags: review?(bkelly)
Comment on attachment 8570576 [details] [diff] [review]

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:

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]

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

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

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?

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.

Flags: needinfo?(bkelly)
Keywords: checkin-needed
Thank you
Closed: 6 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.