Bug 1397385 - Fixup js::ComputeImplicitThis to not leak environments draft
authorTed Campbell <tcampbell@mozilla.com>
Thu, 07 Sep 2017 19:51:42 -0400
changeset 661491 eab21506d9dcc653c0fea1a5d09ae3d6b8f36181
parent 661490 c4ae9d9822c855a2b5f78cad2ad1c10b48ce40ee
child 661492 309022953108679a883bf5cc5ae2365334df96d6
push id78779
push userbmo:tcampbell@mozilla.com
push dateFri, 08 Sep 2017 14:53:54 +0000
bugs1397385
milestone57.0a1
Bug 1397385 - Fixup js::ComputeImplicitThis to not leak environments It was possible to leak environments to script and debugger. This patch simplifies the logic and removes confusing helper functions. MozReview-Commit-ID: 4jEuYE4Q7bi
js/src/jit-test/tests/debug/bug1397385.js
js/src/tests/ecma_6/Function/implicit-this-in-parameter-expression.js
js/src/vm/Interpreter.cpp
js/src/vm/Stack-inl.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/bug1397385.js
@@ -0,0 +1,15 @@
+var g = newGlobal();
+
+g.evaluate(`
+    function testInnerFun(defaultArg = 1) {
+        function innerFun(expectedThis) { return this; }
+        h();
+    }
+`);
+
+g.h = function () {
+    var res = (new Debugger(g)).getNewestFrame().eval('assertEq(innerFun(), this)');
+    assertEq("return" in res, true);
+}
+
+g.testInnerFun();
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_6/Function/implicit-this-in-parameter-expression.js
@@ -0,0 +1,18 @@
+
+function f(a = eval(`
+    function g() {
+        'use strict';
+        return this;
+    }
+
+    with ({}) {
+        g() /* implicit return value */
+    }
+    `)) {
+    return a
+};
+
+assertEq(f(), undefined);
+
+if (typeof reportCompare === "function")
+    reportCompare(true, true);
--- a/js/src/vm/Interpreter.cpp
+++ b/js/src/vm/Interpreter.cpp
@@ -1460,60 +1460,47 @@ HandleError(JSContext* cx, InterpreterRe
  */
 JS_STATIC_ASSERT(JSOP_SETNAME_LENGTH == JSOP_SETPROP_LENGTH);
 
 /* See TRY_BRANCH_AFTER_COND. */
 JS_STATIC_ASSERT(JSOP_IFNE_LENGTH == JSOP_IFEQ_LENGTH);
 JS_STATIC_ASSERT(JSOP_IFNE == JSOP_IFEQ + 1);
 
 /*
- * Compute the implicit |this| parameter for a call expression where the callee
- * funval was resolved from an unqualified name reference to a property on obj
- * (an object on the env chain).
- *
- * We can avoid computing |this| eagerly and push the implicit callee-coerced
- * |this| value, undefined, if either of these conditions hold:
- *
- * 1. The nominal |this|, obj, is a global object.
+ * Compute the implicit |this| value used by a call expression with an
+ * unqualified name reference. The environment the binding was found on is
+ * passed as argument, env.
  *
- * 2. The nominal |this|, obj, has one of LexicalEnvironment or Call class (this
- *    is what IsCacheableEnvironment tests). Such objects-as-envs must be
- *    censored with undefined.
+ * The implicit |this| is |undefined| for all environment types except
+ * WithEnvironmentObject. This is the case for |with(...) {...}| expressions or
+ * if the embedding uses a non-syntactic WithEnvironmentObject.
  *
- * Otherwise, we bind |this| to the result of GetThisValue(). Only names inside
- * |with| statements and embedding-specific environment objects fall into this
- * category.
- *
- * If the callee is a strict mode function, then code implementing JSOP_THIS
- * in the interpreter and JITs will leave undefined as |this|. If funval is a
- * function not in strict mode, JSOP_THIS code replaces undefined with funval's
- * global.
+ * NOTE: A non-syntactic WithEnvironmentObject may have a corresponding
+ * extensible LexicalEnviornmentObject, but it will not be considered as an
+ * implicit |this|. This is for compatibility with the Gecko subscript loader.
  */
 static inline Value
-ComputeImplicitThis(JSObject* obj)
-{
-    if (obj->is<GlobalObject>())
-        return UndefinedValue();
-
-    if (obj->is<NonSyntacticVariablesObject>())
+ComputeImplicitThis(JSObject* env)
+{
+    // Fast-path for GlobalObject
+    if (env->is<GlobalObject>())
         return UndefinedValue();
 
-    if (obj->is<WithEnvironmentObject>())
-        return GetThisValueOfWith(obj);
-
-    if (IsCacheableEnvironment(obj))
-        return UndefinedValue();
+    // WithEnvironmentObjects have an actual implicit |this|
+    if (env->is<WithEnvironmentObject>())
+        return GetThisValueOfWith(env);
 
     // Debugger environments need special casing, as despite being
     // 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());
-
-    return GetThisValue(obj);
+    if (env->is<DebugEnvironmentProxy>())
+        return ComputeImplicitThis(&env->as<DebugEnvironmentProxy>().environment());
+
+    MOZ_ASSERT(env->is<EnvironmentObject>());
+    return UndefinedValue();
 }
 
 static MOZ_ALWAYS_INLINE bool
 AddOperation(JSContext* cx, MutableHandleValue lhs, MutableHandleValue rhs, MutableHandleValue res)
 {
     if (lhs.isInt32() && rhs.isInt32()) {
         int32_t l = lhs.toInt32(), r = rhs.toInt32();
         int32_t t;
--- a/js/src/vm/Stack-inl.h
+++ b/js/src/vm/Stack-inl.h
@@ -23,30 +23,16 @@
 
 #include "jsobjinlines.h"
 #include "jsscriptinlines.h"
 
 #include "jit/BaselineFrame-inl.h"
 
 namespace js {
 
-/*
- * We cache name lookup results only for the global object or for native
- * non-global objects without prototype or with prototype that never mutates,
- * see bug 462734 and bug 487039.
- */
-static inline bool
-IsCacheableEnvironment(JSObject* obj)
-{
-    bool cacheable = obj->is<CallObject>() || obj->is<LexicalEnvironmentObject>();
-
-    MOZ_ASSERT_IF(cacheable, !obj->getOpsLookupProperty());
-    return cacheable;
-}
-
 inline HandleObject
 InterpreterFrame::environmentChain() const
 {
     return HandleObject::fromMarkedLocation(&envChain_);
 }
 
 inline GlobalObject&
 InterpreterFrame::global() const