Bug 1397385 - Fixup js::ComputeImplicitThis to not leak environments r=jandem
authorTed Campbell <tcampbell@mozilla.com>
Thu, 07 Sep 2017 19:51:42 -0400
changeset 379733 aac13bc10a7265c4c6ac073d3678fa5edc22e60d
parent 379732 7e2a1d9ffd7a518ea047dc96ce4d84ac18eeb706
child 379734 0d384af83d69bd3a553bb34a698e0665cd9cbeeb
push id32461
push userkwierso@gmail.com
push dateFri, 08 Sep 2017 20:15:32 +0000
treeherdermozilla-central@dd3736e98e4e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1397385
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1397385 - Fixup js::ComputeImplicitThis to not leak environments r=jandem 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