Closed Bug 1090636 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jorendorff, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

Attachments

(15 files)

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
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.
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?
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: nobody → jorendorff
Status: NEW → ASSIGNED
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)
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)
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)
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)
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-
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+
(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...
(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.
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...
Figured it out. It's in part 6. Debugging.
Blocks: es6
Depends on: 1113980
Depends on: 1112585
Depends on: 1117106
You need to log in before you can comment on or make changes to this bug.