862531 - C++ proxy handlers should have a 'className' method, instead of an 'obj_toString' method

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

BaseProxyHandler and its derived classes should have a 'className' virtual member function that returns the proper class name for use in printed forms of the object. This would 'see through' cross-compartment wrappers and other sorts of proxies. It would be specified not to cause JavaScript code to run, making it safe for use by Debugger (which wants to avoid setting off callees unexpectedly).

This should be visible via some new getter like Debugger.Object.prototype.printableLabel, which would be specified merely to return "something helpful to include in printed representations of the object", with no mention of ES class (which has turned out to be hard to implement as specified, since JSClass's names are not quite right, and not very useful as actually implemented, as it returns "Proxy" for almost every object where a good name would be most valuable).

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 1

[Boris Zbarsky [:bzbarsky]]

Attachment: codegen changes

Comment 2

[Jim Blandy :jimb]

Attachment: Let JavaScript proxies decide what class name to report for themselves.

Will ask for review if try likes the patch:
https://tbpl.mozilla.org/?tree=Try&rev=4457590db58d

Comment 3

[Boris Zbarsky [:bzbarsky]]

Can't we just kill off all the proxy obj_toString stuff now?  And in particular, switch Object.prototype.toString to using the new hook?

Without that, the codegen patch is clearly wrong....

Comment 4

[Jim Blandy :jimb]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Can't we just kill off all the proxy obj_toString stuff now?  And in
> particular, switch Object.prototype.toString to using the new hook?
> 
> Without that, the codegen patch is clearly wrong....

Which overrides of BaseProxyHandler::obj_toString, specifically, do you think should go? It looked to me like the ones I left needed to stay.

In the case of Object.prototype.toString: that calls js::obj_toStringHelper. When 'this' is a proxy, we do call Proxy::obj_toString, whose behavior really depends on which kind of proxy it is; we can't short-circuit that. But if the proxy in question does use BaseProxyhandler::obj_toString, then the patch does change that to invoke ...::className, which will use the most-derived class's definition.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Which overrides of BaseProxyHandler::obj_toString, specifically, do you think should go? 

All of them, and also BaseProxyHandler::obj_toString.  And Proxy::obj_toString should become Proxy::className and call the className on the handlers.

At least in my opinion.

Comment 6

[Jeff Walden [:Waldo]]

The entire obj_toString trap should be replaced wholesale with className, seems to me.  And Object.prototype.toString should be updated to call the className hook.

And why not const jschar*?  Notwithstanding that UTF-8 is the only good encoding, jschar matches JS far better than char.

Comment 7

[Jim Blandy :jimb]

On IRC, bz tells me that the target of a nsOuterWindowProxy is always of class "Window", and that the target of nsChromeOuterWindowProxy is always of class "ChromeWindow"; I wasn't sure about that. With that, it seems like there are no overrides of BaseProxyHandler::obj_toString that actually do anything other than construct strings of the form [object <className>], following the same proxy->target or cross-compartment-wrapper->referent links that the BaseProxyHandler::className overrides would.

What that means is that BaseProxyHandler::obj_toString can go away entirely; since we'll traverse proxies in className overrides the way the obj_toString overrides do now, there should be no visible effect.

Comment 8

[Jim Blandy :jimb]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> The entire obj_toString trap should be replaced wholesale with className,
> seems to me.  And Object.prototype.toString should be updated to call the
> className hook.

Yes --- see the previous comment. I was unsure whether this would preserve the behavior of certain proxies in the browser, but bz explained to me that that wasn't a problem.

> And why not const jschar*?  Notwithstanding that UTF-8 is the only good
> encoding, jschar matches JS far better than char.

I started out with jschar *. But since the className API effectively assumes we're returning a statically allocated string, that entailed changing the actual class names stored in the Class objects to jschar *, and thus changing all the users of js::Class::name in js/src. I started on that, but it turned out to be a larger change than I wanted to take on, and one that is completely independent of the problems being addressed in this bug. I don't think this patch should be gated on converting js::Class::name to jschar *.

Comment 9

[Jeff Walden [:Waldo]]

:-(  Yeah, that makes sense.

Comment 10

[Jim Blandy :jimb]

Attachment: Replace BaseProxyHandler::obj_toString with className.

This should be more what people had in mind. Running tests before taking up humans' time...

Comment 11

[Boris Zbarsky [:bzbarsky]]

The AutoCompartment bit in CrossCompartmentWrapper::className is not obvious to me in the new world, but it wasn't obvious in the old world either, honestly, since we then passed "wrapper" along!

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, and the rest of it looks lovely.

Comment 13

[Jim Blandy :jimb]

Attachment: Replace BaseProxyHandler::obj_toString with className.

Okay, this one builds. Submitted to try:
https://tbpl.mozilla.org/?tree=Try&rev=5b8165d6da09

Comment 14

[Jim Blandy :jimb]

(In reply to Boris Zbarsky (:bz) from comment #11)
> The AutoCompartment bit in CrossCompartmentWrapper::className is not obvious
> to me in the new world, but it wasn't obvious in the old world either,
> honestly, since we then passed "wrapper" along!

If I understand correctly, the old CrossCompartmentWrapper::obj_toString called Wrapper::obj_toString, passing along the same object that it received, because that's invoking the overridden base class's obj_toString - in this case, DirectProxyHandler::obj_toString, which takes care of getting the target object and passing along the call.

So I believe the old CCW::obj_toString is right; and thus I believe that the new CCW::className is right.

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 739715 [details] [diff] [review]
Replace BaseProxyHandler::obj_toString with className.

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

r=me with some comments addressed...

::: js/src/builtin/Object.h
@@ +18,5 @@
>  extern JSBool
>  obj_construct(JSContext *cx, unsigned argc, js::Value *vp);
>  
> +extern const char *
> +obj_classNameHelper(JSContext *cx, HandleObject obj);

Is this naming convention worth following?

How about adding a static method
  static const char *className(JSContext *cx, HandleObject obj);
to JSObject instead?

::: js/src/jit-test/tests/debug/Object-printableLabel.js
@@ +22,5 @@
> +  assertEq(arr[4].class, "Proxy");
> +  assertEq(arr[5].class, "Proxy");
> +};
> +assertEq(hits, 0);
> +g.eval('f(Object.prototype, [], eval, new Date, Proxy.create({}), Proxy.createFunction({}, f));');

Three comments.

1. It seems unfortunate for proxies created by content code to be able to fool the debugger so easily. This is one of those weird cases where we want one thing (Object.prototype.toString) to be fooled by a certain class of proxy (scripted proxies), but not another (the debugger). Whereas another class of proxy (transparent CrossCompartmentWrappers) should fool both. What do you think? Is it worth changing something?

2. Instead of these two properties, I suggest calling the new "printableLabel" property "class" ("className" would also be acceptable), deleting the old "class" property, and then adding D.O methods/properties for querying proxies (in a separate bug). I seriously think it shouldn't be called printableLabel when the underlying primitive is called something so totally different; .class is reasonable even though we're departing from ES3-5 [[Class]].

3. In any case, might as well test with scripted direct proxies (`new Proxy(new Date, {})`).

::: js/src/jswrapper.h
@@ +192,4 @@
>                              CallArgs args) MOZ_OVERRIDE;
>      virtual bool hasInstance(JSContext *cx, HandleObject proxy, MutableHandleValue v, bool *bp);
>      virtual bool objectClassIs(HandleObject obj, ESClassValue classValue, JSContext *cx);
> +    virtual const char *className(JSContext *cx, HandleObject proxy);

MOZ_OVERRIDE here too, and feel free to add it on all the other methods this class overrides.

Comment 16

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #14)
> So I believe the old CCW::obj_toString is right; and thus I believe that the
> new CCW::className is right.

Agreed.

Comment 17

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #15)
> 2. Instead of these two properties, I suggest calling the new
> "printableLabel" property "class" ("className" would also be acceptable),
> deleting the old "class" property

I agree --- this would make the upper layers of the debugger "just work".

> and then adding D.O methods/properties
> for querying proxies (in a separate bug).

Already specified; filed as bug 866307.

Comment 18

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #15)
> Is this naming convention worth following?

No.

> How about adding a static method
>   static const char *className(JSContext *cx, HandleObject obj);
> to JSObject instead?

An excellent suggestion. Done.

Comment 19

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #15)
> 3. In any case, might as well test with scripted direct proxies (`new
> Proxy(new Date, {})`).

Done.

Comment 20

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #15)
> ::: js/src/jswrapper.h
> @@ +192,4 @@
> >                              CallArgs args) MOZ_OVERRIDE;
> >      virtual bool hasInstance(JSContext *cx, HandleObject proxy, MutableHandleValue v, bool *bp);
> >      virtual bool objectClassIs(HandleObject obj, ESClassValue classValue, JSContext *cx);
> > +    virtual const char *className(JSContext *cx, HandleObject proxy);
> 
> MOZ_OVERRIDE here too, and feel free to add it on all the other methods this
> class overrides.

