1525330 - Audit all places where a SharedArrayBuffer can be passed

Status: RESOLVED DUPLICATE of bug 1575425

(Core :: DOM: Core & HTML, task, P2)

Description

[Anne (:annevk)]

Our code has some branching on SharedArrayBuffer where it'll throw, as implemented in https://hg.mozilla.org/mozilla-central/rev/91fe35e7df1fa3124c266c249ad0907320dfd956, but this lacks at least XMLHttpRequest on the main thread and might also lack APIs added since then.

Given that SharedArrayBuffer has been disabled it's in general not clear how well all of this is maintained.

Implementing [AllowShared] in IDL and using that would help here us move to a somewhat less fragile setup.

Note that the plan is to unconditionally enable SharedArrayBuffer in due course and require opt-in for cross-thread usage (i.e., usage that would allow for creating a timer). This means that places where do intentionally allow it, as in WebGL, require a further audit to see if they require this additional opt-in as well.

(See also https://github.com/heycam/webidl/issues/638 for some further ideas on potentially refining [AllowShared].)

Comment 2

[Lars T Hansen [:lth]]

Notably see bug 1231687 which is about adding [AllowShared].

Comment 3

[Lars T Hansen [:lth]]

Also, bug 1225033 probably wants a slight priority boost, since DOM that accesses shared memory will otherwise trigger UB that might upset TSAN, if nothing else. The non-UB functions are now in the engine so this is "only" a matter of exporting them from the engine API and using them everywhere DOM accesses shared memory.

Comment 4

[Anne (:annevk)]

Luke and I went through the code and couldn't figure out why main thread XMLHttpRequest ends up throwing, but

const buffer = new SharedArrayBuffer(8),
      view = new Uint8Array(buffer),
      client = new XMLHttpRequest();
client.open("POST", "/");
client.send(view);

throws NS_ERROR_ILLEGAL_VALUE so this bug might be less problematic than it appears. It's still a standards compliance issue as a TypeError is expected.

We also suspect that where we do accept SharedArrayBuffer (seems to be only WebGL?) it's only used as an input (is that correct for WebGL, Jeff?), meaning no writes happen and no opportunities for creating a high-resolution timer exist.

(We're considering adding SharedArrayBuffer support to the Encoding Standard, including writing to them, but that's being done with full knowledge of these new considerations: https://github.com/whatwg/encoding/issues/172.)

Comment 5

[Kelsey Gilbert [:jgilbert]]

I think we can output to SABs as well (ReadPixels).

Comment 6

[Anne (:annevk)]

Looking at https://www.khronos.org/registry/webgl/specs/latest/1.0/index.html#readpixels (the 2.0 version doesn't seem too different) that shouldn't be problematic high-resolution-timer-wise. Bug 1225033 might be relevant though.

Comment 7

[Anne (:annevk)]

We discussed this again and it might be problematic after all as the rate of writing can be observed. What we discussed was to have

• [AllowSharedRead] (or potentially reuse [AllowShared] for these semantics)
• [AllowSharedWrite]

to be able to explicitly annotate APIs with whether they can write to the passed in buffer. And those where we write to the buffer would have to check the agent cluster's high-resolution timer flag, similar to postMessage().

bz, this should be doable now bug 1359269 is fixed, right? Or are there other blockers apart from the standardization discussion? (Could you mark those as blocking if so?)

Comment 8

[Anne (:annevk)]

Actually, reflecting on this some more, I'm not sure why we changed our opinion here. If the high-resolution timer flag isn't set you don't have multiple event loops to observe the underlying buffer from. So readPixels() will appear atomic to the user in those cases, right? It's only not atomic if there's another thread that can observe the underlying pixels, but all that is gated on the high-resolution timer flag.

If that's correct, it would still be useful to know from bz how hard it is to add [AllowShared], as that would give us more correctness and improve auditing somewhat, but changing its design seems less needed.

Comment 9

[Luke Wagner [:luke]]

Oh hah, good point! The original source of concern, which is still valid, was if the write to the SAB by the host happened off the main thread, e.g., as proposed in https://github.com/whatwg/streams/issues/757. However, that's a very specific case that I don't think exists currently so I agree there is no need for [AllowSharedWrite] now. \o/

Comment 10

[Boris Zbarsky [:bzbarsky]]

bz, this should be doable now bug 1359269 is fixed, right?

I think so, yes. It should be reasonably straightforward to do the parser bits, and probably even simpler to do the codegen bits.... Figure a day or two total including writing tests for someone who's worked on bindings before.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Luke and I went through the code and couldn't figure out why main thread XMLHttpRequest ends up throwing

It does? I just tested using that testcase from comment 4 and I don't get an exception on http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=6851 if I set "javascript.options.shared_memory" true in about:config....

And yes, I agree that code inspection also suggests it should not throw.

Comment 12

[Anne (:annevk)]

I'm pretty positive it did throw back then; not sure how else I would've gotten NS_ERROR_ILLEGAL_VALUE. If SharedArrayBuffer is not enabled you get a different error on a different line. (I do also recall Luke suggesting it looked like it would be treated as empty.)

Either way, I think this suggests we should add IDL support for robustness, even if we decide correctness is not a priority.

Comment 13

[Anne (:annevk)]

It seems that passing a SharedArrayBuffer behaves similar to passing a detached buffer, i.e., it acts as if you pass the empty byte sequence: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=7045 (replace with ArrayBuffer and compare the output). Chrome does throw an exception here at the right time.

That's probably okay, but far from ideal.

Comment 14

[Anne (:annevk)]

Tom and I realized that this non-standard SharedArrayBuffer handling also affects one of the tests in bug 1562667 comment 23 in that rather than throwing an empty buffer of sorts is passed.

Comment 15

[Anne (:annevk)]

Making this no longer block the main resab bug to give that less direct dependencies. Edgar and I expect that bug 1575425 will address this bug as well as SharedArrayBuffer objects can only be passed through IDL annotated as such from then on and only APIs defined through IDL are affected. Leaving this open until that bug is fixed and Edgar can fully confirm this is the case.