Bug 746601 - unexpected 'arguments' read from the debugger should work like f.arguments (r=jimb)
authorLuke Wagner <luke@mozilla.com>
Wed, 25 Apr 2012 18:21:16 -0700
changeset 94414 d21a80aeef05d26063fd7475d6894e3700058410
parent 94413 f45eec2bd4c7f61ea9fd0c9303ebca560580bf2c
child 94415 f3395398d9f3cd6fd5c9de3ad31dc77960a09184
push id22712
push userryanvm@gmail.com
push dateSat, 19 May 2012 00:52:01 +0000
treeherdermozilla-central@642d1a36702f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimb
bugs746601
milestone15.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 746601 - unexpected 'arguments' read from the debugger should work like f.arguments (r=jimb)
js/src/jit-test/tests/debug/Environment-gc-01.js
js/src/jit-test/tests/debug/Frame-environment-04.js
js/src/jit-test/tests/debug/Frame-eval-08.js
js/src/jit-test/tests/debug/Frame-eval-11.js
js/src/jit-test/tests/debug/Frame-eval-12.js
js/src/jit-test/tests/debug/Frame-evalWithBindings-03.js
js/src/jit-test/tests/debug/Frame-onPop-21.js
js/src/vm/Debugger.cpp
js/src/vm/ScopeObject.cpp
--- a/js/src/jit-test/tests/debug/Environment-gc-01.js
+++ b/js/src/jit-test/tests/debug/Environment-gc-01.js
@@ -4,16 +4,16 @@ var g = newGlobal('new-compartment');
 g.eval("function f(x) { return 2 * x; }");
 var dbg = Debugger(g);
 var env;
 dbg.onEnterFrame = function (frame) { env = frame.environment; };
 assertEq(g.f(22), 44);
 dbg.onEnterFrame = undefined;
 
 assertEq(env.find("x"), env);
-assertEq(env.names().join(","), "x");
+assertEq(env.names().join(","), "arguments,x");
 
 gc();
 g.gc(g);
 gc(env);
 
 assertEq(env.find("x"), env);
-assertEq(env.names().join(","), "x");
+assertEq(env.names().join(","), "arguments,x");
--- a/js/src/jit-test/tests/debug/Frame-environment-04.js
+++ b/js/src/jit-test/tests/debug/Frame-environment-04.js
@@ -1,12 +1,12 @@
 // frame.environment can be called from the onEnterFrame hook.
 
 var g = newGlobal('new-compartment');
 g.eval("function f(x) { return 2 * x; }");
 var dbg = Debugger(g);
 var hits = 0;
 dbg.onEnterFrame = function (frame) {
-    assertEq(frame.environment.names().join(","), "x");
+    assertEq(frame.environment.names().join(","), "arguments,x");
     hits++;
 };
 assertEq(g.f(22), 44);
 assertEq(hits, 1);
--- a/js/src/jit-test/tests/debug/Frame-eval-08.js
+++ b/js/src/jit-test/tests/debug/Frame-eval-08.js
@@ -1,15 +1,23 @@
-// Test that 'arguments' access in a function that doesn't expect 'arguments'
-// doesn't crash.
-
-// TODO bug 659577: the debugger should be improved to throw an error in such
-// cases rather than silently returning whatever it finds on the scope chain.
+// The arguments can escape from a function via a debugging hook.
 
 var g = newGlobal('new-compartment');
 var dbg = new Debugger(g);
 
 // capture arguments object and test function
+var args, testfn;
 dbg.onDebuggerStatement = function (frame) {
-    args = frame.eval("arguments");
+    args = frame.eval("arguments").return;
+    testfn = frame.eval("test").return;
 };
 g.eval("function f() { debugger; }");
+g.eval("var test = " + function test(args) {
+        assertEq(args.length, 3);
+        assertEq(args[0], this);
+        assertEq(args[1], f);
+        assertEq(args[2].toString(), "[object Object]");
+        return 42;
+    } + ";");
 g.eval("f(this, f, {});");
