Closed Bug 881480 Opened 11 years ago Closed 7 years ago

Support ES6 symbols used as property names in the debugger

Categories

(DevTools :: Debugger, defect, P2)

Product:
DevTools
Component:
Debugger
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bbenvie, Assigned: nchevobbe)

References

Details

Attachments

(3 files, 3 obsolete files)

rdp-and-symbols-part-1.patch
6.28 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
Bug 881480 - Add ownSymbols to onPrototypeAndProperties;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 881480 - Add ownSymbols to object preview;
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
Right now things that return sets of properties, e.g. `prototypeAndProperties`, returns a JSON dictionary of keys and their values. This won't work for Symbols since they are objects with a unique identity where their name (which is optional) has no bearing on their uniqueness.

As an example, the debugger protocol can't handle the following:

> let a1 = Symbol('a');
> let a2 = Symbol('a');
> let obj = {};
> obj[a1] = 5;
> obj[a2] = 10;
> sendToRemote(obj);

See bug 645416 for details on the implementation of Symbol.
I see three ways of solving this.

1.) For `prototypeAndProperties` you get back JSON that has a field "ownProperties". Add an additional field "ownSymbolProperties" or something that returns an array of properties where each property has the form `{ "key": { ...gripForSymbol..., "descriptor": { ... } }`.
2.) Change the protocol so that property lists change from key/descriptor dictionary to arrays of descriptors where each descriptor has a "key" field on top of the existing "enumerable", "configurable", etc. fields.
3.) Add additional requests for each type of request that currently gets property sets, e.g. the companion to `prototypeAndProperties` would be `symbolProperties` or something. This would mean getting the full list of properties involved two requests.
Option 2 seems the cleanest to me, but it's not backwards compatible. Perhaps by the time Symbols land in SpiderMonkey that will not be a problem, due to all of our products being on a rapid release model. In any case marking as P3 since bug 645416 isn't ready yet.
Depends on: harmony:symbols
Priority: -- → P3
Yeah I agree, #2 is by far the cleanest and I only mentioned the other two in case backward compatibility trumped. Also agree on P3 until Symbols land in SpiderMonkey, at which time this should be bumped to P2.
Symbols landed, so I'm bumping the priority on this. Anyone who tries to debug code with this new feature will have a bad time.
Priority: P3 → P2
Copying Jim's comment from bug 1035976 comment 0:

The remote protocol needs to be extended to support symbol-named properties. At the very least:

- We need to specify what a symbol grip looks like. JSON can't convey symbols, so we'll need something like { type: "symbol", name: "foo" }.

- Anything that deals with object property names or property lists must be expanded or get a companion request for dealing with symbol-named properties.

For background on ES6 symbols, see:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol
Notice that Bug 1039686 wasn't about having a way to debug/dump Symbol per sé, but the fact that the simply attempt to do so, break the console, so that nothing anymore will be logged, even if it worked before, like a simple `console.log("foo")`. That's definitely something that should be fixed, despite the proper support of Symbol – however, I guess that properly support Symbol will fix that behavior too, but if would take too long, maybe could makes at least limit this side effects.
Fair enough, I'll reopen it.
Does it matter that both of these symbols will look the same over the RDP despite being different symbols?

s1 = Symbol("s") // rdp: { type: "symbol", name: "s" }
s2 = Symbol("s") // rdp: { type: "symbol", name: "s" }
s1 == s2 // false

Do we need to have a [weak]map from symbol to unique id so that the client can differentiate these symbols?
Attached patch rdp-and-symbols-part-1.patch (obsolete) — — Splinter Review 
This just adds the RDP value form for symbols. Doesn't deal with objects that have symbol property keys.

https://tbpl.mozilla.org/?tree=Try&rev=3c4c10de4a9f
Attachment #8477829 - Flags: review?(past)
Attached patch rdp-and-symbols-part-1.patch (obsolete) — — Splinter Review 
Extends test to cover well known symbols via Symbol.iterator.

https://tbpl.mozilla.org/?tree=Try&rev=258b60916ca8
Attachment #8477829 - Attachment is obsolete: true
Attachment #8477829 - Flags: review?(past)
Attachment #8477830 - Flags: review?(past)
Comment on attachment 8477830 [details] [diff] [review]
rdp-and-symbols-part-1.patch

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

This is a good first step.

