Remove JSAutoStructuredCloneBuffer from IPC code

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

4 years ago
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.
Assignee

Comment 1

4 years ago
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)
Assignee

Updated

4 years ago
Blocks: 972973
Assignee

Comment 2

4 years ago
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)
Assignee

Updated

4 years ago
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+
Assignee

Updated

4 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/6aa0614b93b8
https://hg.mozilla.org/mozilla-central/rev/0e9db80a5043
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.