759427 - Multiprocess blob support

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This should block b2g v1 because prevents some core gaia apps from running out-of-process, that we absolutely need to have OOP.  At the least: gallery, music, video.

Comment 2

[Jan Varga [:janv]]

Ben, are we allowed to use sync messages for some specific operations ?
For example, when we need to get a new file info (or just id) during serialization of objects (which contain blobs) in a child process.

Comment 3

[Benjamin Smedberg]

You can do sync from child to parent. You cannot/may not do sync from parent to child.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, what bsmedberg said. Of course, if we can figure out a way to do all things async that might be better, but not a strict requirement.

Comment 5

[Dietrich Ayala (:dietrich)]

Blocking internal app development for B2G V1 per Jonas -> blocking+.

Comment 6

[Jan Varga [:janv]]

Talked to Ben, he has time to do this.
I'll resume work on unified quota stuff since we need both pretty soon.

Comment 7

[Dietrich Ayala (:dietrich)]

Per comment #6.

Comment 8

[Andrew Overholt [:overholt]]

Splitting blob and file handle support into separate bugs.  File handle is bug 771288.

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Also blocks MMS DOM API

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This patch adds blob/file support to the message manager, and should unblock most gaia apps (I hope!).

It has a basic test but there may be some cases I've missed.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 643036 [details] [diff] [review]
Patch, v1

cjones, the pickle changes we discussed are in here, can you review those parts? (Or maybe even the whole thing?)

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: MessageManager patch, v1.1

I fixed the pickle stuff so that it asserts our marker now.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 643061 [details] [diff] [review]
MessageManager patch, v1.1

Could you explain the patch a bit.
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIFrameMessageManager.idl#13 certainly doesn't look valid after the patch.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I mean the comment in http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIFrameMessageManager.idl#16

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Well, the comment is already a little crazy, but in so far as it is correct today it will still be correct with this patch :)

> * This is for JS only.

Still true.

> * receiveMessage is called with one parameter, which has the following
> * properties:

Still true.

> *     name:    %message name%,
> *     sync:    %true or false%.

These are unchanged.

> *     json:    %json object or null%,

This was never a JSON object, it's always been just a string. For cloned messages it's now a clone of whatever was sent (so, could be a string, could be an object, etc.).

> *     objects: %array of handles or null, always null if sync is false%

As it says below this has never been implemented and I didn't change it.

> * if the message is synchronous, possible return value is sent back
> * as JSON.

I only added an async message, so this doesn't apply.

> * When the listener is called, 'this' value is the target of the message.

Unchanged.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 643061 [details] [diff] [review]
MessageManager patch, v1.1