Now that I've learned about Object.getOwnPropertySymbols(), I'm also more sympathetic to Brandon's option (1) from comment 1. Perhaps maintaining the symmetry with an "ownPropertySymbols" property in the protocol would be more intuitive (not to mention backwards-compatible)?

::: toolkit/devtools/server/tests/unit/test_symbols-01.js
@@ +10,5 @@
> +function run_test() {
> +  initTestDebuggerServer();
> +  const debuggee = addTestGlobal("test-symbols");
> +  const client = new DebuggerClient(DebuggerServer.connectPipe());
> + 

Nit: trailing whitespace.

@@ +13,5 @@
> +  const client = new DebuggerClient(DebuggerServer.connectPipe());
> + 
> + client.connect(function() {
> +    attachTestTabAndResume(client, "test-symbols", function(response, tabClient, threadClient) {
> +      Task.spawn(function* () {

I think you could simplify this further with addTask, but it's not a big deal either way:

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Task-based_asynchronous_tests

::: toolkit/devtools/server/tests/unit/test_symbols-02.js
@@ +10,5 @@
> +function run_test() {
> +  initTestDebuggerServer();
> +  const debuggee = addTestGlobal("test-symbols");
> +  const client = new DebuggerClient(DebuggerServer.connectPipe());
> + 

Nit: trailing whitespace.
Attachment #8477830 - Flags: review?(past) → review+
Fixed nits. Carrying over r+.
Attachment #8477830 - Attachment is obsolete: true
Attachment #8478375 - Flags: review+
(In reply to Panos Astithas [:past] from comment #13)
> Now that I've learned about Object.getOwnPropertySymbols(), I'm also more
> sympathetic to Brandon's option (1) from comment 1. Perhaps maintaining the
> symmetry with an "ownPropertySymbols" property in the protocol would be more
> intuitive (not to mention backwards-compatible)?

Agreed.
https://hg.mozilla.org/integration/fx-team/rev/086723e296b0
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Summary: The debugger protocol will need a way to handle Symbol-keyed properties → Support ES6 symbols used as property names in the debugger
I'm interested in working on this, once bug 1035973 lands.

My plan is to send symbol keys in `ownSymbols` under `onPrototypeAndProperties`, and then populate a _symbolStore on the `Scope` side.

I'm not yet sure how to handle safeGetterValues though. There might be a lot of duplication happening here if not done carefully.
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
(In reply to Manish Goregaokar [:manishearth] from comment #18)
> I'm not yet sure how to handle safeGetterValues though. There might be a lot
> of duplication happening here if not done carefully.

The getters we consider safe are the ones in platform DOM APIs. Are symbols now being used in them?
Not that I know of. We can file a followup for this if necessary in that case.
(In reply to Panos Astithas [:past] from comment #19)
> The getters we consider safe are the ones in platform DOM APIs. Are symbols
> now being used in them?

To the best of my knowledge: no. In either case, I agree that we can put it off for a follow up if we do discover any.

The only Symbol related thing in the DOM I can think of is Symbol.iterator, but that is a function not a geter.
Comment on attachment 8838364 [details]
Bug 881480: Add ownSymbols to onPrototypeAndProperties;

https://reviewboard.mozilla.org/r/113296/#review115358

The change looks good, but we need a test for such chance.
You could copy the following one to make up one for symbols:
http://searchfox.org/mozilla-central/source/devtools/server/tests/unit/test_objectgrips-04.js
You should assert that ownProperties is empty when there is only symbol attributes,
while asserting that name and descriptor are correct in ownSymbols array.
Also try to test what happens when you used unamed symbols like `Symbol()`.

Also you modified the debugger object inspector, but I imagine you would have to also tweak reps to support symbols inspection in the new console.

::: devtools/server/actors/object.js:272
(Diff revision 1)
>      }
>      for (let name of names) {
>        ownProperties[name] = this._propertyDescriptor(name);
>      }
> +    for (let sym of symbols) {
> +      ownSymbols.push({"name": sym.toString(), "descriptor": this._propertyDescriptor(sym)});

nit: this line is too long, could you format it like this:
ownSymbols.push({
  name: sym.toString(),
  descriptor: this._propertyDescriptor(sym)
});
Attachment #8838364 - Flags: review?(poirot.alex)
The front-end support in the debugger has landed in M-C, I think, but the server side still has not.
Also I think devtools-reps still doesn't handle this.
Manish, do you have some time to add the tests ochameau talked about ? If not, no worries, I can take over this patch and write them.
Flags: needinfo?(manishearth)
Nah, not really. That would be great!
Flags: needinfo?(manishearth)
(In reply to Manish Goregaokar [:manishearth] from comment #28)
> Nah, not really. That would be great!

Thanks !
ochameau, I applied Manish's patch and added a unit test for an object with multiple Symbols of different types.
This change is already picked up by the object inspector in the console, and I'll add the reps support today.
Assignee: manishearth → nchevobbe
Attachment #8838364 - Attachment is obsolete: true
Comment on attachment 8889312 [details]
Bug 881480 - Add ownSymbols to onPrototypeAndProperties;

https://reviewboard.mozilla.org/r/160380/#review165692

Thanks for picking this up!

I'm interested to see how reps supports new actor feature, so if you have the link to the PR or patch...

::: devtools/server/tests/unit/test_objectgrips-16.js:35
(Diff revision 1)
> +  gThreadClient = threadClient;
> +  test_symbol_grip();
> +}
> +
> +function test_symbol_grip() {
> +  gThreadClient.addOneTimeListener("paused", function (event, packet) {

I'm very glad to see you contribute to the server!
I opened bug 1383711 if you want to improve debugger tests readability ;)

::: devtools/server/tests/unit/test_objectgrips-16.js:41
(Diff revision 1)
> +    let args = packet.frame.arguments;
> +
> +    do_check_eq(args[0].class, "Object");
> +
> +    let objClient = gThreadClient.pauseGrip(args[0]);
> +    objClient.getPrototypeAndProperties(function (response) {

If I'm not mistaken, you should be able to do
let response = await objClient.getPrototypeAndProperties();

::: devtools/server/tests/unit/test_objectgrips-16.js:81
(Diff revision 1)
> +      do_check_eq(iteratorSymbol.descriptor.value.class, "Function");
> +
> +      do_check_true(response.prototype != undefined);
> +
> +      let protoClient = gThreadClient.pauseGrip(response.prototype);
> +      protoClient.getOwnPropertyNames(function (response) {

Same here:
let response = await protoClient.getOwnPropertyNames();
And also for gThreadClient.resume and gClient.close.
Attachment #8889312 - Flags: review?(poirot.alex) → review+
> I'm interested to see how reps supports new actor feature, so if you have the link to the PR or patch...

The code to get the symbols from the getPrototypeAndProperties response (i.e. when we expand an object in the console/debugger) is already in the ObjectInspector.
We weren't showing them inline (i.e. in the preview of a collapsed object) yet though.

This is an early patch for doing so https://github.com/devtools-html/devtools-core/pull/513 (lacks some test, i'm waiting this patch to land so I can generate proper stubs from the server responses).
Comment on attachment 8889468 [details]
Bug 881480 - Add ownSymbols to object preview;

https://reviewboard.mozilla.org/r/160498/#review166128

Thanks for using more await!

LGTM if you have a green try.

::: devtools/server/actors/object.js:1493
(Diff revision 1)
> +      continue;
> +    }
> +
> +    preview.ownSymbols.push(Object.assign({
> +      descriptor
> +    }, hooks.createValueGrip(symbol)));

I'm wondering what is the value of using createValueGrip. It will add a redundant 'type: "symbol"' attribute for every object in `ownSymbols`.

The main value of createValueGrip is that it will better handle long strings and create a LongStringActor. In theory, it can happen. Symbol name can be of any size (I tested).

Given that symbols should be rare here, this isn't a big deal. But be careful on the client side, you may receive a LongStringActor.

::: devtools/server/tests/unit/test_objectgrips-16.js:48
(Diff revision 1)
> +    // Checks the result of getPrototypeAndProperties.
> +    check_prototype_and_properties(response);
>  
> -    do_check_eq(args[0].class, "Object");
> +    let protoClient = gThreadClient.pauseGrip(response.prototype);
> +    response = await protoClient.getOwnPropertyNames();
> +    do_check_true(response.ownPropertyNames.toString != undefined);

Please use do_check_neq as it is easier to debug when the assertion fails. It will print both values.

Otherwise it looks weird to check for a toString method. Isn't there a better check? What are you trying to assert?
do_check_true(Array.isArray(response.ownPropertyNames)); ?

Also, is there any value to assert getOwnPropertyNames in this test?

::: devtools/server/tests/unit/test_objectgrips-16.js:89
(Diff revision 1)
> +
> +  do_check_eq(firstUnnamedSymbol.name, undefined);
> +  do_check_eq(firstUnnamedSymbol.descriptor.configurable, true);
> +  do_check_eq(firstUnnamedSymbol.descriptor.enumerable, true);
> +  do_check_eq(firstUnnamedSymbol.descriptor.writable, true);
> +  do_check_eq(firstUnnamedSymbol.descriptor.value, "first unnamed symbol");

You should also check for "type" attribute for each symbol as it is also present:
  do_check_eq(firstUnnamedSymbol.type, "symbol");

::: devtools/server/tests/unit/test_objectgrips-16.js:147
(Diff revision 1)
> -      do_check_eq(iteratorSymbol.descriptor.configurable, true);
> +  do_check_eq(iteratorSymbol.descriptor.configurable, true);
> -      do_check_eq(iteratorSymbol.descriptor.enumerable, true);
> +  do_check_eq(iteratorSymbol.descriptor.enumerable, true);
> -      do_check_eq(iteratorSymbol.descriptor.writable, true);
> +  do_check_eq(iteratorSymbol.descriptor.writable, true);
> -      do_check_eq(iteratorSymbol.descriptor.value.class, "Function");
> +  do_check_eq(iteratorSymbol.descriptor.value.class, "Function");
>  
> -      do_check_true(response.prototype != undefined);
> +  do_check_true(response.prototype != undefined);

I'm not sure there is any value in asserting response.prototype value in this test.
Attachment #8889468 - Flags: review?(poirot.alex) → review+
Comment on attachment 8889468 [details]
Bug 881480 - Add ownSymbols to object preview;

https://reviewboard.mozilla.org/r/160498/#review166128

> I'm wondering what is the value of using createValueGrip. It will add a redundant 'type: "symbol"' attribute for every object in `ownSymbols`.
> 
> The main value of createValueGrip is that it will better handle long strings and create a LongStringActor. In theory, it can happen. Symbol name can be of any size (I tested).
> 
> Given that symbols should be rare here, this isn't a big deal. But be careful on the client side, you may receive a LongStringActor.

I do this because reps handle actor as object properties. This means that for a `{[Symbol("foo")]: "bar"}` object, the Symbol rep will handle the property, with a proper styling (if needed) and name (i.e. "foo" and not the `toString` value of the symbol).

We don't handle LongString for now for Symbol names, but that's a good point and I'll file a bug to handle that in Reps.

> Please use do_check_neq as it is easier to debug when the assertion fails. It will print both values.
> 
> Otherwise it looks weird to check for a toString method. Isn't there a better check? What are you trying to assert?
> do_check_true(Array.isArray(response.ownPropertyNames)); ?
> 
> Also, is there any value to assert getOwnPropertyNames in this test?

This is a leftover from the test I copied this one from. 
I guess the value of it is to check we can retrieve prototype properties from the prototype we get in `getPrototypeAndProperties` function.
I do think there's a better way to test that though, I'll change this line.

> I'm not sure there is any value in asserting response.prototype value in this test.

Yeah, this is already tested somewhere else, this test should be only about symbols, I'll remove this assertion.
Filed an issue for the work for supporting Symbol with longString names : https://github.com/devtools-html/devtools-core/issues/518
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5806714b7da3
Add ownSymbols to onPrototypeAndProperties; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/5a956c89da7d
Add ownSymbols to object preview; r=ochameau
Does this require anything else to be resolved ?
Flags: needinfo?(wkocher)
I don't know. Fitzgen added the leave-open keyword three years ago.
Flags: needinfo?(wkocher)
Flags: needinfo?(nfitzgerald)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #11)
> Created attachment 8477829 [details] [diff] [review]
> rdp-and-symbols-part-1.patch
> 
> This just adds the RDP value form for symbols. Doesn't deal with objects
> that have symbol property keys.

This was only leave-open because my earlier patch just enabled displaying symbols, but didn't add the ability to look at symbol property names. If that has been implemented, then this can close.
Flags: needinfo?(nfitzgerald)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #45)
>
> This was only leave-open because my earlier patch just enabled displaying
> symbols, but didn't add the ability to look at symbol property names. If
> that has been implemented, then this can close.

Yes, this is working now, I thing we can close the bug. Thanks !
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: