Closed Bug 1184414 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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?
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
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.
Attachment #8639015 - Flags: feedback?(jorendorff)
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 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();
Attachment #8639015 - Flags: feedback?(jorendorff) → feedback+
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.
Attachment #8639015 - Attachment is obsolete: true
Attachment #8639481 - Flags: review?(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.)
Attachment #8639481 - Flags: review?(jorendorff) → review+
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.
+     */
Attachment #8639481 - Attachment is obsolete: true
Attachment #8639683 - Flags: review+
Passed on try server (https://treeherder.mozilla.org/#/jobs?repo=try&revision=758816c51959)
Flags: needinfo?(jorendorff)
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/685014edb54e3421439ef30630932ba9c3e2ab2d
changeset:  685014edb54e3421439ef30630932ba9c3e2ab2d
user:       Florian Merz <flomerz@gmail.com>
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.
https://hg.mozilla.org/mozilla-central/rev/685014edb54e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.