>+[scriptable, builtinclass, uuid(7bf4e325-0fd1-45f9-b973-f64f037072b6)]
> interface nsIFrameMessageManager : nsISupports
> {
>   void addMessageListener(in AString aMessage, in nsIFrameMessageListener aListener);
>   void removeMessageListener(in AString aMessage, in nsIFrameMessageListener aListener);
>+
>   [implicit_jscontext,optional_argc]
>   void sendAsyncMessage([optional] in AString messageName, [optional] in jsval obj);
>+
>+  [implicit_jscontext,optional_argc]
>+  void sendAsyncClonedMessage([optional] in AString messageName,
>+                              [optional] in jsval obj);
>+
What is the reason for this new method?
I was told that structured cloning is a superset of json, so
you could just change the implementation of sendAsyncMessage
(and sendSyncMessage)

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 643061 [details] [diff] [review]
MessageManager patch, v1.1

The correctness of this implementation depends on the minimum
alignment provided by the malloc impl.  It must be >= the
requested write/read alignment.

The argument is, given

 WriteAligned(char* buffer, const char* src, size_t nbytes, size_t alignment):
   (1) assert buffer % alignment == 0

   char* dest = reserveSpace(buffer, nbytes, alignment)
   (2) assert buffer % alignment == 0

   dest += alignment - dest % alignment
   (3) assert dest % alignment == 0

   memcpy(dest, src, nbytes)


 ReadAligned(const char* buffer, char** iter, char** dest, size_t nbytes, size_t alignment):
   (4) assert buffer % alignment == 0

   *dest = *iter
   *iter += computeSpace(buffer, nbytes, alignment)

   *dest += alignment - *dest % alignment
   (5) assert *dest % alignment == 0


We're given (1) by the assumption of the memory allocator.  We're
also given (2) by that --- reserving space may realloc, but we
still must have the same min alignment.

(3) is the postcondition of WriteAligned().  It follows from (1)
and (2) and the skip-bytes math.

We're given (4) by the allocator property.  The amount of space
we skip in read is the same amount we reserve in WriteAligned().
Therefore (5) follows from that and (4).

Our allocators provide a minimum alignment of 8 bytes (for
allocations > 4 bytes, which we always use here).  So we can only
support alignments of 4 or 8 bytes.  The rest of the pickle code
depends on 4-byte alignment so we can only support 4 or 8.

>diff --git a/ipc/chromium/src/base/pickle.cc b/ipc/chromium/src/base/pickle.cc

>-bool Pickle::ReadBytes(void** iter, const char** data, int length) const {
>+bool Pickle::ReadBytes(void** iter, const char** data, int length,
>+                       uint32 alignment) const {
>   DCHECK(iter);
>   DCHECK(data);
>+  DCHECK(alignment % 4 == 0) << "Must be at least 32-bit aligned!";
>+

Check that we're working within the limits of our allocator,

DCHECK(alignment == 4 || alignment == 8);

and property (4)

DCHECK(header_ % alignment == 0)

to satisfy the allocator-alignment property.

>   if (!*iter)
>     *iter = const_cast<char*>(payload());
> 
>+  uint32 offset = intptr_t(*iter) % alignment;

|padding| or |paddingLen| is probably a better name for this
 right?

>-  *data = reinterpret_cast<const char*>(*iter);
>+  *data = static_cast<const char*>(*iter) + offset;
> 

Property (5)

DCHECK(*data % alignment == 0)

>-char* Pickle::BeginWrite(uint32 length) {
>-  // write at a uint32-aligned offset from the beginning of the header
>+char* Pickle::BeginWrite(uint32 length, uint32 alignment) {
>+  DCHECK(alignment % 4 == 0) << "Must be at least 32-bit aligned!";
>+

Property (1) above:

  DCHECK(alignment == 4 || alignment == 8);
  DCHECK(header_ % alignment == 0)


>+  // write at an alignment-aligned offset from the beginning of the header
>   uint32 offset = AlignInt(header_->payload_size, sizeof(uint32));
>-  uint32 new_size = offset + AlignInt(length, sizeof(uint32));
>+  uint32 padding = (header_size_ + offset) %  alignment;
>+  uint32 new_size = offset + padding + AlignInt(length, sizeof(uint32));

This code crosses my eyes a little bit, but if you can prove it
follows the proof above, r=me.

>   if (needed_size > capacity_ && !Resize(std::max(capacity_ * 2, needed_size)))

Property (2) above: 

  DCHECK(header_ % alignment == 0)

>+  if (padding) {
>+    memset(buffer, kBytePaddingMarker, padding);
>+    buffer += padding;
>+  }
>+

Property (3) above, postcondition

  DCHECK(buffer % alignment == 0)

r=me with double-checking the padding computation in Write() and adding assertions.  Making a comment out of the proof might be worthwhile too.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 643061 [details] [diff] [review]
MessageManager patch, v1.1

I was trying to compile the patch so that I could have changed the patch
but after fixing failed hunks and other stuff there were still too
many compilation issues... so, a normal review instead.

You should effectively be able to replace the old sendAsyncMessage implementation with
your sendAsyncClonedMessage implementation, but keep the old method name.
Also, supporting sync messaging from child to parent should work the same way.
Currently all the JSObject->serialized_JSON conversion happens in 
nsFrameMessageManager::GetParamsForMessage, so I assume you could have something similar.
It would work for async and sync.


>+++ b/content/base/public/nsIFrameMessageManager.idl
>@@ -3,16 +3,17 @@
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "nsISupports.idl"
> 
> interface nsIDOMWindow;
> interface nsIDocShell;
> interface nsIContent;
>+interface nsIDOMBlob;
Not needed.

> 
> [scriptable, function, uuid(938fcb95-3d63-46be-aa72-94d08fd3b418)]
> interface nsIFrameMessageListener : nsISupports
> {
>   /**
>    * This is for JS only.
>    * receiveMessage is called with one parameter, which has the following
>    * properties:
Update this comment to say that .json is actually structured clone.
(I know, a bit silly, but perhaps too late to change that API)


>+  [implicit_jscontext,optional_argc]
>+  void sendAsyncClonedMessage([optional] in AString messageName,
>+                              [optional] in jsval obj);
Don't add this but reuse the existing method.


The code itself looks ok. If you don't have time to merge sendAsyncClonedMessage implementation to sendAsyncMessage implementation, I could do it,
if there was a patch which compiles ;)

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For the record, unless doing sync stuff is trivial, I don't think we should block on it here.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

It should be trivial.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: IndexedDB patch (WIP)

This is an implementation for IndexedDB that uses the new PBlob. It only works for object stores at this point, but I figure it's close enough to the full patch to get some feedback. Add/Put/Get/GetAll/OpenCursor are all supported.

Note that this patch doesn't do anything to "share" blobs yet, it copies things unnecessarily. We'll need to do that in a followup.

Comment 22

[Jan Varga [:janv]]

looking at the patch ...

Comment 23

[Jan Varga [:janv]]

Comment on attachment 643824 [details] [diff] [review]
IndexedDB patch (WIP)

Ben, I'm still looking at the patch, I found this so far ...

>--- a/dom/indexedDB/IDBObjectStore.cpp
>+++ b/dom/indexedDB/IDBObjectStore.cpp
>+nsresult
>+ConvertBlobActors(ContentParent* aContentParent,
>+                  FileManager* aFileManager,
>+                  const nsTArray<StructuredCloneFile>& aFiles,
>+                  InfallibleTArray<PBlobParent*>& aActors)
>+{
>+  NS_ASSERTION(aContentParent, "Null contentParent!");
>+  NS_ASSERTION(aFileManager, "Null file manager!");
>+
>+  if (!aFiles.IsEmpty()) {
>+    PRUint32 fileCount = aFiles.Length();
>+    aActors.SetCapacity(fileCount);
>+
>+    for (PRUint32 index = 0; index < fileCount; index++) {
>+      const StructuredCloneFile& file = aFiles[index];
>+      NS_ASSERTION(file.mFileInfo, "This should never be null!");
>+
>+      nsCOMPtr<nsIFile> directory = aFileManager->GetDirectory();
>+      if (!directory) {
>+        NS_WARNING("Failed to get directory!");
>+        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
>+      }

GetDirectory() call can be moved outside of the loop

>+
>+      nsCOMPtr<nsIFile> nativeFile =
>+        aFileManager->GetFileForId(directory, file.mFileInfo->Id());
>+      if (!nativeFile) {
>+        NS_WARNING("Failed to get file!");
>+        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
>+      }
>+
>+      nsCOMPtr<nsIDOMBlob> blob = new nsDOMFileFile(nativeFile);

I guess this is just a temp solution. The blob shouldn't be created this way.
Currently, GetOrCreateActorForBlob() will call blob->GetType(), blob->GetName(), size, etc.
and since the blob was initialized using only a native file, it will try to get this info form the os file
So, two problems:
1. main thread I/O, if I'm reading the code correctly, this is called from GetHelper::MaybeSendResponseToChildProcess()
2. file name will be incorrect, content type might be incorrect too, only file size will be ok

>+
>+      nsRefPtr<BlobParent> actor =
>+        aContentParent->GetOrCreateActorForBlob(blob);
>+      NS_ASSERTION(actor, "This should never fail without aborting!");
>+
>+      aActors.AppendElement(actor);
>+    }
>+  }
>+
>+  return NS_OK;
>+}
>+

Comment 24

[Jan Varga [:janv]]

Well, I think I already mentioned this to Jonas some time ago ...
I'm afraid we would have to deserialize the structured clone in the parent process too, to get this right :(

Comment 25

[Jan Varga [:janv]]

The IndexedDB patch looks great otherwise.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: no API change to MM

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Attachment: no API changes to MM

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Attachment: conservative message data serialization

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch

This is almost totally done. It incorporates all the feedback thus far and smaug's fixes. The only thing I still need to do is add the DCHECK stuff that cjones pointed out.

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 647327 [details] [diff] [review]
Patch

r? to:

smaug for message manager stuff
khuey for basic protocol and actor implementation
janv for indexeddb changes

Comment 31

[Josh Matthews [:jdm]]

Comment on attachment 647327 [details] [diff] [review]
Patch

>+run-if.config = ipc

This is not a real thing.

Comment 32

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://tbpl.mozilla.org/?tree=Try&rev=198a04dfee5d shows everything compiling properly.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v5 (no asyncSendCloneMessage)

Fixes the case when sc is empty.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v5, changes only

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 647710 [details] [diff] [review]
v5, changes only

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

::: content/base/src/nsFrameLoader.cpp
@@ +2165,5 @@
> +    if (aData.mDataLength) {
> +      if (!mData.copy(aData.mData, aData.mDataLength)) {
> +        NS_RUNTIMEABORT("OOM");
> +      }
> +      mClosure = aData.mClosure;

Unclear that this is correct... I think it's safer to always assign mClosure even if aData.mDataLength is null.

::: content/base/src/nsFrameMessageManager.cpp
@@ +176,5 @@
>    return true;
>  }
>  
> +static bool
> +GetParamsForMessage(JSContext* aCx,

I really don't like the idea of trying to clone, failing, writing JSON, then parsing JSON, and then cloning the JSON. Is this necessary to pass some test or something? Can we just not do this?

Seems like we should at least print one heck of a warning if this super slow path is reached.

@@ +437,5 @@
>          jsval json = JSVAL_NULL;
> +        if (aCloneData && aCloneData->mDataLength &&
> +            !ReadStructuredClone(ctx, *aCloneData, &json)) {
> +          JS_ClearPendingException(ctx);
> +          json = JSVAL_NULL;

Here you're eating an OOM and you're going to give script a null instead of the data it wanted. Why?

@@ +961,5 @@
> +    if (aData.mDataLength) {
> +      if (!mData.copy(aData.mData, aData.mDataLength)) {
> +        NS_RUNTIMEABORT("OOM");
> +      }
> +      mClosure = aData.mClosure;

Same comment about always setting mClosure

::: content/base/src/nsInProcessTabChildGlobal.cpp
@@ +58,5 @@
> +    if (aData.mDataLength) {
> +      if (!mData.copy(aData.mData, aData.mDataLength)) {
> +        NS_RUNTIMEABORT("OOM");
> +      }
> +      mClosure = aData.mClosure;

Here too.

::: dom/ipc/StructuredCloneUtils.h
@@ +26,5 @@
>  
>  struct
>  StructuredCloneData
>  {
> +  StructuredCloneData() : mData(nullptr), mDataLength(0) {}

Did you need this? I guess it can't hurt, but I'd be surprised if IPDL left it uninitialized somewhere.

Comment 36

[Jan Varga [:janv]]

>--- a/dom/indexedDB/IDBObjectStore.cpp
>+++ b/dom/indexedDB/IDBObjectStore.cpp
>+nsresult
>+ConvertBlobActors(ContentParent* aContentParent,
>+                  FileManager* aFileManager,
>+                  const nsTArray<StructuredCloneFile>& aFiles,
>+                  InfallibleTArray<PBlobParent*>& aActors)
>+{
>+  NS_ASSERTION(aContentParent, "Null contentParent!");
>+  NS_ASSERTION(aFileManager, "Null file manager!");
>+
>+  if (!aFiles.IsEmpty()) {
>+    PRUint32 fileCount = aFiles.Length();
>+    aActors.SetCapacity(fileCount);
>+
>+    for (PRUint32 index = 0; index < fileCount; index++) {
>+      const StructuredCloneFile& file = aFiles[index];
>+      NS_ASSERTION(file.mFileInfo, "This should never be null!");
>+
>+      nsCOMPtr<nsIFile> directory = aFileManager->GetDirectory();
>+      if (!directory) {
>+        NS_WARNING("Failed to get directory!");
>+        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
>+      }

GetDirectory() call can be moved outside of the loop

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

(In reply to ben turner [:bent] from comment #35)
> Comment on attachment 647710 [details] [diff] [review]
> v5, changes only
> 
> Review of attachment 647710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsFrameLoader.cpp
> @@ +2165,5 @@
> > +    if (aData.mDataLength) {
> > +      if (!mData.copy(aData.mData, aData.mDataLength)) {
> > +        NS_RUNTIMEABORT("OOM");
> > +      }
> > +      mClosure = aData.mClosure;
> 
> Unclear that this is correct... I think it's safer to always assign mClosure
> even if aData.mDataLength is null.
Can aData.mClosure contain something if mDataLength is 0 ?


> > +GetParamsForMessage(JSContext* aCx,
> 
> I really don't like the idea of trying to clone, failing, writing JSON, then
> parsing JSON, and then cloning the JSON. Is this necessary to pass some test
> or something?
Yes, t was some contacts test, IIRC.

> Can we just not do this?
I choose the safest possible way (which is very ugly).



> 
> Seems like we should at least print one heck of a warning if this super slow
> path is reached.
Yes

> @@ +437,5 @@
> >          jsval json = JSVAL_NULL;
> > +        if (aCloneData && aCloneData->mDataLength &&
> > +            !ReadStructuredClone(ctx, *aCloneData, &json)) {
> > +          JS_ClearPendingException(ctx);
> > +          json = JSVAL_NULL;
> 
> Here you're eating an OOM and you're going to give script a null instead of
> the data it wanted. Why?
Well, presumably the data is coming from another process, like content process which is possibly evil.
So if it sends invalid data, no need to throw exceptions in another process.


> >  StructuredCloneData
> >  {
> > +  StructuredCloneData() : mData(nullptr), mDataLength(0) {}
> 
> Did you need this? I guess it can't hurt, but I'd be surprised if IPDL left
> it uninitialized somewhere.
Again, I added this when hunting the problem which ended up being 0 length sc.
But I was somewhat horrified to see uninitialized member variables.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #37)

> > Here you're eating an OOM and you're going to give script a null instead of
> > the data it wanted. Why?
> Well, presumably the data is coming from another process, like content
> process which is possibly evil.
> So if it sends invalid data, no need to throw exceptions in another
But you're right, returning would be the right thing to do.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

I think the need to use json is actually a bug in contactsmanager. I'll ask gwagner.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

So in contacts there is interface nsIDOMContactFindOptions which is passed as part
of the message. json can somewhat serialize it (if the JS implementation of the interface doesn't
have all the properties, they become "undefined" or "0").

nsIDOMContactFindOptions shouldn't be an interfaces at all, but a dictionary, but I'm not sure
whether implementing methods which take a dictionary as a parameter is actually supported.
Investigating...

But, for now supporting json messages should be ok (even if it is a bit slower)

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v7

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v7, changes

this misses the return NS_OK change in ReceiveMessage, but that is in the full patch.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 647327 [details] [diff] [review]
Patch

So implicit r+ within my patches

Comment 44

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 647934 [details] [diff] [review]
v7

These changes look good.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

ben, could you upload the patch which fixes IDB tests (the one which passed in ipc-mode, but not
in non-ipc-mode)

Comment 46

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Final patch

This is the final patch. Incorporates all previous patches and review comments from everyone.

Comment 47

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, I will changes nsnull -> nullptr here before I land.

Comment 48

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Final patch landed here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4048c64af6de

I did have to make some changes to nsBlobProtocolHandler (simply moved nsBlobURI out into its own file so that it can be used in nsLayoutModule.cpp), those have rs=sicking.

Comment 49

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4048c64af6de

Comment 50

[[e-mail]]

Comment on attachment 648148 [details] [diff] [review]
Final patch

>+  NS_ENSURE_TRUE(JS_ParseJSON(aCx, static_cast<const jschar*>(PromiseFlatString(json).get()),
>+                              json.Length(), &val), false);
[json is already flat, so no need to PromiseFlatString it. But this is Ms2ger's fault for not using BeginReading in the first place.]

Comment 51

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to [e-mail] from comment #50)
> Comment on attachment 648148 [details] [diff] [review]
> Final patch
> 
> >+  NS_ENSURE_TRUE(JS_ParseJSON(aCx, static_cast<const jschar*>(PromiseFlatString(json).get()),
> >+                              json.Length(), &val), false);
> [json is already flat, so no need to PromiseFlatString it. But this is
> Ms2ger's fault for not using BeginReading in the first place.]

Troll much? The .get() has been there ever since bug 549682 if you feel like you need to assign blame.

Comment 52

[Philipp von Weitershausen [:philikon]]

Comment on attachment 648148 [details] [diff] [review]
Final patch

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

::: content/base/public/nsIFrameMessageManager.idl
@@ +23,3 @@
>     *     name:    %message name%,
>     *     sync:    %true or false%.
> +   *     json:    %structured clone of the sent message data%,

itym

  data: ...

here?

Sorry, somewhat late to the party. I'm actually in that file so I'll correct that as part of the process.