1412852 - Structured clone of WebAssembly.Memory objects with shared memory

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Lars T Hansen [:lth]]

Closely related to 1312817 (posting WebAssembly.Module).

It is necessary to be able to post a WebAssembly.Memory in order to share memory among multiple instances (in multiple agents) of the same Module, cf the threads proposal.

Comment 1

[Lars T Hansen [:lth]]

Attachment: Smoke test

Comment 2

[Lars T Hansen [:lth]]

No DOM interaction required at all for this.

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1412852-structured-clone-wasm-shared-memory-object.patch

Structured cloning for Wasm memory objects with shared memory.  The only tricky bit here is that since a SARB can now grow we need to serialize the (constant) length of the SAB when cloning it, so that it is properly preserved.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1412852-structured-clone-wasm-shared-memory-tests.patch

Test cases for ditto.

In addition to these there's the attachment on this bug called 'smoke test' which could go into the WPT, or, if we can get a reasonable Worker/postMessage abstraction into the shell (as I hope), I'll make a shell version of that available.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1412852-wasm-shared-memory-effects.patch

The smoke test as a single program for JS shell workers (not tested yet; for safekeeping).

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1412852-sab-mailbox-carries-length.patch

We actually have two cloning mechanisms for SAB: StructuredClone, and the SAB mailbox in the JS shell.  The latter has been preserving only the SharedArrayRawBuffer but also needs to preserve the length, now that the length is stored in the SharedArrayBuffer object itself - analogous to the change in StructuredClone.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8925900 [details] [diff] [review]
bug1412852-structured-clone-wasm-shared-memory-object.patch

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

Logically it makes sense, but probably you should have an experienced StructuredClone.cpp reviewer.

::: js/src/vm/StructuredClone.cpp
@@ +1264,2 @@
>      return out.writePair(SCTAG_SHARED_ARRAY_BUFFER_OBJECT, static_cast<uint32_t>(sizeof(p))) &&
> +           out.writeBytes(&byteLength, sizeof(byteLength)) &&

Presumably a SAB can't be serialized into IDB and thus we are not changing a serialization format here that requires a version bump?

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8925900 [details] [diff] [review]
bug1412852-structured-clone-wasm-shared-memory-object.patch

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

::: js/src/jsfriendapi.h
@@ +2260,5 @@
> +/**
> + * Return true if obj is a WasmMemoryObject that holds shared memory.
> + */
> +extern JS_FRIEND_API(bool)
> +JS_IsSharedWasmMemoryObject(JSObject* obj);

Oh, also, it looks like this is only used in StructuredClone.cpp which is I think statically linked so this doesn't need to be a JS_FRIEND_API and a plain wasm::IsSharedMemoryObject() would do fine.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8926429 [details] [diff] [review]
bug1412852-sab-mailbox-carries-length.patch

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

::: js/src/shell/js.cpp
@@ +5557,5 @@
> +static struct {
> +    Mutex*                lock;
> +    SharedArrayRawBuffer* buffer;
> +    uint32_t              length;
> +} sharedArrayBufferMailbox;

How about an ExclusiveData?

