Brandon Benvie [:benvie] Reporter
	Description • 11 years ago

Signature: Object.setPrototypeOf(O, proto)

This is pretty much identical to the `Object.prototype.__proto__` setter.

See ES6 spec (May 2013 draft) section 15.2.3.2.

	Tom S [:evilpie]
	Comment 2 • 10 years ago

We should implement this. It seems easy enough and v8 just landed it: http://code.google.com/p/v8/source/detail?r=18685.

	Jason Orendorff [:jorendorff]
	Comment 4 • 10 years ago

Comment on attachment 8364329 [details] [diff] [review]
patch v1

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

::: js/src/builtin/Object.cpp
@@ +545,5 @@
> +    JSObject::isExtensible(cx, obj, &extensible);
> +    if (!extensible) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_OBJECT_NOT_EXTENSIBLE, "object");
> +        return false;
> +    }

Hmm. I don't think it's necessary to check isExtensible here. Doesn't JSObject::setProto do this for us?

@@ +554,5 @@
> +
> +    if (!success) {
> +        args.rval().setUndefined();
> +        return false;
> +    }

The spec says, "7. If status is false, then throw a TypeError exception." Returning undefined would definitely be a bug.

I think this pretty much only happens if the object is inextensible, so just move the error-reporting code you added above down here.

::: js/src/jit-test/tests/basic/setPrototypeOf.js
@@ +1,4 @@
> +load(libdir + 'asserts.js');
> +
> +function getObjects() {
> +    function func(){};

Nit: no semicolon here.

@@ +9,5 @@
> +            [1, 2, 3],
> +            new Date(),
> +            new Number(1),
> +            new Boolean(true),
> +            new String('str')];

Throw Object.create(null) in here, please.

