Bug 1371936 - Fix erroneous Object as ArrayLike grip; r=bgrins. draft
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Thu, 16 Nov 2017 16:04:42 +0100
changeset 699478 5e6a0ef0da84cf89518164f518a257bd1f0d5fd8
parent 698679 f41930a869a84af81df1a88d8e82323ff3a6509a
child 740641 33289f455feb7d1f9d7a63c8ddcedd0d77e96b74
push id89586
push userbmo:nchevobbe@mozilla.com
push dateFri, 17 Nov 2017 07:54:13 +0000
reviewersbgrins
bugs1371936
milestone59.0a1
Bug 1371936 - Fix erroneous Object as ArrayLike grip; r=bgrins. We only label Object as being ArrayLike if they have consecutive, numeric indexes, starting at 0, and that could contain only a non-numeric length property that matches the actual number of numeric keys in the object. A test is added to make sure we don't regress this. Fix old console frontend tests which relied on the bad implementation of ArrayLike (and delete test cases now covered by the server test). MozReview-Commit-ID: ATF7WypNVhh
devtools/client/webconsole/test/browser_webconsole_output_05.js
devtools/client/webconsole/test/browser_webconsole_output_06.js
devtools/server/actors/object.js
devtools/server/tests/unit/test_objectgrips-array-like-object.js
devtools/server/tests/unit/xpcshell.ini
--- a/devtools/client/webconsole/test/browser_webconsole_output_05.js
+++ b/devtools/client/webconsole/test/browser_webconsole_output_05.js
@@ -128,24 +128,24 @@ var inputTests = [
     output: 'Promise { <state>: "pending", foo: 1 }',
     printOutput: "[object Promise]",
     inspectable: true,
     variablesViewLabel: "Promise"
   },
 
   // 13
   {
-    input: "new Object({1: 'this\\nis\\nsupposed\\nto\\nbe\\na\\nvery" +
+    input: "new Object({0: 'this\\nis\\nsupposed\\nto\\nbe\\na\\nvery" +
            "\\nlong\\nstring\\n,shown\\non\\na\\nsingle\\nline', " +
-           "2: 'a shorter string', 3: 100})",
-    output: '[ <1 empty slot>, "this is supposed to be a very long ' + ELLIPSIS +
+           "1: 'a shorter string', 2: 100})",
+    output: '[ "this is supposed to be a very long ' + ELLIPSIS +
             '", "a shorter string", 100 ]',
     printOutput: "[object Object]",
     inspectable: true,
-    variablesViewLabel: "Object[4]"
+    variablesViewLabel: "Object[3]"
   },
 
   // 14
   {
     input: "new Proxy({a:1},[1,2,3])",
     output: 'Proxy { <target>: Object, <handler>: Array[3] }',
     printOutput: "[object Object]",
     inspectable: true,
--- a/devtools/client/webconsole/test/browser_webconsole_output_06.js
+++ b/devtools/client/webconsole/test/browser_webconsole_output_06.js
@@ -125,145 +125,91 @@ var inputTests = [
     output: 'Object [ "a", "b" ]',
     printOutput: "[object Object]",
     inspectable: true,
     variablesViewLabel: "Object[2]",
   },
 
   // 14
   {
-    input: '({0: "a", 42: "b"})',
-    output: '[ "a", <9 empty slots>, 33 more\u2026 ]',
-    printOutput: "[object Object]",
-    inspectable: true,
-    variablesViewLabel: "Object[43]",
-  },
-
-  // 15
-  {
     input: '({0: "a", 1: "b", 2: "c", 3: "d", 4: "e", 5: "f", 6: "g", ' +
            '7: "h", 8: "i", 9: "j", 10: "k", 11: "l"})',
     output: 'Object [ "a", "b", "c", "d", "e", "f", "g", "h", "i", "j", ' +
             "2 more\u2026 ]",
     printOutput: "[object Object]",
     inspectable: true,
     variablesViewLabel: "Object[12]",
   },
 
-  // 16
+  // 15
   {
     input: '({0: "a", 1: "b", 2: "c", 3: "d", 4: "e", 5: "f", 6: "g", ' +
            '7: "h", 8: "i", 9: "j", 10: "k", 11: "l", m: "n"})',
     output: 'Object { 0: "a", 1: "b", 2: "c", 3: "d", 4: "e", 5: "f", ' +
             '6: "g", 7: "h", 8: "i", 9: "j", 3 more\u2026 }',
     printOutput: "[object Object]",
     inspectable: true,
     variablesViewLabel: "Object",
   },
 
-  // 17
+  // 16
   {
     input: '({" ": "a"})',
     output: 'Object {  : "a" }',
     printOutput: "[object Object]",
     inspectable: true,
     variablesViewLabel: "Object",
   },
 
-  // 18
+  // 17
   {
     input: '({})',
     output: 'Object {  }',
     printOutput: "[object Object]",
     inspectable: true,
     variablesViewLabel: "Object",
   },
 
-  // 19
-  {
-    input: '({length: 0})',
-    output: 'Object [  ]',
-    printOutput: "[object Object]",
-    inspectable: true,
-    variablesViewLabel: "Object[0]",
-  },
-
-  // 20
-  {
-    input: '({length: 1})',
-    output: '[ <1 empty slot> ]',
-    printOutput: "[object Object]",
-    inspectable: true,
-    variablesViewLabel: "Object[1]",
-  },
-
-  // 21
-  {
-    input: '({0: "a", 1: "b", length: 1})',
-    output: 'Object { 0: "a", 1: "b", length: 1 }',
-    printOutput: "[object Object]",
-    inspectable: true,
-    variablesViewLabel: "Object",
-  },
-
-  // 22
+  // 18
   {
     input: '({0: "a", 1: "b", length: 2})',
     output: 'Object [ "a", "b" ]',
     printOutput: "[object Object]",
     inspectable: true,
     variablesViewLabel: "Object[2]",
   },
 
-  // 23
-  {
-    input: '({0: "a", 1: "b", length: 3})',
-    output: '[ "a", "b", <1 empty slot> ]',
-    printOutput: "[object Object]",
-    inspectable: true,
-    variablesViewLabel: "Object[3]",
-  },
-
-  // 24
+  // 19
   {
     input: '({0: "a", 2: "b", length: 2})',
     output: 'Object { 0: "a", 2: "b", length: 2 }',
     printOutput: "[object Object]",
     inspectable: true,
     variablesViewLabel: "Object",
   },
 
-  // 25
-  {
-    input: '({0: "a", 2: "b", length: 3})',
-    output: '[ "a", <1 empty slot>, "b" ]',
-    printOutput: "[object Object]",
-    inspectable: true,
-    variablesViewLabel: "Object[3]",
-  },
-
-  // 26
+  // 20
   {
     input: '({0: "a", b: "b", length: 1})',
     output: 'Object { 0: "a", b: "b", length: 1 }',
     printOutput: "[object Object]",
     inspectable: true,
     variablesViewLabel: "Object",
   },
 
-  // 27
+  // 21
   {
     input: '({0: "a", b: "b", length: 2})',
     output: 'Object { 0: "a", b: "b", length: 2 }',
     printOutput: "[object Object]",
     inspectable: true,
     variablesViewLabel: "Object",
   },
 
-  // 28
+  // 22
   {
     input: '({42: "a"})',
     output: 'Object { 42: "a" }',
     printOutput: "[object Object]",
     inspectable: true,
     variablesViewLabel: "Object",
   },
 ];
