1604688 - Incorrect representation of sparse arrays in WebConsole

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[709922234]

Attachment: Screen Shot 2019-12-18 at 11.28.41 AM.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:73.0) Gecko/20100101 Firefox/73.0

Steps to reproduce:

WebConsole typeing new Array(4)

Actual results:

show Array(4) [ undefined, undefined, undefined, undefined ]

Expected results:

Representing a sparse array

Comment 1

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

The priority flag is not set for this bug.
:nchevobbe, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Sid Vishnoi :sidvishnoi]

Hi!
I dived into this issue and found in reps/grip-array.js, that itemGrip was never null (in arrayIterator), so emptySlot count was never incremented.

Replacing items.push(hooks.createValueGrip(undefined)) with items.push(null) (https://searchfox.org/mozilla-central/source/devtools/server/actors/object/previewers.js#193) seemed to fix the issue, but it appears to have caused some issue with console.table (https://bugzilla.mozilla.org/show_bug.cgi?id=1565781).

I found const desc = Object.getOwnPropertyDescriptor(Cu.waiveXrays(raw), i); is undefined when we've an emptySlot, so following changeset appears to fix the problem:

          // devtools/server/actors/object/previewers.js
          if (desc && !desc.get && !desc.set) {
            let value = Cu.unwaiveXrays(desc.value);
            value = ObjectUtils.makeDebuggeeValueIfNeeded(obj, value);
            items.push(hooks.createValueGrip(value));
+         } else if (!desc) {
+           items.push(null);
          } else {
            items.push(hooks.createValueGrip(undefined));
          }

Should I send a patch? Would it be a good first issue?

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Hello Sid, thanks for investigating.
You can definitely send a patch, that would be much appreciated.
I'm going to assign the bug to you

Comment 4

[Sid Vishnoi :sidvishnoi]

Hi... I can't find where to add test for this.
The closest I've reached is client/webconsole/test/browser/browser_webconsole_console_table.js, but there is nothing for regular console.log. Then I thought client/debugger/packages/devtools-reps/src/reps/tests/array.js should have worked, but it shows outdated results. Do I need to run some build step (I did run mach build)?

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Then I thought client/debugger/packages/devtools-reps/src/reps/tests/array.js should have worked, but it shows outdated results

The test would be in devtools/client/debugger/packages/devtools-reps/src/reps/tests/grip-array.js (array.js tests non-grip arrays.)

The issue with the test is that it consumes stubs (devtools/client/debugger/packages/devtools-reps/src/reps/stubs/grip-array.js ), which are hand-written :/

What could be done is to add a unit test (similar to the other ones we have, like devtools/server/tests/unit/test_objectgrips-17.js ), to check we return null for empty slots, but that we do respect undefined item in arrays.

Let me know how this goes :)

Comment 6

[Sid Vishnoi :sidvishnoi]

Attachment: Bug 1604688 - Use null for empty slots in Array grip preview. r=nchevobbe

Comment 7

[Sid Vishnoi :sidvishnoi]

I'm not sure why xpcshell test failed locally. I'm getting the following output:

 0:40.76 INFO (xpcshell/head.js) | test finished (2)
 0:40.76 PASS  - The grip has an Array class - "Array" === "Array"
 0:40.76 PASS  - The empty slot has null as grip preview - null === null
 0:40.76 PASS  - The undefined value has grip value of type undefined - {"type":"undefined"} deepEqual {"type":"undefined"}
... then after a moment...
 0:40.88 INFO (xpcshell/head.js) | test finished (2)
 0:40.88 PASS  - The grip has an Array class - "Array" === "Array"
 0:40.88 FAIL  - The empty slot has null as grip preview - {"type":"undefined"} === null

I've started a try run to check if it's my local setup failing or something else: https://treeherder.mozilla.org/#/jobs?repo=try&revision=85adc7212495ef5ccdec337fb27ade00cbcb841a

Comment 8

[Sid Vishnoi :sidvishnoi]

Update: Oh. It's failing against worker.
>>> Run thread front test against a worker DebuggerServer was lost in logs.

But I'm am not able to understand how to fix that. test_objectgrips-17.js suggests I need to run tests against systemPrincipal and nullPrincipal and then with/without XRays... but I don't understand how to use and which ones to use (and why).

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

I tried to answer in Phabricator, let me know how this turns out :)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7dacd86c6a96
Use null for empty slots in Array grip preview. r=nchevobbe

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7dacd86c6a96