Closed Bug 1121554 Opened 5 years ago Closed 5 years ago

Include receiver argument in setGeneric hook

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
The setGeneric and related hooks do not support setting properties with a receiver other than the object the hook is being called on, as is done during prototype traversals.  nonNativeSetProperty and nonNativeSetElement have proxy specific workarounds for this, but other non-native objects are affected as well.  With unboxed objects, which need to behavior the same as plain objects, this leads to weird behaviors when sets go through an unboxed object on the prototype chain.  The attached patch removes the proxy workarounds and changes the signature of the hooks to add a receiver.
Attachment #8548997 - Flags: review?(jorendorff)
Comment on attachment 8548997 [details] [diff] [review]
patch

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

r=me with comments.

::: js/src/jit-test/tests/basic/unboxed-object-set-property.js
@@ +1,2 @@
> +
> +// Use the correct receiver when non-native objects are prototypes of other objects.

Not sure what to make of this test. I guess these are non-native sometimes when unboxed objects are enabled?

Can you make a test using TypedObjects? Then it would be testing the right thing even if unboxed objects are turned off.

::: js/src/vm/ScopeObject.cpp
@@ +527,4 @@
>                  MutableHandleValue vp, bool strict)
>  {
>      RootedObject actual(cx, &obj->as<DynamicWithObject>().object());
> +    return JSObject::setGeneric(cx, actual, receiver, id, vp, strict);

I think this is wrong in the case where obj == receiver. Then it should unwrap receiver as well as obj.

I think it's wrong because it seems like it would result in WithObjects being exposed to script, which basically should never happen. The test suite doesn't already cover this? Here's what I'd expect to fail:

    var obj = {
        set x(val) {
            assertEq(this, obj);  // 'this' shouldn't be a WithObject
        }
    }

    with (obj) { x = 1; }

Same for the other two with_Set* signatures.
Attachment #8548997 - Flags: review?(jorendorff) → review+
Attached patch patchSplinter Review
The previous patch was pretty lazy and didn't actually fix the semantics for typed objects to be anything sensible.  This patch takes care of them, and adds a test for them, and is different enough to need a re-review.  I left the semantics of array indexes alone, which are kind of messed up and don't line up with either plain arrays nor typed arrays.  There were several tests that failed without the 'with' object unboxing actually, I didn't test this patch thoroughly enough earlier.
Assignee: nobody → bhackett1024
Attachment #8548997 - Attachment is obsolete: true
Attachment #8556111 - Flags: review?(jorendorff)
Comment on attachment 8556111 [details] [diff] [review]
patch

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

Sorry for the slow review here. You'll have to rebase across bug 1127121 but it won't be so bad.

::: js/src/vm/NativeObject.cpp
@@ +1952,5 @@
> +    return SetPropertyByDefining(cx, obj, receiver, id, vp, strict, false);
> +}
> +
> +bool
> +js::SetNonWritableProperty(JSContext *cx, HandleId id, bool strict)

The way this stuff works is being reworked in bug 1113369, so that this function becomes unnecessary and callers will instead say

    return result.failReadOnly();

I'm sure you'll land before that though. It's got regressions I'm still debugging.

::: js/src/vm/UnboxedObject.cpp
@@ +369,5 @@
> +                return false;
> +            return SetProperty(cx, obj, receiver, id, vp, strict);
> +        }
> +
> +        return SetPropertyByDefining(cx, obj, receiver, id, vp, strict, false);

I take it properties of UnboxedPlainObjects are always writable data properties? (Otherwise this wouldn't be correct.)

@@ +374,3 @@
>      }
>  
> +    return SetPropertyOnProto(cx, obj, receiver, id, vp, strict);

OK. And objects of this type can't have a proxy as their prototype?
Attachment #8556111 - Flags: review?(jorendorff) → review+
Comment on attachment 8556111 [details] [diff] [review]
patch

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

::: js/src/vm/UnboxedObject.cpp
@@ +374,3 @@
>      }
>  
> +    return SetPropertyOnProto(cx, obj, receiver, id, vp, strict);

Never mind, I was misremembering the assertion in SetPropertyOnProto.
Comment on attachment 8556111 [details] [diff] [review]
patch

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

::: js/src/jit-test/tests/TypedObject/set-property-with-prototype.js
@@ +1,3 @@
> +
> +if (!this.hasOwnProperty("TypedObject"))
> +    throw new TypeError();

OK, one last thing - if you want this test to pass silently in builds without ENABLE_BINARYDATA, it can't throw here, right?
https://hg.mozilla.org/mozilla-central/rev/f1348faedf2f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.