1394490 - Fixup handling |this| in presence of NonSyntacticVariablesObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Ted Campbell [:tcampbell]]

In js::BoxNonStrictThis, we use global() directly for the fallback case. This is inconsistent with js::GetNonSyntacticGlobalThis.

Changes with this need to be mirrored into Baseline and Ion.

This is needed to support Bug 1186409 and support code that uses |(function() { return this; })()| to access the same NSVO that a top-level |this| would.

Comment 1

[Ted Campbell [:tcampbell]]

The testcase:

> var script = `
>     var functionThis, evalThis, strictEvalThis, globalThis = this;
>     (eval)("strictEvalThis = this;");
>     (function() { eval("evalThis = this;"); })();
>     (function() { functionThis = this; })();`;
>
> var env = {};
> evaluate(script, { envChainObject: env });
>
> assertEq(env, env.globalThis);
> assertEq(env, env.strictEvalThis);
> assertEq(env, env.evalThis);      // FAILS
> assertEq(env, env.functionThis);  // FAILS

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: Bug 1394490 - Avoid Ion compile for non-syntactic JSOP_FUNCTIONTHIS

Make non-strict JSOP_FUNCTIONTHIS handle non-syntactic global
environments in the same manner as JSOP_GLOBALTHIS and return the
non-syntactic environment instead of the outer global.

Review commit: https://reviewboard.mozilla.org/r/173880/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173880/

Comment 3

[Ted Campbell [:tcampbell]]

This issue applies to indirect calls where |this| is may be undefined inside callee. In the direct call case, we use JSOP_[G]IMPLICITTHIS which handles non-syntactic scopes directly.

> function t() { return this; }
> assertEq(t(), t.call(undefined)); // MISMATCH before patch

This effects subscript loader / XBL. I hope people aren't relying on the two to be different =\

Comment 4

[Ted Campbell [:tcampbell]]

Cancelling review request. This is breaking tests like test_bug993423.html

Comment 5

[Ted Campbell [:tcampbell]]

It isn't correct to call GetNonSyntacticGlobalThis from within a function. In XBL / DOM Event Handlers, we invoke functions with ExtensibleLexicalEnvironments and WithEnvironments. These get mistakenly caught by GetNonSyntacticGlobalThis. We really only want to support the NSVO case for JSOP_FUNCTIONTHIS fallback.

Comment 6

[Ted Campbell [:tcampbell]]

Comment on attachment 8902339 [details]
Bug 1394490 - Avoid Ion compile for non-syntactic JSOP_FUNCTIONTHIS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173880/diff/1-2/

Comment 7

[Ted Campbell [:tcampbell]]

Attachment: Bug 1394490 - Use global lexical this to initialize NSVO lexical

This is already current behavior, but make it explicit to allow cleanup
of NSVOs handling.

Review commit: https://reviewboard.mozilla.org/r/175096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175096/

Comment 8

[Ted Campbell [:tcampbell]]

Attachment: Bug 1394490 - Support NSVOs with JSOP_FUNCTIONTHIS fallback

When a non-strict function is called with undefined |this|, we must
substitute in the global |this|. This patch fixes this behavior when a
NonSyntacticVariablesObject is on envChain.

Review commit: https://reviewboard.mozilla.org/r/175098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175098/

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Bug 1394490 - Handle NSVO like Global for js::ComputeImplicitThis

ComputeImplicitThis is used when invoking a function in a |with| block.
An NSVO should behave like a GlobalObject and return |undefined|.

Review commit: https://reviewboard.mozilla.org/r/175100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175100/

Comment 10

[Ted Campbell [:tcampbell]]

Attachment: Bug 1394490 - Allow NSVO as global lexical |this|

Allow an NSVO to be a valid |this| target and don't skip over in
js::GetThisValue

Review commit: https://reviewboard.mozilla.org/r/175102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175102/

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 8902339 [details]
Bug 1394490 - Avoid Ion compile for non-syntactic JSOP_FUNCTIONTHIS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173880/diff/2-3/

Comment 12

[Ted Campbell [:tcampbell]]

