1341061 - post-increment operator inside "with" ends up checking @@unscopables twice

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Testcase (from es-discuss; see <https://mail.mozilla.org/pipermail/es-discuss/2017-February/047724.html>):

    var a, b, flag = true
    with (a = { x: 7 })
      with (b = { x: 4, get [Symbol.unscopables]() { return { x: flag=!flag } } })
        x++

after that executes, a.x should be 7 and b.x should be 5, as far as I can tell.  We end up with b.x == 8 instead.

The reason for that is that we get @@unscopables twice: the first time from JSOP_BINDNAME calling LookupNameUnqualified etc, and the second time from JSOP_GETNAME calling GetNameOperation which calls js::LookupName which calls js::LookupProperty which calls into with_LookupProperty _again_.

As far as I can tell per spec, only the bindname should end up doing a property lookup kinda thing in the "x++" case.  The get should use the existing Reference, as should the Set.

Comment 1

[Shu-yu Guo [:shu]]

Oo, thanks for debugging this bz.

For these read-modify-write ops, we'll need a GETNAME variant that uses the environment BINDNAME already pushed onto the scope.

Comment 2

[Shu-yu Guo [:shu]]

A note for posterity: AIUI, the Reference returned when looking up a name in the |with| scope, if found, has as its object base the environment record (the "receiver" instead of the actual object where the property was found). So while getting @@unscopables should happen only once, things like looking up the prototype to do the actual get/set happen multiple times.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Refactor NAME-related runtime functions.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Fix Opcodes.h comments to say 'environment' instead of 'scope' to refer to the runtime objects.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Introduce BINDGET[G]NAME ops which fuse together BINDNAME and GETNAME.

Comment 6

[Shu-yu Guo [:shu]]

Jan, I'm having trouble figuring out how to JIT the BINDGETNAME ops I introduced above. The op basically fuses together BINDNAME and GETNAME: it pushes both the environment the name is found on and the value of the name. No other ops currently push two values, and I don't know how to make VMFunctions return 2 values. Any ideas?

In exploring the design space of the fix, I also played around with decomposing BINDGETNAME to 2 ops:
  1. FINDNAME, which finds the env and raises an error if not found (unlike BINDNAME)
  2. GETFOUNDNAME, which gets the name off the env on the top of the stack.

The problem with that approach is that it's inefficient. FINDNAME basically does a LookupNameUnqualified, which, during the lookup, returns a PropertyResult if the name is found. This should be communicated to subsequent GETFOUNDNAME so that we don't always do the slowest, most generic GetProperty. Secondarily, this split also makes the VM code a little messier.

Comment 7

[Shu-yu Guo [:shu]]

Ted, whatever approach we decide on in comment 6, I'd really appreciate someone doing the JIT parts. Up for it?

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8839758 [details] [diff] [review]
Refactor NAME-related runtime functions.

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

Nice refactoring :)

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8839759 [details] [diff] [review]
Fix Opcodes.h comments to say 'environment' instead of 'scope' to refer to the runtime objects.

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

::: js/src/vm/Opcodes.h
@@ +1090,4 @@
>       *   Category: Variables and Scopes
>       *   Type: Variables
>       *   Operands: uint32_t nameIndex
>       *   Stack: => scope

=> env

Comment 10

[Jan de Mooij [:jandem]]

Clearing the NI per our IRC discussion.

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Rename GETXPROP to GETBOUNDNAME for clarity.

Comment 12

[Shu-yu Guo [:shu]]

Attachment: Manually unwrap WithEnvironmentObjects in GETBOUNDNAME.

Also refactor some stuff in this area.

Documenting why I think it's correct to use GETBOUNDNAME only for
dynamic (vs global) lookups, without the normal NAME checks:

1. GETBOUNDNAME doesn't need to check TDZ because BINDNAME does it
   already, and GETBOUNDNAME is always preceded by BINDNAME.

2. '.this' doesn't need to be checked, because '.this' can't be assigned
   to in a compound assignment or inc/dec.

3. For a global name there is never @@unscopables on the global
   environments, so GNAME ops, while doing repeated lookups, remain valid
   optimizations because the repetition is unobservable.

4. We *will* do double @@unscopable lookup in the case of a script
   compiled for a syntactic scope then run under a non-syntactic
   WithEnvironmentObject (e.g. like frame scripts). But this is
   Gecko-specific and outside the spec, so we can do whatever we want
   here.

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 8840236 [details] [diff] [review]
Rename GETXPROP to GETBOUNDNAME for clarity.

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