--- a/devtools/server/actors/object.js
+++ b/devtools/server/actors/object.js
@@ -1936,33 +1936,41 @@ DebuggerServer.ObjectActorPreviewers.Obj
   function PseudoArray({obj, hooks}, grip, rawObj) {
     let length;
 
     let keys = obj.getOwnPropertyNames();
     if (keys.length == 0) {
       return false;
     }
 
-    // If no item is going to be displayed in preview, better display as sparse object.
-    // The first key should contain the smallest integer index (if any).
-    if (keys[0] >= OBJECT_PREVIEW_MAX_ITEMS) {
+    // We don't want to represent Objects as sparse arrays, so every property
+    // should match its index, or be the length property.
+    if (keys.some((key, i) => parseInt(key, 10) !== i && key !== "length")) {
       return false;
     }
 
     // Pseudo-arrays should only have array indices and, optionally, a "length" property.
     // Since integer indices are sorted first, check if the last property is "length".
     if (keys[keys.length - 1] === "length") {
       keys.pop();
       length = DevToolsUtils.getProperty(obj, "length");
     } else {
       // Otherwise, let length be the (presumably) greatest array index plus 1.
       length = +keys[keys.length - 1] + 1;
     }
-    // Check if length is a valid array length, i.e. is a Uint32 number.
-    if (typeof length !== "number" || length >>> 0 !== length) {
+
+    // If they are no numeric keys, or if the length does not represent the actual
+    // object length, or is not a valid array length, i.e. is a Uint32 number,
+    // do not label the object as ArrayLike.
+    if (
+      keys.length === 0 ||
+      keys.length !== length ||
+      typeof length !== "number" ||
+      length >>> 0 !== length
+    ) {
       return false;
     }
 
     // Ensure all keys are increasing array indices smaller than length. The order is not
     // guaranteed for exotic objects but, in most cases, big array indices and properties
     // which are not integer indices should be at the end. Then, iterating backwards
     // allows us to return earlier when the object is not completely a pseudo-array.
     let prev = length;
new file mode 100644
--- /dev/null
+++ b/devtools/server/tests/unit/test_objectgrips-array-like-object.js
@@ -0,0 +1,67 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that objects are labeled as ArrayLike only when they have sequential
+// numeric keys, and if they have a length property, that it matches the number
+// of numeric keys. (See Bug 1371936)
+
+async function run_test() {
+  do_test_pending();
+  await run_test_with_server(DebuggerServer);
+  await run_test_with_server(WorkerDebuggerServer);
+  do_test_finished();
+}
+
+async function run_test_with_server(server) {
+  initTestDebuggerServer(server);
+  const debuggee = addTestGlobal("test-grips", server);
+  debuggee.eval(function stopMe(arg1) {
+    debugger;
+  }.toString());
+
+  const dbgClient = new DebuggerClient(server.connectPipe());
+  await dbgClient.connect();
+  const [,, threadClient] = await attachTestTabAndResume(dbgClient, "test-grips");
+
+  // Currying test function so we don't have to pass the debuggee and clients
+  const isArrayLike = object => test_object_grip_is_array_like(
+    debuggee, dbgClient, threadClient, object);
+
+  equal(await isArrayLike({}), false, "An empty object is not ArrayLike");
+  equal(await isArrayLike({length: 0}), false,
+    "An object with only a length property is not ArrayLike");
+  equal(await isArrayLike({2: "two"}), false,
+    "Object not starting at 0 is not ArrayLike");
+  equal(await isArrayLike({0: "zero", 2: "two"}), false,
+    "Object with non-consecutive numeric keys is not ArrayLike");
+  equal(await isArrayLike({0: "zero", 2: "two", length: 2}), false,
+    "Object with non-consecutive numeric keys is not ArrayLike");
+  equal(await isArrayLike({0: "zero", 1: "one", 2: "two", three: 3}), false,
+    "Object with a non-numeric property other than `length` is not ArrayLike");
+  equal(await isArrayLike({0: "zero", 1: "one", 2: "two", three: 3, length: 3}), false,
+    "Object with a non-numeric property other than `length` is not ArrayLike");
+  equal(await isArrayLike({0: "zero", 1: "one", 2: "two", length: 30}), false,
+    "Object with a wrongful `length` property is not ArrayLike");
+
+  equal(await isArrayLike({0: "zero"}), true);
+  equal(await isArrayLike({0: "zero", 1: "two"}), true);
+  equal(await isArrayLike({0: "zero", 1: "one", 2: "two", length: 3}), true);
+
+  await dbgClient.close();
+}
+
+async function test_object_grip_is_array_like(debuggee, dbgClient, threadClient, object) {
+  return new Promise((resolve, reject) => {
+    threadClient.addOneTimeListener("paused", async function (event, packet) {
+      let [grip] = packet.frame.arguments;
+      await threadClient.resume();
+      resolve(grip.preview.kind === "ArrayLike");
+    });
+
+    debuggee.eval(`
+      stopMe(${JSON.stringify(object)});
+    `);
+  });
+}
--- a/devtools/server/tests/unit/xpcshell.ini
+++ b/devtools/server/tests/unit/xpcshell.ini
@@ -172,16 +172,17 @@ reason = only ran on B2G
 [test_objectgrips-13.js]
 [test_objectgrips-14.js]
 [test_objectgrips-15.js]
 [test_objectgrips-16.js]
 [test_objectgrips-17.js]
 [test_objectgrips-18.js]
 [test_objectgrips-19.js]
 [test_objectgrips-20.js]
+[test_objectgrips-array-like-object.js]
 [test_promise_state-01.js]
 [test_promise_state-02.js]
 [test_promise_state-03.js]
 [test_interrupt.js]
 [test_stepping-01.js]
 [test_stepping-02.js]
 [test_stepping-03.js]
 [test_stepping-04.js]