1480012 - Make JS shell SAB mailbox accept Wasm memories and modules as well

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P1)

Description

[Lars T Hansen [:lth]]

The shell uses a single-element mailbox mechanism for transmitting a SAB from one agent to others, see GetSharedArrayBuffer and SetSharedArrayBuffer in shell/js.cpp.

This mechanism is currently restricted to SharedArrayBuffer values but it should be generalized to WebAssembly memory objects so that we can run cross-thread wasm tests in the shell.

(Structured clone already handles memory objects.)

Comment 1

[Lars T Hansen [:lth]]

Structured cloning can already handle WebAssembly modules as well (not in the JS shell, but via browser hooks).  The mailbox mechanism should really handle modules too.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1480012-generalize-mailbox.patch

The paranoia around reference count overflow exhibited in this patch is a result of a security issue we had where it was possible, through nefarious use of structured cloning, to overflow the refcount on the SharedArrayRawBuffer.  In principle it is possible to overflow the refcount on the Module by just reading the mailbox a large number of times, creating a lot of WasmModuleObjects.  Since we're in the shell this does not matter, but in the interest of hygiene I've done the error checking anyway.

Comment 3

[Lars T Hansen [:lth]]

Attachment: worker-threads.js

Well, this crashes the shell so something must be running.  The problem seems to be looking up wasm protos in the shell worker.  Investigating.

In general we've had problems in the past with shell workers because they are just running in the same memory as the parent thread; they don't always have proper mutual exclusion on data structures that would not be shared in the browser but are shared here.

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8996670 [details] [diff] [review]
bug1480012-generalize-mailbox.patch

OK, cancelling review because there are several gremlins on the shell worker side having to do with proto initialization and probably other things.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1480012-generalize-mailbox-v2.patch

Triggered a minor footgun: must remember to explicitly ensureConstructor for the WebAssembly object to ensure that prototypes are created.  Now works as it should with test case attached to this bug (for illustration).

Comment 6

[Lars T Hansen [:lth]]

Attachment: worker-threads.js

TC evolved and cleaned up; will land under different bug#; for illustration only.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8996712 [details] [diff] [review]
bug1480012-generalize-mailbox-v2.patch

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

::: js/public/RefCounted.h
@@ +76,5 @@
> +            uint32_t oldCount = mRefCnt;
> +            uint32_t newCount = oldCount + 1;
> +            if (newCount > INT32_MAX)
> +                return false;
> +            if (mRefCnt.compareExchange(oldCount, newCount))

Since MozRefCountType is a uintptr_t, I think the ref-count shouldn't be able to overflow.  If that's right, then let's remove this hunk.

::: js/src/shell/js.cpp
@@ +5740,5 @@
> +//
> +// These object types are shareable:
> +//
> +//   - SharedArrayBuffer
> +//   - WasmMemoryObject (only when mapping shared memory)

nit: perhaps "(when constructed with shared:true)"

@@ +5854,5 @@
> +            if (mbx->tag == MailboxTag::SharedArrayBuffer) {
> +                newObj = maybesab;
> +            } else {
> +                if (!GlobalObject::ensureConstructor(cx, cx->global(), JSProto_WebAssembly))
> +                    return false;

missing dropReference() on this failure path.  How about doing it instead with a MakeScopeExit() instead?

@@ +7079,5 @@
> +"  may be null.  setSharedObject performs an ordering memory barrier.\n"),
> +
> +    JS_FN_HELP("getSharedArrayBuffer", GetSharedObject, 0, 0,
> +"getSharedArrayBuffer()",
> +"  Obsolete alias for getSharedObject().\n"),

Are there so many test-cases that they can't be updated to the new name?

Comment 8

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #7)
> Comment on attachment 8996712 [details] [diff] [review]
> bug1480012-generalize-mailbox-v2.patch
> 
> Review of attachment 8996712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/RefCounted.h
> @@ +76,5 @@
> > +            uint32_t oldCount = mRefCnt;
> > +            uint32_t newCount = oldCount + 1;
> > +            if (newCount > INT32_MAX)
> > +                return false;
> > +            if (mRefCnt.compareExchange(oldCount, newCount))
> 
> Since MozRefCountType is a uintptr_t, I think the ref-count shouldn't be
> able to overflow.  If that's right, then let's remove this hunk.

It is indeed uintptr_t, but observe that AddRef() checks that it never overflows int32_t, so that inconsistency needs to be resolved too.

Even then "shouldn't be able to overflow" is a weak defense.  The refcount on the SharedArrayRawBuffer shouldn't have been able to overflow either, but even so a bug elsewhere (a missing decref along an error path) allowed it to.

> @@ +7079,5 @@
> > +"  may be null.  setSharedObject performs an ordering memory barrier.\n"),
> > +
> > +    JS_FN_HELP("getSharedArrayBuffer", GetSharedObject, 0, 0,
> > +"getSharedArrayBuffer()",
> > +"  Obsolete alias for getSharedObject().\n"),
> 
> Are there so many test-cases that they can't be updated to the new name?

Some of the code using the old names is upstream in the test262 repo, in the SpiderMonkey support for atomics tests.  I think what we need to do here is land the new names and then start looking for places we can update, it may take a while to make the transition.  I will create a couple more patches here to update the names locally, but upstreaming the change will require some care.

Comment 9

[Lars T Hansen [:lth]]

Attachment: bug1480012-generalize-mailbox-v3.patch

Addresses all review comments except for uses in test262-related files of the old names.

Asking for feedback on the updates to the refcounting code.  I've added a static_assert on the size and cleaned up the run-time assertions a bit, and removed the CAS loop.  Let me know what you think.

Comment 10

[Lars T Hansen [:lth]]

Comment on attachment 8996986 [details] [diff] [review]
bug1480012-generalize-mailbox-v3.patch

Carrying Luke's r+ from previous version.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8996986 [details] [diff] [review]
bug1480012-generalize-mailbox-v3.patch

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

Ah, great catch and fix regarding lingering i32 assumptions.  Didn't know about getSharedArrayObject() in test262; makes sense.

::: js/src/shell/js.cpp
@@ +5845,5 @@
>              if (!buf->addReference()) {
>                  JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_SC_SAB_REFCNT_OFLO);
>                  return false;
>              }
> +            auto dropBuf = MakeScopeExit([buf]() { buf->dropReference(); });

I just recently learned (from grepping existing MakeScopeExit() uses to see if it was still the "turn a lambda into an RAII guard" of the day) you can elide empty parens of lambdas.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61310ac63cfd
Generalize JS shell mailbox to Wasm memories and modules.  r=luke

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/61310ac63cfd