881480 - Support ES6 symbols used as property names in the debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Brandon Benvie [:benvie]]

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.

Comment 1

[Brandon Benvie [:benvie]]

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.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 3

[Brandon Benvie [:benvie]]

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.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Fair enough, I'll reopen it.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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?

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

https://tbpl.mozilla.org/?tree=Try&rev=3c4c10de4a9f

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: rdp-and-symbols-part-1.patch

Extends test to cover well known symbols via Symbol.iterator.

https://tbpl.mozilla.org/?tree=Try&rev=258b60916ca8

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: rdp-and-symbols-part-1.patch

Fixed nits. Carrying over r+.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/086723e296b0

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/086723e296b0

Comment 18

[Manish Goregaokar [:manishearth]]

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.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 20

[Manish Goregaokar [:manishearth]]

Not that I know of. We can file a followup for this if necessary in that case.

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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 22

[Manish Goregaokar [:manishearth]]

Attachment: Bug 881480: Add ownSymbols to onPrototypeAndProperties;

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

Comment 23

[Manish Goregaokar [:manishearth]]

Client side at https://github.com/devtools-html/debugger.html/pull/2038

Comment 24

[Alexandre Poirot [:ochameau]]

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)
});

Comment 25

[Tom Tromey :tromey]

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.

Comment 26

[Tom Tromey :tromey]

I filed https://github.com/devtools-html/reps/issues/108 for the reps bit.

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

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.

Comment 28

[Manish Goregaokar [:manishearth]]

Nah, not really. That would be great!

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Manish Goregaokar [:manishearth] from comment #28)
> Nah, not really. That would be great!

Thanks !

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 881480 - Add ownSymbols to onPrototypeAndProperties;

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

Comment 31

[Nicolas Chevobbe [:nchevobbe]]

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.

Comment 32

[Alexandre Poirot [:ochameau]]

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.

Comment 33

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 881480 - Add ownSymbols to object preview;

This adds an `ownSymbols` and an `ownSymbolsLength` properties to the
grip `preview` property so we can retrieve them in the frontend.
This also refactors the newly introduced test for symbols and add a
function which test the content of the preview property in the grip.

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

Comment 34

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8889312 [details]
Bug 881480 - Add ownSymbols to onPrototypeAndProperties;

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

Comment 35

[Nicolas Chevobbe [:nchevobbe]]

> 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 36

[Alexandre Poirot [:ochameau]]

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.

Comment 37

[Nicolas Chevobbe [:nchevobbe]]

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.

Comment 38

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8889468 [details]
Bug 881480 - Add ownSymbols to object preview;

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

Comment 39

[Nicolas Chevobbe [:nchevobbe]]

Filed an issue for the work for supporting Symbol with longString names : https://github.com/devtools-html/devtools-core/issues/518

Comment 40

[Nicolas Chevobbe [:nchevobbe]]

TRY push looks fine https://treeherder.mozilla.org/#/jobs?repo=try&revision=74fe4325b32da6454456b3c2e3ae0b51abe582a9, let's push this

Comment 41

[Pulsebot]

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

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5806714b7da3
https://hg.mozilla.org/mozilla-central/rev/5a956c89da7d

Comment 43

[Nicolas Chevobbe [:nchevobbe]]

Does this require anything else to be resolved ?

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I don't know. Fitzgen added the leave-open keyword three years ago.

Comment 45

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 46

[Nicolas Chevobbe [:nchevobbe]]

(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 !