1201806 - Remove JSAutoStructuredCloneBuffer from IPC code

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

We still have some use of JSAutoStructuredCloneBuffer in IPC code.
Would be nice to port them to StructuredCloneIPCHelper. I wrote a patch but I would also like to do:

1. move StructuredCloneIPCHelper to mozilla::ipc namespace instead mozilla::dom

2. maybe rename it to StructuredCloneIPCBuffer or something else. In the end, that class is not just an helper anymore.

Comment 1

[Andrea Marchesini [:baku]]

Smaug, this is a review request. Would be really really nice to review this code because you reviewed all the previous patches. But in case, move it to a different reviewer. Thanks!

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8656993 [details] [diff] [review]
part 1 - Remove OwningSerializedStructuredCloneBuffer and use StructuredCloneIPCHelper everywhere in IPC code.

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

::: dom/ipc/StructuredCloneIPCHelper.cpp
@@ +117,5 @@
> +  }
> +
> +  memcpy(data, mData, mDataLength);
> +  mData = data;
> +  mDataOwned = eJSAllocated;

return true;

Comment 3

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

I'll review this.

I'll reopen review queue by the end of the week.
(I'm not sure bugzilla's close-review-queue really works for me.
I would need two queues. Review queue, which is limited, say to 30 reviews per week, and then overflowing review queue where review requests go if that limit of 30 is reached.)

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - nsStructuredCloneContainer should use StructuredCloneIPCHelper

Comment 5

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

Comment on attachment 8656993 [details] [diff] [review]
part 1 - Remove OwningSerializedStructuredCloneBuffer and use StructuredCloneIPCHelper everywhere in IPC code.

>         if (aRetVal) {
>-          JSAutoStructuredCloneBuffer buffer;
>-          if (!buffer.write(cx, rval)) {
>+          ErrorResult rv;
>+          StructuredCloneIPCHelper* helper = aRetVal->AppendElement();
>+          helper->Write(cx, rval, rv);
>+          if (NS_WARN_IF(rv.Failed())) {
>             nsString msg = aMessage + NS_LITERAL_STRING(": message reply cannot be cloned. Are you trying to send an XPCOM object?");
> 
>             nsCOMPtr<nsIConsoleService> console(do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>             if (console) {
>               nsCOMPtr<nsIScriptError> error(do_CreateInstance(NS_SCRIPTERROR_CONTRACTID));
>               error->Init(msg, EmptyString(), EmptyString(),
>                           0, 0, nsIScriptError::warningFlag, "chrome javascript");
>               console->LogMessage(error);
>             }
> 
>             JS_ClearPendingException(cx);
>             continue;
>           }
>-
>-          OwningSerializedStructuredCloneBuffer* data = aRetVal->AppendElement();
>-          buffer.steal(&data->data, &data->dataLength);
This changes behavior in case of writing error.
The old code didn't end up appending unless write succeeded. I think we want that behavior.
So, call RemoveElement(helper) in the if (NS_WARN_IF(rv.Failed())) { or something like that

Comment 6

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

Comment on attachment 8657708 [details] [diff] [review]
part 2 - nsStructuredCloneContainer should use StructuredCloneIPCHelper



>   enum {
>     eNone,
>     eAllocated,
>-    eJSAllocated
>   } mDataOwned;
> };
Could you actually remove eAllocated and keep eJSAllocated

Comment 7

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef170aeae765

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa0614b93b8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9db80a5043

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6aa0614b93b8
https://hg.mozilla.org/mozilla-central/rev/0e9db80a5043