Fix expanding longString when the property is a getter. (#6785)

When we have a getter value, it is stored in a different property in the node. Which means that if the getter returns a longString, we couldn't assign the fullText to the non-getter property (and as a result, an exception was thrown). This is not a common bug since we only automatically evaluate safe getters, i.e. native property getters that we know are side-effect free. A test is added to make sure we handle this as expected.
firefox-devtools · Aug 15, 2018 · 74b0d76 · 74b0d76
1 parent 5204b69
commit 74b0d76
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 5 deletions.
diff --git a/packages/devtools-reps/src/object-inspector/stubs/grip.js b/packages/devtools-reps/src/object-inspector/stubs/grip.js
@@ -44,4 +44,21 @@ stubs.set("proto-properties-symbols", {
   ]
 });
 
+stubs.set("longs-string-safe-getter", {
+  ownProperties: {
+    baseVal: {
+      getterValue: {
+        type: "longString",
+        initial: "data:image/png;base64,initial",
+        length: 95080,
+        actor: "server1.conn1.child1/longString28"
+      },
+      getterPrototypeLevel: 1,
+      enumerable: true,
+      writable: true
+    }
+  },
+  from: "server1.conn1.child1/propertyIterator30"
+});
+
 module.exports = stubs;
diff --git a/packages/devtools-reps/src/object-inspector/tests/component/__snapshots__/expand.js.snap b/packages/devtools-reps/src/object-inspector/tests/component/__snapshots__/expand.js.snap
@@ -30,6 +30,14 @@ exports[`ObjectInspector - state does not throw when expanding a block node 2`]
 "
 `;
 
+exports[`ObjectInspector - state expanding a getter returning a longString does not throw 1`] = `
+"
+▼ {…}
+|  ▼ baseVal: \\"data:image/png;base64,initial<<<<\\"
+▶︎ Proxy { <target>: {…}, <handler>: (3) […] }
+"
+`;
+
 exports[`ObjectInspector - state expands if user selected some text and clicked the arrow 1`] = `
 "
 ▶︎ Object { p0: \\"0\\", p1: \\"1\\", p2: \\"2\\", p3: \\"3\\", p4: \\"4\\", p5: \\"5\\", p6: \\"6\\", p7: \\"7\\", p8: \\"8\\", p9: \\"9\\", … }

diff --git a/packages/devtools-reps/src/object-inspector/tests/component/expand.js b/packages/devtools-reps/src/object-inspector/tests/component/expand.js
@@ -365,4 +365,46 @@ describe("ObjectInspector - state", () => {
     expect(recordTelemetryEvent.mock.calls).toHaveLength(4);
     expect(recordTelemetryEvent.mock.calls[3][0]).toEqual("object_expanded");
   });
+
+  it("expanding a getter returning a longString does not throw", async () => {
+    const LongStringClientMock = require("../__mocks__/long-string-client");
+
+    const wrapper = mount(
+      ObjectInspector(
+        generateDefaults({
+          injectWaitService: true,
+          loadedProperties: new Map([
+            ["root-1", gripPropertiesStubs.get("longs-string-safe-getter")]
+          ]),
+          focusable: false,
+          createLongStringClient: grip =>
+            LongStringClientMock(grip, {
+              substring: function(initiaLength, length, cb) {
+                cb({
+                  substring: "<<<<"
+                });
+              }
+            })
+        })
+      )
+    );
+
+    const store = wrapper.instance().getStore();
+
+    wrapper
+      .find(".node")
+      .at(0)
+      .simulate("click");
+    wrapper.update();
+
+    const onPropertiesLoaded = waitForDispatch(store, "NODE_PROPERTIES_LOADED");
+    wrapper
+      .find(".node")
+      .at(1)
+      .simulate("click");
+    await onPropertiesLoaded;
+
+    wrapper.update();
+    expect(formatObjectInspector(wrapper)).toMatchSnapshot();
+  });
 });
diff --git a/packages/devtools-reps/src/object-inspector/utils/node.js b/packages/devtools-reps/src/object-inspector/utils/node.js
@@ -52,11 +52,11 @@ function getType(item: Node): Symbol {
 }
 
 function getValue(item: Node): RdpGrip | ObjectInspectorItemContentsValue {
-  if (item && item.contents && item.contents.hasOwnProperty("value")) {
+  if (nodeHasValue(item)) {
     return item.contents.value;
   }
 
-  if (item && item.contents && item.contents.hasOwnProperty("getterValue")) {
+  if (nodeHasGetterValue(item)) {
     return item.contents.getterValue;
   }
 
@@ -83,6 +83,14 @@ function nodeHasChildren(item: Node): boolean {
   return Array.isArray(item.contents);
 }
 
+function nodeHasValue(item: Node): boolean {
+  return item && item.contents && item.contents.hasOwnProperty("value");
+}
+
+function nodeHasGetterValue(item: Node): boolean {
+  return item && item.contents && item.contents.hasOwnProperty("getterValue");
+}
+
 function nodeIsObject(item: Node): boolean {
   const value = getValue(item);
   return value && value.type === "object";
@@ -595,12 +603,15 @@ function makeNodesForProperties(
 }
 
 function setNodeFullText(loadedProps: GripProperties, node: Node): Node {
-  if (nodeHasFullText(node)) {
+  if (nodeHasFullText(node) || !nodeIsLongString(node)) {
     return node;
   }
 
-  if (nodeIsLongString(node)) {
-    node.contents.value.fullText = loadedProps.fullText;
+  const { fullText } = loadedProps;
+  if (nodeHasValue(node)) {
+    node.contents.value.fullText = fullText;
+  } else if (nodeHasGetterValue(node)) {
+    node.contents.getterValue.fullText = fullText;
   }
 
   return node;