::: js/src/vm/Opcodes.h
@@ +1997,2 @@
>      /*
>       * Pops the top of stack value, gets an extant property value of it,

would be nice to say what "bound" is.
(and also remove "extant" maybe?

Comment 14

[Shu-yu Guo [:shu]]

Attachment: Test.

Will remove once https://github.com/tc39/test262/pull/869 merges and we
sync.

Comment 15

[Tooru Fujisawa [:arai]]

Comment on attachment 8840237 [details] [diff] [review]
Manually unwrap WithEnvironmentObjects in GETBOUNDNAME.

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

thank you for detailed explanation :)

::: js/src/vm/NativeObject.cpp
@@ +2135,5 @@
> +    // up via BINDNAME, calling HasProperty on the WithEnvironmentObject is
> +    // equivalent to calling HasBinding a second time. This results in the
> +    // incorrect behavior of performing the @@unscopables check again.
> +    RootedObject env(cx, MaybeUnwrapSyntacticWithEnvironment(envArg));
> +    RootedValue receiver(cx, ObjectValue(*env));

wouldn't it be nice to avoid having 2 roots for them?
just moving `MaybeUnwrapSyntacticWithEnvironment(envArg)` to ObjectValue argument will reduce extra root.

Comment 16

[Tooru Fujisawa [:arai]]

Comment on attachment 8840249 [details] [diff] [review]
Test.

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

::: js/src/tests/test262/local/unscopables-inc-dec.js
@@ +20,5 @@
> +---*/
> +
> +var a, b, flag = true;
> +with (a = { x: 7 }) {
> +  with (b = { x: 4, get [Symbol.unscopables]() { return { x: flag=!flag }; } }) {

might be nice to count how many times the getter is called.
to catch the case that it's called 3 times (maybe just a overkill tho)

Comment 17

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #15)
> Comment on attachment 8840237 [details] [diff] [review]
> Manually unwrap WithEnvironmentObjects in GETBOUNDNAME.
> 
> Review of attachment 8840237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> thank you for detailed explanation :)
> 
> ::: js/src/vm/NativeObject.cpp
> @@ +2135,5 @@
> > +    // up via BINDNAME, calling HasProperty on the WithEnvironmentObject is
> > +    // equivalent to calling HasBinding a second time. This results in the
> > +    // incorrect behavior of performing the @@unscopables check again.
> > +    RootedObject env(cx, MaybeUnwrapSyntacticWithEnvironment(envArg));
> > +    RootedValue receiver(cx, ObjectValue(*env));
> 
> wouldn't it be nice to avoid having 2 roots for them?
> just moving `MaybeUnwrapSyntacticWithEnvironment(envArg)` to ObjectValue
> argument will reduce extra root.

Note that |env| is also passed to the GetProperty calls below, so moving that Unwrap call into the ObjectValue isn't correct.

Comment 18

[Tooru Fujisawa [:arai]]

(In reply to Shu-yu Guo [:shu] from comment #17)
> Note that |env| is also passed to the GetProperty calls below, so moving
> that Unwrap call into the ObjectValue isn't correct.

wow, overlooked that.
thanks.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82cdbd24d06f
Refactor NAME-related runtime functions. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb11cb34069
Fix Opcodes.h comments to say 'environment' instead of 'scope' to refer to the runtime objects. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/76c74d43a9b0
Rename GETXPROP to GETBOUNDNAME for clarity. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/559f43c43369
Manually unwrap WithEnvironmentObjects in GETBOUNDNAME. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/456c1dcfe087
Test. (r=arai)

Comment 20

[Shu-yu Guo [:shu]]

Since we're using GETBOUNDNAME (GETXPROP) for non-global name lookups only, I don't think there's a pressing need to JIT anything any longer.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing crashtest 366271-1.html and mochitest test_bug841466.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5a2c80de72fc3b32c2ef44006e75c44917453d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b972ab5bd1a8cc31c7fe60ba0862cd8a845a709
https://hg.mozilla.org/integration/mozilla-inbound/rev/49187b2a839656915a614feac8de963bbe35483e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e6402a66c388736bb193fcd4c9d40594785ca4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/39be2cc943a7c3b48104811065ae0768de8eeaf6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cc682c2db247433a4aad347f0d20ddbc968eb26b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log crashtest: https://treeherder.mozilla.org/logviewer.html#?job_id=80073717&repo=mozilla-inbound
[task 2017-02-24T21:31:44.077217Z] 21:31:44     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/layout/base/crashtests/366271-1.html | load failed: timed out waiting for reftest-wait to be removed

Failure log mochitest: [task 2017-02-24T21:31:44.077217Z] 21:31:44     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/layout/base/crashtests/366271-1.html | load failed: timed out waiting for reftest-wait to be removed
[task 2017-02-24T21:55:38.374356Z] 21:55:38     INFO - TEST-START | dom/html/test/test_bug841466.html
[task 2017-02-24T21:55:38.599943Z] 21:55:38     INFO - ++DOMWINDOW == 30 (0x7f658b720400) [pid = 3356] [serial = 1032] [outer = 0x7f658f12d800]
[task 2017-02-24T21:55:38.883324Z] 21:55:38     INFO - TEST-INFO | started process screentopng
[task 2017-02-24T21:55:40.760073Z] 21:55:40     INFO - TEST-INFO | screentopng: exit 0
[task 2017-02-24T21:55:40.765548Z] 21:55:40     INFO - TEST-UNEXPECTED-FAIL | dom/html/test/test_bug841466.html | TypeError: 'get localName' called on an object that does not implement interface Element. 
[task 2017-02-24T21:55:40.765932Z] 21:55:40     INFO -     onclick@dom/html/test/test_bug841466.html:1:96
[task 2017-02-24T21:55:40.767340Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:24:3
[task 2017-02-24T21:55:40.767588Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:15:1
[task 2017-02-24T21:55:40.770465Z] 21:55:40     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-02-24T21:55:40.772535Z] 21:55:40     INFO - TEST-UNEXPECTED-FAIL | dom/html/test/test_bug841466.html | TypeError: 'get localName' called on an object that does not implement interface Element. 
[task 2017-02-24T21:55:40.774501Z] 21:55:40     INFO -     onclick@dom/html/test/test_bug841466.html:1:96
[task 2017-02-24T21:55:40.779147Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:24:3
[task 2017-02-24T21:55:40.785473Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:15:1
[task 2017-02-24T21:55:40.787618Z] 21:55:40     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-02-24T21:55:40.789890Z] 21:55:40     INFO - TEST-UNEXPECTED-FAIL | dom/html/test/test_bug841466.html | TypeError: 'get localName' called on an object that does not implement interface Element. 
[task 2017-02-24T21:55:40.793373Z] 21:55:40     INFO -     onclick@dom/html/test/test_bug841466.html:1:96
[task 2017-02-24T21:55:40.798716Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:24:3
[task 2017-02-24T21:55:40.800464Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:15:1
[task 2017-02-24T21:55:40.802578Z] 21:55:40     INFO - TEST-FAIL | dom/html/test/test_bug841466.html | Bug 101019 - got "select", expected "keygen"
[task 2017-02-24T21:55:40.805206Z] 21:55:40     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-02-24T21:55:40.808745Z] 21:55:40     INFO - TEST-UNEXPECTED-FAIL | dom/html/test/test_bug841466.html | TypeError: 'get localName' called on an object that does not implement interface Element. 
[task 2017-02-24T21:55:40.813463Z] 21:55:40     INFO -     onclick@dom/html/test/test_bug841466.html:1:96
[task 2017-02-24T21:55:40.818716Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:24:3
[task 2017-02-24T21:55:40.820958Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:15:1
[task 2017-02-24T21:55:40.822834Z] 21:55:40     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-02-24T21:55:40.824801Z] 21:55:40     INFO - TEST-UNEXPECTED-FAIL | dom/html/test/test_bug841466.html | TypeError: 'get localName' called on an object that does not implement interface Element. 
[task 2017-02-24T21:55:40.826614Z] 21:55:40     INFO -     onclick@dom/html/test/test_bug841466.html:1:96
[task 2017-02-24T21:55:40.830135Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:24:3
[task 2017-02-24T21:55:40.834934Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:15:1
[task 2017-02-24T21:55:40.836982Z] 21:55:40     INFO - TEST-PASS | dom/html/test/test_bug841466.html | expected value bar from expando on element select 
[task 2017-02-24T21:55:40.839413Z] 21:55:40     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-02-24T21:55:40.841637Z] 21:55:40     INFO - TEST-UNEXPECTED-FAIL | dom/html/test/test_bug841466.html | TypeError: 'get localName' called on an object that does not implement interface Element. 
[task 2017-02-24T21:55:40.844276Z] 21:55:40     INFO -     onclick@dom/html/test/test_bug841466.html:1:96
[task 2017-02-24T21:55:40.848455Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:24:3
[task 2017-02-24T21:55:40.850421Z] 21:55:40     INFO -     @dom/html/test/test_bug841466.html:15:1

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e4be5b2b23d
Refactor NAME-related runtime functions. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a295d7cb1e9b
Fix Opcodes.h comments to say 'environment' instead of 'scope' to refer to the runtime objects. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd554e057c6
Rename GETXPROP to GETBOUNDNAME for clarity. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/80067470282b
Manually unwrap WithEnvironmentObjects in GETBOUNDNAME. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/561a3cd11c7f
Test. (r=arai)

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3e4be5b2b23d
https://hg.mozilla.org/mozilla-central/rev/a295d7cb1e9b
https://hg.mozilla.org/mozilla-central/rev/0dd554e057c6
https://hg.mozilla.org/mozilla-central/rev/80067470282b
https://hg.mozilla.org/mozilla-central/rev/561a3cd11c7f

Comment 24

[Boris Zbarsky [:bzbarsky]]

What was the issue that caused comment 21?

Comment 25

[Shu-yu Guo [:shu]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #24)
> What was the issue that caused comment 21?

When a getter is called with a WithEnvirnomentObject as the receiver, we unwrap the WithEnvironmentObject, since the getter expects the unwrapped object to be the receiver. This needs to happen regardless of the WithEnvironmentObject being syntactic or non-syntactic. In one of my refactoring patches above, I incorrectly refactored code in FetchName to only unwrap syntactic WithEnvironmentObjects.

The test in comment 21 has an svg with an onload script: http://searchfox.org/mozilla-central/source/layout/base/crashtests/366271-1-frame.svg#3

That onload script uses |location|, which is a getter on a non-syntactic WithEnvironmentObject on the env chain. The incorrect refactoring caused that getter to fail.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a03a46e5e8a
Followup: remove local copy of unscopables-inc-dec.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a03a46e5e8a