803783 - Use structured clone instead of JSON strings for SendSyncMessage return value

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[[:fabrice] Fabrice Desré]

Currently this has only been implemented for sendAsyncMessage

Comment 1

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

Olli, how much work would this be? And would this be safe enough to take on aurora?

Comment 2

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

Specifically, what B2G need is the ability to send a structured clone in the response to a syncronous message.

Comment 3

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

Ben should know better how difficult this would be to implement.
(But I'll try to remember to look at this tomorrow.)

Comment 4

[Matthew N. [:MattN]]

So I guess this is the next e10s papercut on my list. Luckily this limitation is documented (albeit without a bug #) at [1] so I didn't waste much time because of this. The comment should be updated as part of this bug.

Olli/Ben, is this something that can be easily fixed in the next week or so? Or should I workaround this in some code I'm writing?

[1] https://mxr.mozilla.org/mozilla-central/source/dom/base/nsIMessageManager.idl?rev=b76c5d271fea&mark=196-197#172

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

Getting the structured clone IPC to work was a little tricky. We already have SerializedStructuredCloneBuffer, which we use for sending the "data" part of the message. However, SerializedStructuredCloneBuffer doesn't own its buffer. When I tried to use it for the result, I ran into this problem:

When sending a message, the generated IPDL code for SendSyncMessage gets a Message* as a reply. We use ParamTraits to read the SerializedStructuredCloneBuffer out of the Message*. SerializedStructuredCloneBuffer borrows part of the Message's data for its own data. Then, when SendSyncMessage returns, the reply Message* is deallocated and the SerializedStructuredCloneBuffer we get back contains garbage.

So I created this type OwningSerializedStructuredCloneBuffer that copies out the data instead. Desktop tests are green with this patch, but I'm still waiting for a full try run.

Comment 6

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

Comment on attachment 8600225 [details] [diff] [review]
patch

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

::: dom/base/nsFrameMessageManager.cpp
@@ +686,5 @@
>    if (aArgc >= 3 && aObjects.isObject()) {
>      objects = &aObjects.toObject();
>    }
>  
> +  InfallibleTArray<OwningSerializedStructuredCloneBuffer> retval;

So... Since you're touching all of these anyway, how about just making them all nsTArray? InfallibleTArray is awful.

@@ +711,1 @@
>        return NS_ERROR_UNEXPECTED;

Seems like we should have something stronger here... MOZ_ASSERT? NS_WARNING? Because this shouldn't ever fail unless a child process gets hacked and sends us garbage.

@@ +1194,1 @@
>              continue;

This is a little weird (granted, the code did this before). But shouldn't we insert a |undefined| or something into the array here?

::: ipc/glue/IPCMessageUtils.h
@@ +97,5 @@
> +  explicit OwningSerializedStructuredCloneBuffer(const JSAutoStructuredCloneBuffer& aOther)
> +   : SerializedStructuredCloneBuffer(aOther)
> +  {}
> +
> +  virtual ~OwningSerializedStructuredCloneBuffer()

SerializedStructuredCloneBuffer doesn't have a virtual destructor, and it doesn't look like you're inheriting it in any other classes, so I don't understand why making this destructor virtual is useful.

@@ +100,5 @@
> +
> +  virtual ~OwningSerializedStructuredCloneBuffer()
> +  {
> +    if (data) {
> +      js_free(data);

Might as well use plain malloc/free in this non-js code.

@@ +779,5 @@
> +  typedef mozilla::OwningSerializedStructuredCloneBuffer paramType;
> +
> +  static void Write(Message* aMsg, const paramType& aParam)
> +  {
> +    ParamTraits<mozilla::SerializedStructuredCloneBuffer>::Write(aMsg, aParam);

Maybe just inherit ParamTraits<SerializedStructuredCloneBuffer> so you don't have to implement Write/Log methods?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1eec08c595b

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1eec08c595b

Comment 9

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

Comment on attachment 8600225 [details] [diff] [review]
patch

>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    if (!ParamTraits<mozilla::SerializedStructuredCloneBuffer>::Read(aMsg, aIter, aResult)) {
>+      return false;
>+    }
>+
>+    if (aResult->data) {
>+      uint64_t* data = static_cast<uint64_t*>(js_malloc(aResult->dataLength));
>+      if (!data) {
>+        return false;
>+      }
>+      memcpy(data, aResult->data, aResult->dataLength);
>+      aResult->data = data;
>+    }
So other comments[1] say that data for structured cloning needs to be 64 aligned, and 
ParamTraits<mozilla::SerializedStructuredCloneBuffer>::Read does ensure that for aResult->data, but
then we clone the data to a new buffer. What guarantees the alignment there?
Maybe I'm missing something here.



[1] https://hg.mozilla.org/releases/mozilla-release/annotate/c77dd0d90ddb/ipc/glue/IPCMessageUtils.h#l759

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

My understanding is that malloc is supposed to return memory that is aligned "suitable for any type of object" [1]. So it should be aligned for uint64_t.

Also, it looks like this code is no longer in the tree.

[1] https://msdn.microsoft.com/en-us/library/ycsb6wwf.aspx

Comment 11

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

That code has just moved, but it is still there.

But ok thanks, if malloc really guarantees proper alignment that makes my life easier 
(while optimizing some of mm message passing)

Comment 12

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

Apparently malloc isn't guaranteed to allocate with 64 bit alignment in case the allocation is small.
But I guess we're dealing with >64bits allocations here, or I'll just ensure we allocation at least sizeof(uint64_t) or something

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The size will always be a multiple of sizeof(uint64_t):
http://mxr.mozilla.org/mozilla-central/source/js/src/vm/StructuredClone.cpp#722
I guess this don't prove it's not zero. But I don't think it can be zero.

Comment 14

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

And if it is zero, then presumably we would never write to it and so the alignment wouldn't matter.

Comment 15

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

I was talking about malloc in general ;)  Whoever deals with ipc layer shouldn't need to look at
js engine implementation details.


(Anyhow, I'll consider alignment issue when trying to reduce the memory copies we do while sending message manager messages)