948583 - Implementing __proto__ in object initializers via setproperty-like behavior isn't a good idea

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Doing so this way makes it impossible for code processing JSOP_INITPROP to assume some initial object state, in general.  If the JSOP_INITPROP occurs after a __proto__ JSOP_INITPROP, then the object has potentially escaped (through the setproperty-like action that occurs when evaluating the __proto__ member).

I think we want to split JSOP_INITPROP into that plus JSOP_INITPROTO, and have JSOP_INITPROTO do something special to set the object's prototype that doesn't potentially invoke user-defined code.  I don't much care about the perf of such objects, but it does seem bad to actually expose them to user code, before they're fully created.

Comment 1

[Jason Orendorff [:jorendorff]]

Agreed. The ES6 specification of __proto__ in object literals requires this change in behavior.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-__proto___-property-names-in-object-initialisers

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Split JSOP_MUTATEPROTO out of JSOP_INITPROP, not changing existing semantics

Because of the mess of destructuring patterns also being parsed as part of object literals, it seems best to treat __proto__ as a special-case within the current code path when parsing object literals (that is, in the bit guarded by |if (op == JSOP_INITPROP || op == JSOP_MUTATEPROTO)| in this patch).

The way |begin| is slotted in and computed for a PNK_MUTATEPROTO is not especially pretty.  But it's the simplest, least invasive way to do it now.

This doesn't make JSOP_MUTATEPROTO not do setprop-y things.  That requires some of efaust's recent prototype in my review queue.  Once that lands, changing the interpreter and js::jit::MutatePrototype to the desired semantics should be easy.

It's unfortunate the baseline compiler will happily compile when a new op is added to jsopcode.tbl but BaselineCompiler.{cpp,h} go untouched.  Sigh.  You almost got a beautifully-implemented patch whose JIT parts were dead code.  (Although the perf impact on literals with __proto__ would have been wonderful!)

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Split JSOP_MUTATEPROTO out of JSOP_INITPROP, not changing existing semantics

Actually, for simplicity/debuggability, let's separate out the new-op patch from the make-it-not-depend-on-Object.prototype.__proto__ patch.  Also poking efaust in case he can get to this first, and/or wants to expand his domain of knowledge.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Switch JSOP_MUTATEPROTO over to JSObject::setProto

And the bit to change the functionality.  Setting aside __proto__'s marshalling bits, this isn't identical code being invoked: it doesn't include the CheckAccess call that's in the __proto__ setter.  Given the object being modified is always a partially-constructed object literal, same-origin same-page and all that, I can't imagine how this call could ever legitimately fail.  But I could be missing something, so flagging extra peoples for a look.

We really should merge that CheckAccess into JSObject::setProto (and perhaps into setPrototypeOf implementations in proxies) at some point to get rid of all this confusion nonsense.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8347666 [details] [diff] [review]
Split JSOP_MUTATEPROTO out of JSOP_INITPROP, not changing existing semantics

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

::: js/src/jit/LIR-Common.h
@@ +600,4 @@
>  };
>  
>  // Takes in an Object and a Value.
> +class LMutateProto : public LCallInstructionHelper<0, 1 + BOX_PIECES, 0>

I looked at all the changes in js/src/jit and they seem correct to me (here: no output, one pointer operand and one boxed value operand) but a JIT hacker should also review.

::: js/src/jsopcode.tbl
@@ +414,5 @@
>  
>  OPDEF(JSOP_CALLELEM,      193, "callelem",   NULL,    1,  2,  1, JOF_BYTE |JOF_ELEM|JOF_TYPESET|JOF_LEFTASSOC)
>  
> +/* __proto__: v inside an object initializer. */
> +OPDEF(JSOP_MUTATEPROTO,   194, "mutateproto",NULL,    1,  2,  1, JOF_BYTE)

Don't forget to bump the bytecode version in Xdr.h.

Comment 6

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> I looked at all the changes in js/src/jit and they seem correct to me (here:
> no output, one pointer operand and one boxed value operand) but a JIT hacker
> should also review.

Mostly this was copy-paste from the JSOP_INITPROP stuff, then munge to the new functionality, fwiw.

> Don't forget to bump the bytecode version in Xdr.h.

http://cdn.memegenerator.net/instances/500x/43909610.jpg

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8347668 [details] [diff] [review]
Switch JSOP_MUTATEPROTO over to JSObject::setProto

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

Wait a minute --- this is a behavior change, so it needs tests.

Particularly around what changed: __proto__: 17, __proto__: undefined, and how it behaves when Object.prototype.__proto__ has been deleted or replaced with a different setter (it shouldn't fire) or undefined setter (it shouldn't matter).

Also, if you can stand it, if Object.prototype's prototype is a proxy, none of its traps should fire.

r=me with that.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Switch JSOP_MUTATEPROTO over to JSObject::setProto, with moar tests

(In reply to Jason Orendorff [:jorendorff] from comment #7)
> Particularly around what changed: __proto__: 17, __proto__: undefined

These didn't change, as __proto__'s setter always only did anything if passed an object or null.

> and how it behaves when Object.prototype.__proto__ has been deleted or
> replaced with a different setter (it shouldn't fire) or undefined setter
> (it shouldn't matter).

Done.

> Also, if you can stand it, if Object.prototype's prototype is a proxy, none
> of its traps should fire.

Meany-head.  (Done.)

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8348115 [details] [diff] [review]
Switch JSOP_MUTATEPROTO over to JSObject::setProto, with moar tests

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

A lot of this goes over my head. I could dig in more, but IIUC I'm only really reviewing the removal of the CheckAccess call. Please correct me if I'm wrong.

Assuming this only applies to object literals (and not proto mutation in the general case), this is fine. That CheckAccess hook is just about prevent the mutation of Location objects.

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 8347666 [details] [diff] [review]
Split JSOP_MUTATEPROTO out of JSOP_INITPROP, not changing existing semantics

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

The JIT pieces of this also look good to me. Well done navigating the complications of adding an IM instruction. r=me.

::: js/src/frontend/Parser.cpp
@@ +6897,3 @@
>                      handler.setListFlag(literal, PNX_NONCONST);
>  
> +                if (op == JSOP_MUTATEPROTO

hyper nit: this is slightly unsightly to me, but if it's the style of the area, then I cannot complain.

::: js/src/jit/MIR.h
@@ +1609,5 @@
> +    MMutateProto(MDefinition *obj, MDefinition *value)
> +    {
> +        setOperand(0, obj);
> +        setOperand(1, value);
> +        setResultType(MIRType_None);

Is this true? I guess technicall not, but it leaves the value on the stack?

Comment 11

[Eric Faust [:efaust]]

Comment on attachment 8348115 [details] [diff] [review]
Switch JSOP_MUTATEPROTO over to JSObject::setProto, with moar tests

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

Good. r=me

::: js/src/jit/VMFunctions.cpp
@@ +197,5 @@
> +
> +    bool succeeded;
> +    if (!JSObject::setProto(cx, obj, newProto, &succeeded))
> +        return false;
> +    MOZ_ASSERT(succeeded);

not JS_ASSERT()? If we are trying to move away from JS_ASSERT(), then it was used in the previous patch?

Comment 12

[Bobby Holley (:bholley)]

(jorendorff says MOZ_ASSERT for new code btw).

Comment 13

[Jeff Walden [:Waldo]]

I cribbed MUTATEPROTO entirely from INITPROP, so any behavior of leaving values on the stack and all comes from it.  :-)

I switched these over to MOZ_ASSERT on landing.  Mostly I've been inconsistent about it because of not wanting to be too inconsistent with adjacent code.  New stuff I've been better about it, tho.

First part (adding JSOP_MUTATEPROTO) landed.  Second part will land either tomorrow, or the day after.  Not sure which yet, depends how much I want an intervening nightly or two to detect bugs in one half or the other.

Comment 14

[Jeff Walden [:Waldo]]

And a typo-fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/49700610a9b7

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out all three in https://hg.mozilla.org/integration/mozilla-inbound/rev/30a122811943 and https://hg.mozilla.org/integration/mozilla-inbound/rev/4358d0cb5d70 for some PGO-only build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=33360382&tree=Mozilla-Inbound

Dholbert thinks this might just be needs-clobber, but I had already pushed the backout before that was mentioned.

Comment 16

[Daniel Holbert [:dholbert]]

To be clear, since not all of the original csets made it into comments here -- this originally landed as follows:
1) A "no bug" helper-patch:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/64d44d0d790c
2) This bug's actual patch:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/3807c2bc06a2
3) The followup from comment 14 (49700610a9b7), which fixed a typo in the "no bug" helper-patch (64d44d0d790c).

(And then, comment 15 backed out all three of those csets.)

Comment 17

[Jeff Walden [:Waldo]]

Blah.  Not enough time/pleasant computing environment where I am right now (not on my normal laptop, it no worky) to deal with this today, will wait til tomorrow or maybe the day after.  I am somewhat loathe to just push a CLOBBER and declare this "fixed", so I want to look into that at least briefly before doing the expedient seeming-fix.

Comment 18

[Jeff Walden [:Waldo]]

First part landed, with a CLOBBER.  Fixing the apparent clobber-specific bustage seems not super-difficult, but the trick is to convert jsopcode.tbl into a vm/Opcodes.h higher-order macro and get rid of jsoplengen.cpp and associated weirdness.  That's way too much to do to hold up this patch, so I'm punting it for now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e86f6fef07

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f2e86f6fef07

Comment 20

[Gregory Szorc [:gps]]

Please file Core :: Build Config bugs for all checkins requiring a clobber. Have them block bug 941904 (aliased as "clobber").

I'll file a bug shortly.

Comment 21

[Jeff Walden [:Waldo]]

Sorry, forgot to note that I filed bug 963434 already to fix the jsopcode issue -- by removing jsopcode.tbl entirely, and making it a higher-order macro with no host-built binaries needed.

Comment 22

[Jeff Walden [:Waldo]]

Final bit of this is landed, now that JSOP_MUTATEPROTO is in good shape:

https://hg.mozilla.org/integration/mozilla-inbound/rev/96ae326e19d0

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96ae326e19d0