Closed Bug 1201806 Opened 5 years ago Closed 5 years ago

Remove JSAutoStructuredCloneBuffer from IPC code

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(2 files)

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.
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!
Flags: needinfo?(bugs)
Blocks: 972973
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;
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.)
Flags: needinfo?(bugs)
Attachment #8656993 - Flags: review?(bugs)
Attachment #8656993 - Attachment description: others.patch → part 1 - Remove OwningSerializedStructuredCloneBuffer and use StructuredCloneIPCHelper everywhere in IPC code.
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
Attachment #8656993 - Flags: review?(bugs) → review+
Attachment #8657708 - Flags: review?(bugs)
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
Attachment #8657708 - Flags: review?(bugs) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.