These patches fix various rough edges around computing |this| when an NSVO is on environment chain. I also cleaned up the few edge cases where |this| was directly global() instead of global()->lexicalEnvironment()->thisValue() or nsvo->lexicalEnvironment()->thisValue(). This will let use us support both FrameScript (this === global) and JSM (this === nsvo) while using NSVO.

TODO:
  - IndirectEval currently always uses the global for |this|
  - BaselineCompiler::emit_JSOP_INITGLEXICAL() doesn't detect nonsyntactic environments. In practice the value is ignored, but it is a bit of a footgun that we pass around the wrong enviroment.
  - GlobalNameConflictsCheck in Ion doesn't handle nonsyntactic environments
  - Allow caller of LexicalEnvironmentObject::create to control |this|. After this we can finish Bug 1395360.

Comment 13

[Ted Campbell [:tcampbell]]

Comment on attachment 8903306 [details]
Bug 1394490 - Use global lexical this to initialize NSVO lexical

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175096/diff/1-2/

Comment 14

[Ted Campbell [:tcampbell]]

Comment on attachment 8903307 [details]
Bug 1394490 - Support NSVOs with JSOP_FUNCTIONTHIS fallback

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175098/diff/1-2/

Comment 15

[Ted Campbell [:tcampbell]]

Comment on attachment 8903308 [details]
Bug 1394490 - Handle NSVO like Global for js::ComputeImplicitThis

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175100/diff/1-2/

Comment 16

[Ted Campbell [:tcampbell]]

Comment on attachment 8903309 [details]
Bug 1394490 - Allow NSVO as global lexical |this|

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175102/diff/1-2/

Comment 17

[Ted Campbell [:tcampbell]]

Comment on attachment 8903306 [details]
Bug 1394490 - Use global lexical this to initialize NSVO lexical

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175096/diff/2-3/

Comment 18

[Ted Campbell [:tcampbell]]

Comment on attachment 8903307 [details]
Bug 1394490 - Support NSVOs with JSOP_FUNCTIONTHIS fallback

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175098/diff/2-3/

Comment 19

[Ted Campbell [:tcampbell]]

Comment on attachment 8903308 [details]
Bug 1394490 - Handle NSVO like Global for js::ComputeImplicitThis

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175100/diff/2-3/

Comment 20

[Ted Campbell [:tcampbell]]

Comment on attachment 8903309 [details]
Bug 1394490 - Allow NSVO as global lexical |this|

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175102/diff/2-3/

Comment 21

[Ted Campbell [:tcampbell]]

Attachment: Test for FrameScript loader environment behavior

xpcshell-based test case. I need to flush it out a bit more before checkin.

Comment 22

[Ted Campbell [:tcampbell]]

These patches are cleanup and scaffolding to support Bug 1186409. Content script and FrameScript behavior is preserved, but it will now be possible to support JSMs with NSVO environments.

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 8902339 [details]
Bug 1394490 - Avoid Ion compile for non-syntactic JSOP_FUNCTIONTHIS

https://reviewboard.mozilla.org/r/173880/#review180494