@@ +28,5 @@
> +}
> +
> +// check if Object.setPrototypeOf works with coercible values
> +for(var i = 0; i < coercibleValues.length; i++) {
> +    var value = coercibleValues[i];

You can write:

    for (var value of coercibleValues) {

@@ +31,5 @@
> +for(var i = 0; i < coercibleValues.length; i++) {
> +    var value = coercibleValues[i];
> +    assertThrowsInstanceOf(function() {
> +        Object.getPrototypeOf(value);
> +    }, TypeError, "");

This is redundant -- you assert the same thing a few lines below.

@@ +53,5 @@
> +var objects = getObjects();
> +for (var i = 0; i < objects.length; i++) {
> +    var object = objects[i];
> +    for (var j = 0; j < valuesWithoutNull.length; j++) {
> +        var proto = valuesWithoutNull[j];

for (var object of getObjects()) {
        for (var proto of valuesWithoutNull) {
            ....

	Sankha Narayan Guria [:sankha] Assignee
	Comment 5 • 10 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Hmm. I don't think it's necessary to check isExtensible here. Doesn't
> JSObject::setProto do this for us?

setProto checks if the object is extensible or not but doesn't throw a TypeError exception (which I think is correct according to the spec's [[SetPrototypeOf]] (V) section.

But V8's implementation checks for a TypeError when trying to use Object.setPrototypeOf on inextensible object. Is that correct?

	Tom S [:evilpie]
	Comment 6 • 10 years ago

Comment on attachment 8364329 [details] [diff] [review]
patch v1

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

::: js/src/builtin/Object.cpp
@@ +526,5 @@
> +static bool
> +obj_setPrototypeOf(JSContext *cx, unsigned argc, Value *vp) {
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (args[0].isNullOrUndefined()) {

You can't access these without making sure that args.length() >= 2.

@@ +537,5 @@
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_NOT_AN_OBJECT);
> +        return false;
> +    }
> +
> +    RootedObject obj(cx, ToObject(cx, args[0]));

The spec says something about:
4. If Type(O) is not Object, then return O.

In any case this is missing a null check.

You should really use the usual // Step annotation.

@@ +541,5 @@
> +    RootedObject obj(cx, ToObject(cx, args[0]));
> +    RootedObject newProto(cx, args[1].toObjectOrNull());
> +
> +    bool extensible;
> +    JSObject::isExtensible(cx, obj, &extensible);

if (!JSObject::isExtensible(cx, obj, &extensible))
 return false;

	Jason Orendorff [:jorendorff]
	Comment 7 • 10 years ago

Sankha, the first bug evilpie noted is testable, so please add tests:

assertThrowsInstanceOf(() => Object.setPrototypeOf(), TypeError);
assertThrowsInstanceOf(() => Object.setPrototypeOf({}), TypeError);


(In reply to Tom Schuster [:evilpie] from comment #6)
> > +    RootedObject obj(cx, ToObject(cx, args[0]));
> 
> The spec says something about:
> 4. If Type(O) is not Object, then return O.
> 
> In any case this is missing a null check.

Right. This is an OOM case so it's not easily testable.

	Jason Orendorff [:jorendorff]
	Comment 8 • 10 years ago

(In reply to Sankha Narayan Guria [:sankha93] from comment #5)
> (In reply to Jason Orendorff [:jorendorff] from comment #4)
> > Hmm. I don't think it's necessary to check isExtensible here. Doesn't
> > JSObject::setProto do this for us?
> 
> setProto checks if the object is extensible or not but doesn't throw a
> TypeError exception (which I think is correct according to the spec's
> [[SetPrototypeOf]] (V) section.

Yes, that's correct. setProto will set succeeded to false. This is equivalent to [[SetPrototypeOf]] returning false in step 5.

According to the specification, we must check in step 7 whether this happened, and if it did, throw a TypeError.

> But V8's implementation checks for a TypeError when trying to use
> Object.setPrototypeOf on inextensible object. Is that correct?

I'm sure what V8 is doing is right, but let's talk about testable behavior and the specification, not V8.

	Jason Orendorff [:jorendorff]
	Comment 10 • 10 years ago

Comment on attachment 8365150 [details] [diff] [review]
patch v2

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

Nice work. The "Step" comments really do help make the code more obviously correct.

r=me with the following changes.

::: js/src/builtin/Object.cpp
@@ +528,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (args.length() < 2) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_MORE_ARGS_NEEDED,
> +                             "Object.setPrototypeOf", "1", "s");

Please change "s" to the empty string "".

This is because with the "s", the message would be "requires more than 1 arguments" which is ungrammatical in English. (Don't ask me to explain why "more than 1" requires a singular noun. It's magic.)

@@ +552,5 @@
> +        return true;
> +    }
> +
> +    /* Step 5-6. */
> +    RootedObject obj(cx, ToObject(cx, args[0]));

Since step 4 established that args[0].isObject(), please just do &args[0].toObject() here.

JS::ToObject() is more complex; it handles primitive values (converting them to wrapper objects) and it can fail by running out of memory, which args[0].toObject() can't.

::: js/src/js.msg
@@ +223,5 @@
>  MSG_DEF(JSMSG_NEGATIVE_REPETITION_COUNT, 170, 0, JSEXN_RANGEERR, "repeat count must be non-negative")
>  MSG_DEF(JSMSG_INVALID_FOR_OF_INIT,    171, 0, JSEXN_SYNTAXERR, "for-of loop variable declaration may not have an initializer")
>  MSG_DEF(JSMSG_INVALID_MAP_ITERABLE,   172, 0, JSEXN_TYPEERR, "iterable for map should have array-like objects")
>  MSG_DEF(JSMSG_NOT_A_CODEPOINT,        173, 1, JSEXN_RANGEERR, "{0} is not a valid code point")
> +MSG_DEF(JSMSG_NOT_AN_OBJECT,          174, 0, JSEXN_TYPEERR, "Object prototype may only be an Object or null")

Instead of adding a new error message, please use JSMSG_NOT_EXPECTED_TYPE.

That error message takes 3 arguments:
  "Object.setPrototypeOf",
  "an object or null",
  InformalValueTypeName(args[1])

	Jeff Walden [:Waldo]
	Comment 11 • 10 years ago

Review commentary:

<jwalden> jorendorff: for the Object.setPrototypeOf thing, we should make that code call GlobalObject::warnOnceAboutPrototypeMutation
<jwalden> jorendorff: which currently doesn't warn at all, but will once I land a patch in the next few days
<jorendorff> jwalden: or make setProto do so
<jwalden> jorendorff: we use setProto in Gecko for dumbness, and we use it inside the engine, I think, as well, for { __proto__: null }
<jwalden> jorendorff: so I think we can't do it there just yet
<jorendorff> mmkay
<jorendorff> sankha93: jwalden has a review comment for you!
*jwalden would kind of like to make { __proto__: ... } warn, but thinks it probably should be treated differently, at least initially, because there's much less use of the actual setter than of the name in object literals
<jwalden> :-)

I'll be flipping that method to actually warn in a very few days, in bug 948227, after I land the second half of bug 948583.  That's delayed while I fix regressions from the first half of that bug, but that should happen Really Soon, and definitely before uplift.  So for now just add the

    RootedObject setPrototypeOf(cx, &args.callee());
    if (!GlobalObject::warnOnceAboutPrototypeMutation(cx, setPrototypeOf))
        return false;

to the method and don't worry that it doesn't actually perform a warning.

	Sankha Narayan Guria [:sankha] Assignee
	Comment 12 • 10 years ago

fine?Addressed Jason's comments in this. Jeff can you see if the changes you suggested is fine?

	Sankha Narayan Guria [:sankha] Assignee
	Comment 13 • 10 years ago

Oops! The text got scrambled. It should have been:

Addressed Jason's comments in this. Jeff can you see if the change you suggested is fine?

	Jeff Walden [:Waldo]
	Comment 14 • 10 years ago

Comment on attachment 8365535 [details] [diff] [review]
patch v3

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

Vague skimming, didn't really look closely at behavior.

::: js/src/builtin/Object.cpp
@@ +523,5 @@
>      return true;
>  }
>  
> +static bool
> +obj_setPrototypeOf(JSContext *cx, unsigned argc, Value *vp) {

{ on new line

@@ +554,5 @@
> +    }
> +
> +    RootedObject setPrototypeOf(cx, &args.callee());
> +    if (!GlobalObject::warnOnceAboutPrototypeMutation(cx, setPrototypeOf))
> +        return false;

Please move this all the way up to the top, just after the |CallArgs args| bit.  We want to warn regardless whether any intermediate error-tests cause execution to halt, because such behavior likely indicates a bug in the user's code.

	Sankha Narayan Guria [:sankha] Assignee
	Comment 15 • 10 years ago

Carrying over r+ from previous comments.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/00d8b2e04b15

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 10 years ago

Backed out for jit-test asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab170f0289da

https://tbpl.mozilla.org/php/getParsedLog.php?id=35053719&tree=Mozilla-Inbound

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 10 years ago

Looks like the asserts were only happening in the rootanalysis builds.

	Tom S [:evilpie]
	Comment 19 • 10 years ago

Oh sorry I just pushed, this because I confused the normal SM builds with the hazards builds. Maybe we are lucky.

https://hg.mozilla.org/integration/mozilla-inbound/rev/893100c94698

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/893100c94698

	u279076
	Comment 21 • 10 years ago

Tagging as [qa-] since this has in-testsuite coverage. Please needinfo me if this needs QA attention before we release.

	Chris Peterson [:cpeterson]
	Comment 22 • 10 years ago

ES6's Object.setPrototypeOf is probably worth relnoting.

	Sylvestre Ledru [:Sylvestre]
	Comment 23 • 10 years ago

Thanks. Added in the release notes for 31.
I will add a link to the documentation once it is available.

	Florian Scholz (Open Web Docs)
	Comment 24 • 10 years ago

(In reply to Sylvestre Ledru [:sylvestre] from comment #23)
> Thanks. Added in the release notes for 31.
> I will add a link to the documentation once it is available.

Reference:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf

Firefox 31 for developers:
https://developer.mozilla.org/en-US/Firefox/Releases/31#JavaScript

	Sylvestre Ledru [:Sylvestre]
	Comment 25 • 10 years ago

Thanks. Release note updated.