896540 - GC: Convert JS_SetProperty* to take MutableHandleValue

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

To fix rooting analysis warnings about unsafe references, JS_SetProperty and JS_SetPropertyById can be converted to take a MutableHandleValue for their out parameter rather than a JS::Value*.

There are whole bunch of references to these functions in the browser, and most of them already root the value.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Why is this a MutableHandleValue, not a HandleValue?

Comment 2

[Jon Coppeard (:jonco)]

Attachment: convert-set-property

Changes to js/src

Comment 3

[Jon Coppeard (:jonco)]

Attachment: convert-set-property-browser

Changes in the rest of the browser

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Boris Zbarsky (:bz) from comment #1)

I don't know why this is, but it's mutable all the way through the system, so I'm preserving the existing behaviour.

Comment 5

[Boris Zbarsky [:bzbarsky]]

It's a Value* because of 1) rooting concerns and 2) To give get and set ops the same signature, iirc.

I suspect that in practice there is no need for it to be mutable at the API level, and would be very interested in us moving away from that.

Followup is probably fine for that, but we should do that followup.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 779253 [details] [diff] [review]
convert-set-property

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

r=me

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Boris Zbarsky (:bz) from comment #5)

Right, that sounds like good idea.  I've raised bug 896949 for this.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 779254 [details] [diff] [review]
convert-set-property-browser

> +++ b/dom/bluetooth/BluetoothUtils.cpp

Ugh.  Clearly we're not running the static analysis on this file.  :(  Want to just fix the aObj hazard too by making it a handle and using a Rooted in the callers?  Also, maybe move the decl of jsData into the block where it's actually used (you'll need to add curlies for that case of the switch) so it's clearly not a rooting hazard?  Followup is ok for this part.

> +++ b/dom/camera/CameraRecorderProfiles.h
>-    JSObject* video;
>+    JS::Rooted<JSObject*> object(aCx);

I'd prefer we keep the existing naming (and the separate stack variables for "video" and "audio").

r=me with those nits

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Boris Zbarsky (:bz) from comment #8)

Cheers, I'll just go ahead and fix the BluetoothUtils.cpp rooting in this bug.

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60f09edcad4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8de6a56d2797

Comment 11

[Boris Zbarsky [:bzbarsky]]

For what it's worth, it's better to fold patches like this together, since the browser doesn't build with just one of them pushed and that makes bisecting suck....

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60f09edcad4f
https://hg.mozilla.org/mozilla-central/rev/8de6a56d2797

Comment 13

[Florian Scholz (Open Web Docs)]

The docs look updated to me (thanks, :arai!)
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_SetProperty

Technical review appreciated.