1184414 - X.[[SetPrototypeOf]](Y) should succeed if X.[[Prototype]] is already Y

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Jeff Walden noticed this while reviewing in bug 987514.

js> var a = {}, b = Object.freeze(Object.create(a));
js> Object.setPrototypeOf(b, a)
typein:2:1 TypeError: can't set prototype of this object
js> Reflect.setPrototypeOf(b, a)
false

How hard could this be?

Comment 1

[Florian Dieminger [:fiji]]

I would like to work on this. 

jsobj.cpp -> js::SetPrototype 
Object.cpp -> obj_setPrototypeOf 

seem to be a good starting point. Step 4 of [1] is missing there. I plan to start there and find my way.

I'll update here in the few days, hopefully already with a patch.

[1] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-setprototypeof-v

Comment 2

[Florian Dieminger [:fiji]]

Attachment: Bug_1184414_Update_SetPrototypeOf_for_SameValue.patch

This should fix this issue. I had two options:

1.) use SameValue for comparision as in the spec:

    RootedObject objProto(cx);
    if (GetPrototype(cx, obj, &objProto)) {
        bool same;
        RootedValue objProtoVal(cx, js::ObjectOrNullValue(objProto));
        RootedValue protoVal(cx, js::ObjectOrNullValue(proto));
        if (js::SameValue(cx, objProtoVal, protoVal, &same) && same) {
            return result.succeed();
        }
    }

2.) Just compare if |obj.[[Prototype]]| == |proto| like this:

    RootedObject objProto(cx);
    if (GetPrototype(cx, obj, &objProto)) {
        if (objProto == proto) {
            return result.succeed();
        }
    }

Right now the patch contains 1.). Would 2.) be sufficient here? Since |obj| and |proto| are already HandleObjects this should be fine.

However, some feedback would be nice and if you already fixed that just let me know.

Comment 3

[Tom S [:evilpie]]

Doing 2. is okay, but you should copy the following comment that we have in NativeDefineProperty:
        // The spec says to use SameValue, but since the values in
        // question are objects, we can just compare pointers.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8639015 [details] [diff] [review]
Bug_1184414_Update_SetPrototypeOf_for_SameValue.patch

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

Nice tests. Thanks for the patch!

Please post an updated one and let's land it.

::: js/src/jsobj.cpp
@@ +2396,5 @@
>  bool
>  js::SetPrototype(JSContext* cx, HandleObject obj, HandleObject proto, JS::ObjectOpResult& result)
>  {
> +    /* ES6 9.1.2 step 4 if |obj.[[Prototype]]| has SameValue as |proto| return true */
> +    RootedObject objProto(cx);

Please move this new code down about 25 lines, so it happens just before ES6 9.1.2 step 5.

(We'll be changing this code again in bug 888969, and then it'll suddenly be an observable bug if your new code is before the obj->hasLazyPrototype check rather than after.)

@@ +2401,5 @@
> +    if (GetPrototype(cx, obj, &objProto)) {
> +        bool same;
> +        RootedValue objProtoVal(cx, js::ObjectOrNullValue(objProto));
> +        RootedValue protoVal(cx, js::ObjectOrNullValue(proto));
> +        if (js::SameValue(cx, objProtoVal, protoVal, &same) && same) {

If a function like GetPrototype() returns false (indicating an error), the caller must either handle the error or return false/null right away.

However, I don't think we should call GetPrototype() here. Instead, please weaken the assertion in JSObject::getProto() from:
        MOZ_ASSERT(!uninlinedIsProxy());
to this:
        MOZ_ASSERT(!hasLazyPrototype());

We have a comment in jsobj.h that mentions this assertion, so please change that, too:
     * 1. obj->getProto() returns the prototype, but asserts if obj is a proxy.
should now say:
     * 1. obj->getProto() returns the prototype, but asserts if obj is a proxy
     *    with a relevant getPrototype() handler.

Similarly, we don't need to call SameValue() here. HandleObject::operator==(JSObject*) is equivalent.

So I think this code can be reduced to 3 lines:

    // ES6 9.1.2 steps 3-4.
    if (proto == obj->getProto())
        return result.succeed();

Comment 5

[Florian Dieminger [:fiji]]

Attachment: Bug_1184414_Update_SetPrototypeOf_for_SameValue.patch

Thank for the input Tom and Jason. I'm slowly getting somewhere in the JS Engine code base.

I've adopted the patch to your input. After a review I can push it to treeherder tomorrow evening.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8639481 [details] [diff] [review]
Bug_1184414_Update_SetPrototypeOf_for_SameValue.patch

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

Thanks.

The comment Tom mentioned in comment 3 would be a nice touch, but feel free to land this either way. (I wrote comment 4 without having read comment 2 or 3 -- oops.)

Comment 7

[Florian Dieminger [:fiji]]

Attachment: Bug_1184414_Update_SetPrototypeOf_for_SameValue.patch

I added Toms comment and carried on the r+. Running it to the try-server right now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=758816c51959

This is change in the comment:

-    /* ES6 9.1.2 step 3-4 if |obj.[[Prototype]]| has SameValue as |proto| retur
+    /*
+     * ES6 9.1.2 step 3-4 if |obj.[[Prototype]]| has SameValue as |proto| retur
+     * Since the values in question are objects, we can just compare pointers.
+     */

Comment 8

[Florian Dieminger [:fiji]]

Passed on try server (https://treeherder.mozilla.org/#/jobs?repo=try&revision=758816c51959)

Comment 9

[Jason Orendorff [:jorendorff]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/685014edb54e3421439ef30630932ba9c3e2ab2d
changeset:  685014edb54e3421439ef30630932ba9c3e2ab2d
user:       Florian Merz <[e-mail]>
date:       Tue Jul 28 07:35:22 2015 +0200
description:
Bug 1184414 - X.[[SetPrototypeOf]](Y) should succeed if X.[[Prototype]] is already Y. r=jorendorff.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/685014edb54e