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

RESOLVED FIXED in mozilla37

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(15 attachments)

4.80 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.64 KB, patch
efaust
: review+
Details | Diff | Splinter Review
1.86 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.67 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.68 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.60 KB, patch
efaust
: review+
Details | Diff | Splinter Review
4.38 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.24 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.50 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.48 KB, patch
efaust
: review+
Details | Diff | Splinter Review
3.62 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.84 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.76 KB, patch
bholley
: review+
Details | Diff | Splinter Review
14.07 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.82 KB, patch
efaust
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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?
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Comment 10

4 years ago
Created attachment 8527973 [details] [diff] [review]
part 1 - Factor out SetExistingProperty from SetPropertyHelper. No change in behavior
Attachment #8527973 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
(Assignee)

Comment 11

4 years ago
Created attachment 8527974 [details] [diff] [review]
part 2 - Move SetExistingProperty above SetPropertyHelper. No change in behavior
Attachment #8527974 - Flags: review?(efaustbmo)
(Assignee)

Comment 12

4 years ago
Created attachment 8527975 [details] [diff] [review]
part 3 - Shadow an existing property even if pobj == obj
Attachment #8527975 - Flags: review?(efaustbmo)
(Assignee)

Comment 13

4 years ago
Created attachment 8527977 [details] [diff] [review]
part 4 - Update SetPropertyByDefining to implement ES6 draft rev 28 section 9.1.9 steps 5.c-f
Attachment #8527977 - Flags: review?(efaustbmo)
(Assignee)

Comment 14

4 years ago
Created attachment 8527978 [details] [diff] [review]
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.
Attachment #8527978 - Flags: review?(efaustbmo)
(Assignee)

Comment 15

4 years ago
Created attachment 8527979 [details] [diff] [review]
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.
Attachment #8527979 - Flags: review?(efaustbmo)
(Assignee)

Comment 16

4 years ago
Created attachment 8527981 [details] [diff] [review]
part 7 - Assigning to an array element via a proxy should trigger the "shadowing" code
Attachment #8527981 - Flags: review?(efaustbmo)
(Assignee)

Comment 17

4 years ago
Created attachment 8527983 [details] [diff] [review]
part 8 - Factor out SetDenseOrTypedArrayElement from SetExistingProperty. No change in behavior
Attachment #8527983 - Flags: review?(efaustbmo)
(Assignee)

Comment 18

4 years ago
Created attachment 8527985 [details] [diff] [review]
part 9 - Move SetDenseOrTypedArrayElement above SetExistingProperty. No change in behavior
Attachment #8527985 - Flags: review?(efaustbmo)
(Assignee)

Comment 19

4 years ago
Created attachment 8527987 [details] [diff] [review]
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.
Attachment #8527987 - Flags: review?(efaustbmo)
(Assignee)

Comment 20

4 years ago
Created 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
Attachment #8527988 - Flags: review?(efaustbmo)
(Assignee)

Comment 21

4 years ago
Created attachment 8527989 [details] [diff] [review]
part 12 - Move NativeSet. No change in behavior
Attachment #8527989 - Flags: review?(efaustbmo)
(Assignee)

Comment 22

4 years ago
Created attachment 8527990 [details] [diff] [review]
part 13 - Reimplement the mHasPrototype part of Proxy::set as a call to BaseProxyHandler::set
Attachment #8527990 - Flags: review?(bobbyholley)
(Assignee)

Comment 23

4 years ago
Created attachment 8527992 [details] [diff] [review]
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.
Attachment #8527992 - Flags: review?(efaustbmo)
(Assignee)

Comment 24

4 years ago
Created attachment 8527993 [details] [diff] [review]
part 15 - Optimize away the HasOwnProperty call in SetPropertyByDefining, in the common case. No change in behavior, theoretically
Attachment #8527993 - Flags: review?(efaustbmo)
(Assignee)

Comment 25

4 years ago
I've rebased this stack so many times I don't even know if anything is real anymore. But it keeps passing the tests.
Attachment #8527990 - Flags: review?(bobbyholley) → review+
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.
Attachment #8527973 - Flags: review?(efaustbmo) → review+
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.
Attachment #8527974 - Flags: review?(efaustbmo) → review-
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.
Attachment #8527975 - Flags: review?(efaustbmo) → review-

Updated

3 years ago
Attachment #8527974 - Flags: review- → review+
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 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.
Attachment #8527975 - Flags: review- → review+
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;

:)
Attachment #8527977 - Flags: review?(efaustbmo) → review+
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?
Attachment #8527978 - Flags: review?(efaustbmo) → review+
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.
Attachment #8527979 - Flags: review?(efaustbmo) → review+
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.
Attachment #8527981 - Flags: review?(efaustbmo) → review+
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.
Attachment #8527983 - Flags: review?(efaustbmo) → review+
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.
Attachment #8527985 - Flags: review?(efaustbmo) → review+
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# (++++)
Attachment #8527987 - Flags: review?(efaustbmo) → review+
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.
Attachment #8527988 - Flags: review?(efaustbmo) → review+
Comment on attachment 8527989 [details] [diff] [review]
part 12 - Move NativeSet. No change in behavior

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

APPROVED.
Attachment #8527989 - Flags: review?(efaustbmo) → review+
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.
Attachment #8527992 - Flags: review?(efaustbmo) → review+
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.
Attachment #8527993 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 43

3 years ago
(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...
(Assignee)

Comment 44

3 years ago
(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.
(Assignee)

Comment 48

3 years ago
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...
(Assignee)

Comment 49

3 years ago
Figured it out. It's in part 6. Debugging.
Blocks: 694100

Updated

3 years ago
Depends on: 1113980
Depends on: 1112585

Updated

3 years ago
Depends on: 1117106
You need to log in before you can comment on or make changes to this bug.