GC: Handlify js/public/StructuredClone.h

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
These are mostly already rooted (except for one hazard), so is just a matter of pushing the handles up through the interface.
What is the signature for JS_StructuredClone itself going to be?
(Assignee)

Comment 2

5 years ago
Created attachment 821081 [details] [diff] [review]
structured_clone_rooting-js-v0.diff

Meant to post this last night, but there was some build bustage after the merge with Steve's recent changes to Serialize.

This is the SpiderMonkey half, including the new interface.
Attachment #821081 - Flags: review?(sphink)
Attachment #821081 - Flags: feedback?(continuation)
(Assignee)

Comment 3

5 years ago
Created attachment 821086 [details] [diff] [review]
structured_clone_rooting-browser-v0.diff

And the browser changes to use the handlified API.
Attachment #821086 - Flags: review?(continuation)
Comment on attachment 821081 [details] [diff] [review]
structured_clone_rooting-js-v0.diff

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

The JS API signature looks reasonable to me.
Attachment #821081 - Flags: feedback?(continuation) → feedback+
Comment on attachment 821081 [details] [diff] [review]
structured_clone_rooting-js-v0.diff

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

Ugh. Thanks for reminding me how much I dislike the current structured clone API.

::: js/src/builtin/TestingFunctions.cpp
@@ +1303,5 @@
>          return false;
>  
>      RootedValue deserialized(cx);
>      if (!JS_ReadStructuredClone(cx, obj->data(), obj->nbytes(),
> +                                JS_STRUCTURED_CLONE_VERSION, &deserialized, NULL, NULL)) {

These should be switched to nullptr. (I notice we currently have 77 NULLs and 4185 nullptrs in the tree.)
Attachment #821081 - Flags: review?(sphink) → review+
Comment on attachment 821086 [details] [diff] [review]
structured_clone_rooting-browser-v0.diff

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

This looks good to me, but you should get a DOM peer to review it.
Attachment #821086 - Flags: review?(continuation)
Attachment #821086 - Flags: review?(bugs)
Attachment #821086 - Flags: feedback+
Comment on attachment 821086 [details] [diff] [review]
structured_clone_rooting-browser-v0.diff

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

::: content/base/src/nsFrameMessageManager.cpp
@@ +416,5 @@
>                      const JS::Value& aJSON,
>                      JSAutoStructuredCloneBuffer& aBuffer,
>                      StructuredCloneClosure& aClosure)
>  {
> +  JS::Rooted<JS::Value> v(aCx, aJSON);

I'd push this root up to the callers.

Comment 8

5 years ago
Comment on attachment 821086 [details] [diff] [review]
structured_clone_rooting-browser-v0.diff

> GetParamsForMessage(JSContext* aCx,
>                     const JS::Value& aJSON,
>                     JSAutoStructuredCloneBuffer& aBuffer,
>                     StructuredCloneClosure& aClosure)
> {
>-  if (WriteStructuredClone(aCx, aJSON, aBuffer, aClosure)) {
>+  JS::Rooted<JS::Value> v(aCx, aJSON);
>+  if (WriteStructuredClone(aCx, v, aBuffer, aClosure)) {
At some point we'll need to change GetParamsForMessage to take
Handle, but I guess that could happen when we either make
messagemanager to use webidl, or make idl to use Handles.

>-  if (!buffer.write(aCx, aMessage, aTransfer, &kPostMessageCallbacks, &scInfo))
>+  JS::Rooted<JS::Value> message(aCx, aMessage);
>+  JS::Rooted<JS::Value> transfer(aCx, aTransfer);
>+  if (!buffer.write(aCx, message, transfer, &kPostMessageCallbacks, &scInfo))

Same thing here, except that caller Handlefying happens when window object is moved
to webidl I guess.


So, looks ok to me.
Attachment #821086 - Flags: review?(bugs) → review+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f6219c6fb61

(In reply to :Ms2ger from comment #7)
> Comment on attachment 821086 [details] [diff] [review]
> structured_clone_rooting-browser-v0.diff
> 
> Review of attachment 821086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsFrameMessageManager.cpp
> @@ +416,5 @@
> >                      const JS::Value& aJSON,
> >                      JSAutoStructuredCloneBuffer& aBuffer,
> >                      StructuredCloneClosure& aClosure)
> >  {
> > +  JS::Rooted<JS::Value> v(aCx, aJSON);
> 
> I'd push this root up to the callers.

It looks like there are some potential performance issues with doing that and I didn't want to get this bug side tracked in that discussion.

Perhaps there is a bug for webidling this code that we can link to so that this doesn't get dropped?
https://hg.mozilla.org/mozilla-central/rev/0f6219c6fb61
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.