856038 - JS debugger uses class.name to display type of DOM objects

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Peter Van der Beken [:peterv]]

Using class.name means that if we change the internal implementation of DOM binding objects to use proxies the type suddenly becomes "[object Proxy]". Note that the WebIDL spec specifically overrides Object.prototype.toString to avoid these kind of issues: http://dev.w3.org/2006/webapi/WebIDL/#toString.

This came up as a result of the patch in bug 855971, where we change the HTMLDocument binding to use a proxy and so have to change a bunch of debugger tests. But it already happens for a number of existing proxy-based bindings (NodeList, HTMLCollection, ...).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Indeed, is there a reason we're not just using Object.prototype.toString here internally?

Comment 2

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

We have bug 848740 already on file about this and my assumption was that this was a bug where the debugger didn't get to view the debuggee from the right compartment, and was presented with a cross-compartment wrapper instead:

https://wiki.mozilla.org/Debugger#Debugger.Object

Are you suggesting that Debugger.Object.prototype.class should be using toString internally or that the debugger server should be using Debugger.Object.prototype.toString instead?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> my assumption was that this was a bug where the debugger didn't get to view the debuggee
> from the right compartment

That might be happening too, but certainly for Window it'll happen even same-compartment as things stand.

> Are you suggesting that Debugger.Object.prototype.class should be using toString
> internally or that the debugger server should be using
> Debugger.Object.prototype.toString instead?

That really depends on what you're doing...

Having .class call toString makes no sense if .class is defined as [[Class]] (as per the docs linked above).  On the other hand, why does anyone ever care about [[Class]] in practice?

Object.prototype.toString.call(obj) will return return "[object " + obj.class + "]" for non-proxies.  For proxies it will call the proxy's toString trap, which should generally do the right thing.  I _think_ in practice it will always end up returning something of the form "[object something]" right now.  So we _could_ change .class on the Debugger.Object to do the toString and then parse out the "class".  Not sure if that's useful.

Either way, we should make sure that when we're getting a string to show callers we end up doing the toString thing somehow.  Either by changing what .class means or by adding a new accessor on Debugger.Object.

Comment 4

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

The debugger uses Debugger.Object.prototype.class to get to the referent's [[Class]] and construct the same output as toString without actually calling it, in order to avoid side-effects. It's also required by the protocol to send the [[Class]]:

https://wiki.mozilla.org/Remote_Debugging_Protocol#Grips

I think that the Debugger API always gets the debuggee's class, therefore not triggering the proxy's toString trap:

http://dxr.mozilla.org/mozilla-central/js/src/vm/Debugger.cpp#l4118

If I'm reading this correctly, our best (only?) option is to use your suggestion to have it call toString and parse out the class, right?

Comment 5

[Boris Zbarsky [:bzbarsky]]

So the thing is....  For WebIDL objects, per spec, the [[Class]] is "Object".  Basically, [[Class]] is pretty useless in practice.  We could have some sort of concept of "class" that doesn't match ES [[Class]], of course.  We could even nix the obj_toString proxy trap, replace it with an obj_getClass trap, and return this "class" from it.

On the other hand, if what we really care about is "canonical Object.prototype.toString without side-effects" as opposed to "class", then we should just have debugger API for calling js::obj_toStringHelper or something....

Comment 6

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

I believe bug 862531 fixed this. Please reopen if you feel otherwise.