1090636 - Update js::baseops::SetPropertyHelper to implement ES6 9.1.9 [[Set]] for native objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The stack in bug 1090537 leaves a lot left undone. My plans:

1. Most uses of obj in SetPropertyHelper are wrong; they should be using receiver or pobj. Until bug 1090537, I think the bugs were mostly hopeless because receiver wasn't dependable anyway. Now, fixing them is a mere exercise of intuition; the only part that requires actual thought is getting test coverage in place...

2. All code that runs after an existing property is found should be factored out into a helper function, SetExistingProperty, to complement SetNonexistentProperty.

3. After that, whatever's left in SetPropertyHelper can be pretty much scrapped and rewritten to match the spec. In particular the interaction with non-native objects on the prototype chain can be fixed.

Comment 1

[Jason Orendorff [:jorendorff]]

Status: OK, my stack in this bug implements the spec, which says that the assignment to a data property eventually bottoms out in a [[DefineOwnProperty]] call.

That's how I came across this nice comment:

         * FIXME: This is still wrong for various array types, and will set the wrong attributes
         *        by accident, but we can't use NativeLookupOwnProperty in this case, because of resolve
         *        loops.

Yay?

Comment 2

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=88e4abc86d3e

Comment 3

[Jason Orendorff [:jorendorff]]

Current status here:

I implemented the part of ES6 where if Receiver is a proxy, we actually call its defineProperty handler in non-accessor cases, to finish off assignment.

If the proxy already has an own property, my code defines with JSPROP_IGNORE_PERMANENT | JSPROP_IGNORE_ENUMERATE | JSPROP_IGNORE_READONLY, per spec.

