Closed Bug 1380709 Opened 2 years ago Closed 2 years ago

Object inspector does not show (Weak)Map/Set entries.

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox55 unaffected, firefox56- verified, firefox57 verified)

VERIFIED FIXED
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Status
firefox55 --- unaffected
firefox56 - verified
firefox57 --- verified

People

(Reporter: Oriol, Assigned: nchevobbe)

References

()

Details

(Whiteboard: [reserve-console-html])

Attachments

(3 files, 4 obsolete files)

1. Open the new console frontend
2. Enter new Map([[0,0],[1,1],[2,2],[3,3],[4,4],[5,5],[6,6],[7,7],[8,8],[9,9],[10,10]])
3. Not all the entries fit in the preview, so expand the object

Result: object inspector does not show the Map entries.
Whiteboard: [console-html] [triage]
Depends on: 1380790
At the moment, we are only fetching properties, not entries in the console.
We could use http://searchfox.org/mozilla-central/source/devtools/shared/client/main.js#2643 to load entries in http://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/actions/messages.js#122-127.
Summary: Object inspector does not show Map entries. → Object inspector does not show (Weak)Map/Set entries.
Duplicate of this bug: 1380707
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [console-html] [triage] → [console-html]
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Flags: qe-verify? → qe-verify+
Priority: P2 → P1
Iteration: --- → 56.3 - Jul 24
QA Contact: iulia.cristescu
No longer depends on: 1380790
Depends on: 1383797
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Attachment #8892014 - Attachment is obsolete: true
Comment on attachment 8892466 [details]
Bug 1380709 - Add mochitests and mocha tests to ensure entries are loaded as expected.