@@ +5583,5 @@
>      JSObject* newObj = nullptr;
>  
>      {
> +        sharedArrayBufferMailbox.lock->lock();
> +        auto unlockMailbox = MakeScopeExit([]() { sharedArrayBufferMailbox.lock->unlock(); });

.. which would also allow an 'auto lock = sharedArrayBufferMailbox.lock()' here to take care of unlock.

Comment 10

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #7)
> Comment on attachment 8925900 [details] [diff] [review]
> bug1412852-structured-clone-wasm-shared-memory-object.patch
> 
> Review of attachment 8925900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/StructuredClone.cpp
> @@ +1264,2 @@
> >      return out.writePair(SCTAG_SHARED_ARRAY_BUFFER_OBJECT, static_cast<uint32_t>(sizeof(p))) &&
> > +           out.writeBytes(&byteLength, sizeof(byteLength)) &&
> 
> Presumably a SAB can't be serialized into IDB and thus we are not changing a
> serialization format here that requires a version bump?

Indeed that was my thinking.

Comment 11

[Lars T Hansen [:lth]]

Comment on attachment 8925900 [details] [diff] [review]
bug1412852-structured-clone-wasm-shared-memory-object.patch

Recap:

Structured cloning for Wasm memory objects with shared memory.  These Memory objects cannot be serialized to disk or persisted in other ways - just like SharedArrayBuffer cannot - so I think it is enough to keep this code internal to the JS engine and to basically handle this like we do SAB.

There's a change here to how we handle SharedArrayBuffer: Since a SharedArrayRawBuffer can now grow (via wasm) we need to also serialize the SAB length that is now held in the SAB object (independently of its SARB) when cloning the SAB, so that it is properly preserved in the clone.

(Also see comment 8, which will be addressed before landing.)

Comment 12

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8925900 [details] [diff] [review]
bug1412852-structured-clone-wasm-shared-memory-object.patch

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

::: js/src/vm/StructuredClone.cpp
@@ +1256,5 @@
>          return false;
>  
> +    // We must serialize the length so that the buffer object arrives in the
> +    // receiver with the same length, and not with the length read from the
> +    // rawbuf - that length can be different, and it can change at any time.

So how does that work? What happens if I have half a dozen in-flight clone buffers and the length shrinks to less than what is stored in those buffers?

I tried to trace it through, but I see a call to

SharedArrayBufferObject::New(context(), rawbuf, byteLength);

and I don't see that signature in SharedArrayObject.h? I only see

    // Create a SharedArrayBufferObject with a new SharedArrayRawBuffer.
    static SharedArrayBufferObject* New(JSContext* cx,
                                        uint32_t length,
                                        HandleObject proto = nullptr);

    // Create a SharedArrayBufferObject using an existing SharedArrayRawBuffer.
    static SharedArrayBufferObject* New(JSContext* cx,
                                        SharedArrayRawBuffer* buffer,
                                        HandleObject proto = nullptr);

This seems to be a different form of the latter? I'd like to understand this path before giving r+; can you explain what's expected and what's going on?

@@ +1278,5 @@
> +
> +    // If this changes, might need to change what we write.
> +    MOZ_ASSERT(WasmMemoryObject::RESERVED_SLOTS == 2);
> +
> +    Rooted<WasmMemoryObject*> memoryObj(context(), &CheckedUnwrap(obj)->as<WasmMemoryObject>());

Doesn't this need to check if memoryObj is nullptr and ReportAccessDenied(context())?

@@ +2001,5 @@
>  bool
>  JSStructuredCloneReader::readSharedArrayBuffer(uint32_t nbytes, MutableHandleValue vp)
>  {
> +    uint32_t byteLength;
> +    in.readBytes(&byteLength, sizeof(byteLength));

All of the readBytes() calls here (new and existing) should check the return value. I thought we'd done MOZ_MUST_USE on it, but it looks like not. Doing that would require fixing up at least one other place that I see. Filed bug 1417558.

If it's a pain to in.reportTruncated() here, it'd still be better to MOZ_CRASH.

Comment 13

[Lars T Hansen [:lth]]

(In reply to Steve Fink [:sfink] [:s:] from comment #12)
> Comment on attachment 8925900 [details] [diff] [review]
> bug1412852-structured-clone-wasm-shared-memory-object.patch
> 
> Review of attachment 8925900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/StructuredClone.cpp
> @@ +1256,5 @@
> >          return false;
> >  
> > +    // We must serialize the length so that the buffer object arrives in the
> > +    // receiver with the same length, and not with the length read from the
> > +    // rawbuf - that length can be different, and it can change at any time.
> 
> So how does that work? What happens if I have half a dozen in-flight clone
> buffers and the length shrinks to less than what is stored in those buffers?

The length of the SharedArrayRawBuffer cannot shrink, only grow.

> I tried to trace it through, but I see a call to
> 
> SharedArrayBufferObject::New(context(), rawbuf, byteLength);
> 
> and I don't see that signature in SharedArrayObject.h?

Sorry about that, that API is in a different patch in a now massive patch queue.  Go to bug 1389464, look in the patch "bug1389464-wasm-shared-memory-object-v2.patch".  The gist is that there's a second reserved slot on SharedArrayBufferObject that holds the length for the buffer, while the underlying SharedArrayRawBuffer may have a greater length (but the same address). 

> @@ +1278,5 @@
> > +
> > +    // If this changes, might need to change what we write.
> > +    MOZ_ASSERT(WasmMemoryObject::RESERVED_SLOTS == 2);
> > +
> > +    Rooted<WasmMemoryObject*> memoryObj(context(), &CheckedUnwrap(obj)->as<WasmMemoryObject>());
> 
> Doesn't this need to check if memoryObj is nullptr and
> ReportAccessDenied(context())?

Possibly, the wrapper machinery is something I don't know well.  Will investigate.

> @@ +2001,5 @@
> >  bool
> >  JSStructuredCloneReader::readSharedArrayBuffer(uint32_t nbytes, MutableHandleValue vp)
> >  {
> > +    uint32_t byteLength;
> > +    in.readBytes(&byteLength, sizeof(byteLength));
> 
> All of the readBytes() calls here (new and existing) should check the return
> value. I thought we'd done MOZ_MUST_USE on it, but it looks like not. Doing
> that would require fixing up at least one other place that I see. Filed bug
> 1417558.
> 
> If it's a pain to in.reportTruncated() here, it'd still be better to
> MOZ_CRASH.

OK, will look into it.

Comment 14

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #13)
> (In reply to Steve Fink [:sfink] [:s:] from comment #12)
> > Comment on attachment 8925900 [details] [diff] [review]
> > bug1412852-structured-clone-wasm-shared-memory-object.patch
> > 
> > Review of attachment 8925900 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +1278,5 @@
> > > +
> > > +    // If this changes, might need to change what we write.
> > > +    MOZ_ASSERT(WasmMemoryObject::RESERVED_SLOTS == 2);
> > > +
> > > +    Rooted<WasmMemoryObject*> memoryObj(context(), &CheckedUnwrap(obj)->as<WasmMemoryObject>());
> > 
> > Doesn't this need to check if memoryObj is nullptr and
> > ReportAccessDenied(context())?
> 
> Possibly, the wrapper machinery is something I don't know well.  Will
> investigate.

I don't think anything more is needed here.  The pattern with &CheckedUnwrap(obj)->as<Whatever>() is endemic in StructuredClone.cpp.  The assumption seems to be made everywhere that CheckedUnwrap() never returns null, and as<> cannot return null as it returns a reference (and asserts if the downcast fails); nor can it return a reference to an object whose address is null.  So memoryObj is never null.

Comment 15

[Lars T Hansen [:lth]]

Attachment: bug1412852-structured-clone-wasm-shared-memory-object-v2.patch

Updated with fixes addressing comments from Luke and Steve.  Carry Luke's f+.

Comment 16

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8929455 [details] [diff] [review]
bug1412852-structured-clone-wasm-shared-memory-object-v2.patch

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

(In reply to Lars T Hansen [:lth] from comment #14)
> I don't think anything more is needed here.  The pattern with
> &CheckedUnwrap(obj)->as<Whatever>() is endemic in StructuredClone.cpp.  The
> assumption seems to be made everywhere that CheckedUnwrap() never returns
> null

I talked to bz. It is endemic, and it's wrong. Bug 1418528. Please null-check your new uses. Although in this case, you can get away with just MOZ_ASSERT(memoryObj) because:


> and as<> cannot return null as it returns a reference (and asserts if
> the downcast fails); nor can it return a reference to an object whose
> address is null.  So memoryObj is never null.

Heh. This chain of logic is incomplete. If the downcast fails, it would assert in a DEBUG build and randomly corrupt things in an opt build. But it won't fail here because it's guarded by wasm::IsSharedWasmMemoryObject(), which does the unwrap and check (and happily, it's in this patch too.) It even does a proper null check, and given that you're already depending on the other things that IsSharedWasmMemoryObject implies, there's no need to handle the impossible null. But it should be asserted, just so it's clear from immediate context that we're checking the return value in some way.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8fb22afd5aa
Structured clone WebAssembly.Memory objects. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2f237ccf5bf
Test cases for structured cloning of wasm memory objects. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb88efd97a28
Make the JS shell SAB mailbox transmit the length. r=luke

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e8fb22afd5aa
https://hg.mozilla.org/mozilla-central/rev/a2f237ccf5bf
https://hg.mozilla.org/mozilla-central/rev/bb88efd97a28