+
+var cv = testfn.apply(null, [args]);
+assertEq(cv.return, 42);
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/Frame-eval-11.js
@@ -0,0 +1,15 @@
+// The arguments can escape from a function via a debugging hook.
+
+var g = newGlobal('new-compartment');
+var dbg = new Debugger(g);
+
+// capture arguments object and test function
+var hits = 0;
+dbg.onDebuggerStatement = function (frame) {
+    assertEq(frame.older.eval('arguments[0]').return, 'ponies');
+    hits++;
+};
+g.eval("function g() { debugger; }");
+g.eval("function f() { g(); }");
+g.eval("f('ponies')");
+assertEq(hits, 1);
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/Frame-eval-12.js
@@ -0,0 +1,19 @@
+// The arguments can escape from a function via a debugging hook.
+
+var g = newGlobal('new-compartment');
+var dbg = new Debugger(g);
+
+// capture arguments object and test function
+var hits = 0;
+dbg.onDebuggerStatement = function (frame) {
+    try {
+        frame.older.environment.parent.getVariable('arguments')
+    } catch (e) {
+        assertEq(''+e, "Error: Debugger scope is not live");
+        hits++;
+    }
+};
+g.eval("function h() { debugger; }");
+g.eval("function f() { var x = 0; return function() { x++; h() } }");
+g.eval("f('ponies')()");
+assertEq(hits, 1);
--- a/js/src/jit-test/tests/debug/Frame-evalWithBindings-03.js
+++ b/js/src/jit-test/tests/debug/Frame-evalWithBindings-03.js
@@ -1,17 +1,16 @@
 // arguments works in evalWithBindings (it does not interpose a function scope)
-// when the function expects 'arguments'
 var g = newGlobal('new-compartment');
 var dbg = new Debugger;
 var global = dbg.addDebuggee(g);
 var hits = 0;
 dbg.onDebuggerStatement = function (frame) {
     var argc = frame.arguments.length;
     assertEq(argc, 7);
     assertEq(frame.evalWithBindings("arguments[prop]", {prop: "length"}).return, argc);
     for (var i = 0; i < argc; i++)
         assertEq(frame.evalWithBindings("arguments[i]", {i: i}).return, frame.arguments[i]);
     hits++;
 };
-g.eval("function f() { eval('arguments'); debugger; }");
+g.eval("function f() { debugger; }");
 g.eval("f(undefined, -0, NaN, '\uffff', Array.prototype, Math, f);");
 assertEq(hits, 1);
--- a/js/src/jit-test/tests/debug/Frame-onPop-21.js
+++ b/js/src/jit-test/tests/debug/Frame-onPop-21.js
@@ -11,16 +11,17 @@ dbg.onDebuggerStatement = function handl
 };
 
 function handlePop(c) {
     log += ')';
 
     // Arguments must be live.
     assertEq(this.eval('a').return, 'frieze');
     assertEq(this.eval('b = "architrave"').return, 'architrave');
+    assertEq(this.eval('arguments[1]').return, 'architrave');
     assertEq(this.eval('b').return, 'architrave');
 
     // function-scope variables must be live.
     assertEq(this.eval('x').return, 'entablature');
     assertEq(this.eval('y = "cornice"').return, 'cornice');
     assertEq(this.eval('y').return, 'cornice');
 }
 
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -3007,17 +3007,17 @@ CheckThisFrame(JSContext *cx, const Call
     if (!thisobj->getPrivate()) {
         if (thisobj->getReservedSlot(JSSLOT_DEBUGFRAME_OWNER).isUndefined()) {
             JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_INCOMPATIBLE_PROTO,
                                  "Debugger.Frame", fnname, "prototype object");
             return NULL;
         }
         if (checkLive) {
             JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_DEBUG_NOT_LIVE,
-                                 "Debugger.Frame", fnname, "stack frame");
+                                 "Debugger.Frame");
             return NULL;
         }
     }
     return thisobj;
 }
 
 #define THIS_FRAME(cx, argc, vp, fnname, args, thisobj, fp)                  \
     CallArgs args = CallArgsFromVp(argc, vp);                                \
