1033927 - Drop support for custom [object XrayWrapper [object ClassName]] stringification

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Right now, XrayWrapper::getPropertyDescriptor resolves a custom toString method that does all kinds of crazy stuff to produce the familiar "[object XrayWrapper [object HTMLDocument]]" stringification.

This code is problematic in a bunch of ways. In particular, it doesn't work when HasPrototype=1. This has two consequences:
* We don't get the [object XrayWrapper [...]] annotation for JSXrayTraits (and for the OpaqueXrayTraits I'm adding).
* It makes peter's life more difficult in bug 787070.

jimb recently did some refactoring of the proxy toString stuff. obj_toString is now gone, and has been replaced by a much simpler className() trap. The default obj_toString that we'll inherit off the Xrayed Object.prototype will invoke this, so I think we can leverage it to avoid defining any custom properties at all. I'll attach a patch.

Comment 1

[Bobby Holley (:bholley)]

Hm, it looks like the className trap can't allocate and can only return static strings. That makes doing this a pretty huge PITA, so I'm inclined to try dropping support for the special ToString behavior and just use the underlying className.

The addons-mxr story looks much better than I thought. The only two addons that are checking for this (either via /XrayWrapper/ or .indexOf('XrayWrapper')) are Firebug and AsYouWish, and we know how to get in touch with them (I've CCed them in this bug).

I'm going to try dropping the [object XrayWrapper[...]] prefixing for WebIDL objects in preparation for bug 787070 and see if anything breaks.

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Jim Blandy :jimb]

Sorry my refactoring in bug 862531 ended up getting in your way. At the time, I justified returning a static string by saying:

"For now, all our use cases would be returning constant strings, so a reasonable first implementation could specify that the callee owns the returned string. If we need dynamically-generated class names later, we can add the necessary complications."

Comment 4

[Bobby Holley (:bholley)]

(In reply to Jim Blandy :jimb from comment #3)
> Sorry my refactoring in bug 862531 ended up getting in your way. At the
> time, I justified returning a static string by saying:
> 
> "For now, all our use cases would be returning constant strings, so a
> reasonable first implementation could specify that the callee owns the
> returned string. If we need dynamically-generated class names later, we can
> add the necessary complications."

I think that's reasonable. And if we can get away without the extra |object XrayWrapper| goop, I think we're probably better off in the long run.

Historically it's been useful for developers poking around in the web console and such to recognize that they're dealing with an XrayWrapper. But now that we have more advanced pretty printing, I think that (if we want that) we can just move that indicator logic to devtools and base it off of Cu.isXrayWrapper.

Comment 5

[Brett Zamir]

Can I ask whether this is expected to affect the SDK? I'm merely using wrap(), but on any conceivable SDK object...

Comment 6

[Bobby Holley (:bholley)]

(In reply to Brett Zamir from comment #5)
> Can I ask whether this is expected to affect the SDK? I'm merely using
> wrap(), but on any conceivable SDK object...

Addons MXR shows that at least one version of AsYouWish was bundling an old version of SpecialPowers, which included a /XrayWrapper/ test for XrayWrappers. The current version of SpecialPowers uses Cu.isXrayWrapper, which should work just fine.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Drop support for custom [object XrayWrapper [object ClassName]] stringification. v1

One orange test, which I've fixed. Uploading and flagging for review.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Bobby Holley (:bholley) from comment #1)
> The addons-mxr story looks much better than I thought. The only two addons
> that are checking for this (either via /XrayWrapper/ or
> .indexOf('XrayWrapper')) are Firebug and AsYouWish, and we know how to get
> in touch with them (I've CCed them in this bug).
Thanks for the heads up.
(the change is just fine for Firebug)

Honza

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 8450682 [details] [diff] [review]
Drop support for custom [object XrayWrapper [object ClassName]] stringification. v1

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

r=me, I'd already done something like this in my patches for bug 787070.

Comment 10

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74c612bb0147

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bobby Holley (:bholley) from comment #4)
> Historically it's been useful for developers poking around in the web
> console and such to recognize that they're dealing with an XrayWrapper. But
> now that we have more advanced pretty printing, I think that (if we want
> that) we can just move that indicator logic to devtools and base it off of
> Cu.isXrayWrapper.

What sort of more advanced pretty printing do we have now? If I'm developing in JS, how do I find out that the object I have is an Xray wrapper?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8450682 [details] [diff] [review]
Drop support for custom [object XrayWrapper [object ClassName]] stringification. v1

I guess I'll sr this given that our devtools should be better now.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #11)
> (In reply to Bobby Holley (:bholley) from comment #4)
> > Historically it's been useful for developers poking around in the web
> > console and such to recognize that they're dealing with an XrayWrapper. But
> > now that we have more advanced pretty printing, I think that (if we want
> > that) we can just move that indicator logic to devtools and base it off of
> > Cu.isXrayWrapper.
> 
> What sort of more advanced pretty printing do we have now? If I'm developing
> in JS, how do I find out that the object I have is an Xray wrapper?

For code, Cu.isXrayWrapper. The pretty printing I'm referring to is from the Web Console and Browser Scratchpad, which no longer gives you the original .toString() anyway. My assumption is that that the toString() override was mostly useful for people poking around at things interactively, or using console.log().

I guess people might be doing alert(someObj). But it's not clear to me that this is an important enough feature to justify this behavior. In general, people shouldn't need to know that they have an XrayWrapper at all.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74c612bb0147