Closed Bug 924089 Opened 11 years ago Closed 10 years ago

Enable SharedWorker by default

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
relnote-firefox --- 29+

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

(Keywords: dev-doc-needed, feature, Whiteboard: [qa-])

Attachments

(1 file)

SharedWorker is currently pref'd off by default. We need to figure out when the right time is to expose it. What sort of considerations do we need to make?

The only three big problems I see are:

1. SharedWorker cannot be created inside any other Worker/SharedWorker (i.e. all SharedWorkers must currently be top-level).
2. The MessagePort of a SharedWorker is actually a 'WorkerMessagePort' until we unify it with our existing MessagePort objects.
3. The MessagePort of SharedWorker cannot currently be passed to any other window/thread via PostMessage and the transferrable map.

Can anyone think of anything else? Are these problems enough to warrant hiding SharedWorker by default?
We still have to solve the CSP problem before turning this on IMO.
Do we support cross-process SharedWorkers?
(In reply to ben turner [:bent] (needinfo? encouraged) from comment #0)
> 1. SharedWorker cannot be created inside any other Worker/SharedWorker (i.e.
> all SharedWorkers must currently be top-level).

To be clear, Chrome has never supported this (you can't even create nested Workers). IE10 only supports Worker and nested Worker. IE11 preview doesn't have SharedWorker either as far as I can tell. So I don't think this one is a big deal.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> We still have to solve the CSP problem before turning this on IMO.

I'm not sure this is a hard blocker.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> Do we support cross-process SharedWorkers?

No.
(In reply to comment #4)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > Do we support cross-process SharedWorkers?
> 
> No.

This will be a hard-blocker for using SharedWorkers in web pages on b2g, right?
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> This will be a hard-blocker for using SharedWorkers in web pages on b2g,
> right?

Maybe... We could certainly use SharedWorker per-process with no sharing between processes. Then pages with iframes could all talk for instance. I'm not sure that that is good enough.
(In reply to comment #6)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> > This will be a hard-blocker for using SharedWorkers in web pages on b2g,
> > right?
> 
> Maybe... We could certainly use SharedWorker per-process with no sharing
> between processes. Then pages with iframes could all talk for instance. I'm not
> sure that that is good enough.

Yeah, I think it won't be, since one important use case of SharedWorker would be to share code/logic between multiple "tabs".  That won't work in Firefox OS for web pages.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #7)
> Yeah, I think it won't be, since one important use case of SharedWorker
> would be to share code/logic between multiple "tabs".

On desktop, sure. On mobile I'd say it's far less clear.

We know that Gaia needs SharedWorker for Haida, other apps might use it to communicate with their multiple windows, etc.

But I'm not really sure that there are good use cases for sharing data between multiple tabs on mobile... Especially given that we can't really hold more than a few tabs open before we run out of memory? Enabling only for apps is an option I guess but I'd rather not do that. It doesn't seem to me like it's worth holding the whole feature for this.
Hmm, yeah, there is definitely no clear cut argument for either of the two alternatives here.  Do you know if Chrome supports SharedWorkers across tabs on Android/iOS?  They are a multi-process browser at least on Android.

The other thing to note is that there are probably no significant websites out there today which use SharedWoker, and if people build up the expectation that SharedWorker will work across tabs (which should be true in our desktop and Android ports) then websites will not work properly in the Browser app, which kind of sucks. :/
> 1. SharedWorker cannot be created inside any other Worker/SharedWorker (i.e.
> all SharedWorkers must currently be top-level).

Definitely doesn't block. This is a low priority even IMO.

> 2. The MessagePort of a SharedWorker is actually a 'WorkerMessagePort' until
> we unify it with our existing MessagePort objects.
> 3. The MessagePort of SharedWorker cannot currently be passed to any other
> window/thread via PostMessage and the transferrable map.

These both will be solved once we unify MessagePorts, right? How much of that work is remaining? I wouldn't say that this strictly speaking blocks, though I think it might be worth slipping a release to get this.

> We still have to solve the CSP problem before turning this on IMO.

I'll leave this up to the security team. I suspect that this doesn't block, but they would definitely need to give the go-ahead here.

> Do we support cross-process SharedWorkers?

This doesn't block.
Sid, who should we be talking to in order to evaluate the CSP policy wrt SharedWorkers?
Flags: needinfo?(sstamm)
baku is working on the MessagePort bits I think?
Flags: needinfo?(amarchesini)
Yes, this is my todo list. I hope to finish it as soon as possible.
Flags: needinfo?(amarchesini)
bent: Garret probably has a handle on this.
Flags: needinfo?(sstamm) → needinfo?(grobinson)
There is no official consensus in the WG on how to handle Shared Workers. The spec says that if a document with a CSP creates a worker, we must apply the CSP to the worker as well. It is not clear what to do in the case of shared workers.

khuey recently initiated a discussion on the mailing list [1] to clarify, and the consensus in the responses was leaning strongly to applying CSP from the script file's headers, rather than the owning document's, and possibly changing the spec so this is true for all workers, Shared and otherwise.

I can drive this in the WG and see if we can get to a consensus for the 1.1 spec, which we are closing to closing.

[1] http://lists.w3.org/Archives/Public/public-webappsec/2013Sep/0074.html
Flags: needinfo?(grobinson)
The most immediate question is: Does doing anything for CSP in workers block turning on shared workers? If it does, then that makes getting a conclusion (and a patch) even more urgent.

Either way, definitely appreciate if you can help drive a conclusion in the WG.
Flags: needinfo?(grobinson)
(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #16)
> The most immediate question is: Does doing anything for CSP in workers block
> turning on shared workers?

Ideally we would support the current 1.0 spec for Shared Workers in the same way we do for Private Workers. There is some question over how to determine which CSP to apply to a Shared Worker; for now, we could use the CSP of the document that creates the worker, as we do for Private Workers. This is what Chrome does.

I am writing a spec diff now to clarify the behavior of CSP with respect to web workers. Even if the spec changes are approved, they will still be part of CSP 1.1 and it will be some time before we would expect them to be implemented and enabled. I think we should endeavor to apply CSP in a 1.0 spec-compliant fashion for now, and work towards achieving a clearer set of behaviors (as proprosed by khuey on the WG list) after this has landed.

Since the deadline is soon, I can take a stab at implementing this now, following the implementation of CSP for Private Workers. Should I open a separate bug to track progress?
Flags: needinfo?(grobinson)
I don't think you need to do anything to get that behavior now. The SharedWorker will currently retain the CSP from the page that first created it. Personally I think that is not what we should do but I'm happy to wait for spec guidance before changing anything.
(In reply to ben turner [:bent] (needinfo? encouraged) from comment #18)
> I don't think you need to do anything to get that behavior now. The
> SharedWorker will currently retain the CSP from the page that first created
> it.

I was hoping that would be the case, and after testing it with the tests I wrote for Chrome, that appears to be the case! Awesome. I think it would be good to update dom/workers/test/test_csp.html to test a Shared Worker as well (I can do this). Otherwise, I don't think anything in CSP is blocking Shared Workers from landing in this release.

I have a question I'm interested in getting some perspective on. At the moment, the only place where we check Workers for CSP violations is when they try to use eval. Another CSP directive that could be sensibly applied to Workers is connect-src [1]. ATM, Chrome applies connect-src to workers; Firefox doesn't. Is this something that we should file a followup bug over? (I don't think it's serious enough to block this)

[1] https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#connect-src
Let's deal with connect-src in a separate bug.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #20)
> Let's deal with connect-src in a separate bug.

bug 929292
Now that we don't have 'WorkerMessagePort' any longer I think we're ready to do this. Any objections?
What did we decide to do with the b2g browser?
"This doesn't block" from comment 10.
Sure, but are we going to leave SharedWorker disabled there?
No, I think it's fine to turn it on everywhere.
I asked the Blink and Google Docs folks about our current limitation in passing the port around.  Google Docs doesn't use that feature, so enabling this won't break Google Docs.  The Blink folks were not sure if this is something that other content relies on though.  Perhaps we can enable this for Nightly/Aurora and wait a bit to see if somebody reports a broken website?  Since the breakage will probably be in form of JS execution being aborted because of the unhandled exception, we will hopefully get bug reports.

Jonas, Ben, what do you think?
Flags: needinfo?(jonas)
Flags: needinfo?(bent.mozilla)
(Note that the Google Docs SharedWorker usage won't apply to us any way, since it uses other APIs which are Chrome only.)
I think it's unlikely that we will get useful feedback about what breaks with these changes until beta or even release.
I'm fine either way. As long as we enable on nightly/aurora we unblock the B2G cases that I'm focused on.

But of course it'd be great to get this into end user's hands. It might be ok to say that we only support passing ports around once we implement the MessageChannel API. It's a bit of a cop-out I agree, but it seems somewhat unlikely that we'll break people here.
Flags: needinfo?(jonas)
I agree with this. Let's do nightly/aurora now, b2g now (we can allow from main process only for now, that will block all content), and then fix for real as soon as our end-of-year deadlines are met.
Flags: needinfo?(bent.mozilla)
(In reply to comment #31)
> I agree with this. Let's do nightly/aurora now, b2g now (we can allow from main
> process only for now, that will block all content), and then fix for real as
> soon as our end-of-year deadlines are met.

Sounds good!
Keywords: feature
Just enabling in the parent process won't help b2g at all. All shared workers there are going to be running in the child process.
Update on CSP from the WG: we're adding spec text to specify that the policy governing a shared worker should be sent as a header with the worker script (unless the worker is being created from a URI scheme like data: or javascript:, in which case it will inherit the parent document's policy).

We also debated which CSP directive should govern valid sources of Shared Worker scripts. Currently script-src is used, but because Shared Workers are created in a separate origin and cannot directly access the parent document, Workers actually have a very different security model. We debated several options, such as including it under frame-src (iframes are similar, security-wise), or creating a new directive e.g. "child-src" that would handle iframes, Shared Workers, and possible future creations that have similar security properties. This will be discussed further on the list.
Blocks: 929292
(In reply to Jonas Sicking (:sicking) Vacation Nov23-Dec2. Please ni? if you want feedback from comment #33)
> Just enabling in the parent process won't help b2g at all. All shared
> workers there are going to be running in the child process.

Hmm, yeah, I should have pushed harder in comment 5, since I think that is basically the same problem. :(
So, what's the plan now? AFAICS, enable at least for Nightly & Aurora, and see what we get from that? Or should it run through release?

Ben, can you do this before the merge, please, so it hits Fx 28? Or is there other work to be done before we can do that?
Flags: needinfo?(bent.mozilla)
Attached patch Patch, v1Splinter Review
Attachment #8344418 - Flags: review?(ehsan)
Flags: needinfo?(bent.mozilla)
Comment on attachment 8344418 [details] [diff] [review]
Patch, v1

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

::: dom/tests/mochitest/general/test_interfaces.html
@@ +453,5 @@
>      "Selection",
>      "SettingsLock",
>      "SettingsManager",
>      {name: "ShadowRoot", pref: "dom.webcomponents.enabled"},
> +    {name: "SharedWorker", pref: "dom.workers.sharedWorkers.enabled"},

I prefer |release: false| so that we don't ship this accidentally.
I don't think that makes sense... Right now the pref is tied to RELEASE_BUILD but it will not always be. The presence of the interface is always dependent on the pref.
Comment on attachment 8344418 [details] [diff] [review]
Patch, v1

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

Note that technically test_interfaces.html changes require a DOM module peer review, so please get one.  Thanks!
Attachment #8344418 - Flags: review?(ehsan) → review+
I think we're good to turn this on in release builds too.
(In reply to Jonas Sicking (:sicking) needinfo? or feedback? encouraged. from comment #41)
> I think we're good to turn this on in release builds too.

Even without bug 929292?  (And the fact that it won't work on b2g everywhere?)
Flags: needinfo?(jonas)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #42)
> (In reply to Jonas Sicking (:sicking) needinfo? or feedback? encouraged.
> from comment #41)
> > I think we're good to turn this on in release builds too.
> 
> Even without bug 929292?  (And the fact that it won't work on b2g
> everywhere?)

Asked in a meeting, the answer to both is yes.  In that case, Ben, please get rid of the RELEASE_BUILD stuff in the patch!
Flags: needinfo?(jonas)
Bug 929292 applies equally to dedicated workers, so I don't see that it should block shared workers.

And B2G will only be affected in the browser app since other apps run in a single process. It's not great, but its also not stop-ship IMHO.

If we really are worried we could limit shared workers to apps. But there's plenty of time to do that as a followup since we just branched.
https://hg.mozilla.org/mozilla-central/rev/11f269e4597e
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Is there anything Firefox Desktop QA can help with here?
Flags: needinfo?(bent.mozilla)
(In reply to Ioana Budnar, QA [:ioana] from comment #47)
> Is there anything Firefox Desktop QA can help with here?

I'm going to assume no due to lack of response and the fact that this has in-testsuite coverage.
Flags: needinfo?(bent.mozilla)
Whiteboard: [qa-]
For information, we don't have any documentation for this feature:
https://developer.mozilla.org/en-US/docs/Web/API/SharedWorker
and marked as not "Not supported"
(In reply to Sylvestre Ledru [:sylvestre] from comment #49)
> For information, we don't have any documentation for this feature:
> https://developer.mozilla.org/en-US/docs/Web/API/SharedWorker
> and marked as not "Not supported"

Yes, hence the dev-doc-needed keyword.  :-)
Yep. It is just that we release next week and I wanted to add a link to the documentation from the release notes.
You need to log in before you can comment on or make changes to this bug.