Closed Bug 1397385 Opened 3 years ago Closed 3 years ago

ComputeImplicitThis leaks VarEnvironmentObjects to script

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

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: nobody → tcampbell
Status: NEW → ASSIGNED
Blocks: 1385429
Duplicate of this bug: 1385429
Could we include the test case in bug 1385429 comment 5 in this patch?
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+
(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.
Fixed test cases as requested. Also did same whitelist for GetThisValue as discussed with :jandem on IRC.
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+
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.
Would this comment be good?
Attachment #8905684 - Flags: review?(jdemooij)
Attachment #8905768 - Flags: review?(jdemooij)
Attachment #8905231 - Flags: review?(jdemooij)
Attachment #8905769 - Flags: review?(jdemooij)
Attachment #8905231 - Flags: review?(jdemooij)
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 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 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 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 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+
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
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)
Revised per comments.
Attachment #8905684 - Attachment is obsolete: true
Attachment #8912907 - Flags: review?(tcampbell)
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+
Attachment #8912907 - Attachment is obsolete: true
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 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) \
>     /*
I should not be allowed to operate heavy machinery today.
Attachment #8912908 - Attachment is obsolete: true
Attachment #8912910 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5b9b4480c4
Clarify use of JSOP_GIMPLICITTHIS. r=tcampbell
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/17a2791683e8

Seemed like a good idea anyway.
You need to log in before you can comment on or make changes to this bug.