1232973 - Prevent shared memory from being sent into or out of SharedWorker and ServiceWorker

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Lars T Hansen [:lth]]

The semantics of shared memory with SharedWorker or ServiceWorker are themselves not problematic, I think, but with e10s in the mix it's going to be complicated (see bug 1069316).  For the purposes of letting SAB+Atomics riding the trains we should make sure it's not possible to transmit shared memory objects except among plain Workers and a main thread all residing in the same process.

(This is analogous to bug 1231333 preemptively disallowing futexWait on the main thread until there's agreement on what to do there.)

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

FWIW, SharedWorker currently always runs in the same process.  In multiple content process e10s configurations you actually get separate SharedWorkers per child process.  Not sure if we will fix that in the future or not.

For ServiceWorker, though, we are definitely moving the worker thread out into a separate process.  That work will happen in Q1.

So it seems reasonable to implement this to me.

Comment 2

[Lars T Hansen [:lth]]

It looks like this will take the form of a flag to JSAutoStructuredCloneBuffer::write() that says "it is ok to transfer stuff that can't be copied" which means SharedArrayBuffer and mapped-memory ArrayBuffer.  The latter objects are created by some XHR code in workers and are transfered by the JS engine StructuredClone mechanism, but there appears to be no guard on cross-process transfering, which would *probably* be wrong.  (I could be wrong about that, eg, if the mappings are always read-only then the memory could be copied just fine, but I've yet to find code that performs such copying in that eventuality; there are also other ways I could be wrong.)

The flag to JSAutoStructuredCloneBuffer is probably just going to be mSupportedContext!=DifferentProcess in the StructuredCloneHolder, ie, we'll have a little API churn in the JS engine but not much code churn in the DOM code.

Then there needs to be some sane error handling in the DOM code, which I assume is in place already but perhaps we can report a useful error.

Comment 3

[Lars T Hansen [:lth]]

Attachment: WIP

This is uninvasive, probably just about right, and it passes my test cases.  I need to figure out whether to worry about mapped-memory ArrayBuffer before offering it for review, however.

Comment 4

[Lars T Hansen [:lth]]

With better test cases: That patch takes care of SharedWorkers but not ServiceWorkers.  With the move to a separate process that might just take care of itself as the context for cloning to ServiceWorkers will become DifferentProcess, but it would be nice to know for sure.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1232973-part1-structured-clone.patch

Make structured clone take a flag that will cause it to throw for non-serializable data (currently only SharedArrayBuffer).

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1232973-part2-narrow-transfer.patch

Generalize the notion of TransferringSupported to distinguish between situations where any object can be transferred and only serializable objects can be transferred, and then make interactions to and from service workers and shared workers use the latter method.  This effectively prevents SharedArrayBuffers from being transferred into and out of those worker types.

Test case for ServiceWorker:
https://lars-t-hansen.github.io/ecmascript_sharedmem/tests/test_serviceWorkerNotSharing.html

Test case for SharedWorker:
http://lars-t-hansen.github.io/ecmascript_sharedmem/test-html/test_sharedWorkerNotSharing.html

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8704169 [details] [diff] [review]
bug1232973-part2-narrow-transfer.patch

Andrea has been all over structured clone recently, he can take this.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8704169 [details] [diff] [review]
bug1232973-part2-narrow-transfer.patch

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

2 things:
. Can you add a test?
. Instead adding a new TransferSupport type, can you use the context? In the end you want to support the sharedBuffer only if the context is SameProcess.

::: dom/base/StructuredCloneHolder.cpp
@@ +177,5 @@
>  bool
>  StructuredCloneHolderBase::Write(JSContext* aCx,
>                                   JS::Handle<JS::Value> aValue,
> +                                 JS::Handle<JS::Value> aTransfer,
> +                                 bool onlySerializable)

aOnlySerializable

::: dom/base/StructuredCloneHolder.h
@@ +88,5 @@
>    // Like Write() but it supports the transferring of objects.
>    bool Write(JSContext* aCx,
>               JS::Handle<JS::Value> aValue,
> +             JS::Handle<JS::Value> aTransfer,
> +             bool onlySerializable);

aOnlySerializable

Comment 9

[Lars T Hansen [:lth]]

(In reply to Andrea Marchesini (:baku) from comment #8)
> Comment on attachment 8704169 [details] [diff] [review]
> bug1232973-part2-narrow-transfer.patch
> 
> Review of attachment 8704169 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 2 things:
> . Can you add a test?

Test cases are referenced above (they are in the semi-official repo for this feature and will remain available and will probably move with the spec when that moves to Ecma's github repo) and one of them requires an https server for the serviceWorker case, but I can probably add a test for SharedWorker if you think that's adequate.

> . Instead adding a new TransferSupport type, can you use the context? In the
> end you want to support the sharedBuffer only if the context is SameProcess.

That's not the same thing.  The point here is to disallow memory from being shared with ServiceWorker and SharedWorker regardless of whether they are implemented same-process or different-process.  It is true (see discussion above) that ServiceWorker will be moved out-of-process but for SharedWorker that does not seem to be the case, or at least it's open, and I don't know what's going to happen when e10s is disabled, either.

Comment 10

[Lars T Hansen [:lth]]

After IRC discussion with :baku and some more thinking:

1) If we were to move the current patch from a TransferSupport type to a Context type, we'd want to create a new Context type that describes the contexts that we're trying to communicate with, consider something like "SameProcessDifferentThreadOnlyDedicatedWorkers" for example.

2) But the context problem is a little broader than the current patch makes it.  In addition to sending a SAB to/from ServiceWorker and SharedWorker, we need to contend with MessageChannels (where the receiver may not be visible to the sender and hence throwing an exception at the sender is not an option) and there's also postMessage between windows (a doc to a nested frame and vice versa, as one example).  The shared memory spec does not address the latter issues yet.

(A similar issue to MessageChannels has come up separately in the context of the JS StructuredClone code, where the receiver may have disabled shared memory without that being detectable to the sender; if anyone signals an error it must be the receiver.)

Not that it has any real bearing on the matter, but presumably in the window-to-window case it's possible for the two windows to be running in separate processes.

I expect that the only use case that anyone is truly interested in supporting initially is for memory to be shared between a window and its (possibly nested) dedicated workers.  Everyone would be happy if we blocked everything else initially, including window-to-window communication.  (I'm saying this as somebody who does not do much web development, so I could be wrong about that, but "go slow" has served us well so far.)  I think that locking down sharing to that level should be the goal of the work in this bug.

Comment 11

[Lars T Hansen [:lth]]

Comment on attachment 8704157 [details] [diff] [review]
bug1232973-part1-structured-clone.patch

Withdrawing r? for the time being, until we can agree on design parameters.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Hey Lars, we are going to need this for multiple content processes. Would you be interested in working on this bug again?

Comment 13

[Lars T Hansen [:lth]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)

> Hey Lars, we are going to need this for multiple content processes. Would
> you be interested in working on this bug again?

In principle yes, but until mid-August it'll have to be a ways down my priority list so it's not likely that I'll get anything done on it before then.  When do you need it?  :)

Comment 14

[Lars T Hansen [:lth]]

A closely related patch landed in bug 1302036: it implements restrictions on structured clone serialization, so that the serializing side can express restrictions on whether it should be possible to SC a SharedArrayBuffer.  It is expressed in terms of a policy object, JS::CloneDataPolicy, which carries attributes expressing the desired restrictions.

Comment 15

[Lars T Hansen [:lth]]

Pertinent fact:

* A SharedWorker can create nested dedicated Workers, but a ServiceWorker cannot

To summarize:

* When a Window or a SharedWorker posts a SAB, the receipient must be one of the poster's
  dedicated workers (or their dedicated workers).  Specifically, if a Window creates a SharedWorker
  it cannot post a SAB to that SharedWorker, and a SharedWorker cannot post a SAB to any
  Window that references it.

* When a Worker posts a SAB, the recipient must be one if the Worker's nested dedicated workers,
  or the Worker's parent (which can be a Window or a SharedWorker)

* A ServiceWorker must not be allowed to post a SAB to anywhere

* The cloning Context is not really the concern here, because while DifferentProcess implies 
  no sharing, we want the sharing prohibitions to be independent of that -- that a SharedWorker
  is always in-process with the agent that created it, say, is an implementation artifact we
  must ignore (unless this is in a spec somewhere).

* The MessageChannel makes it impossible to know the eventual destination of a cloned value.  While it
  is probably desirable to be able to send shared memory on a MessageChannel (so long as the recipient
  is otherwise valid), it is probably acceptable for now to just disallow sending shared memory on
  MessageChannels, if that makes anything simpler.  Fixing this problem will probably require some
  kind of filter on MessageChannel messages as they are moved between processes.

In short, it looks to me like we can (in a restricted but acceptable implementation that prohibits the use of MessagePort) always know at the sender whether shared memory is acceptable.  This probably means disabling cloning of SharedArrayBuffer in nsGlobalWindow.cpp and ServiceWorkerPrivate.cpp.  Comprehensive test case coming up.

Comment 16

[Lars T Hansen [:lth]]

The changes that landed in bug 1302036 ought to be sufficient to prevent us ever from trying to transfer a SAB cross-process.
The changes landing in bug 1359461 we will release-assert if there's an attempt to do that.  There is work to be done here to allow SABs always to be transfered when that should be allowed and to prevent it when it shouldn't (document -> shared worker) and to properly signal error (bug 1359017); the details here depend on pending changes to the HTML spec and there are test cases to go along with those changes that will ferret out problems.

In short, I don't think this bug is actually blocking e10s-multi any longer.

Comment 17

[Lars T Hansen [:lth]]

So far as I can tell, this is "fixed" because our overly restrictive mechanism for transmitting SharedArrayBuffer (we disallow it to be posted on MessagePort / MessageChannel) prevents any sharing from happening.

There are test cases that are landing with the changes to HTML that incorporate SharedArrayBuffer in structured cloning, etc, that will guide us in the proper implementation of that, and which will catch improper sharing as well.