Bug 1663847 part 2 - Change Debugger.Environment.callee getter to calleeScript getter. r=tcampbell,jdescottes
authorJan de Mooij <jdemooij@mozilla.com>
Fri, 11 Sep 2020 09:24:34 +0000
changeset 548295 e6c6a83c4de71878ccfa1b3a39d8ace923943695
parent 548294 530e5373b9acdef4943c157aa580194a6a4fd4cc
child 548296 30cd33572b3d5c4bfa8e5883396dc3aa84a62e0b
push id37776
push userbtara@mozilla.com
push dateFri, 11 Sep 2020 15:10:42 +0000
treeherdermozilla-central@b133e2d673e8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcampbell, jdescottes
bugs1663847, 1414684
milestone82.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 1663847 part 2 - Change Debugger.Environment.callee getter to calleeScript getter. r=tcampbell,jdescottes The callee getter returned |undefined| for certain functions because it's hard to recover the callee consistently for all environments (and we can't return the internal canonical function). See also bug 1414684. This patch fixes that by exposing the script instead of the callee. Devtools is only interested in the displayName and parameterNames properties and those are also available on scripts (the previous patch adds Script.parameterNames). Differential Revision: https://phabricator.services.mozilla.com/D89701
devtools/server/actors/environment.js
devtools/server/actors/inspector/event-collector.js
devtools/server/tests/xpcshell/test_framebindings-06.js
js/src/debugger/Environment.cpp
js/src/debugger/Environment.h
js/src/doc/Debugger/Debugger.Environment.md
js/src/jit-test/tests/debug/Environment-callee-01.js
js/src/jit-test/tests/debug/Environment-callee-02.js
js/src/jit-test/tests/debug/Environment-callee-03.js
js/src/jit-test/tests/debug/Environment-callee-04.js
js/src/jit-test/tests/debug/Environment-calleeScript-01.js
js/src/jit-test/tests/debug/Environment-calleeScript-02.js
js/src/jit-test/tests/debug/Environment-calleeScript-03.js
js/src/jit-test/tests/debug/Environment-nondebuggee.js
js/src/jit-test/tests/debug/Environment-optimizedOut-01.js
js/src/jit-test/tests/debug/wasm-06.js
js/src/jit-test/tests/modules/debugger-frames.js
--- a/devtools/server/actors/environment.js
+++ b/devtools/server/actors/environment.js
@@ -38,17 +38,17 @@ const EnvironmentActor = ActorClassWithS
   /**
    * Return an environment form for use in a protocol message.
    */
   form: function() {
     const form = { actor: this.actorID };
 
     // What is this environment's type?
     if (this.obj.type == "declarative") {
-      form.type = this.obj.callee ? "function" : "block";
+      form.type = this.obj.calleeScript ? "function" : "block";
     } else {
       form.type = this.obj.type;
     }
 
     form.scopeKind = this.obj.scopeKind;
 
     // Does this environment have a parent?
     if (this.obj.parent) {
@@ -62,22 +62,22 @@ const EnvironmentActor = ActorClassWithS
       form.object = createValueGrip(
         this.obj.object,
         this.getParent(),
         this.threadActor.objectGrip
       );
     }
 
     // Is this the environment created for a function call?
-    if (this.obj.callee) {
-      form.function = createValueGrip(
-        this.obj.callee,
-        this.getParent(),
-        this.threadActor.objectGrip
-      );
+    if (this.obj.calleeScript) {
+      // Client only uses "displayName" for "function".
+      // Create a fake object actor containing only "displayName" as replacement
+      // for the no longer available obj.callee (see bug 1663847).
+      // See bug 1664218 for cleanup.
+      form.function = { displayName: this.obj.calleeScript.displayName };
     }
 
     // Shall we list this environment's bindings?
     if (this.obj.type == "declarative") {
       form.bindings = this.bindings();
     }
 
     return form;
@@ -93,18 +93,18 @@ const EnvironmentActor = ActorClassWithS
     // TODO: this part should be removed in favor of the commented-out part
     // below when getVariableDescriptor lands (bug 725815).
     if (typeof this.obj.getVariable != "function") {
       // if (typeof this.obj.getVariableDescriptor != "function") {
       return bindings;
     }
 
     let parameterNames;
-    if (this.obj.callee) {
-      parameterNames = this.obj.callee.parameterNames;
+    if (this.obj.calleeScript) {
+      parameterNames = this.obj.calleeScript.parameterNames;
     } else {
       parameterNames = [];
     }
     for (const name of parameterNames) {
       const arg = {};
       const value = this.obj.getVariable(name);
 
       // TODO: this part should be removed in favor of the commented-out part
--- a/devtools/server/actors/inspector/event-collector.js
+++ b/devtools/server/actors/inspector/event-collector.js
@@ -608,21 +608,21 @@ class JQueryLiveEventCollector extends M
       if (displayName && displayName.startsWith("proxy/")) {
         return true;
       }
 
       // If the anonymous function is inside the |proxy| function and the
       // function gets name at compile time by SetFunctionName, its guessed
       // atom doesn't contain "proxy/".  In that case, check if the caller is
       // "proxy" function, as a fallback.
-      const calleeDO = funcDO.environment.callee;
-      if (!calleeDO) {
+      const calleeDS = funcDO.environment.calleeScript;
+      if (!calleeDS) {
         return false;
       }
-      const calleeName = calleeDO.displayName;
+      const calleeName = calleeDS.displayName;
       return calleeName == "proxy";
     }
 
     function getFirstFunctionVariable(funcDO) {
       // The handler function inside the |proxy| function should point the
       // unwrapped function via environment variable.
       const names = funcDO.environment.names();
       for (const varName of names) {
--- a/devtools/server/tests/xpcshell/test_framebindings-06.js
+++ b/devtools/server/tests/xpcshell/test_framebindings-06.js
@@ -7,29 +7,29 @@ add_task(
   threadFrontTest(async ({ threadFront, debuggee }) => {
     const packet = await executeOnNextTickAndWaitForPause(
       () => evalCode(debuggee),
       threadFront
     );
 
     const env = await packet.frame.getEnvironment();
     equal(env.type, "function");
-    equal(env.function.name, "banana3");
+    equal(env.function.displayName, "banana3");
     let parent = env.parent;
     equal(parent.type, "block");
     ok("banana3" in parent.bindings.variables);
     parent = parent.parent;
     equal(parent.type, "function");
-    equal(parent.function.name, "banana2");
+    equal(parent.function.displayName, "banana2");
     parent = parent.parent;
     equal(parent.type, "block");
     ok("banana2" in parent.bindings.variables);
     parent = parent.parent;
     equal(parent.type, "function");
-    equal(parent.function.name, "banana");
+    equal(parent.function.displayName, "banana");
 
     await threadFront.resume();
   })
 );
 
 function evalCode(debuggee) {
   debuggee.eval(
     "function banana(x) {\n" +
--- a/js/src/debugger/Environment.cpp
+++ b/js/src/debugger/Environment.cpp
@@ -13,16 +13,17 @@
 #include <string.h>  // for strlen, size_t
 #include <utility>   // for move
 
 #include "jsapi.h"        // for Rooted, CallArgs, MutableHandle
 #include "jsfriendapi.h"  // for GetErrorMessage, GetPropertyKeys
 
 #include "debugger/Debugger.h"          // for Env, Debugger, ValueToIdentifier
 #include "debugger/Object.h"            // for DebuggerObject
+#include "debugger/Script.h"            // for DebuggerScript
 #include "frontend/BytecodeCompiler.h"  // for IsIdentifier
 #include "gc/Rooting.h"                 // for RootedDebuggerEnvironment
 #include "gc/Tracer.h"       // for TraceManuallyBarrieredCrossCompartmentEdge
 #include "js/HeapAPI.h"      // for IsInsideNursery
 #include "vm/Compartment.h"  // for Compartment
 #include "vm/EnvironmentObject.h"  // for JSObject::is, DebugEnvironmentProxy
 #include "vm/JSAtom.h"             // for Atomize, PinAtom
 #include "vm/JSContext.h"          // for JSContext
@@ -116,17 +117,17 @@ struct MOZ_STACK_CLASS DebuggerEnvironme
 
   CallData(JSContext* cx, const CallArgs& args, HandleDebuggerEnvironment env)
       : cx(cx), args(args), environment(env) {}
 
   bool typeGetter();
   bool scopeKindGetter();
   bool parentGetter();
   bool objectGetter();
-  bool calleeGetter();
+  bool calleeScriptGetter();
   bool inspectableGetter();
   bool optimizedOutGetter();
 
   bool namesMethod();
   bool findMethod();
   bool getVariableMethod();
   bool setVariableMethod();
 
@@ -248,23 +249,23 @@ bool DebuggerEnvironment::CallData::obje
   if (!environment->getObject(cx, &result)) {
     return false;
   }
 
   args.rval().setObject(*result);
   return true;
 }
 
-bool DebuggerEnvironment::CallData::calleeGetter() {
+bool DebuggerEnvironment::CallData::calleeScriptGetter() {
   if (!environment->requireDebuggee(cx)) {
     return false;
   }
 
-  RootedDebuggerObject result(cx);
-  if (!environment->getCallee(cx, &result)) {
+  RootedDebuggerScript result(cx);
+  if (!environment->getCalleeScript(cx, &result)) {
     return false;
   }
 
   args.rval().setObjectOrNull(result);
   return true;
 }
 
 bool DebuggerEnvironment::CallData::inspectableGetter() {
@@ -370,17 +371,17 @@ bool DebuggerEnvironment::requireDebugge
   return true;
 }
 
 const JSPropertySpec DebuggerEnvironment::properties_[] = {
     JS_DEBUG_PSG("type", typeGetter),
     JS_DEBUG_PSG("scopeKind", scopeKindGetter),
     JS_DEBUG_PSG("parent", parentGetter),
     JS_DEBUG_PSG("object", objectGetter),
-    JS_DEBUG_PSG("callee", calleeGetter),
+    JS_DEBUG_PSG("calleeScript", calleeScriptGetter),
     JS_DEBUG_PSG("inspectable", inspectableGetter),
     JS_DEBUG_PSG("optimizedOut", optimizedOutGetter),
     JS_PS_END};
 
 const JSFunctionSpec DebuggerEnvironment::methods_[] = {
     JS_DEBUG_FN("names", namesMethod, 0), JS_DEBUG_FN("find", findMethod, 1),
     JS_DEBUG_FN("getVariable", getVariableMethod, 1),
     JS_DEBUG_FN("setVariable", setVariableMethod, 2), JS_FS_END};
@@ -467,35 +468,38 @@ bool DebuggerEnvironment::getObject(JSCo
   } else {
     object.set(referent());
     MOZ_ASSERT(!object->is<DebugEnvironmentProxy>());
   }
 
   return owner()->wrapDebuggeeObject(cx, object, result);
 }
 
-bool DebuggerEnvironment::getCallee(JSContext* cx,
-                                    MutableHandleDebuggerObject result) const {
+bool DebuggerEnvironment::getCalleeScript(
+    JSContext* cx, MutableHandleDebuggerScript result) const {
   if (!referent()->is<DebugEnvironmentProxy>()) {
     result.set(nullptr);
     return true;
   }
 
   JSObject& scope = referent()->as<DebugEnvironmentProxy>().environment();
   if (!scope.is<CallObject>()) {
     result.set(nullptr);
     return true;
   }
 
-  RootedObject callee(cx, &scope.as<CallObject>().callee());
-  if (IsInternalFunctionObject(*callee)) {
-    callee = nullptr;
+  Rooted<BaseScript*> script(cx, scope.as<CallObject>().callee().baseScript());
+
+  DebuggerScript* scriptObject = owner()->wrapScript(cx, script);
+  if (!scriptObject) {
+    return false;
   }
 
-  return owner()->wrapNullableDebuggeeObject(cx, callee, result);
+  result.set(scriptObject);
+  return true;
 }
 
 bool DebuggerEnvironment::isDebuggee() const {
   MOZ_ASSERT(referent());
   MOZ_ASSERT(!referent()->is<EnvironmentObject>());
 
   return owner()->observesGlobal(&referent()->nonCCWGlobal());
 }
--- a/js/src/debugger/Environment.h
+++ b/js/src/debugger/Environment.h
@@ -47,18 +47,18 @@ class DebuggerEnvironment : public Nativ
   void trace(JSTracer* trc);
 
   DebuggerEnvironmentType type() const;
   mozilla::Maybe<ScopeKind> scopeKind() const;
   MOZ_MUST_USE bool getParent(JSContext* cx,
                               MutableHandleDebuggerEnvironment result) const;
   MOZ_MUST_USE bool getObject(JSContext* cx,
                               MutableHandleDebuggerObject result) const;
-  MOZ_MUST_USE bool getCallee(JSContext* cx,
-                              MutableHandleDebuggerObject result) const;
+  MOZ_MUST_USE bool getCalleeScript(JSContext* cx,
+                                    MutableHandleDebuggerScript result) const;
   bool isDebuggee() const;
   bool isOptimized() const;
 
   static MOZ_MUST_USE bool getNames(JSContext* cx,
                                     HandleDebuggerEnvironment environment,
                                     MutableHandle<IdVector> result);
   static MOZ_MUST_USE bool find(JSContext* cx,
                                 HandleDebuggerEnvironment environment,
--- a/js/src/doc/Debugger/Debugger.Environment.md
+++ b/js/src/doc/Debugger/Debugger.Environment.md
@@ -96,21 +96,21 @@ ECMAScript terminology), or `null` if th
 ### `object`
 A [`Debugger.Object`][object] instance referring to the object whose
 properties this environment reflects. If this is a declarative
 environment record, this accessor throws a `TypeError` (since
 declarative environment records have no such object). Both `"object"`
 and `"with"` environments have `object` properties that provide the
 object whose properties they reflect as variable bindings.
 
-### `callee`
+### `calleeScript`
 If this environment represents the variable environment (the top-level
 environment within the function, which receives `var` definitions) for
 a call to a function <i>f</i>, then this property's value is a
-[`Debugger.Object`][object] instance referring to <i>f</i>. Otherwise,
+[`Debugger.Script`][script] instance referring to <i>f</i>'s script. Otherwise,
 this property's value is `null`.
 
 ### `optimizedOut`
 True if this environment is optimized out. False otherwise. For example,
 functions whose locals are never aliased may present optimized-out
 environments. When true, `getVariable` returns an ordinary JavaScript
 object whose `optimizedOut` property is true on all bindings, and
 `setVariable` throws a `ReferenceError`.
deleted file mode 100644
--- a/js/src/jit-test/tests/debug/Environment-callee-04.js
+++ /dev/null
@@ -1,22 +0,0 @@
-// We shouldn't hand out environment callees when we can only provide the
-// internal function object, not the live function object. (We should never
-// create Debugger.Object instances referring to internal function objects.)
-
-var g = newGlobal({newCompartment: true});
-var dbg = new Debugger(g);
-
-dbg.onDebuggerStatement = function (frame) {
-  assertEq(frame.older.environment.parent.callee, null);
-}
-
-g.evaluate(`
-
-           function h() { debugger; }
-           (function () {
-             return function () {
-               h();
-               return 1;
-             }
-           })()();
-
-           `);
rename from js/src/jit-test/tests/debug/Environment-callee-01.js
rename to js/src/jit-test/tests/debug/Environment-calleeScript-01.js
--- a/js/src/jit-test/tests/debug/Environment-callee-01.js
+++ b/js/src/jit-test/tests/debug/Environment-calleeScript-01.js
@@ -1,27 +1,23 @@
-// |jit-test| skip-if: !isTypeInferenceEnabled()
-// Test depends on singleton/clone behavior (the .callee getter calls
-// IsInternalFunctionObject).
-
-// Debugger.Environment.prototype.callee reveals the callee of environments
-// that have them.
+// Debugger.Environment.prototype.calleeScript reveals the script of function
+// environments.
 
 var g = newGlobal({newCompartment: true});
 var dbg = new Debugger;
 var gw = dbg.addDebuggee(g);
 
 function check(code, expectedType, expectedCallee) {
   print("check(" + JSON.stringify(code) + ")");
   var hits;
   dbg.onDebuggerStatement = function (frame) {
     hits++;
     var env = frame.environment;
     assertEq(env.type, expectedType);
-    assertEq(env.callee, expectedCallee);
+    assertEq(env.calleeScript, expectedCallee ? expectedCallee.script : null);
   };
   hits = 0;
   g.eval(code);
   assertEq(hits, 1);
 }
 
 check('debugger;', 'declarative', null);
 check('with({}) { debugger; };', 'with', null);
rename from js/src/jit-test/tests/debug/Environment-callee-02.js
rename to js/src/jit-test/tests/debug/Environment-calleeScript-02.js
--- a/js/src/jit-test/tests/debug/Environment-callee-02.js
+++ b/js/src/jit-test/tests/debug/Environment-calleeScript-02.js
@@ -1,25 +1,25 @@
-// Debugger.Environment.prototype.callee gets the right closure.
+// Debugger.Environment.prototype.calleeScript gets the right script.
 
 var g = newGlobal({newCompartment: true});
 var dbg = new Debugger;
 var gw = dbg.addDebuggee(g);
 
 g.eval('function f(x) { return function (y) { eval(""); debugger; return x + y; } }');
 g.eval('var g = f(2);');
 g.eval('var h = f(3);');
 
 function check(fun, label) {
   print("check(" + label + ")");
   var hits;
   dbg.onDebuggerStatement = function (frame) {
     hits++;
     var env = frame.environment;
-    assertEq(env.callee, gw.makeDebuggeeValue(fun));
+    assertEq(env.calleeScript, gw.makeDebuggeeValue(fun).script);
   };
   hits = 0;
   fun();
   assertEq(hits, 1);
 }
 
 check(g.g, 'g.g');
 check(g.h, 'g.h');
rename from js/src/jit-test/tests/debug/Environment-callee-03.js
rename to js/src/jit-test/tests/debug/Environment-calleeScript-03.js
--- a/js/src/jit-test/tests/debug/Environment-callee-03.js
+++ b/js/src/jit-test/tests/debug/Environment-calleeScript-03.js
@@ -1,22 +1,22 @@
 // Environments of different instances of the same generator have the same
-// callee. I love this job.
+// calleeScript.
 
 var g = newGlobal({newCompartment: true});
 var dbg = new Debugger;
 var gw = dbg.addDebuggee(g);
 
 function check(gen, label) {
   print("check(" + label + ")");
   var hits;
   dbg.onDebuggerStatement = function (frame) {
     hits++;
     var env = frame.environment;
-    assertEq(env.callee, gw.makeDebuggeeValue(g.f));
+    assertEq(env.calleeScript, gw.makeDebuggeeValue(g.f).script);
   };
   hits = 0;
   gen.next();
   assertEq(hits, 1);
 }
 
 g.eval('function* f(x) { debugger; yield x; }');
 g.eval('var g = f(2);');
--- a/js/src/jit-test/tests/debug/Environment-nondebuggee.js
+++ b/js/src/jit-test/tests/debug/Environment-nondebuggee.js
@@ -7,17 +7,17 @@ var dbg = new Debugger;
 let gw = dbg.addDebuggee(g);
 var log;
 
 function check(env) {
   assertEq(env.inspectable, false);
   assertThrowsInstanceOf(() => env.type, Error);
   assertThrowsInstanceOf(() => env.object, Error);
   assertThrowsInstanceOf(() => env.parent, Error);
-  assertThrowsInstanceOf(() => env.callee, Error);
+  assertThrowsInstanceOf(() => env.calleeScript, Error);
 
   assertThrowsInstanceOf(() => env.names(), Error);
   assertThrowsInstanceOf(() => env.find('x'), Error);
   assertThrowsInstanceOf(() => env.getVariable('x'), Error);
   assertThrowsInstanceOf(() => env.setVariable('x'), Error);
 }
 
 dbg.onDebuggerStatement = function (frame) {
--- a/js/src/jit-test/tests/debug/Environment-optimizedOut-01.js
+++ b/js/src/jit-test/tests/debug/Environment-optimizedOut-01.js
@@ -1,12 +1,8 @@
-// |jit-test| skip-if: !isTypeInferenceEnabled()
-// Test depends on singleton/clone behavior (the .callee getter calls
-// IsInternalFunctionObject).
-
 // Optimized out scopes should be considered optimizedOut.
 
 var g = newGlobal({newCompartment: true});
 var dbg = new Debugger;
 dbg.addDebuggee(g);
 
 g.eval("" + function f() {
   var x = 42;
@@ -17,31 +13,31 @@ g.eval("" + function f() {
 });
 
 dbg.onEnterFrame = function (f) {
   if (f.callee && (f.callee.name === undefined)) {
     blockenv = f.environment.parent;
     assertEq(blockenv.optimizedOut, true);
     assertEq(blockenv.inspectable, true);
     assertEq(blockenv.type, "declarative");
-    assertEq(blockenv.callee, null);
+    assertEq(blockenv.calleeScript, null);
     assertEq(blockenv.names().indexOf("y") !== -1, true);
 
     funenv = blockenv.parent;
     assertEq(funenv.optimizedOut, true);
     assertEq(funenv.inspectable, true);
     assertEq(funenv.type, "declarative");
-    assertEq(funenv.callee, f.older.callee);
+    assertEq(funenv.calleeScript, f.older.script);
     assertEq(funenv.names().indexOf("x") !== -1, true);
 
     globalenv = funenv.parent.parent;
     assertEq(globalenv.optimizedOut, false);
     assertEq(globalenv.inspectable, true);
     assertEq(globalenv.type, "object");
-    assertEq(globalenv.callee, null);
+    assertEq(globalenv.calleeScript, null);
 
     dbg.removeDebuggee(g);
 
     assertEq(blockenv.inspectable, false);
     assertEq(funenv.inspectable, false);
   }
 }
 
--- a/js/src/jit-test/tests/debug/wasm-06.js
+++ b/js/src/jit-test/tests/debug/wasm-06.js
@@ -47,17 +47,17 @@ runWasmWithDebugger(
             assertEq(frame.older, evalFrame);
             assertEq(frame.type, 'wasmcall');
 
             let env = frame.environment;
             assertEq(env instanceof Object, true);
             assertEq(env.inspectable, true);
             assertEq(env.parent !== null, true);
             assertEq(env.type, 'declarative');
-            assertEq(env.callee, null);
+            assertEq(env.calleeScript, null);
             assertEq(Array.isArray(env.names()), true);
             assertEq(env.names().length, 0);
 
             frame.onPop = function() {
                 onLeaveFrameCalled++;
                 testComplete = true;
             };
        };
--- a/js/src/jit-test/tests/modules/debugger-frames.js
+++ b/js/src/jit-test/tests/modules/debugger-frames.js
@@ -26,17 +26,17 @@ var dbg = Debugger(g2);
 dbg.onDebuggerStatement = function (frame) {
     // The current frame is a module frame.
     assertEq(frame.type, 'module');
     assertEq(frame.this, undefined);
 
     // The frame's environement is a module environment.
     let env = frame.environment;
     assertEq(env.type, 'declarative');
-    assertEq(env.callee, null);
+    assertEq(env.calleeScript, null);
 
     // Top level module definitions and imports are visible.
     assertArrayEq(env.names().sort(), ['a', 'b', 'c', 'x', 'y', 'z']);
     assertArrayEq(['a', 'b', 'c'].map(env.getVariable, env), [1, 2, 3]);
     assertArrayEq(['x', 'y', 'z'].map(env.getVariable, env), [4, 5, 6]);
 
     // Cannot set imports or const bindings.
     assertThrowsInstanceOf(() => env.setVariable('a', 10), TypeError);