Closed
Bug 1397385
Opened 7 years ago
Closed 7 years ago
ComputeImplicitThis leaks VarEnvironmentObjects to script
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: tcampbell, Assigned: tcampbell)
References
Details
Attachments
(4 files, 3 obsolete files)
ComputeImplicitThis does not handle VarEnvironmentObjects (and ModuleEnvironmentObjects) correctly. For example, |(function (a = eval("function g() { return this; }; with({}) { g() }")) { return a })()| will return the environment that the function default parameter is expanded in. Since the holder of |g| is the VarEnvironmentObject, it gets passed to ComputeImplicitThis which returns the env instead of undefined.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tcampbell
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
Could we include the test case in bug 1385429 comment 5 in this patch?
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8905231 [details] Bug 1397385 - Fixup js::ComputeImplicitThis to not leak environments https://reviewboard.mozilla.org/r/177020/#review182266 Good find! This is much nicer. ::: js/src/tests/ecma_6/Function/implicit-this-in-parameter-expression.js:15 (Diff revision 1) > + } > + `)) { > + return a > +}; > + > +assertEq(f(), undefined); Unlike jit-tests, jstests need to report success: if (typeof reportCompare === "function") reportCompare(true, true);
Attachment #8905231 -
Flags: review?(jdemooij) → review+
Comment 5•7 years ago
|
||
(In reply to Jim Blandy :jimb from comment #3) > Could we include the test case in bug 1385429 comment 5 in this patch? Agreed a test in jit-test/tests/debug/ would be nice. Should probably remove the non-standard uneval and add a correctness check.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Fixed test cases as requested. Also did same whitelist for GetThisValue as discussed with :jandem on IRC.
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8905231 [details] Bug 1397385 - Fixup js::ComputeImplicitThis to not leak environments https://reviewboard.mozilla.org/r/177020/#review182404 Clearing review as discussed on IRC.
Attachment #8905231 -
Flags: review+
Comment 12•7 years ago
|
||
The rewrite of ComputeImplicitThis makes the problem unlikely, but it might be good to test calling a function found in a module, so that we exercise the case where env is an EnvironmentModuleObject.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8905768 -
Flags: review?(jdemooij)
Attachment #8905231 -
Flags: review?(jdemooij)
Attachment #8905769 -
Flags: review?(jdemooij)
Assignee | ||
Updated•7 years ago
|
Attachment #8905231 -
Flags: review?(jdemooij)
Assignee | ||
Comment 17•7 years ago
|
||
Alright, did some refactoring so it is a less magic. This fixes the VarEnvironmentObject leak, but otherwise is just pushing code around. I tried to document some of the oddities expected of our embedding now that they can't hide as well in the code. One large source of ugliness is the SubscriptLoader. It has silly compatibility requirements: GlobalThis -> with target ImplicitThis -> with target FunctionThis(null) -> global This is why https://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/js/src/vm/Interpreter.cpp#140 doesn't match https://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/js/src/vm/Interpreter.cpp#163 I'd like to do more investigation on DebugEnvironmentProxies and see if Bug 1397049 is related as a separate bug.
Comment 18•7 years ago
|
||
Comment on attachment 8905684 [details] [diff] [review] Clarify use of JSOP_GIMPLICITTHIS. Review of attachment 8905684 [details] [diff] [review]: ----------------------------------------------------------------- Forwarding to Ted.
Attachment #8905684 -
Flags: review?(jdemooij) → review?(tcampbell)
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8905231 [details] Bug 1397385 - Fixup js::ComputeImplicitThis to not leak environments https://reviewboard.mozilla.org/r/177020/#review182750 ::: js/src/vm/Interpreter.cpp:1487 (Diff revision 6) > - return UndefinedValue(); > + if (env->is<GlobalObject>()) > - > - if (obj->is<NonSyntacticVariablesObject>()) > return UndefinedValue(); > > - if (obj->is<WithEnvironmentObject>()) > + // WithEnvironmentObject have an actual implicit |this| Nit: s/have/has/ or s/Object/Objects/ ? Also add a period at the end while you're here. ::: js/src/vm/Interpreter.cpp:1497 (Diff revision 6) > // non-syntactic, they wrap syntactic environments and should not be > // treated like other embedding-specific non-syntactic environments. > - if (obj->is<DebugEnvironmentProxy>()) > - return ComputeImplicitThis(&obj->as<DebugEnvironmentProxy>().environment()); > + if (env->is<DebugEnvironmentProxy>()) > + return ComputeImplicitThis(&env->as<DebugEnvironmentProxy>().environment()); > > - return GetThisValue(obj); > + MOZ_ASSERT(env->is<EnvironmentObject>()); Invariants! \o/
Attachment #8905231 -
Flags: review?(jdemooij) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8905768 [details] Bug 1397385 - Refactor js::GetThisValue https://reviewboard.mozilla.org/r/177570/#review182752 ::: js/src/vm/EnvironmentObject.cpp:1074 (Diff revision 1) > MOZ_ASSERT(isExtensible()); > Value v = getReservedSlot(THIS_VALUE_OR_SCOPE_SLOT); > if (v.isObject()) { > - // If `v` is a Window, return the WindowProxy instead. We called > - // GetThisValue (which also does ToWindowProxyIfWindow) when storing > - // the value in THIS_VALUE_OR_SCOPE_SLOT, but it's possible the > + // A WindowProxy may have been attached after this environment was > + // created so check ToWindowProxyIfWindow again. For example, > + // GlobalObject::createInternal will construct it's lexical environment Nit: s/it's/its/
Attachment #8905768 -
Flags: review?(jdemooij) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8905769 [details] Bug 1397385 - Simplify js::GetThisValue https://reviewboard.mozilla.org/r/177572/#review182756 Much simpler! Very nice. ::: js/src/jsobj.cpp:3259 (Diff revision 1) > } > > Value > js::GetThisValue(JSObject* obj) > { > + // Outerize globals Nit: I'd prefer sticking to Window/WindowProxy inside spidermonkey. Inner/outer windows are more of a Gecko-ism (SpiderMonkey used to have innerObject/outerObject Class hooks but I removed them a few years ago). Maybe say something like: Use the WindowProxy if the global is a Window, as the Window must never be exposed to JS.
Attachment #8905769 -
Flags: review?(jdemooij) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7e2a1d9ffd7a Refactor js::GetThisValue r=jandem https://hg.mozilla.org/integration/autoland/rev/aac13bc10a72 Fixup js::ComputeImplicitThis to not leak environments r=jandem https://hg.mozilla.org/integration/autoland/rev/0d384af83d69 Simplify js::GetThisValue r=jandem
Assignee | ||
Comment 26•7 years ago
|
||
Comment on attachment 8905684 [details] [diff] [review] Clarify use of JSOP_GIMPLICITTHIS. Review of attachment 8905684 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Opcodes.h @@ +1626,5 @@ > macro(JSOP_STRICTSETGNAME, 156, "strict-setgname", NULL, 5, 2, 1, JOF_ATOM|JOF_NAME|JOF_PROPSET|JOF_DETECTING|JOF_GNAME|JOF_CHECKSTRICT) \ > /* > * Pushes the implicit 'this' value for calls to the associated name onto > + * the stack; only used when we know the implicit this, if any, will be our > + * first non-syntactic scope. This was never quite right in first place. The implicit this can be found on any non-syntactic scope (or the global). Should be something like 'only used when the implicit this might be found on a non-syntactic scope outside this script'. @@ +1633,5 @@ > + * objects on its scope chain, which are non-syntactic environments that > + * refer to syntactic environments. So this opcode does not imply that the > + * function being called was found on some ordinary JS object that is safe > + * to supply as a 'this` value; it may have been found on a CallObject or > + * VarEnvironmentObject referred to by a DebugEnvironmentProxy. First sentence helps. The second sentence confuses me because the holder could already be a CallObject so the filtering is already required. Instead point out that 'As a result, the binding we want may be held by a syntactic environments such as CallObject / VarEnvrionmentObject.'
Attachment #8905684 -
Flags: review?(tcampbell)
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7e2a1d9ffd7a https://hg.mozilla.org/mozilla-central/rev/aac13bc10a72 https://hg.mozilla.org/mozilla-central/rev/0d384af83d69
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 28•7 years ago
|
||
Revised per comments.
Attachment #8905684 -
Attachment is obsolete: true
Attachment #8912907 -
Flags: review?(tcampbell)
Assignee | ||
Comment 29•7 years ago
|
||
Comment on attachment 8912907 [details] [diff] [review] Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD Review of attachment 8912907 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for updating this doc.
Attachment #8912907 -
Flags: review?(tcampbell) → review+
Comment 30•7 years ago
|
||
Updated•7 years ago
|
Attachment #8912907 -
Attachment is obsolete: true
Comment 31•7 years ago
|
||
Comment on attachment 8912908 [details] [diff] [review] Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD r=tcampbell Forgot to update patch per last suggestion on IRC. Carrying over r+.
Attachment #8912908 -
Attachment description: Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD → Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD r=tcampbell
Attachment #8912908 -
Flags: review+
Comment 32•7 years ago
|
||
Comment on attachment 8912908 [details] [diff] [review] Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD r=tcampbell ># HG changeset patch ># User Jim Blandy <jimb@mozilla.com> ># Date 1504820250 25200 ># Thu Sep 07 14:37:30 2017 -0700 ># Node ID 62c6fed3a473a19e2820f88ae796bef12be2d74c ># Parent 756e10aa8bbd416cbc49b7739f78fb81d5525477 >Bug 1397385: Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD r=tcampbell > >diff --git a/js/src/vm/Opcodes.h b/js/src/vm/Opcodes.h >--- a/js/src/vm/Opcodes.h >+++ b/js/src/vm/Opcodes.h >@@ -1621,18 +1621,24 @@ 1234567890123456789012345678901234567890 > * Category: Variables and Scopes > * Type: Free Variables > * Operands: uint32_t nameIndex > * Stack: env, val => val > */ \ > macro(JSOP_STRICTSETGNAME, 156, "strict-setgname", NULL, 5, 2, 1, JOF_ATOM|JOF_NAME|JOF_PROPSET|JOF_DETECTING|JOF_GNAME|JOF_CHECKSTRICT) \ > /* > * Pushes the implicit 'this' value for calls to the associated name onto >- * the stack; only used when we know this implicit this will be our first >- * non-syntactic scope. >+ * the stack; only used when the implicit this might be derived from a >+ * non-syntactic scope (instead of the global itself). >+ * >+ * Note that code evaluated via the Debugger API uses DebugEnvironmentProxy >+ * objects on its scope chain, which are non-syntactic environments that >+ * refer to syntactic environments. As a result, the binding we want may be >+ * held by a syntactic environments such as CallObject or >+ * VarEnvrionmentObject. > * > * Category: Variables and Scopes > * Type: This > * Operands: uint32_t nameIndex > * Stack: => this > */ \ > macro(JSOP_GIMPLICITTHIS, 157, "gimplicitthis", "", 5, 0, 1, JOF_ATOM) \ > /*
Comment 33•7 years ago
|
||
I should not be allowed to operate heavy machinery today.
Attachment #8912908 -
Attachment is obsolete: true
Attachment #8912910 -
Flags: review+
Updated•7 years ago
|
Keywords: checkin-needed
Comment 34•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5b9b4480c4 Clarify use of JSOP_GIMPLICITTHIS. r=tcampbell
Keywords: checkin-needed
Comment 35•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/17a2791683e8 Seemed like a good idea anyway.
Comment 36•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5d5b9b4480c4
You need to log in
before you can comment on or make changes to this bug.
Description
•