1199695 - Mark computed property names as effectful in BCE

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test cases:
---
new (Object.setPrototypeOf(function(){ ({[new.target]: 0}) }, null))


RegExp.prototype.toString = () => { throw "toStr" };
(function() { ({[/./]: 0}) })()
---

Expected: Throws TypeError
Actual: No TypeError

Comment 1

[Jeff Walden [:Waldo]]

BytecodeEmitter::checkSideEffects needs to indicate PNK_COMPUTED_NAME can have effects, and a comment should be added by it noting that the ensuing ToPropertyKey means an effect-free kid doesn't guarantee the computed property name overall is effect-free.

Comment 2

[Sankha Narayan Guria [:sankha]]

Attachment: v1

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8759304 [details] [diff] [review]
v1

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2113,5 @@
> +        MOZ_ASSERT(pn->isArity(PN_UNARY));
> +        return checkSideEffects(pn->pn_kid, answer);
> +
> +      // ToPropertyKey on a effect-free child doesn't mean computed property
> +      // name is effect free.

My previous comment as to the fix was a little bit tunnel-vision in the moment, IMO.  I had to stare at it now for a bit to make sense of it -- and the example failures even more.  Let's try rewording this a little, and let's add super-short examples demonstrating exactly why side effects must be assumed:

"""
Even if the name expression is effect-free, performing ToPropertyKey on it might not be effect-free.  For example:

  RegExp.prototype.toString = () => { throw 42; };
  ({ [/regex/]: 0 }); // ToPropertyKey(/regex/) throws 42

  function Q() {
    ({ [new.target]: 0 });
  }
  Q.toString = () => { throw 17; };
  new Q; // new.target will be Q, ToPropertyKey(Q) throws 17
"""

::: js/src/tests/ecma_6/Expressions/computed-property-side-effects.js
@@ +2,5 @@
> +// http://creativecommons.org/licenses/publicdomain/
> +
> +//-----------------------------------------------------------------------------
> +var BUGNUMBER = 1199695;
> +var summary = "Mark computed property names as effectful in BCE"

"Computed property names must be considered as always effectful even when the name expression isn't effectful, because calling ToPropertyKey on some non-effectful expressions has user-modifiable behavior"

@@ +14,5 @@
> +assertThrowsInstanceOf(() => {
> +  new (Object.setPrototypeOf(function(){
> +    ({[new.target]: 0})
> +  }, null))
> +}, TypeError);

This is a bit gnarly.  The reason the [new.target] throws a TypeError is that it tries to stringify new.target, which is the function, whose [[Prototype]] was set to null, which incidentally meant .toString and .valueOf both became undefined, meaning ToString would fail.  That's a bit more involved than the narrow, precise thing we want to test.  Please modify this test to use the two examples I put in the revised comment by the code changes, rather than this more-belabored test that involves more distinct behaviors to mentally execute to test/verify behavior.

Comment 4

[Sankha Narayan Guria [:sankha]]

Attachment: v2

I have made the changes to the tests, can you take a quick look?

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8760032 [details] [diff] [review]
v2

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2113,5 @@
> +        MOZ_ASSERT(pn->isArity(PN_UNARY));
> +        return checkSideEffects(pn->pn_kid, answer);
> +
> +      // Even if the name expression is effect-free, performing ToPropertyKey on
> +      // it might not be effect-free.

Please add the example I mentioned in my comment -- just having it in a test prevents regressions, but the code remains less-readable and less-understandable.

::: js/src/tests/ecma_6/Expressions/computed-property-side-effects.js
@@ +5,5 @@
> +var BUGNUMBER = 1199695;
> +var summary =
> +  "Computed property names must be considered as always effectful even when " +
> +  "the name expression isn't effectful, because calling ToPropertyKey on " +
> +  "some non-effectful expressions has user-modifiable behavior"

Semicolon after this.

@@ +12,5 @@
> +
> +/**************
> + * BEGIN TEST *
> + **************/
> +RegExp.prototype.toString = () => { throw 42; };

Blank line before this, to visually separate comment from code?

Comment 6

[Sankha Narayan Guria [:sankha]]

Attachment: v3

Comment 7

[Jason Orendorff [:jorendorff]]

Can this land now?

Comment 8

[Jeff Walden [:Waldo]]

There are a couple tweaky things I want to do here, but otherwise yes.  I'll land this with those tweaks when I'm on my laptop (yes) and the tree reopens (likely not til Monday).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e185710e8818
Mark computed property names as effectful in BCE.  r=jwalden

Comment 10

[Phil Ringnalda (:philor)]

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