Fix display of return value / thrown exception in scopes (#4569)

flow-typed/debugger-html.js

@@ -155,9 +155,13 @@ declare module "debugger-html" {
   declare type Why =
     | {|
         exception: string | Grip,
-        type: "exception"
+        type: "exception",
+        frameFinished?: Object
       |}
-    | { type: string };
+    | {
+        type: string,
+        frameFinished?: Object
+      };
 
   /**
  * Why is the Debugger Paused?
@@ -185,6 +189,7 @@ declare module "debugger-html" {
  * @static
  */
   declare type Pause = {
+    frame: Frame,
     frames: Frame[],
     why: Why,
     loadedObjects?: LoadedObject[]

src/test/mochitest/browser.ini

@@ -92,6 +92,7 @@ skip-if = os == "linux" # bug 1351952
 [browser_dbg-pretty-print-console.js]
 [browser_dbg-pretty-print-paused.js]
 [browser_dbg-preview.js]
+[browser_dbg-returnvalues.js]
 [browser_dbg-scopes-mutations.js]
 [browser_dbg-search-file.js]
 skip-if = os == "win" # Bug 1393121

src/test/mochitest/browser_dbg-returnvalues.js

@@ -0,0 +1,69 @@
+function getLabel(dbg, index) {
+  return findElement(dbg, "scopeNode", index).innerText;
+}
+
+function getValue(dbg, index) {
+  return findElement(dbg, "scopeValue", index).innerText;
+}
+
+function toggleScopes(dbg) {
+  return findElement(dbg, "scopesHeader").click();
+}
+
+async function testReturnValue(dbg, val) {
+  invokeInTab("return_something", val);
+  await waitForPaused(dbg);
+
+  // "Step in" 3 times to get to the point where the debugger can
+  // see the return value.
+  await stepIn(dbg);
+  await stepIn(dbg);
+  await stepIn(dbg);
+
+  is(getLabel(dbg, 1), "return_something", "check for return_something");
+
+  // We don't show "undefined" but we do show other falsy values.
+  let label = getLabel(dbg, 2);
+  if (val === "undefined") {
+    ok(label !== "<return>", "do not show <return> for undefined");
+  } else {
+    is(label, "<return>", "check for <return>");
+    // The "uneval" call here gives us the way one would write `val` as
+    // a JavaScript expression, similar to the way the debugger
+    // displays values, so this test works when `val` is a string.
+    is(getValue(dbg, 2), uneval(val), `check value is ${uneval(val)}`);
+  }
+
+  await resume(dbg);
+  assertNotPaused(dbg);
+}
+
+async function testThrowValue(dbg, val) {
+  invokeInTab("throw_something", val);
+  await waitForPaused(dbg);
+
+  // "Step in" once to get to the point where the debugger can see the
+  // exception.
+  await stepIn(dbg);
+
+  is(getLabel(dbg, 1), "callee", "check for callee");
+  is(getLabel(dbg, 2), "<exception>", "check for <exception>");
+  // The "uneval" call here gives us the way one would write `val` as
+  // a JavaScript expression, similar to the way the debugger
+  // displays values, so this test works when `val` is a string.
+  is(getValue(dbg, 2), uneval(val), `check exception is ${uneval(val)}`);
+
+  await resume(dbg);
+  await waitForPaused(dbg);
+  await resume(dbg);
+  assertNotPaused(dbg);
+}
+
+add_task(async function() {
+  const dbg = await initDebugger("doc-return-values.html");
+  toggleScopes(dbg);
+  await togglePauseOnExceptions(dbg, true, false);
+
+  await testReturnValue(dbg, "to sender");
+  await testThrowValue(dbg, "a fit");
+});

src/test/mochitest/head.js

@@ -256,6 +256,15 @@ function waitForSelectedSource(dbg, url) {
   );
 }
 
+/**
+ * Assert that the debugger is not currently paused.
+ * @memberof mochitest/asserts
+ * @static
+ */
+function assertNotPaused(dbg) {
+  ok(!isPaused(dbg), "client is not paused");
+}
+
 /**
  * Assert that the debugger is paused at the correct location.
  *
@@ -726,14 +735,20 @@ function waitForActive(dbg) {
  * Invokes a global function in the debuggee tab.
  *
  * @memberof mochitest/helpers
- * @param {String} fnc
+ * @param {String} fnc The name of a global function on the content window to
+ *                     call. This is applied to structured clones of the
+ *                     remaining arguments to invokeInTab.
+ * @param {Any} ...args Remaining args to serialize and pass to fnc.
  * @return {Promise}
  * @static
  */
-function invokeInTab(fnc) {
-  info(`Invoking function ${fnc} in tab`);
-  return ContentTask.spawn(gBrowser.selectedBrowser, fnc, function*(fnc) {
-    content.wrappedJSObject[fnc](); // eslint-disable-line mozilla/no-cpows-in-tests, max-len
+function invokeInTab(fnc, ...args) {
+  info(`Invoking in tab: ${fnc}(${args.map(uneval).join(",")})`);
+  return ContentTask.spawn(gBrowser.selectedBrowser, { fnc, args }, function*({
+    fnc,
+    args
+  }) {
+    content.wrappedJSObject[fnc](...args); // eslint-disable-line mozilla/no-cpows-in-tests, max-len
   });
 }
 

src/utils/scopes.js

@@ -5,7 +5,6 @@
 // @flow
 
 import { toPairs } from "lodash";
-import { get } from "lodash";
 import { simplifyDisplayName } from "./frame";
 import type { Frame, Pause, Scope, BindingContents } from "debugger-html";
 
@@ -74,30 +73,34 @@ function getSourceBindingVariables(
   return bound.concat(unused);
 }
 
-export function getSpecialVariables(pauseInfo: Pause, path: string) {
-  const thrown = get(pauseInfo, "why.frameFinished.throw", undefined);
-
-  const returned = get(pauseInfo, "why.frameFinished.return", undefined);
-
+export function getFramePopVariables(pauseInfo: Pause, path: string) {
   const vars = [];
 
-  if (thrown !== undefined) {
-    vars.push({
-      name: "<exception>",
-      path: `${path}/<exception>`,
-      contents: { value: thrown }
-    });
-  }
+  if (pauseInfo.why && pauseInfo.why.frameFinished) {
+    const frameFinished = pauseInfo.why.frameFinished;
 
-  if (returned !== undefined) {
-    // Do not display a return value of "undefined",
-    if (!returned || !returned.type || returned.type !== "undefined") {
+    // Always display a `throw` property if present, even if it is falsy.
+    if (Object.prototype.hasOwnProperty.call(frameFinished, "throw")) {
       vars.push({
-        name: "<return>",
-        path: `${path}/<return>`,
-        contents: { value: returned }
+        name: "<exception>",
+        path: `${path}/<exception>`,
+        contents: { value: frameFinished.throw }
       });
     }
+
+    if (Object.prototype.hasOwnProperty.call(frameFinished, "return")) {
+      const returned = frameFinished.return;
+
+      // Do not display undefined. Do display falsy values like 0 and false. The
+      // protocol grip for undefined is a JSON object: { type: "undefined" }.
+      if (typeof returned !== "object" || returned.type !== "undefined") {
+        vars.push({
+          name: "<return>",
+          path: `${path}/<return>`,
+          contents: { value: returned }
+        });
+      }
+    }
   }
 
   return vars;
@@ -137,7 +140,6 @@ export function getScopes(
   const scopes = [];
 
   let scope = selectedScope;
-  const pausedScopeActor = get(pauseInfo, "frame.scope.actor");
   let scopeIndex = 1;
 
   do {
@@ -159,9 +161,13 @@ export function getScopes(
         ? getSourceBindingVariables(bindings, sourceBindings, key)
         : getBindingVariables(bindings, key);
 
-      // show exception, return, and this variables in innermost scope
-      if (scope.actor === pausedScopeActor) {
-        vars = vars.concat(getSpecialVariables(pauseInfo, key));
+      // On the innermost scope of a frame that is just about to be popped, show
+      // the return value or the exception being thrown as special variables.
+      if (
+        scope.actor === selectedScope.actor &&
+        selectedFrame.id === pauseInfo.frame.id
+      ) {
+        vars = vars.concat(getFramePopVariables(pauseInfo, key));
       }
 
       if (scope.actor === selectedScope.actor) {