Closed Bug 1604688 Opened 4 years ago Closed 4 years ago

Incorrect representation of sparse arrays in WebConsole

Categories

(DevTools :: Console, defect, P3)

73 Branch
defect

Tracking

(firefox74 fixed)

RESOLVED FIXED
Firefox 74
Tracking Status
firefox74 --- fixed

People

(Reporter: 709922234, Assigned: sidvishnoi)

Details

Attachments

(2 files)

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

Component: Untriaged → Console
Product: Firefox → DevTools

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(nchevobbe)
Flags: needinfo?(nchevobbe)
Priority: -- → P3
Summary: Sparse array shows error in WebConsole → Incorrect representation of sparse arrays in WebConsole

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?

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

Assignee: nobody → sidvishnoi8
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

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)?

Flags: needinfo?(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 :)

Flags: needinfo?(nchevobbe)

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

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).

Flags: needinfo?(nchevobbe)

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

Flags: needinfo?(nchevobbe)
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7dacd86c6a96
Use null for empty slots in Array grip preview. r=nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: