Closed Bug 1394490 Opened 7 years ago Closed 7 years ago

Fixup handling |this| in presence of NonSyntacticVariablesObject

Categories

(Core :: JavaScript Engine, enhancement, P1)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

Details

Attachments

(6 files, 2 obsolete files)

Bug 1394490 - Avoid Ion compile for non-syntactic JSOP_FUNCTIONTHIS
59 bytes, text/x-review-board-request
jandem
: review+
Details
Bug 1394490 - Use global lexical this to initialize NSVO lexical
59 bytes, text/x-review-board-request
jandem
: review+
Details
Bug 1394490 - Support NSVOs with JSOP_FUNCTIONTHIS fallback
59 bytes, text/x-review-board-request
jandem
: review+
Details
Bug 1394490 - Handle NSVO like Global for js::ComputeImplicitThis
59 bytes, text/x-review-board-request
jandem
: review+
Details
Bug 1394490 - Allow NSVO as global lexical |this|
59 bytes, text/x-review-board-request
jandem
: review+
Details
0006-Bug-1394490-Javascript-loader-environments-test.patch
8.18 KB, patch
kmag
: review+
Details | Diff | Splinter Review 
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.
Blocks: 1186409
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
Attachment #8902339 - Flags: review?(jdemooij)
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 =\
Attachment #8902339 - Flags: review?(jdemooij)
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.
Assignee: nobody → tcampbell
Summary: JSOP_FUNCTIONTHIS should support non-syntactic lexicals in fallback case → Fixup handling |this| in presence of NonSyntacticVariablesObject
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.
Blocks: 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.
Attachment #8902339 - Flags: review?(jdemooij)
Attachment #8903306 - Flags: review?(jdemooij)
Attachment #8903307 - Flags: review?(jdemooij)
Attachment #8903308 - Flags: review?(jdemooij)
Attachment #8903309 - Flags: review?(jdemooij)
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 {}
Attachment #8902339 - Flags: review?(jdemooij) → review+
Comment on attachment 8903306 [details]
Bug 1394490 - Use global lexical this to initialize NSVO lexical

https://reviewboard.mozilla.org/r/175096/#review180498
Attachment #8903306 - Flags: review?(jdemooij) → review+
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
Attachment #8903307 - Flags: review?(jdemooij) → review+
Comment on attachment 8903308 [details]
Bug 1394490 - Handle NSVO like Global for js::ComputeImplicitThis

https://reviewboard.mozilla.org/r/175100/#review180504
Attachment #8903308 - Flags: review?(jdemooij) → review+
Comment on attachment 8903309 [details]
Bug 1394490 - Allow NSVO as global lexical |this|

https://reviewboard.mozilla.org/r/175102/#review180510
Attachment #8903309 - Flags: review?(jdemooij) → review+
[leave-open] Check-in some test cases and open follow-up bugs.
Keywords: leave-open
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
Blocks: 1396050
Status: NEW → ASSIGNED
Priority: -- → P1
This adds some xpcshell tests that use our different embeddings and ensure environment behavior hasn't changed on us.
Attachment #8903444 - Attachment is obsolete: true
Attachment #8904698 - Flags: review?(kmaglione+bmo)
Test the environment seen by our embeddings hasn't changed.

(This time with correct patch file...)
Attachment #8904698 - Attachment is obsolete: true
Attachment #8904698 - Flags: review?(kmaglione+bmo)
Attachment #8904699 - Flags: review?(kmaglione+bmo)
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.
Attachment #8904699 - Flags: review?(kmaglione+bmo) → review+
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/753fa54f005b
Javascript loader environments test. r=kmag
Keywords: leave-open
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).
https://hg.mozilla.org/mozilla-central/rev/753fa54f005b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: