Bug 1552648 - Avoid too much recursion when inspecting nested promises. r=nchevobbe,devtools-backward-compat-reviewers

Differential Revision: https://phabricator.services.mozilla.com/D96317

devtools/client/fronts/object.js

@@ -239,6 +239,43 @@ class ObjectFront extends FrontClassWithSpec(objectSpec) {
     return super.displayString();
   }
 
+  /**
+   * Request the state of a promise.
+   */
+  async getPromiseState() {
+    if (this._grip.class !== "Promise") {
+      console.error("getPromiseState is only valid for promise grips.");
+      return null;
+    }
+
+    let response, promiseState;
+    try {
+      response = await super.promiseState();
+      promiseState = response.promiseState;
+    } catch (error) {
+      // Before Firefox 85 (bug 1552648), the promiseState request didn't exist.
+      // The promise state was directly included in the grip.
+      if (error.message.includes("unrecognizedPacketType")) {
+        promiseState = this._grip.promiseState;
+        response = { promiseState };
+      } else {
+        throw error;
+      }
+    }
+
+    const { value, reason } = promiseState;
+
+    if (value) {
+      promiseState.value = getAdHocFrontOrPrimitiveGrip(value, this);
+    }
+
+    if (reason) {
+      promiseState.reason = getAdHocFrontOrPrimitiveGrip(reason, this);
+    }
+
+    return response;
+  }
+
   /**
    * Request the target and handler internal slots of a proxy.
    */
@@ -356,24 +393,6 @@ function getAdHocFrontOrPrimitiveGrip(packet, parentFront) {
  * @param {String|Number|Object} packet: The packet returned by the server
  */
 function createChildFronts(objectFront, packet) {
-  // Handle Promise fullfilled and rejected values
-  if (packet.class == "Promise" && packet.promiseState) {
-    if (packet.promiseState.state == "fulfilled" && packet.promiseState.value) {
-      packet.promiseState.value = getAdHocFrontOrPrimitiveGrip(
-        packet.promiseState.value,
-        objectFront
-      );
-    } else if (
-      packet.promiseState.state == "rejected" &&
-      packet.promiseState.reason
-    ) {
-      packet.promiseState.reason = getAdHocFrontOrPrimitiveGrip(
-        packet.promiseState.reason,
-        objectFront
-      );
-    }
-  }
-
   if (packet.preview) {
     const { message, entries } = packet.preview;
 

devtools/client/shared/components/object-inspector/utils/client.js

@@ -79,6 +79,10 @@ async function getFullText(longStringFront, item) {
   }
 }
 
+async function getPromiseState(objectFront) {
+  return objectFront.getPromiseState();
+}
+
 async function getProxySlots(objectFront) {
   return objectFront.getProxySlots();
 }
@@ -100,5 +104,6 @@ module.exports = {
   enumSymbols,
   getPrototype,
   getFullText,
+  getPromiseState,
   getProxySlots,
 };

devtools/client/shared/components/object-inspector/utils/load-properties.js

@@ -9,6 +9,7 @@ const {
   getPrototype,
   enumSymbols,
   getFullText,
+  getPromiseState,
   getProxySlots,
 } = require("devtools/client/shared/components/object-inspector/utils/client");
 
@@ -24,6 +25,7 @@ const {
   nodeIsEntries,
   nodeIsMapEntry,
   nodeIsPrimitive,
+  nodeIsPromise,
   nodeIsProxy,
   nodeNeedsNumericalBuckets,
   nodeIsLongString,
@@ -77,6 +79,10 @@ function loadItemProperties(item, client, loadedProperties) {
     promises.push(getFullText(longStringFront, item));
   }
 
+  if (shouldLoadItemPromiseState(item, loadedProperties)) {
+    promises.push(getPromiseState(getObjectFront()));
+  }
+
   if (shouldLoadItemProxySlots(item, loadedProperties)) {
     promises.push(getProxySlots(getObjectFront()));
   }
@@ -104,6 +110,10 @@ function mergeResponses(responses) {
       data.fullText = response.fullText;
     }
 
+    if (response.promiseState) {
+      data.promiseState = response.promiseState;
+    }
+
     if (response.proxyTarget && response.proxyHandler) {
       data.proxyTarget = response.proxyTarget;
       data.proxyHandler = response.proxyHandler;
@@ -198,6 +208,10 @@ function shouldLoadItemFullText(item, loadedProperties = new Map()) {
   return !loadedProperties.has(item.path) && nodeIsLongString(item);
 }
 
+function shouldLoadItemPromiseState(item, loadedProperties = new Map()) {
+  return !loadedProperties.has(item.path) && nodeIsPromise(item);
+}
+
 function shouldLoadItemProxySlots(item, loadedProperties = new Map()) {
   return !loadedProperties.has(item.path) && nodeIsProxy(item);
 }
@@ -211,5 +225,6 @@ module.exports = {
   shouldLoadItemPrototype,
   shouldLoadItemSymbols,
   shouldLoadItemFullText,
+  shouldLoadItemPromiseState,
   shouldLoadItemProxySlots,
 };

devtools/client/shared/components/object-inspector/utils/node.js

@@ -281,11 +281,8 @@ function nodeNeedsNumericalBuckets(item) {
   );
 }
 
-function makeNodesForPromiseProperties(item) {
+function makeNodesForPromiseProperties(loadedProps, item) {
-  const {
+  const { reason, value, state } = loadedProps.promiseState;
-    promiseState: { reason, value, state },
-  } = getValue(item);
-
   const properties = [];
 
   if (state) {
@@ -577,10 +574,6 @@ function makeNodesForProperties(objProps, parent) {
     }, this);
   }
 
-  if (nodeIsPromise(parent)) {
-    nodes.push(...makeNodesForPromiseProperties(parent));
-  }
-
   if (nodeHasEntries(parent)) {
     nodes.push(makeNodesForEntries(parent));
   }
@@ -805,6 +798,10 @@ function getChildren(options) {
     return addToCache(makeNodesForMapEntry(item));
   }
 
+  if (nodeIsPromise(item) && hasLoadedProps) {
+    return addToCache(makeNodesForPromiseProperties(loadedProps, item));
+  }
+
   if (nodeIsProxy(item) && hasLoadedProps) {
     return addToCache(makeNodesForProxyProperties(loadedProps, item));
   }

devtools/client/shared/components/test/node/components/object-inspector/utils/__snapshots__/promises.test.js.snap

@@ -13,12 +13,6 @@ Array [
         "value": Object {
           "actor": "server2.conn2.child1/obj36",
           "class": "Promise",
-          "promiseState": Object {
-            "reason": Object {
-              "type": "3",
-            },
-            "state": "rejected",
-          },
           "type": "object",
         },
       },
@@ -42,12 +36,6 @@ Array [
         "value": Object {
           "actor": "server2.conn2.child1/obj36",
           "class": "Promise",
-          "promiseState": Object {
-            "reason": Object {
-              "type": "3",
-            },
-            "state": "rejected",
-          },
           "type": "object",
         },
       },

devtools/client/shared/components/test/node/components/object-inspector/utils/promises.test.js

@@ -31,24 +31,24 @@ describe("promises utils function", () => {
   });
 
   it("makeNodesForPromiseProperties", () => {
-    const promise = {
+    const item = {
       path: "root",
       contents: {
         value: {
           actor: "server2.conn2.child1/obj36",
-          promiseState: {
-            state: "rejected",
-            reason: {
-              type: "3",
-            },
-          },
           class: "Promise",
           type: "object",
         },
       },
     };
+    const promiseState = {
+      state: "rejected",
+      reason: {
+        type: "3",
+      },
+    };
 
-    const properties = makeNodesForPromiseProperties(promise);
+    const properties = makeNodesForPromiseProperties({promiseState}, item);
     expect(properties).toMatchSnapshot();
   });
 });

devtools/client/webconsole/test/browser/_webconsole.ini

@@ -307,6 +307,7 @@ skip-if = (os == "linux" && fission && !ccov) || (os == "win" && fission) #Bug 1
 [browser_webconsole_object_inspector_getters_shadowed.js]
 [browser_webconsole_object_inspector_key_sorting.js]
 [browser_webconsole_object_inspector_local_session_storage.js]
+[browser_webconsole_object_inspector_nested_promise.js]
 [browser_webconsole_object_inspector_nested_proxy.js]
 [browser_webconsole_object_inspector_selected_text.js]
 [browser_webconsole_object_inspector_scroll.js]

devtools/client/webconsole/test/browser/browser_webconsole_object_inspector_nested_promise.js

@@ -0,0 +1,85 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Check evaluating and expanding promises in the console.
+const TEST_URI =
+  "data:text/html;charset=utf8," +
+  "<h1>Object Inspector on deeply nested promises</h1>";
+
+add_task(async function testExpandNestedPromise() {
+  const hud = await openNewTabAndConsole(TEST_URI);
+
+  await SpecialPowers.spawn(gBrowser.selectedBrowser, [], function() {
+    let nestedPromise = Promise.resolve({});
+    for (let i = 0; i < 5; ++i) {
+      Object.setPrototypeOf(nestedPromise, null);
+      nestedPromise = Promise.resolve(nestedPromise);
+    }
+    content.wrappedJSObject.console.log("oi-test", nestedPromise);
+  });
+
+  const node = await waitFor(() => findMessage(hud, "oi-test"));
+  const oi = node.querySelector(".tree");
+  const [promiseNode] = getObjectInspectorNodes(oi);
+
+  expandObjectInspectorNode(promiseNode);
+  await waitFor(() => getObjectInspectorNodes(oi).length > 1);
+  checkChildren(promiseNode, [`<state>`, `<value>`]);
+
+  const valueNode = findObjectInspectorNode(oi, "<value>");
+  expandObjectInspectorNode(valueNode);
+  await waitFor(() => getObjectInspectorChildrenNodes(valueNode).length > 0);
+  checkChildren(valueNode, [`<state>`, `<value>`]);
+});
+
+add_task(async function testExpandCyclicPromise() {
+  const hud = await openNewTabAndConsole(TEST_URI);
+
+  await SpecialPowers.spawn(gBrowser.selectedBrowser, [], function() {
+    let resolve;
+    const cyclicPromise = new Promise(r => {
+      resolve = r;
+    });
+    Object.setPrototypeOf(cyclicPromise, null);
+    const otherPromise = Promise.reject(cyclicPromise);
+    otherPromise.catch(() => {});
+    Object.setPrototypeOf(otherPromise, null);
+    resolve(otherPromise);
+    content.wrappedJSObject.console.log("oi-test", cyclicPromise);
+  });
+
+  const node = await waitFor(() => findMessage(hud, "oi-test"));
+  const oi = node.querySelector(".tree");
+  const [promiseNode] = getObjectInspectorNodes(oi);
+
+  expandObjectInspectorNode(promiseNode);
+  await waitFor(() => getObjectInspectorNodes(oi).length > 1);
+  checkChildren(promiseNode, [`<state>`, `<value>`]);
+
+  const valueNode = findObjectInspectorNode(oi, "<value>");
+  expandObjectInspectorNode(valueNode);
+  await waitFor(() => getObjectInspectorChildrenNodes(valueNode).length > 0);
+  checkChildren(valueNode, [`<state>`, `<reason>`]);
+
+  const reasonNode = findObjectInspectorNode(oi, "<reason>");
+  expandObjectInspectorNode(reasonNode);
+  await waitFor(() => getObjectInspectorChildrenNodes(reasonNode).length > 0);
+  checkChildren(reasonNode, [`<state>`, `<value>`]);
+});
+
+function checkChildren(node, expectedChildren) {
+  const children = getObjectInspectorChildrenNodes(node);
+  is(
+    children.length,
+    expectedChildren.length,
+    "There is the expected number of children"
+  );
+  children.forEach((child, index) => {
+    ok(
+      child.textContent.includes(expectedChildren[index]),
+      `Expected "${expectedChildren[index]}" child`
+    );
+  });
+}

devtools/server/actors/object.js

@@ -165,8 +165,9 @@ const proto = {
 
     this.hooks.incrementGripDepth();
 
-    if (g.class == "Promise") {
+    // TODO (bug 1676476): remove this and instead add a previewer for promises.
-      g.promiseState = this._createPromiseState();
+    if (g.class == "Promise" && this.hooks.getGripDepth() < 3) {
+      g.promiseState = this.promiseState().promiseState;
     }
 
     if (g.class == "Function") {
@@ -250,7 +251,7 @@ const proto = {
   /**
    * Returns an object exposing the internal Promise state.
    */
-  _createPromiseState: function() {
+  promiseState: function() {
     const { state, value, reason } = getPromiseState(this.obj);
     const promiseState = { state };
 
@@ -267,7 +268,7 @@ const proto = {
       promiseState.timeToSettle = this.obj.promiseTimeToResolution;
     }
 
-    return promiseState;
+    return { promiseState };
   },
 
   /**

devtools/server/tests/xpcshell/test_objectgrips-nested-promise.js

@@ -0,0 +1,57 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+/* eslint-disable no-shadow, max-nested-callbacks */
+
+"use strict";
+
+Services.prefs.setBoolPref("security.allow_eval_with_system_principal", true);
+registerCleanupFunction(() => {
+  Services.prefs.clearUserPref("security.allow_eval_with_system_principal");
+});
+
+add_task(
+  threadFrontTest(async ({ threadFront, debuggee }) => {
+    const packet = await executeOnNextTickAndWaitForPause(
+      () => evalCode(debuggee),
+      threadFront
+    );
+
+    const [grip1, grip2] = packet.frame.arguments;
+    strictEqual(grip1.class, "Promise", "promise1 has a promise grip.");
+    strictEqual(grip2.class, "Promise", "promise2 has a promise grip.");
+
+    const objClient1 = threadFront.pauseGrip(grip1);
+    const objClient2 = threadFront.pauseGrip(grip2);
+    const { promiseState: state1 } = await objClient1.getPromiseState();
+    const { promiseState: state2 } = await objClient2.getPromiseState();
+
+    strictEqual(state1.state, "fulfilled", "promise1 was fulfilled.");
+    strictEqual(state1.value, objClient2, "promise1 fulfilled with promise2.");
+    ok(!state1.hasOwnProperty("reason"), "promise1 has no rejection reason.");
+
+    strictEqual(state2.state, "rejected", "promise2 was rejected.");
+    strictEqual(state2.reason, objClient1, "promise2 rejected with promise1.");
+    ok(!state2.hasOwnProperty("value"), "promise2 has no resolution value.");
+
+    await threadFront.resume();
+  })
+);
+
+function evalCode(debuggee) {
+  debuggee.eval(
+    function stopMe(arg) {
+      debugger;
+    }.toString()
+  );
+
+  debuggee.eval(`
+    var resolve;
+    var promise1 = new Promise(r => {resolve = r});
+    Object.setPrototypeOf(promise1, null);
+    var promise2 = Promise.reject(promise1);
+    promise2.catch(() => {});
+    Object.setPrototypeOf(promise2, null);
+    resolve(promise2);
+    stopMe(promise1, promise2);
+  `);
+}

devtools/server/tests/xpcshell/xpcshell.ini

@@ -160,6 +160,7 @@ skip-if = true # breakpoint sliding is not supported bug 1525685
 [test_objectgrips-fn-apply-01.js]
 [test_objectgrips-fn-apply-02.js]
 [test_objectgrips-fn-apply-03.js]
+[test_objectgrips-nested-promise.js]
 [test_objectgrips-nested-proxy.js]
 [test_promise_state-01.js]
 [test_promise_state-02.js]

devtools/shared/specs/object.js

@@ -92,6 +92,14 @@ types.addDictType("object.originalSourceLocation", {
   functionDisplayName: "string",
 });
 
+types.addDictType("object.promiseState", {
+  state: "string",
+  value: "nullable:object.descriptor",
+  reason: "nullable:object.descriptor",
+  creationTimestamp: "number",
+  timeToSettle: "nullable:number",
+});
+
 types.addDictType("object.proxySlots", {
   proxyTarget: "object.descriptor",
   proxyHandler: "object.descriptor",
@@ -188,6 +196,10 @@ const objectSpec = generateActorSpec({
         rejectionStack: RetVal("array:object.originalSourceLocation"),
       },
     },
+    promiseState: {
+      request: {},
+      response: RetVal("object.promiseState"),
+    },
     proxySlots: {
       request: {},
       response: RetVal("object.proxySlots"),