::: js/src/jit/IonBuilder.cpp:12453
(Diff revision 3)
>          return Ok();
>      }
>  
> +    // Beyond this point we may need to access non-syntactic global. Ion doesn't
> +    // currently support this so just abort.
> +    if (script()->hasNonSyntacticScope()) {

Nit: no {}

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 8903306 [details]
Bug 1394490 - Use global lexical this to initialize NSVO lexical

https://reviewboard.mozilla.org/r/175096/#review180498

Comment 25

[Jan de Mooij [:jandem]]

Comment on attachment 8903307 [details]
Bug 1394490 - Support NSVOs with JSOP_FUNCTIONTHIS fallback

https://reviewboard.mozilla.org/r/175098/#review180502

::: js/src/vm/Interpreter.cpp:135
(Diff revision 3)
> +
> +    // If there is a NSVO on environment chain, use it as basis for fallback
> +    // global |this|. This gives a consistent definition of global lexical
> +    // |this| between function and global contexts.
> +    //
> +    // NOTE: If only non-synactic WithEnvironments are on the chain, we use the

Nit: non-syntactic, typo

Comment 26

[Jan de Mooij [:jandem]]

Comment on attachment 8903308 [details]
Bug 1394490 - Handle NSVO like Global for js::ComputeImplicitThis

https://reviewboard.mozilla.org/r/175100/#review180504

Comment 27

[Jan de Mooij [:jandem]]

Comment on attachment 8903309 [details]
Bug 1394490 - Allow NSVO as global lexical |this|

https://reviewboard.mozilla.org/r/175102/#review180510

Comment 28

[Ted Campbell [:tcampbell]]

[leave-open] Check-in some test cases and open follow-up bugs.

Comment 29

[Ted Campbell [:tcampbell]]

Comment on attachment 8902339 [details]
Bug 1394490 - Avoid Ion compile for non-syntactic JSOP_FUNCTIONTHIS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173880/diff/3-4/

Comment 30

[Ted Campbell [:tcampbell]]

Comment on attachment 8903306 [details]
Bug 1394490 - Use global lexical this to initialize NSVO lexical

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175096/diff/3-4/

Comment 31

[Ted Campbell [:tcampbell]]

Comment on attachment 8903307 [details]
Bug 1394490 - Support NSVOs with JSOP_FUNCTIONTHIS fallback

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175098/diff/3-4/

Comment 32

[Ted Campbell [:tcampbell]]

Comment on attachment 8903308 [details]
Bug 1394490 - Handle NSVO like Global for js::ComputeImplicitThis

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175100/diff/3-4/

Comment 33

[Ted Campbell [:tcampbell]]

Comment on attachment 8903309 [details]
Bug 1394490 - Allow NSVO as global lexical |this|

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175102/diff/3-4/

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83971e2f5337
Avoid Ion compile for non-syntactic JSOP_FUNCTIONTHIS r=jandem
https://hg.mozilla.org/integration/autoland/rev/f44c599a0110
Use global lexical this to initialize NSVO lexical r=jandem
https://hg.mozilla.org/integration/autoland/rev/6b95d9be372f
Support NSVOs with JSOP_FUNCTIONTHIS fallback r=jandem
https://hg.mozilla.org/integration/autoland/rev/9441edee0b33
Handle NSVO like Global for js::ComputeImplicitThis r=jandem
https://hg.mozilla.org/integration/autoland/rev/1d750d1ce2fc
Allow NSVO as global lexical |this| r=jandem

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/83971e2f5337
https://hg.mozilla.org/mozilla-central/rev/f44c599a0110
https://hg.mozilla.org/mozilla-central/rev/6b95d9be372f
https://hg.mozilla.org/mozilla-central/rev/9441edee0b33
https://hg.mozilla.org/mozilla-central/rev/1d750d1ce2fc

Comment 36

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1382844-Add-JS_FAKECPUCOUNT-environment-variable.patch

This adds some xpcshell tests that use our different embeddings and ensure environment behavior hasn't changed on us.

Comment 37

[Ted Campbell [:tcampbell]]

Attachment: 0006-Bug-1394490-Javascript-loader-environments-test.patch

Test the environment seen by our embeddings hasn't changed.

(This time with correct patch file...)

Comment 38

[Kris Maglione [:kmag]]

Comment on attachment 8904699 [details] [diff] [review]
0006-Bug-1394490-Javascript-loader-environments-test.patch

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

::: js/xpconnect/tests/unit/test_FrameScriptEnvironment.js
@@ +19,5 @@
> +    try { void fi; bound += "fi,"; } catch (e) {}
> +    try { void fd; bound += "fd,"; } catch (e) {}
> +
> +    // FrameScript loader should share |this| access
> +    if (bound != "gt,ed,ei,fo,fi,fd,")

We should check `isStrict` here, too. Otherwise when we turn mandatory strict mode for frame scripts, this will fail.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/753fa54f005b
Javascript loader environments test. r=kmag

Comment 40

[Ted Campbell [:tcampbell]]

Added strict check to test_FrameScriptEnvironment. Also put that whole test case in an add_task() so that I can receive message in parent process (test was always passing otherwise).

Comment 41

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

https://hg.mozilla.org/mozilla-central/rev/753fa54f005b