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

Status: VERIFIED FIXED

(DevTools :: Console, defect, P1)

Description

[Oriol Brufau [:Oriol]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

This is tracked at https://github.com/devtools-html/devtools-core/issues/494

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

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.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

Review commit: https://reviewboard.mozilla.org/r/163022/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163022/

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Attachment: To delete

Review commit: https://reviewboard.mozilla.org/r/163024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163024/

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8892013 [details]
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163022/diff/1-2/

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8892013 [details]
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163022/diff/2-3/

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1380709 - Fetch Map/Set entries when expanding the Object Inspector.

Add getObjectEntries and loadObjectEntries as props for the ObjectInspector so
we can display entries on Maps and Sets.

Review commit: https://reviewboard.mozilla.org/r/163432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163432/

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1380709 - Add mochitests and mocha tests to ensure entries are loaded as expected.

This adds several tests to make sure we can expand Sets and Maps entries, and that we manage
those properties in the store as expected.
We also add a stub for a Set object and take this as an opportunity to fix the stub name.

Review commit: https://reviewboard.mozilla.org/r/163438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163438/

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8892013 [details]
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163022/diff/3-4/

Comment 11

[Brian Grinstead [: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/#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 12

[Nicolas Chevobbe [:nchevobbe]]

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 13

[Nicolas Chevobbe [:nchevobbe]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163438/diff/1-2/

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

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 15

[Brian Grinstead [: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

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

Comment 16

[Brian Grinstead [:bgrins]]

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

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8892013 [details]
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163022/diff/4-5/

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8892464 [details]
Bug 1380709 - Fetch Map/Set entries when expanding the Object Inspector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163432/diff/1-2/

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163438/diff/2-3/

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

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 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8892013 [details]
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

https://reviewboard.mozilla.org/r/163022/#review168940

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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 23

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 25

[Brian Grinstead [:bgrins]]

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.

Comment 26

[Brian Grinstead [:bgrins]]

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)

Comment 27

[Brian Grinstead [:bgrins]]

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.

Comment 28

[Brian Grinstead [:bgrins]]

Going to reland on top of Bug 1386724

Comment 29

[Brian Grinstead [:bgrins]]

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 30

[Brian Grinstead [:bgrins]]

Attachment: Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

Review commit: https://reviewboard.mozilla.org/r/164102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164102/

Comment 31

[Brian Grinstead [:bgrins]]

Attachment: Bug 1380709 - Fetch Map/Set entries when expanding the Object Inspector.

Add getObjectEntries and loadObjectEntries as props for the ObjectInspector so
we can display entries on Maps and Sets.

Review commit: https://reviewboard.mozilla.org/r/164104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164104/

Comment 32

[Brian Grinstead [:bgrins]]

Attachment: Bug 1380709 - Add mochitests and mocha tests to ensure entries are loaded as expected.

This adds several tests to make sure we can expand Sets and Maps entries, and that we manage
those properties in the store as expected.
We also add a stub for a Set object and take this as an opportunity to fix the stub name.

Review commit: https://reviewboard.mozilla.org/r/164106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164106/

Comment 33

[Brian Grinstead [:bgrins]]

Comment on attachment 8893093 [details]
Bug 1380709 - reps v0.11.0: update reps bundle from GitHub;

https://reviewboard.mozilla.org/r/164102/#review169424

Comment 34

[Brian Grinstead [:bgrins]]

Comment on attachment 8893094 [details]
Bug 1380709 - Fetch Map/Set entries when expanding the Object Inspector.

https://reviewboard.mozilla.org/r/164104/#review169426

Comment 35

[Brian Grinstead [:bgrins]]

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

Comment 36

[Pulsebot]

Pushed by [e-mail]:
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

Comment 37

[Brian Grinstead [: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

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/322f930053e8
https://hg.mozilla.org/mozilla-central/rev/8e9068bcdc8c
https://hg.mozilla.org/mozilla-central/rev/69f4c2e35bb8

Comment 39

[Brian Grinstead [:bgrins]]

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

Comment 40

[Aki Sasaki (not active)]

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

Comment 41

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/89af946d5eca
https://hg.mozilla.org/releases/mozilla-beta/rev/70af3b39d7ae
https://hg.mozilla.org/releases/mozilla-beta/rev/c142603186d2

Comment 44

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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

Comment 45

[Gerry Chang [:gchang]]

Since it's verified in 56, no need to track for 56.