Structured clone of WebAssembly.Memory objects with shared memory

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

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.
Posted file Smoke test
No DOM interaction required at all for this.
Component: DOM: Workers → JavaScript Engine
Priority: P2 → P1
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.
Attachment #8925900 - Flags: review?(luke)
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.
Attachment #8925902 - Flags: review?(luke)
The smoke test as a single program for JS shell workers (not tested yet; for safekeeping).
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.
Attachment #8926429 - Flags: review?(luke)
Summary: Must be able to postMessage WebAssembly.Memory objects to workers → Structured clone of WebAssembly.Memory objects with shared memory
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?
Attachment #8925900 - Flags: review?(luke) → feedback+
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.
Attachment #8925902 - Flags: review?(luke) → review+
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.
Attachment #8926429 - Flags: review?(luke) → review+
(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 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.)
Attachment #8925900 - Flags: review?(sphink)
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.
(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.
(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.
Updated with fixes addressing comments from Luke and Steve.  Carry Luke's f+.
Attachment #8925900 - Attachment is obsolete: true
Attachment #8925900 - Flags: review?(sphink)
Attachment #8929455 - Flags: review?(sphink)
Attachment #8929455 - Flags: feedback+
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.
Attachment #8929455 - Flags: review?(sphink) → review+
Pushed by lhansen@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.