Closed Bug 1035973 Opened 5 years ago Closed 4 years ago

[jsdbg2] Debugger needs support for Symbols

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jimb, Assigned: manishearth)

References

Details

Attachments

(1 file, 3 obsolete files)

The Debugger API doesn't know anything about ES6 symbols. We at least need Debugger.Object methods to enumerate symbol-named properties (analogous to Object.getOwnPropertySymbols).

Fortunately, since Symbols are immutable, I think we can treat them like the other primitive types, and not wrap them with some "Debugger.Symbol" type. That keeps things simple.
Blocks: 1035976
Hey, I'd like to work on this. Got any pointers for what I should do?

Currently looking at the DebuggerObject.getOwnPropertyNames() source
Flags: needinfo?(jimb)
Hi Manish!

(In reply to Manish Goregaokar [:manishearth] from comment #1)
> Hey, I'd like to work on this. Got any pointers for what I should do?
> 
> Currently looking at the DebuggerObject.getOwnPropertyNames() source

All you have to do to iterate symbols instead of string keys is pass a different set of flags to `Get[Own]PropertyKeys`. For example, see the implementation of `Object.getOwnPropertySymbols`: https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Object.cpp#808

(Aside: I am kind of surprised we aren't using the "own" variant here... Worth digging into and documenting or fixing to use the "own" variant.)

Here are all the flags: https://dxr.mozilla.org/mozilla-central/source/js/src/jsfriendapi.h#963-969

I think the best thing to do would be to split out `DebuggerObject_getOwnPropertyKeys` into a helper method that takes these flags as a parameter, and then have `DebuggerObject_getOwnPropertyKeys` and the new `DebuggerObject_getOwnPropertySymbols` call this helper with their corresponding flags.

Tests are in `js/src/jit-test/tests/debug/*`. See https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Running_Automated_JavaScript_Tests#Running_jit-tests for info on running them.

Don't forget to add docs for this new method to `js/src/doc/Debugger/Debugger.Object.md` :)

Good luck!
Flags: needinfo?(jimb)
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Attachment #8623021 - Flags: review?(nfitzgerald)
Comment on attachment 8623021 [details] [diff] [review]
Add Debugger.Object.getOwnPropertySymbols()

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

::: js/src/jit-test/tests/debug/Object-getOwnPropertySymbols.js
@@ +10,5 @@
> +    g.eval("obj = " + code);
> +    var actual = gobj.getOwnPropertyDescriptor("obj").value.getOwnPropertySymbols().map((x) => x.toString());
> +
> +    assertEq(JSON.stringify(actual.sort().map((x) => x.toString())),
> +             JSON.stringify(actual.sort().map((x) => x.toString())));

Drive-by comment: should use "expected" here.
I'm an idiot, fixed, thanks :)
Attachment #8623021 - Attachment is obsolete: true
Attachment #8623021 - Flags: review?(nfitzgerald)
Attachment #8623040 - Flags: review?(nfitzgerald)
Comment on attachment 8623040 [details] [diff] [review]
Add Debugger.Object.getOwnPropertySymbols()

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

Looks great!

Pretty much r=me, but I want to glance at the requested tests one time before I stamp it.

::: js/src/doc/Debugger/Debugger.Object.md
@@ +247,5 @@
>      if <code>Object.getOwnPropertyNames(<i>referent</i>)</code> had been
>      called in the debuggee, and the result copied in the scope of the
>      debugger's global object.
>  
> +`getOwnPropertyNames()`

"Symbols" ;)

::: js/src/jit-test/tests/debug/Object-getOwnPropertySymbols.js
@@ +17,5 @@
> +test("{}");
> +test("(function() {let x = Symbol(); let o = {}; o[x] = 1; return o;})()");
> +test("(function() {let x = Symbol('foo'); let o = {}; o[x] = 1; return o;})()");
> +test("(function() {let x = Symbol('foo'); let y = Symbol('bar'); \
> +                   let o = {}; o[x] = 1; o[y] = 2; return o;})()");
\ No newline at end of file

Let's also add tests for:

* Symbols with spaces: `Symbol('with many spaces')`
* Well known symbols, like `Symbol.iterator`
* Symbol properties on function objects
* Symbol properties on array objects
* Symbol properties on objects with a null prototype: `Object.create(null)`
* Deleted symbol properties: `obj[sym] = 1; delete obj[sym];`
* Symbol properties of cross compartment wrappers (see Object-getOwnPropertyNames-02.js)
Attachment #8623040 - Flags: review?(nfitzgerald)
Done!
Attachment #8623040 - Attachment is obsolete: true
Attachment #8623100 - Flags: review?(nfitzgerald)
Comment on attachment 8623100 [details] [diff] [review]
Add Debugger.Object.getOwnPropertySymbols()

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

Great! Thanks :)

Want to make use of this new API in the remote debugging protocol server + client and the debugger frontend too? Bug 881480 ;)

::: js/src/jit-test/tests/debug/Object-getOwnPropertySymbols-01.js
@@ +15,5 @@
> +}
> +
> +test("{}");
> +test("Array.prototype"); // Symbol.iterator
> +test("Object.create(null)"); // Symbol.iterator

Note that there is no Symbol.iterator on Object.create(null), so this comment is misleading. Might want to test both Object.create(null) with no symbol properties and add some symbol properties to an Object.create(null) object.

@@ +19,5 @@
> +test("Object.create(null)"); // Symbol.iterator
> +test("(function() {let x = Symbol(); let o = {}; o[x] = 1; return o;})()");
> +test("(function() {let x = Symbol('foo'); let o = {}; o[x] = 1; return o;})()");
> +test("(function() {let x = Symbol('foo'); let y = Symbol('bar'); \
> +                   let o = {}; o[x] = 1; o[y] = 2; return o;})()");

FYI, we have ` strings now, which can be multiline. Feel free to use those here or not; I don't feel strongly at all.
Attachment #8623100 - Flags: review?(nfitzgerald) → review+
Fixed.
Attachment #8623100 - Attachment is obsolete: true
Attachment #8623121 - Flags: review?(nfitzgerald)
Attachment #8623121 - Flags: review?(nfitzgerald) → review+
Keywords: checkin-needed
Is there a Try link handy for this? :)
Keywords: checkin-needed
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28c2eddd0e3c

Will re-tag when there's another run. Sorry I forgot; it's been a while :)
https://hg.mozilla.org/mozilla-central/rev/8c7ba8666890
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.