Closed Bug 896949 Opened 6 years ago Closed 6 years ago

JS_SetProperty APIs should take an immutable parameter

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(2 files, 1 obsolete file)

As suggested in bug 896540, the property value passed to JS_SetProperty() and JS_SetPropertyById() should be made immutable.
Change arguments from MutableHandleValue to HandleValue in JS engine code.
Attachment #780400 - Flags: review?(jwalden+bmo)
Change arguments from MutableHandleValue to HandleValue in JS browser code.
Attachment #780401 - Flags: review?(bzbarsky)
Attachment #780400 - Attachment description: change-set-property-arg-js → 1 - change-set-property-arg-js
Comment on attachment 780401 [details] [diff] [review]
2 - change-set-property-arg-browser

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

::: content/xbl/src/nsXBLProtoImplField.cpp
@@ +281,5 @@
>  
>    if (installed) {
>      JS::Rooted<JS::Value> v(cx,
>                              args.length() > 0 ? args[0] : JS::UndefinedValue());
> +    if (!::JS_SetPropertyById(cx, thisObj, id, v)) {

This should become JS_SetPropertyById(cx, thisObj, id, args.handleOrUndefinedAt(0))

::: dom/plugins/base/Makefile.in
@@ +1,1 @@
> +nsjs#

Uh...
Get rid of spurious change
Attachment #780401 - Attachment is obsolete: true
Attachment #780401 - Flags: review?(bzbarsky)
Attachment #780435 - Flags: review?(bzbarsky)
Comment on attachment 780400 [details] [diff] [review]
1 - change-set-property-arg-js

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

::: js/src/jsapi-tests/testRegExpInstanceProperties.cpp
@@ +55,5 @@
>           CHECK(!r.empty());
>      }
>  
>      jsval v = INT_TO_JSVAL(17);
> +    CHECK(JS_SetProperty(cx, regexpProto, "foopy", v));

...how does this even work?  jsval certainly isn't JS::Handle<JS::Value>.

::: js/src/jsapi.cpp
@@ +4307,4 @@
>  {
>      RootedObject obj(cx, objArg);
>      RootedId id(cx, idArg);
> +    RootedValue value(cx, v);

I guess the next step is to get rid of the mutable handle to setGeneric/setProperty/setElement.  Fine by me doing this incrementally, at least the publicly-used APIs will get it right, and not regress further.
Attachment #780400 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)

> ...how does this even work? 

Ah, it seems testRegExpInstanceProperties.cpp is missing from the moz.build file!
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Comment on attachment 780400 [details] [diff] [review]

Removed in the following changeset: https://hg.mozilla.org/mozilla-central/rev/48489a602029

The bug number given in that seems wrong though.
Comment on attachment 780435 [details] [diff] [review]
2 - change-set-property-arg-browser

r=me, but please fold the two changesets together before pushing; see bug 896540 comment 11.
Attachment #780435 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/51846dce90d3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.