https://reviewboard.mozilla.org/r/163438/#review168796

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_object_inspector_entries.js:12
(Diff revision 1)
> +
> +// Check expanding/collapsing maps and sets in the console.
> +const TEST_URI = "data:text/html;charset=utf8,<h1>test Object Inspector</h1>";
> +
> +add_task(async function () {
> +  let toolbox = await openNewTabAndToolbox(TEST_URI, "webconsole");

There's a helper for openNewTabAndConsole that returns a hud and can replace these two lines

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_object_inspector_entries.js:57
(Diff revision 1)
> +    mediumSetOi,
> +    largeSetOi,
> +  ] = objectInspectors;
> +
> +  await testSmallMap(smallMapOi);
> +  await testMediumMap(mediumMapOi);

I don't see extra UI behavior covered in the mediumMap and mediumSet tests as compared to smallMap and smallSet, so I'd be inclined to remove them unless if I'm missing something. If it's just checking that the correct # of entries are returned, that should be captured by any existing object client tests and the unit tests for the store.
Comment on attachment 8892466 [details]
Bug 1380709 - Add mochitests and mocha tests to ensure entries are loaded as expected.

https://reviewboard.mozilla.org/r/163438/#review168796

> I don't see extra UI behavior covered in the mediumMap and mediumSet tests as compared to smallMap and smallSet, so I'd be inclined to remove them unless if I'm missing something. If it's just checking that the correct # of entries are returned, that should be captured by any existing object client tests and the unit tests for the store.

yeah, I'm kind of testing a case from the object inspector: 
if we have all the entries in the preview property of the Grip (<= 10 entries), then we don't call loadEntries.
This is why we have small and medium, large is for testing buckets.
Those are already tested in the ObjectInspector, and if I had to remove one it would be the small one, which is less "risky"
Comment on attachment 8892466 [details]
Bug 1380709 - Add mochitests and mocha tests to ensure entries are loaded as expected.

https://reviewboard.mozilla.org/r/163438/#review168796

> There's a helper for openNewTabAndConsole that returns a hud and can replace these two lines

DONE

> yeah, I'm kind of testing a case from the object inspector: 
> if we have all the entries in the preview property of the Grip (<= 10 entries), then we don't call loadEntries.
> This is why we have small and medium, large is for testing buckets.
> Those are already tested in the ObjectInspector, and if I had to remove one it would be the small one, which is less "risky"

DONE
Comment on attachment 8892466 [details]
Bug 1380709 - Add mochitests and mocha tests to ensure entries are loaded as expected.

https://reviewboard.mozilla.org/r/163438/#review168828

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_object_inspector_entries.js:16
(Diff revision 2)
> +add_task(async function () {
> +  const hud = await openNewTabAndConsole(TEST_URI);
> +  const store = hud.ui.newConsoleOutput.getStore();
> +  // Adding logging each time the store is modified in order to check
> +  // the store state in case of failure.
> +  store.subscribe(() => {

I think this would be nice as an option to openNewTabAndConsole so that it can be opted into when debugging a local or intermittent failure.
Attachment #8892466 - Flags: review?(bgrinstead) → review+
Comment on attachment 8892464 [details]
Bug 1380709 - Fetch Map/Set entries when expanding the Object Inspector.

https://reviewboard.mozilla.org/r/163432/#review168830

Apparently we have a lot of boilerplate
Attachment #8892464 - Flags: review?(bgrinstead) → review+
Comment on attachment 8892466 [details]
Bug 1380709 - Add mochitests and mocha tests to ensure entries are loaded as expected.

https://reviewboard.mozilla.org/r/163438/#review168828

> I think this would be nice as an option to openNewTabAndConsole so that it can be opted into when debugging a local or intermittent failure.

I'll do this as a follow up since many tests have it
Comment on attachment 8892013 [details]
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

https://reviewboard.mozilla.org/r/163022/#review168940
Attachment #8892013 - Flags: review?(bgrinstead) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f85fbccf2c1f
reps v0.11.0: update reps bundle from GitHub; r=bgrins
https://hg.mozilla.org/integration/autoland/rev/306169f86028
Fetch Map/Set entries when expanding the Object Inspector. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/44cfd92f709c
Add mochitests and mocha tests to ensure entries are loaded as expected. r=bgrins
Comment on attachment 8892466 [details]
Bug 1380709 - Add mochitests and mocha tests to ensure entries are loaded as expected.

https://reviewboard.mozilla.org/r/163438/#review168828

> I'll do this as a follow up since many tests have it

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1386523 for this
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Whiteboard: [console-html] → [reserve-console-html]
Backed out for failing devtools' browser_dynamic_tool_enabling.js on Windows 8 x64 debug with e10s:

https://hg.mozilla.org/integration/autoland/rev/a2010cbc4da9137a1010f92b87326aed1191d30d
https://hg.mozilla.org/integration/autoland/rev/d7ab99a5fc8adc2b10d421baa43ff4787647b927
https://hg.mozilla.org/integration/autoland/rev/13b1281ab1a4cf1009768d524278447abd26f969

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=44cfd92f709cd8888549565a917d4cea6ee5d379&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=120394754&repo=autoland

07:33:19     INFO - TEST-START | devtools/client/framework/test/browser_dynamic_tool_enabling.js
07:33:19     INFO - GECKO(2404) | ++DOCSHELL 0000004451E0A000 == 1 [pid = 4048] [id = {87f9581c-3712-45b9-99a0-29da91a2542a}]
07:33:19     INFO - GECKO(2404) | ++DOMWINDOW == 1 (0000004451E0B000) [pid = 4048] [serial = 1] [outer = 0000000000000000]
07:33:19     INFO - TEST-INFO | started process screenshot
07:33:19     INFO - TEST-INFO | screenshot: exit 0
07:33:19     INFO - TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_dynamic_tool_enabling.js | Exception thrown - at chrome://mochitests/content/browser/devtools/client/framework/test/browser_dynamic_tool_enabling.js:21 - TypeError: el is null
Flags: needinfo?(nchevobbe)
Huh, I get the same error locally on a clean m-c checkout with or without these patches applied. I'm not sure what this would have done to trigger an error in automation, or why it passes on m-c with win8 x64 debug e10s.
I think this patch series caused browser_dynamic_tool_enabling.js to become the first test in it's chunk, and it fails if initDevTools hasn't been called (which happens if another test hasn't been run)
Depends on: 1386724
Confirmed the failure was just bad luck due to chunking. Going to land a fix for that in Bug 1386724 and then we can re-land.
Going to reland on top of Bug 1386724
Flags: needinfo?(nchevobbe)
I'm unable to reopen the mozreview request since I didn't originally open it.  Going to re-push Nicolas' patches to mozreview and see if I can land it that way
Comment on attachment 8893093 [details]
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

https://reviewboard.mozilla.org/r/164102/#review169424
Attachment #8893093 - Flags: review?(bgrinstead) → review+
Comment on attachment 8893094 [details]
Bug 1380709 - Fetch Map/Set entries when expanding the Object Inspector.

https://reviewboard.mozilla.org/r/164104/#review169426
Attachment #8893094 - Flags: review?(bgrinstead) → review+
Comment on attachment 8893095 [details]
Bug 1380709 - Add mochitests and mocha tests to ensure entries are loaded as expected.

https://reviewboard.mozilla.org/r/164106/#review169430
Attachment #8893095 - Flags: review?(bgrinstead) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/322f930053e8
reps v0.11.0: update reps bundle from GitHub; r=bgrins
https://hg.mozilla.org/integration/autoland/rev/8e9068bcdc8c
Fetch Map/Set entries when expanding the Object Inspector. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/69f4c2e35bb8
Add mochitests and mocha tests to ensure entries are loaded as expected. r=bgrins
[Tracking Requested - why for this release]: This is a feature for the new webconsole UI (which is on in Dev Edition but off in Beta). It adds supports for inspecting entries in Set and Map objects
Attachment #8893093 - Attachment is obsolete: true
Attachment #8893094 - Attachment is obsolete: true
Attachment #8893095 - Attachment is obsolete: true
Comment on attachment 8892013 [details]
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1381630
[User impact if declined]: Map and Set entries won't be seen in the webconsole in Dev Edition
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: Just the three patches in this bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: This is only affecting code running inside the webconsole iframe. This only affects Dev Edition.
[String changes made/needed]: None
Attachment #8892013 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/projects/date/rev/f85fbccf2c1f82a78e7a622e464eef2c918cb04f
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub; r=bgrins

https://hg.mozilla.org/projects/date/rev/306169f86028f9576dcc93590fe9de2d8eb1aaeb
Bug 1380709 - Fetch Map/Set entries when expanding the Object Inspector. r=bgrins

https://hg.mozilla.org/projects/date/rev/322f930053e8e79d769adacecc90bdfe50095c45
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub; r=bgrins

https://hg.mozilla.org/projects/date/rev/8e9068bcdc8c41d587ded07eb69a0f4d311879ab
Bug 1380709 - Fetch Map/Set entries when expanding the Object Inspector. r=bgrins

https://hg.mozilla.org/projects/date/rev/69f4c2e35bb82ed0710fde6783c7a0c8a890ef1c
Bug 1380709 - Add mochitests and mocha tests to ensure entries are loaded as expected. r=bgrins
Managed to reproduce the initial issue on 57.0a1 (2017-07-13). I can confirm the issue is verified fixed on 57.0a1 (2017-08-09) using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS 10.11.6.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment on attachment 8892013 [details]
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

Fix for web console issues, verified in nightly. Let's uplift for beta 2.
Attachment #8892013 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Also confirm that the issue is fixed on 56.0b2 Devedition (20170810180547) using Windows 10 x64, Ubuntu 14.04 x64 and Mac OS 10.11.6. On 56.0b2 build1 (20170810180547) the entries are shown, but in the old webconsole UI (which I assume that is's expected per comment 37).
Since it's verified in 56, no need to track for 56.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.