929800 - GC: Handlify js/public/StructuredClone.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

These are mostly already rooted (except for one hazard), so is just a matter of pushing the handles up through the interface.

Comment 1

[Andrew McCreight [:mccr8]]

What is the signature for JS_StructuredClone itself going to be?

Comment 2

[Terrence Cole [:terrence]]

Attachment: 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.

Comment 3

[Terrence Cole [:terrence]]

Attachment: structured_clone_rooting-browser-v0.diff

And the browser changes to use the handlified API.

Comment 4

[Andrew McCreight [:mccr8]]

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.

Comment 5

[Steve Fink [:sfink] [:s:]]

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.)

Comment 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

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

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.

Comment 9

[Terrence Cole [:terrence]]

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?

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f6219c6fb61