Closed Bug 1172920 Opened 9 years ago Closed 8 years ago

DevTools: Map/Set entries should be visible in the Variables view

Categories

(DevTools :: Console, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: jsnajdr, Assigned: jsnajdr)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(4 files, 5 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150608030201

Steps to reproduce:

Try to inspect a Map/Set object in the Variables view.



Expected results:

I should be able to inspect the individual entries. Right now, I can see only the object properties, length, prototype etc. The entries are completely hidden.

There are several workarounds - add a [...map] watch expression, log the variable in console (console pretty-printing works), but Variables view inspection is sorely missing anyway.
Attached image GoogleChrome.png
This got mentioned again today on twitter: https://twitter.com/steltenpower/status/631838818505728000
Google Chrome does show the entries when expanding a Set or a Map in the console (see screenshot).
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Console
Ever confirmed: true
Version: 41 Branch → unspecified
Attached image Firefox.png
With the same code, here's the result in Firefox.
You can still see and expand entries, but clicking on the word "Map" only shows the map's properties in the sidebar. It'd be nice if it also showed all the entries.
Patrick, can you assign a difficulty= to this? Might be a good next bug?
Flags: needinfo?(pbrosset)
Priority: -- → P2
Whiteboard: [polish-backlog]
Assignee: nobody → lin.w.clark
(In reply to Jeff Griffiths (:canuckistani) from comment #3)
> Patrick, can you assign a difficulty= to this? Might be a good next bug?
Definitely. I'm probably not the best to assess the dificulty for this bug though, and it looks like it was already assigned, which is great!
Flags: needinfo?(pbrosset)
Whiteboard: [polish-backlog] → [polish-backlog][difficulty=medium]
This bug should include adding support for WeakMap and WeakSet to the variables view as well.
Assignee: lclark → nobody
I noticed recently that ArrayBuffer entries also are not displayed.
Added a first simple version of a patch. It displays the Map entries, the attached screenshot shows how it looks like.

It supports only the Map type, and uses the 'preview' field returned in the object grip. That preview is limited to 10 entries (OBJECT_PREVIEW_MAX_ITEMS), so there is a lot more work needed to remove this limitation.

Todo:

1. In order to display all the Map entries, a new ObjectClient method for enumerating the entries will be added. Similar to 'enumProperties', will return an iterator... Also, if there are many entries in the map, slicing should be implemented.

2. Extend the support to other similar classes: WeakMap, Set, WeakSet, ArrayBuffer, maybe others.

3. The objects displayed in the entries view should have the configurable/writable/enumerable/etc properties set properly. Now they are all undefined/falsy. I'll have to figure out what these flags mean in this case and set them properly.
Requesting review from Victor, as he is the main author of the existing code. Please reassign if there
is a more suitable reviewer for this.

Asking jdescottes for feedback about the tests - can the 2 tests related to this bug be enabled in e10s && debug?
Attachment #8728936 - Flags: review?(vporof)
Attachment #8728936 - Flags: feedback?(jdescottes)
Attachment #8726124 - Attachment is obsolete: true
This patch adds support for displaying Map and Set entries in the variables view. Also supports the Weak* classes.

Here is a detailed overview of what I did:

Server Actors:
- Added new method 'enumEntries' to the 'ObjectActor' class. This creates a 'PropertyIteratorActor' with a new option flag 'enumEntries', which in turn creates an iterator on the Map/Set entries instead of properties.

- There are functions enumMapEntries and others that do the actual hard work of extracting the entries from the object and making them available as a generator. This code, including all the Xray handling stuff, is copied without change from the Map/Set previewers.

- These generators are used at two places - in the PropertyIteratorActor, which always enumerates everything, and also in the previewers, which are interested only in the first 10 entries.

- I don't understand very well when waiveXrays is need and when it isn't. For example, for Array objects, the previewer calls getOwnPropertyDescriptor on a waived object, but for stringifier and for enumeration in PropertyIteratorActor constructor, it doesn't waiveXrays. I'm confused there, because all three operations look basically the same and should behave the same way.

Frontend Controller and View:
- Added a new pseudo-item <entries> and populate it with _populateFromEntries when expanded

- Added a new type 'mapEntry', to correctly preview the Map entries - like "key -> value"

- The _populatePropertySlices doesn't need an aIterator argument - it's always in the aGrip.propertyIterator

- Did a small refactoring that makes sure the internal items like <entries> or <state> are always at the top, have the internalItem flag set to true, and are always in the _enum container of the variable view.

- Also, marked the [0...xxx] property slices as internalItems, moving them to the _enum container. They were previously in the _nonenum, which is wrong. I found this bug when tuning the tests.

- Another fixed little bug: if you created a Promise with 3000+ properties to trigger slicing, then the special <state> properties were not added to it, because that code path was skipped in _populateFromObject. Now it's fixed.

Tests:
- added a new test browser_dbg_variables-view-map-set.js

- added new test cases for large Map and Set to browser_dbg_variables-view-large-array-buffer.js. This test was extremely verbose and repetitive, with a lot of copy&paste, so I did a complete refactoring to turn it into a reasonable piece of code.

Unsolved problems:
- what if the debugger client connects to an old server that doesn't support the ObjectActor.enumEntries method? Is there any way to detect what the server supports? Or catch some exception in the client and handle it?

- the property descriptors contain the configurable/enumerable/writable/extensible flags. What are the correct values for the temporary pseudo-objects returned from Map.entries() or Set.values()?

- the waiveXrays issue mentioned above. For Arrays, sometimes it's called, sometimes it isn't. What's the proper policy for Maps and Sets?
New version of the patch: removed some forgotten unused code (found by eslint)
Attachment #8728984 - Flags: review?(vporof)
Attachment #8728984 - Flags: feedback?(jdescottes)
Attachment #8728936 - Attachment is obsolete: true
Attachment #8728936 - Flags: review?(vporof)
Attachment #8728936 - Flags: feedback?(jdescottes)
Comment on attachment 8728984 [details] [diff] [review]
DevTools: Map/Set entries should be visible in the Variables view

The new debugger test looks good to me! And it works fine locally in e10s & non e10s.

I just noticed the "skip e10s debug" was already present in the patch I pushed to try, so here is another one without the skip : https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bf29f79ed2d

However I think you should keep the skip here, to be consistent with the rest of the suite. It will be investigated and hopefully removed with the others in Bug 1103839.
Attachment #8728984 - Flags: feedback?(jdescottes) → feedback+
Assignee: nobody → jsnajdr
Status: NEW → ASSIGNED
Looking at the try pushes, it seems that browser_dbg_variables-view-large-array-buffer.js is timeouting frequently on Linux 64 debug. Even though an intermittent is logged for this test, orangefactor shows it's very rare [1]. The timeout threshold is 45 seconds per mochitest.

Looking at previous (successful) try pushes for browser_dbg_variables-view-large-array-buffer [2], I can find execution times of ~30seconds. Which is pretty slow, but well below the timeout threshold of 45 seconds.

You added a few test cases in browser_dbg_variables-view-large-array-buffer, so the extra time makes sense. This should be addressed either by splitting the test, or by requesting a longer timeout (see [3]).

[1] https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1248533&endday=2016-03-11&startday=2016-02-01&tree=trunk
[2] https://treeherder.mozilla.org/logviewer.html#?job_id=17874882&repo=try#L12573
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest#Asynchronous_Tests
(In reply to Julian Descottes [:jdescottes] from comment #15)
> Looking at the try pushes, it seems that
> browser_dbg_variables-view-large-array-buffer.js is timeouting frequently on
> Linux 64 debug.

Yep, I noticed these timeouts, too. The test creates objects with large number of properties (10000) and checks if they are displayed property in the variables view. Before my patch, it was testing Array and Object, and I also added cases for Map and Set. So the test now does 2 times more work.

I think splitting the test is not very feasible - it defines one testing function and runs it on multiple objects. Splitting would require putting the common code in head.js or some other module (I'm not sure if it's possible to have common code anywhere else than head.js in mochitests) and it would harm the clarity and readability of the test.

Requesting a longer timeout looks like a better option (and well justified) to me.
(In reply to Jarda Snajdr [:jsnajdr] from comment #16)
> 
> Requesting a longer timeout looks like a better option (and well justified)
> to me.

I agree, would probably do the same here.

> (I'm not sure if it's possible to have common code anywhere else than head.js
> in mochitests) and it would harm the clarity and readability of the test.

Learned that recently, you actually can, using Services.scriptloader.loadSubScript. Some of our head.js have a dedicated helper for this. 
For instance loadHelperScript defined at https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/markup/test/head.js#45 , which is used in markup/test/browser_markup_css_completion_style_attribute_01.js and markup/test/browser_markup_css_completion_style_attribute_02.js .
New version of the patch:
- use requestLongerTimeout(4) in the large-array-buffer mochitest
- if the debugger server doesn't support ObjectActor.enumEntries, catch the error on client and report
Attachment #8729511 - Flags: review?(vporof)
Attachment #8728984 - Attachment is obsolete: true
Attachment #8728984 - Flags: review?(vporof)
Hi,

I apologize for seeing this too late and only providing feedback late as a consequence.
Providing feedback with zero authority. I just know the ES spec a bit and care about the Firefox Devtools to be my best ally.

(In reply to Jarda Snajdr [:jsnajdr] from comment #11)
> Server Actors:
> - Added new method 'enumEntries' to the 'ObjectActor' class. This creates a
> 'PropertyIteratorActor' with a new option flag 'enumEntries', which in turn
> creates an iterator on the Map/Set entries instead of properties.

Maps, Sets and their weak versions are objects and can have regular properties as well. Granted, it'll probably be a rare thing, but TC39 left this option open, so there should be a way (even a complicated one) to inspect these. Is there a way with the current solution?

> Unsolved problems:
> - the property descriptors contain the
> configurable/enumerable/writable/extensible flags. What are the correct
> values for the temporary pseudo-objects returned from Map.entries() or
> Set.values()?

What do you mean by "pseudo-objects"? The object returned are iterators and are regular objects that you could even create by yourself, but for sure there values are unimportant because set by the engine.


> - the waiveXrays issue mentioned above. For Arrays, sometimes it's called,
> sometimes it isn't. What's the proper policy for Maps and Sets?

No idea, but ni'ing Nick who I believe will know.


As a drive-by comment, for l.343 of file devtools/client/shared/widgets/VariablesViewController.jsm, you can use Array.prototype.includes [1]. It's standard, supported by Firefox, good stuff.
(same for devtools/shared/client/main.js l.2482)

As a personal preference, knowing the ES spec, instead of "<entries>", I would prefer "[[SetData]]", "[[MapData]]", etc. as in the spec [2] for Set.


[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes
[2] see step 2 of http://www.ecma-international.org/ecma-262/6.0/#sec-set-iterable
Flags: needinfo?(nfitzgerald)
Thanks for feedback David!

(In reply to David Bruant from comment #20)
> Maps, Sets and their weak versions are objects and can have regular
> properties as well. Granted, it'll probably be a rare thing, but TC39 left
> this option open, so there should be a way (even a complicated one) to
> inspect these. Is there a way with the current solution?

For Maps and Sets, two iterators are created: one for the regular Object properties, and then, when <entries> are expanded, another one with enumEntries:true. Therefore, the fact that Map is an instance of Object is respected and you can inspect all its properties.

Similar approach with two iterators is used already on Arrays - first, we enumerate the enumerated properties (i.e., the array items), and then the other, non-enumerable properties like 'length' and anything else you might set on the array object.

> What do you mean by "pseudo-objects"? The object returned are iterators and
> are regular objects that you could even create by yourself, but for sure
> there values are unimportant because set by the engine.

What I mean is that the iterator and the entries returned by the iterator are temporary objects used only to populate the variables view. Then they are thrown away and GC-ed. The displayed entries of a Map look like this:

<entries>
  0: key0 -> value0
    key: key0
    value: value0

There is no real object that would have the "0", "key" or "value" properties. So, how do I figure out the values of configurable/writable etc? Normally, Object.getOwnPropertyDescriptor would tell me, but not here.

> you can use Array.prototype.includes.

Thanks for pointing this out, I'll use it in the next version of the patch.

> As a personal preference, knowing the ES spec, instead of "<entries>", I
> would prefer "[[SetData]]", "[[MapData]]", etc. as in the spec [2] for Set.

I used the existing convention of naming pseudo-properties with <angle-brackets>. It's used for example for inspecting Promise state or Function's closure. There is already some discussion about this in bug 866877. Earlier today, I added a detailed overview of the current state of affairs to that bug - pointing out things I think are inconsistent and added some suggestions.
Comment on attachment 8729511 [details] [diff] [review]
DevTools: Map/Set entries should be visible in the Variables view

Review of attachment 8729511 [details] [diff] [review]:
-----------------------------------------------------------------

It'd be a good idea for someone else to review the backend changes as well.
Attachment #8729511 - Flags: review?(vporof) → review+
Comment on attachment 8729511 [details] [diff] [review]
DevTools: Map/Set entries should be visible in the Variables view

Asking Tom for a review of the backend part, because he wrote the Map-inspection code in the previewers. 

There is some related discussion in bug 757185 about how the backend should inspect Maps and Weakmaps. I'm not sure if that's obsolete or if it's still relevant today.
Attachment #8729511 - Flags: review+ → review?(ttromey)
Attachment #8729511 - Flags: review?(vporof)
Attachment #8729511 - Flags: review?(vporof) → review+
We shouldn't need to waiveXrays on arrays. Map, Set, (and I think) WeakMap don't have meaningful/useful xrays, so they may need to stay. See bug 1155700.

In general, we should be using Debugger.Object's capabilities to inspect debuggee objects, and the use of the raw object, and waiving xrays in addition to that, is a bit scary. It is very easy to accidentally run debuggee code via getters or proxies, and if we do that then the debugger fails to do its job properly.
Flags: needinfo?(nfitzgerald)
See Also: → 1155700
Comment on attachment 8729511 [details] [diff] [review]
DevTools: Map/Set entries should be visible in the Variables view

Review of attachment 8729511 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for being patient, waiting for this review.

Overall I think this looks good.  A lot of it seems like a reasonably straightforward refactoring.

I didn't see a test for the case where a Set (et al) has an extra property.  I think a test for this would be good to have, to ensure the right thing is in fact happening.

::: devtools/client/debugger/test/mochitest/browser_dbg_variables-view-map-set.js
@@ +61,5 @@
> +    }
> +  }
> +
> +  /* Test the sets */
> +  for (let varName of [ "set", "weakSet"]) {

Extra space before "set".

::: devtools/client/shared/widgets/VariablesView.jsm
@@ +2537,5 @@
>      this._addEventListeners();
>  
>      if (this._initialDescriptor.enumerable ||
>          this._nameString == "this" ||
> +        this._internalItem) {

Nice cleanup.

::: devtools/client/shared/widgets/VariablesViewController.jsm
@@ +329,5 @@
>     */
>    _populateFromObject: function(aTarget, aGrip) {
> +    if (aGrip.class === "Promise" && aGrip.promiseState) {
> +      const { state, value, reason } = aGrip.promiseState;
> +      aTarget.addItem("<state>", { value: state }, { internalItem: true });

Thanks for adding internalItem here.

::: devtools/server/actors/object.js
@@ +228,5 @@
>      this.iterators.add(actor);
>      return { iterator: actor.grip() };
>    },
>  
> +  onEnumEntries: function(request) {

This needs a jsdoc comment.

@@ +713,1 @@
>   *        - ignoreSafeGetters Boolean

The new comment lines here need a leading "*" to match the prevailing style.

@@ +755,5 @@
> +          names.push(idx);
> +          ownProperties[idx] = {
> +            enumerable: true,
> +            value: {
> +              type: 'mapEntry',

We standardized on double quotes in our eslint config.

::: devtools/shared/client/main.js
@@ +2489,5 @@
> +        return { iterator: new PropertyIteratorClient(this._client, aResponse.iterator) };
> +      }
> +      return aResponse;
> +    }
> +    // telemetry: "ENUMENTRIES"

I don't know whether we want telemetry here; but if not, I think it's better to just remove this.
Attachment #8729511 - Flags: review?(ttromey) → review+
New (and hopefully the last) version of the patch that addresses issues from review and adds some other improvements:

- use template strings in mochitests - provides better string formatting
- check that extra properties on the Map/Set object are displayed
- check that the Map/Set has the right count of entries
- add jsdoc comments where needed
- use Array.prototype.includes instead of indexOf
- enumMapEntries and friends return object with Symbol.iterator property - makes iterating more elegant
Attachment #8732774 - Flags: review?(ttromey)
Attachment #8729511 - Attachment is obsolete: true
Attachment #8732774 - Flags: review?(ttromey) → review?(vporof)
Attachment #8732774 - Flags: review?(vporof) → review?(ttromey)
Comment on attachment 8732774 [details] [diff] [review]
DevTools: Map/Set entries should be visible in the Variables view

Review of attachment 8732774 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.  I have a few nits but nothing serious; no need to re-request review for these.

::: devtools/client/debugger/test/mochitest/browser.ini
@@ +545,5 @@
>  skip-if = e10s && debug
>  [browser_dbg_variables-view-large-array-buffer.js]
>  skip-if = e10s && debug
> +[browser_dbg_variables-view-map-set.js]
> +skip-if = e10s && debug

Is this skip-if really needed?

::: devtools/client/debugger/test/mochitest/browser_dbg_variables-view-map-set.js
@@ +10,5 @@
> +"use strict";
> +
> +const TAB_URL = EXAMPLE_URL + "doc_map-set.html";
> +
> +var test = Task.async(function* () {

I think eslint wants to see "function*()" here -- i.e., no space.  I think it's generally best if new files are "born eslint-clean".

::: devtools/shared/client/main.js
@@ +2495,5 @@
> +   */
> +  enumEntries: DebuggerClient.requester({
> +    type: "enumEntries"
> +  }, {
> +    before: function (packet) {

A nit: no space between "function" and "(".
Attachment #8732774 - Flags: review?(ttromey) → review+
(In reply to Tom Tromey :tromey from comment #28)
> 
> ::: devtools/client/debugger/test/mochitest/browser.ini
> @@ +545,5 @@
> >  skip-if = e10s && debug
> >  [browser_dbg_variables-view-large-array-buffer.js]
> >  skip-if = e10s && debug
> > +[browser_dbg_variables-view-map-set.js]
> > +skip-if = e10s && debug
> 
> Is this skip-if really needed?
> 

I proposed to skip this test and address it with the other debugger tests in Bug 1103839. Devtools tests in e10s+debug are only run on Windows on our integration platforms (Bug 1242986). Given the low platform coverage, I thought we should remain consistent with the other tests.

That being said, we did a try push without the skip[1]. It was successful on e10s. The test also passes locally with e10s & debug on. 

=> we can unskip it (it won't have a big impact on the actual coverage) or handle this in Bug 1103839.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bf29f79ed2d
(In reply to Julian Descottes [:jdescottes] from comment #29)

> I proposed to skip this test and address it with the other debugger tests in
> Bug 1103839.

Either way is fine by me, thanks.
- unskipped the large-array-buffer and map-set mochitests
- fixed two nits reported by tromey

Will submit a try run and then land if OK.
Attachment #8732774 - Attachment is obsolete: true
Attachment #8736179 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/50c354c8516f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Keywords: dev-doc-needed
See Also: → 1270179
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.