1567483 - No concept of agents enforced for SharedArrayBuffer

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P3)

Description

[Anne (:annevk)]

I've discovered that we don't implement the specification-prescribed boundary for SharedArrayBuffer objects, in particular when it comes to documents.

This might have to be enforced somehow on the DOM side short-term (and if so, feel free to move to DOM: Core & HTML), but I wanted to file it here to make the JavaScript team aware of this issue.

In particular, if A embeds cross-site B, they can still share memory in Firefox. This is a violation of the HTML Standard and not forward-compatible with Fission. You can find a web-platform-tests test here:

• https://github.com/web-platform-tests/wpt/pull/17909

I think we could solve this by when serializing SharedArrayBuffer objects we also serialize a reference to the tab group and the current "site" and do a comparison when deserializing, but ideally we'd have actual concepts of agents and agent clusters we could do comparisons with. (There are upcoming proposals where within a tab group a site might have multiple agent clusters at which point this would become critical.)

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a enhancement, but please change it back in case of error.

Comment 2

[Jason Orendorff [:jorendorff]]

The particular rule that we don't implement is step 12.1 of StructuredDeserialize:

[...]

12. Otherwise, if serialized.[[Type]] is "SharedArrayBuffer", then:

    1. If targetRealm's corresponding agent cluster is not serialized.[[AgentCluster]], then then throw a "DataCloneError" DOMException.

Lars is probably the best person to take a look at this.

Comment 3

[Lars T Hansen [:lth]]

Lars is unlikely to get around to looking at this for quite a long time, so if it's important it should go to somebody else.

Comment 4

[Jason Orendorff [:jorendorff]]

Well, we should probably implement this restriction before shipping SharedArrayBuffer. Luke, are we planning to ship it?

Comment 5

[Anne (:annevk)]

We are planning to ship it, but as mentioned in OP this could be implemented without new infrastructure in the JavaScript engine and maybe we should go that way for now. However, if agent clusters start getting used for more things and with different keys it would really help to have dedicated support.

Either way, it'll need to apply to SharedArrayBuffer and WebAssembly.Module objects as they both have an agent cluster restriction (see https://webassembly.github.io/spec/web-api/#serialization). I think those are the only objects that make agent clusters web-observable for now.

Comment 6

[Luke Wagner [:luke]]

Does the JS engine even have any reified notion of agent cluster? I would think this needed to be implemented in Gecko using the appropriate first-class notion of agent cluster (docgroup?). IIRC, baku wrote and understand the relevant Gecko bits of structured (de)serialization where this could be enforced.

Comment 7

[Andrea Marchesini [:baku]]

We have a concept of "TabGroup" and a cocept of "DocGroup", yes, but they are not kept in consideration by StructuredCloneHolder. There are a couple of things we could do here:

1. Find a way to have unique IDs per TabGroup and DocGroup and serialize these IDs into the StructuredCloneHolder buffer.
2. When deserializing, we should check if the docGroup ID matches with the serialized one and pass a isSameCluster boolean to the JS engine.

Note that we need to support workers as well. Each worker should inherit the docGroup/tabGroup of the parent context. We should check what serviceWorker should use as a docGroup and TabGroup.

Andrew, is it something you can work on?

Comment 8

[Luke Wagner [:luke]]

IIUC, with the "same agent cluster" restriction, we never need to serialize a SAB (or WebAssembly.Module) across process boundaries (IIRC, we simply fail on any attempt to do cross-process structured serialization of a SAB now), so we don't have a hard requirement to get a unique/serializable ID. Instead, just as we hold a pointer to the (ref-counted) SAB raw buffer inside the structured clone buffer, it seems like we could hold pointers to the DocGroup. (I don't know if this is actually easier, since, when holding a pointer, one has to worry about lifetime/ref-counting, but if DocGroup allows thread-safe ref-counting, maybe it is, so I just thought I'd point out this option.)

Comment 9

[Tom Tung [:tt, :ttung]]

I happen to be investigating having the agent cluster in Gecko these days and maybe I can work on this.

The remaining thing I'm not so sure is when it comes to service workers / shared workers v.s. other globals. I'll schedule a meeting to get some advice from :asuth about that.

ni myself to remind me of this issue

Comment 10

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #8)

IIUC, with the "same agent cluster" restriction, we never need to serialize a SAB (or WebAssembly.Module) across process boundaries (IIRC, we simply fail on any attempt to do cross-process structured serialization of a SAB now), so we don't have a hard requirement to get a unique/serializable ID. Instead, just as we hold a pointer to the (ref-counted) SAB raw buffer inside the structured clone buffer, it seems like we could hold pointers to the DocGroup. (I don't know if this is actually easier, since, when holding a pointer, one has to worry about lifetime/ref-counting, but if DocGroup allows thread-safe ref-counting, maybe it is, so I just thought I'd point out this option.)

The case that keeps coming up is that a serialized SAB can be hidden inside an opaque datum that can flow cross-process and then come back to the originator, notably a message port object being broadcast in such a way that agents not in the cluster can see it and broadcast it back. In this case a SAB or a module can flow indirectly cross process and should (in an ideal world) be extractable from the port without an error when the container comes back to the originating process. I'd love dearly if this case turned out not to be possible, so that I can stop bringing it up, but I've yet to have anyone tell me so ;-( Anyway, to support this without cross-process shared memory and without leaking orphaned SABs we need some kind of interesting cross-process reference counting /notification scheme, and some kind of reified value for agent cluster, probably, that is meaningful cross-process.

Comment 11

[Lars T Hansen [:lth]]

Jason, this strikes me as JS : General and not a Wasm bug? I realize WebAssembly.Memory was mentioned, but the SC algorithm and SAB are both in JS, and the fact that the logic applies to a Memory is a fallout from the Memory using a SAB. (Anyway, blocks a P3 bug so is P3.)

Comment 12

[Andrew Overholt [:overholt]]

(In reply to Lars T Hansen [:lth] from comment #11)

(Anyway, blocks a P3 bug so is P3.)

The meta is P3 just because we traditionally mark meta bugs as P3.

Do we really this for resab?

Comment 13

[Lars T Hansen [:lth]]

I don't know that this truly blocks shipping SAB, as Anne said we might for the near future be able to hack something into the DOM to make it safe enough, without breaking important use cases. It's hard for me to say.

Comment 14

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #9)
Will probably do that on bug 1583251

Comment 15

[Anne (:annevk)]

Given https://github.com/whatwg/html/issues/4920 we probably want to enforce this mostly outside the JavaScript engine for the foreseeable future. It probably still makes sense to have agent clusters as a thing at some point due to their presence in the ECMAScript standard, but no rush.

Comment 16

[Anne (:annevk)]

Agent clusters are now implemented in DOM and meet the needs for resab (and soon Fission). Marking this as WORKSFORME therefore. If there's interest at some point in having the JavaScript engine manage this we can reevaluate.