If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla23

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Blocks: 856038
(Assignee)

Updated

5 years ago
Summary: C++ proxy handlers should have a 'getClass' method → C++ proxy handlers should have a 'className' method
Created attachment 738558 [details] [diff] [review]
codegen changes
Attachment #738558 - Flags: review?(peterv)
Attachment #738558 - Attachment is patch: true
Attachment #738558 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 2

5 years ago
Created attachment 738644 [details] [diff] [review]
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
Assignee: general → jimb
Status: NEW → ASSIGNED
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....
(Assignee)

Updated

5 years ago
Attachment #738644 - Flags: review?(jorendorff)
(Assignee)

Comment 4

5 years ago
(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.
> 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.
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.
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
(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 *.
:-(  Yeah, that makes sense.
(Assignee)

Comment 10

5 years ago
Created attachment 738775 [details] [diff] [review]
Replace BaseProxyHandler::obj_toString with className.

This should be more what people had in mind. Running tests before taking up humans' time...
Attachment #738644 - Attachment is obsolete: true
Attachment #738644 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Summary: C++ proxy handlers should have a 'className' method → C++ proxy handlers should have a 'className' method, instead of an 'obj_toString' method
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!
Oh, and the rest of it looks lovely.
(Assignee)

Comment 13

5 years ago
Created attachment 739715 [details] [diff] [review]
Replace BaseProxyHandler::obj_toString with className.

Okay, this one builds. Submitted to try:
https://tbpl.mozilla.org/?tree=Try&rev=5b8165d6da09
Attachment #738775 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #739715 - Flags: review?(jorendorff)
Attachment #738558 - Flags: review?(peterv) → review+
(Assignee)

Comment 14

5 years ago
(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 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.
Attachment #739715 - Flags: review?(jorendorff) → review+
(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.
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Comment 20

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
Try push for revised patch:
https://tbpl.mozilla.org/?tree=Try&rev=6019b52c4c25
(Assignee)

Comment 22

5 years ago
Created attachment 742625 [details] [diff] [review]
Replace BaseProxyHandler::obj_toString with className. r=jorendorff

Revised per comments.
Attachment #739715 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Found a bunch of Mochitests that need revision.
(Assignee)

Comment 24

5 years ago
Created attachment 742906 [details] [diff] [review]
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?
Attachment #738558 - Attachment is obsolete: true
Attachment #742625 - Attachment is obsolete: true
Flags: needinfo?(past)
(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.
Flags: needinfo?(past)
(Assignee)

Comment 26

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

Comment 27

5 years ago
That's a nice clean Try push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9d66cb5f791
Flags: in-testsuite+
Target Milestone: --- → mozilla23
Yep, that makes things in the web console much happier.  ;)
https://hg.mozilla.org/mozilla-central/rev/e9d66cb5f791
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 867946
You need to log in before you can comment on or make changes to this bug.