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.

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

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 =\

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

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.

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.

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

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 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 on attachment 8903306 [details] Bug 1394490 - Use global lexical this to initialize NSVO lexical https://reviewboard.mozilla.org/r/175096/#review180498

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 on attachment 8903308 [details] Bug 1394490 - Handle NSVO like Global for js::ComputeImplicitThis https://reviewboard.mozilla.org/r/175100/#review180504

Comment on attachment 8903309 [details] Bug 1394490 - Allow NSVO as global lexical |this| https://reviewboard.mozilla.org/r/175102/#review180510

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

Pushed by tcampbell@mozilla.com: 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

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

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

Test the environment seen by our embeddings hasn't changed. (This time with correct patch file...)

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.

Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/753fa54f005b Javascript loader environments test. r=kmag

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).