Closed Bug 896540 Opened 6 years ago Closed 6 years ago

GC: Convert JS_SetProperty* to take MutableHandleValue

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jonco, Assigned: jonco)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

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.
Why is this a MutableHandleValue, not a HandleValue?
Summary: GC: Convert JS_SetPrototype* to take MutableHandleValue → GC: Convert JS_SetProperty* to take MutableHandleValue
Changes to js/src
Attachment #779253 - Flags: review?(terrence)
Changes in the rest of the browser
(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.
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 on attachment 779253 [details] [diff] [review]
convert-set-property

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

r=me
Attachment #779253 - Flags: review?(terrence) → review+
(In reply to Boris Zbarsky (:bz) from comment #5)

Right, that sounds like good idea.  I've raised bug 896949 for this.
Attachment #779254 - Flags: review?(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
Attachment #779254 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #8)

Cheers, I'll just go ahead and fix the BluetoothUtils.cpp rooting in this bug.
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....
https://hg.mozilla.org/mozilla-central/rev/60f09edcad4f
https://hg.mozilla.org/mozilla-central/rev/8de6a56d2797
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 773686
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.