Done.

Comment 21

[Jim Blandy :jimb]

Try push for revised patch:
https://tbpl.mozilla.org/?tree=Try&rev=6019b52c4c25

Comment 22

[Jim Blandy :jimb]

Attachment: Replace BaseProxyHandler::obj_toString with className. r=jorendorff

Revised per comments.

Comment 23

[Jim Blandy :jimb]

Found a bunch of Mochitests that need revision.

Comment 24

[Jim Blandy :jimb]

Attachment: Replace BaseProxyHandler::obj_toString with className. r=jorendorff

This fixes almost all the mochitest failure, mostly by changing "Proxy" to "Window". However, there's one that I don't really understand:

 4:54.98 TEST-UNEXPECTED-FAIL| chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_propertyview-09.js | Should have the right property value for |InstallTrigger|. - Got [object Object], expected
 4:54.98 Stack trace:
 4:54.98     JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 527
 4:54.98     JS frame :: chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_propertyview-09.js :: test/<.run :: line 57
 4:54.98     native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
 4:54.98 

Panos, what is this test really expecting? Why is it expecting the empty string?

I tried to find out what InstallTrigger really is, and it seems like it's a getter property that replaces itself on the first use. Doesn't that mean that the value displayed depends on whether it's been referenced or not? Could we choose a less-magic global to check here?

Comment 25

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

(In reply to Jim Blandy :jimb from comment #24)
> I tried to find out what InstallTrigger really is, and it seems like it's a
> getter property that replaces itself on the first use. Doesn't that mean
> that the value displayed depends on whether it's been referenced or not?
> Could we choose a less-magic global to check here?

Certainly, the InstallTrigger test was a relatively recent change to the test. These tests are sometimes modified as we try to test assumptions about the window object that are often in flux, so this is to be expected. Even the number of window properties that we test, 3, is rather arbitrary, but sufficient for the purposes of this test.

You could swap in another common window property, like document or location, which used to be a problem in the past (pre-Paris-bindings IIRC), or pick another one that seems stable.

Comment 26

[Jim Blandy :jimb]

Okay, debugger mochitests should be clean. An abundance of caution:
https://tbpl.mozilla.org/?tree=Try&rev=ccaca28f627b

Comment 27

[Jim Blandy :jimb]

That's a nice clean Try push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9d66cb5f791

Comment 28

[Boris Zbarsky [:bzbarsky]]

Yep, that makes things in the web console much happier.  ;)

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9d66cb5f791