--- a/js/src/vm/ScopeObject.cpp
+++ b/js/src/vm/ScopeObject.cpp
@@ -303,20 +303,17 @@ CallObject::getVarOp(JSContext *cx, JSOb
     DebugOnly<JSScript *> script = callobj.getCalleeFunction()->script();
     JS_ASSERT_IF(!cx->okToAccessUnaliasedBindings, script->varIsAliased(i));
 
     if (StackFrame *fp = callobj.maybeStackFrame())
         *vp = fp->varSlot(i);
     else
         *vp = callobj.var(i);
 
-    /* This can only happen via the debugger. Bug 659577 will remove it. */
-    if (vp->isMagic(JS_OPTIMIZED_ARGUMENTS))
-        *vp = UndefinedValue();
-
+    JS_ASSERT(!vp->isMagic(JS_OPTIMIZED_ARGUMENTS));
     return true;
 }
 
 JSBool
 CallObject::setVarOp(JSContext *cx, JSObject *obj, jsid id, JSBool strict, Value *vp)
 {
     CallObject &callobj = obj->asCall();
 
@@ -1268,41 +1265,116 @@ namespace js {
  *  - The engine has made certain assumptions about the possible reads/writes
  *    in a scope. DebugScopeProxy allows us to prevent the debugger from
  *    breaking those assumptions. Examples include adding shadowing variables
  *    or changing the property attributes of bindings.
  *  - The engine makes optimizations that are observable to the debugger. The
  *    proxy can either hide these optimizations or make the situation more
  *    clear to the debugger. An example is 'arguments'.
  */
-struct DebugScopeProxy : BaseProxyHandler
+class DebugScopeProxy : public BaseProxyHandler
 {
+    static bool isArguments(JSContext *cx, jsid id)
+    {
+        return id == NameToId(cx->runtime->atomState.argumentsAtom);
+    }
+
+    static bool isFunctionScope(ScopeObject &scope)
+    {
+        return scope.isCall() && !scope.asCall().isForEval();
+    }
+
+    /*
+     * In theory, every function scope contains an 'arguments' bindings.
+     * However, the engine only adds a binding if 'arguments' is used in the
+     * function body. Thus, from the debugger's perspective, 'arguments' may be
+     * missing from the list of bindings.
+     */
+    static bool isMissingArgumentsBinding(ScopeObject &scope)
+    {
+        return isFunctionScope(scope) &&
+               !scope.asCall().getCalleeFunction()->script()->argumentsHasLocalBinding();
+    }
+
+    /*
+     * This function creates an arguments object when the debugger requests
+     * 'arguments' for a function scope where the arguments object has been
+     * optimized away (either because the binding is missing altogether or
+     * because !ScriptAnalysis::needsArgsObj).
+     */
+    static bool checkForMissingArguments(JSContext *cx, jsid id, ScopeObject &scope,
+                                         ArgumentsObject **maybeArgsObj)
+    {
+        *maybeArgsObj = NULL;
+
+        if (!isArguments(cx, id) || !isFunctionScope(scope))
+            return true;
+
+        JSScript *script = scope.asCall().getCalleeFunction()->script();
+        if (script->needsArgsObj())
+            return true;
+
+        StackFrame *fp = scope.maybeStackFrame();
+        if (!fp) {
+            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_DEBUG_NOT_LIVE,
+                                 "Debugger scope");
+            return false;
+        }
+
+        *maybeArgsObj = ArgumentsObject::createUnexpected(cx, fp);
+        return true;
+    }
+
+  public:
     static int family;
     static DebugScopeProxy singleton;
 
     DebugScopeProxy() : BaseProxyHandler(&family) {}
 
     bool getPropertyDescriptor(JSContext *cx, JSObject *proxy, jsid id, bool set,
                                PropertyDescriptor *desc) MOZ_OVERRIDE
     {
         return getOwnPropertyDescriptor(cx, proxy, id, set, desc);
     }
 
     bool getOwnPropertyDescriptor(JSContext *cx, JSObject *proxy, jsid id, bool set,
                                   PropertyDescriptor *desc) MOZ_OVERRIDE
     {
+        ScopeObject &scope = proxy->asDebugScope().scope();
+
+        ArgumentsObject *maybeArgsObj;
+        if (!checkForMissingArguments(cx, id, scope, &maybeArgsObj))
+            return false;
+
+        if (maybeArgsObj) {
+            PodZero(desc);
+            desc->obj = proxy;
+            desc->attrs = JSPROP_READONLY | JSPROP_ENUMERATE | JSPROP_PERMANENT;
+            desc->value = ObjectValue(*maybeArgsObj);
+            return true;
+        }
+
         AutoAllowUnaliasedVarAccess a(cx);
-        ScopeObject &scope = proxy->asDebugScope().scope();
         return JS_GetPropertyDescriptorById(cx, &scope, id, JSRESOLVE_QUALIFIED, desc);
     }
 
     bool get(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id, Value *vp) MOZ_OVERRIDE
     {
+        ScopeObject &scope = proxy->asDebugScope().scope();
+
+        ArgumentsObject *maybeArgsObj;
+        if (!checkForMissingArguments(cx, id, scope, &maybeArgsObj))
+            return false;
+
+        if (maybeArgsObj) {
+            *vp = ObjectValue(*maybeArgsObj);
+            return true;
+        }
+
         AutoAllowUnaliasedVarAccess a(cx);
-        ScopeObject &scope = proxy->asDebugScope().scope();
         return scope.getGeneric(cx, &scope, id, vp);
     }
 
     bool set(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id, bool strict,
                      Value *vp) MOZ_OVERRIDE
     {
         AutoAllowUnaliasedVarAccess a(cx);
         ScopeObject &scope = proxy->asDebugScope().scope();
@@ -1312,31 +1384,62 @@ struct DebugScopeProxy : BaseProxyHandle
     bool defineProperty(JSContext *cx, JSObject *proxy, jsid id, PropertyDescriptor *desc) MOZ_OVERRIDE
     {
         return Throw(cx, id, JSMSG_CANT_REDEFINE_PROP);
     }
 
     bool getOwnPropertyNames(JSContext *cx, JSObject *proxy, AutoIdVector &props) MOZ_OVERRIDE
     {
         ScopeObject &scope = proxy->asDebugScope().scope();
+
+        if (isMissingArgumentsBinding(scope) &&
+            !props.append(NameToId(cx->runtime->atomState.argumentsAtom)))
+        {
+            return false;
+        }
+
         return GetPropertyNames(cx, &scope, JSITER_OWNONLY, &props);
     }
 
     bool delete_(JSContext *cx, JSObject *proxy, jsid id, bool *bp) MOZ_OVERRIDE
     {
         return js_ReportValueErrorFlags(cx, JSREPORT_ERROR, JSMSG_CANT_DELETE,
                                         JSDVG_IGNORE_STACK, IdToValue(id), NULL,
                                         NULL, NULL);
     }
 
     bool enumerate(JSContext *cx, JSObject *proxy, AutoIdVector &props) MOZ_OVERRIDE
     {
         ScopeObject &scope = proxy->asDebugScope().scope();
+
+        if (isMissingArgumentsBinding(scope) &&
+            !props.append(NameToId(cx->runtime->atomState.argumentsAtom)))
+        {
+            return false;
+        }
+
         return GetPropertyNames(cx, &scope, 0, &props);
     }
+
+    bool has(JSContext *cx, JSObject *proxy, jsid id, bool *bp) MOZ_OVERRIDE
+    {
+        ScopeObject &scope = proxy->asDebugScope().scope();
+
+        if (isArguments(cx, id) && isFunctionScope(scope)) {
+            *bp = true;
+            return true;
+        }
+
+        JSBool found;
+        if (!JS_HasPropertyById(cx, &scope, id, &found))
+            return false;
+
+        *bp = found;
+        return true;
+    }
 };
 
 }  /* namespace js */
 
 int DebugScopeProxy::family = 0;
 DebugScopeProxy DebugScopeProxy::singleton;
 
 /* static */ DebugScopeObject *