But there must be some code somewhere (can't tell where yet) that isn't passing through the IGNORE attrs, and the property is being redefined after all --- as a non-configurable non-enumerable non-writable data property, which is absurd behavior for assignment and causes failures. Debugger symbols not behaving for me. More in a bit.

Comment 4

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=e9e7ae726fb7

Comment 5

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=c11cc6ef820c

Comment 6

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=6b4107da825b

Comment 7

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=78f44d5b4929

Comment 8

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=48b468d9146f

Comment 9

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=100918d7f798

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - Factor out SetExistingProperty from SetPropertyHelper. No change in behavior

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Move SetExistingProperty above SetPropertyHelper. No change in behavior

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: part 3 - Shadow an existing property even if pobj == obj

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: part 4 - Update SetPropertyByDefining to implement ES6 draft rev 28 section 9.1.9 steps 5.c-f

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: part 5 - In SetExistingProperty, correctly handle assigning to array.length

This one is a little complicated. There is some code in SetExistingProperty that is supposed to determine whether or not we should shadow a property that wasn't found on the receiver. This code does not work correctly for array.length properties. (array.length is a data property, so of course it should be shadowed -- but it is non-shadowable() because hasSlot() is false.)

Having fixed that bug, the call to SetArrayLength is only necessary in the non-shadowing case. (In fact, it would be unnecessary altogether but for Parallel JS.) This in turn makes 'attrs' unnecessary, so delete that.

This is a behavior change (a bug fix) in the case of proxies, as the test illustrates.

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: part 6 - Change DefineNativeProperty to support redefining array elements

As it stands, this code is relatively obscure, so the fact that it's amazingly busted is of little consequence. But once we implement ES6 assignment, this code will be fairly easy to trigger when assigning to any Proxy whose target is an Array.

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: part 7 - Assigning to an array element via a proxy should trigger the "shadowing" code

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: part 8 - Factor out SetDenseOrTypedArrayElement from SetExistingProperty. No change in behavior

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: part 9 - Move SetDenseOrTypedArrayElement above SetExistingProperty. No change in behavior

Comment 19

[Jason Orendorff [:jorendorff]]

Attachment: part 10 - Refactor SetExistingProperty. No change in behavior

At this point, SetPropertyHelper has been factored into five parts:
  SetPropertyByDefining
  SetNonexistentProperty
  SetDenseOrTypedArrayElement
  SetExistingProperty
  SetPropertyHelper

each with a comment on top and a reasonably clear responsibility. The longest is less than 100 lines, including comments.

Comment 20

[Jason Orendorff [:jorendorff]]

Attachment: part 11 - Pass pobj (the object that contains the element) to SetDenseOrTypedArrayElement rather than obj. Reflect.set could trigger the bug being fixed here, so we leave a sleeper test for it

Comment 21

[Jason Orendorff [:jorendorff]]

Attachment: part 12 - Move NativeSet. No change in behavior

Comment 22

[Jason Orendorff [:jorendorff]]

Attachment: part 13 - Reimplement the mHasPrototype part of Proxy::set as a call to BaseProxyHandler::set

Comment 23

[Jason Orendorff [:jorendorff]]

Attachment: part 14 - Rewrite SetPropertyHelper

With this patch:

*   SetPropertyHelper no longer calls lookupProperty hooks, which is nice
    since there's no such thing in ES6.

*   As SetPropertyHelper walks the prototype chain, as soon as it reaches a
    non-native object, it calls that object's set hook, passing the original
    receiver, and returns.

    This means Proxy set traps will now be called per spec when triggered via
    the prototype chain from a native object.

    It also means that legacy scripted indirect proxies (Proxy.create) will
    now sometimes have their set trap called when it wouldn't have been called
    before. This seems to be confined to fuzztests in practice, since it only
    matters when the proxy is used as the prototype of an ordinary object.
    But it could cause trouble.

    The same situation affects other proxies too: BaseProxyHandler::set() and
    its overrides will all be called in new situations. It's inevitable though;
    Reflect.set was going to enable all these situations anyway.

Comment 24

[Jason Orendorff [:jorendorff]]

Attachment: part 15 - Optimize away the HasOwnProperty call in SetPropertyByDefining, in the common case. No change in behavior, theoretically

Comment 25

[Jason Orendorff [:jorendorff]]

I've rebased this stack so many times I don't even know if anything is real anymore. But it keeps passing the tests.

Comment 26

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=d7ad3f57a2e4

Comment 27

[Eric Faust [:efaust]]

Comment on attachment 8527973 [details] [diff] [review]
part 1 - Factor out SetExistingProperty from SetPropertyHelper. No change in behavior

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

Dear god, yes. This is so much better. Every one of the patches from this stack makes me happy inside.

Comment 28

[Eric Faust [:efaust]]

Comment on attachment 8527974 [details] [diff] [review]
part 2 - Move SetExistingProperty above SetPropertyHelper. No change in behavior

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

APPROVED.

Comment 29

[Eric Faust [:efaust]]

Comment on attachment 8527975 [details] [diff] [review]
part 3 - Shadow an existing property even if pobj == obj

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

I agree.

Comment 30

[Eric Faust [:efaust]]

Comment on attachment 8527975 [details] [diff] [review]
part 3 - Shadow an existing property even if pobj == obj

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

I agree.

Comment 31

[Eric Faust [:efaust]]

Comment on attachment 8527975 [details] [diff] [review]
part 3 - Shadow an existing property even if pobj == obj

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

Ok, third time's the charm. I have a bad ListBox selection and scroll interaction.

Comment 32

[Eric Faust [:efaust]]

Comment on attachment 8527977 [details] [diff] [review]
part 4 - Update SetPropertyByDefining to implement ES6 draft rev 28 section 9.1.9 steps 5.c-f

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

::: js/src/vm/NativeObject.cpp
@@ +1952,5 @@
>  {
> +    // Step 5.c-d: Test whether receiver has an existing own property
> +    // receiver[id]. The spec calls [[GetOwnProperty]]; js::HasOwnProperty is
> +    // the same thing except faster in the non-proxy case. (Once
> +    // SetPropertyHelper is better-behaved, we will be able top optimize away

typo: "we will be able top optimize away"

@@ +1989,5 @@
> +                return receiver->reportNotExtensible(cxArg);
> +            if (mode == SequentialExecution &&
> +                cxArg->asJSContext()->compartment()->options().extraWarnings(cxArg->asJSContext()))
> +            {
> +                return receiver->reportNotExtensible(cxArg, JSREPORT_STRICT | JSREPORT_WARNING);

I wish we could unify "strict error" and "extra warning" into a single call somehow? This has nothing to do with the patch, just with general error handling procedure.

@@ +2013,5 @@
> +    // Step 5.e-f. Define the new data property.
> +    unsigned attrs =
> +        existing
> +        ? JSPROP_IGNORE_ENUMERATE | JSPROP_IGNORE_READONLY | JSPROP_IGNORE_PERMANENT
> +        : JSPROP_ENUMERATE;

:)

Comment 33

[Eric Faust [:efaust]]

Comment on attachment 8527978 [details] [diff] [review]
part 5 - In SetExistingProperty, correctly handle assigning to array.length

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

::: js/src/jit-test/tests/proxy/testDirectProxySetArray3.js
@@ +7,5 @@
> +        hits++;
> +
> +        // ES6 draft rev 28 (2014 Oct 14) 9.1.9 step 5.e.i.
> +        // Since the property already exists, the system only changes
> +        // the value. desc is otherwise empty.

Can this comment go in the other testDirectProxySet9.js, or whatever, from the previous patch as well?

Comment 34

[Eric Faust [:efaust]]

Comment on attachment 8527979 [details] [diff] [review]
part 6 - Change DefineNativeProperty to support redefining array elements

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

Glad to see that array FIXME removed.

::: js/src/vm/NativeObject.cpp
@@ +1459,5 @@
> +        //   - js_InitNumber is called again, this time to resolve Number.
> +        //   - It creates a second Number constructor, which trips an assertion.
> +        //
> +        // Therefore we do a special lookup that does not call the resolve hook.
> +        NativeLookupOwnPropertyNoResolve(cx, obj, id, &shape);

This is a good and needed stopgap. There should still be a special define hook for resolve hooks that we force everyone to use by fiat that can't call resolve hooks. I think Waldo wanted this, and I can't disagree.

Comment 35

[Eric Faust [:efaust]]

Comment on attachment 8527981 [details] [diff] [review]
part 7 - Assigning to an array element via a proxy should trigger the "shadowing" code

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

Nice test.

::: js/src/jit-test/tests/proxy/testDirectProxySetReceiverLookup.js
@@ +32,5 @@
> +var fullDesc = {
> +    configurable: true,
> +    enumerable: true,
> +    writable: true,
> +    value: "pizza"

As an aside, I find it amusing that all your tests have a *different* arbitrary string in them.

Comment 36

[Eric Faust [:efaust]]

Comment on attachment 8527983 [details] [diff] [review]
part 8 - Factor out SetDenseOrTypedArrayElement from SetExistingProperty. No change in behavior

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

APPROVED.

Comment 37

[Eric Faust [:efaust]]

Comment on attachment 8527985 [details] [diff] [review]
part 9 - Move SetDenseOrTypedArrayElement above SetExistingProperty. No change in behavior

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

APPROVED.

Comment 38

[Eric Faust [:efaust]]

Comment on attachment 8527987 [details] [diff] [review]
part 10 - Refactor SetExistingProperty. No change in behavior

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

Yes. Yes Yes Yes. Yes. You have no idea how happy I am to see the inscrutable |shape = nullptr| crap go. r# (++++)

Comment 39

[Eric Faust [:efaust]]

Comment on attachment 8527988 [details] [diff] [review]
part 11 - Pass pobj (the object that contains the element) to SetDenseOrTypedArrayElement rather than obj. Reflect.set could trigger the bug being fixed here, so we leave a sleeper test for it

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

I continue to be appreciative of sleeper tests.

Comment 40

[Eric Faust [:efaust]]

Comment on attachment 8527989 [details] [diff] [review]
part 12 - Move NativeSet. No change in behavior

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

APPROVED.

Comment 41

[Eric Faust [:efaust]]

Comment on attachment 8527992 [details] [diff] [review]
part 14 - Rewrite SetPropertyHelper

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

This is much better.

I wonder if we will still have observable trap incorrectness in the browser with scripted direct proxies due to various DOM set traps still calling lookupProperty.

Comment 42

[Eric Faust [:efaust]]

Comment on attachment 8527993 [details] [diff] [review]
part 15 - Optimize away the HasOwnProperty call in SetPropertyByDefining, in the common case. No change in behavior, theoretically

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

Nice.

Comment 43

[Jason Orendorff [:jorendorff]]

(In reply to Eric Faust [:efaust] from comment #32)
> > +                return receiver->reportNotExtensible(cxArg);
> > +            if (mode == SequentialExecution &&
> > +                cxArg->asJSContext()->compartment()->options().extraWarnings(cxArg->asJSContext()))
> > +            {
> > +                return receiver->reportNotExtensible(cxArg, JSREPORT_STRICT | JSREPORT_WARNING);
> 
> I wish we could unify "strict error" and "extra warning" into a single call
> somehow? This has nothing to do with the patch, just with general error
> handling procedure.

Not a bad idea. I didn't file a follow-up bug because I don't think I have time to pursue it, but feel free, as they say...

Comment 44

[Jason Orendorff [:jorendorff]]

(In reply to Eric Faust [:efaust] from comment #33)
> ::: js/src/jit-test/tests/proxy/testDirectProxySetArray3.js
> > +        // ES6 draft rev 28 (2014 Oct 14) 9.1.9 step 5.e.i.
> > +        // Since the property already exists, the system only changes
> > +        // the value. desc is otherwise empty.
> 
> Can this comment go in the other testDirectProxySet9.js, or whatever, from
> the previous patch as well?

Done.

Comment 45

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=01724b7ae3f0

Comment 46

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/388e4a23e7fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/db5a8d2f9a23
https://hg.mozilla.org/integration/mozilla-inbound/rev/6425c0662751
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5a38a0df65d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f322eac33cfa
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ecbc62d82a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bd9d8951ca7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6971076e6d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a2ae51047a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7047fa3072d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/14f7fecc080c
https://hg.mozilla.org/integration/mozilla-inbound/rev/60a8d1a15268
https://hg.mozilla.org/integration/mozilla-inbound/rev/79c9be8158ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd21b295afaa
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbcf9c812bb5

Comment 47

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=a9f53d6ff958

Comment 48

[Jason Orendorff [:jorendorff]]

After a great try server run Wednesday night (except for one rebasing error which I fixed) this completely destroyed inbound this morning, but with nothing actionable in the logs. The browser builds and seems to run fine for me locally.

I pushed to the try server again. In theory that should come back all red with nothing actionable in the logs. Then I can try-bisect, again...

Comment 49

[Jason Orendorff [:jorendorff]]

Figured it out. It's in part 6. Debugging.

Comment 50

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=6a038bbe8eb9

Comment 51

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/383dece1cbad
https://hg.mozilla.org/integration/mozilla-inbound/rev/2676562ab2b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/44c3c726c7ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c02f7db4eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ee531a0dbcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/16ff20b59b8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e89dd69118
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b7f3844bab
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a7c8a5513c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e28c83356e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e867a56b167
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fe6d2492dc8
https://hg.mozilla.org/integration/mozilla-inbound/rev/38c02eda337d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a71021584ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/7837628feeca

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/383dece1cbad
https://hg.mozilla.org/mozilla-central/rev/2676562ab2b3
https://hg.mozilla.org/mozilla-central/rev/44c3c726c7ae
https://hg.mozilla.org/mozilla-central/rev/a9c02f7db4eb
https://hg.mozilla.org/mozilla-central/rev/9ee531a0dbcb
https://hg.mozilla.org/mozilla-central/rev/16ff20b59b8f
https://hg.mozilla.org/mozilla-central/rev/f2e89dd69118
https://hg.mozilla.org/mozilla-central/rev/b4b7f3844bab
https://hg.mozilla.org/mozilla-central/rev/86a7c8a5513c
https://hg.mozilla.org/mozilla-central/rev/f4e28c83356e
https://hg.mozilla.org/mozilla-central/rev/6e867a56b167
https://hg.mozilla.org/mozilla-central/rev/1fe6d2492dc8
https://hg.mozilla.org/mozilla-central/rev/38c02eda337d
https://hg.mozilla.org/mozilla-central/rev/6a71021584ff
https://hg.mozilla.org/mozilla-central/rev/7837628feeca