1067501 - Make stringification of DOM Xrays use Object.prototype.toString

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 1

[Peter Van der Beken [:peterv]]

Comment on attachment 8489512 [details] [diff] [review]
v1

Actually, this isn't right.

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 8489512 [details] [diff] [review]
v1

Actually, it should work fine (and it's green on try). It works by returning the WebIDL toString for the DOM object if there is one (XrayResolveNativeProperty will find it), and falling back to XrayToString (calling JS_BasicObjectToString) if there isn't one. In the future we'll just not return the XrayToString one, but Object.prototype will be on the prototype chain and so we'll get its toString as the fallback.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8489512 [details] [diff] [review]
v1

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

Hm, two things seem questionable to me:

1 - Why do we need a special JSNative to toString interface objects? Shouldn't the inherited fun_toString give us the right answer, at least in-content (and eventually, with the bug 787070, over Xrays as well)? 
2 - If there is a reason for (1), it still seems wrong to pass the target's toString JSFunction back to the Xray caller. Standard Xray semantics mint an entirely new function, right?

Comment 4

[Bobby Holley (:bholley)]

(Specially, it seems like any hack for interface objects here, if any, should go away in bug 787070, but I don't see that in those patches).

Comment 5

[Peter Van der Beken [:peterv]]

(In reply to Bobby Holley (:bholley) from comment #3)
> 1 - Why do we need a special JSNative to toString interface objects?
> Shouldn't the inherited fun_toString give us the right answer, at least
> in-content (and eventually, with the bug 787070, over Xrays as well)? 

Because we can't always use a function object, iirc mostly for instanceof (and so see http://hg.mozilla.org/mozilla-central/annotate/488d490da742/dom/bindings/BindingUtils.cpp#l465).

> 2 - If there is a reason for (1), it still seems wrong to pass the target's
> toString JSFunction back to the Xray caller. Standard Xray semantics mint an
> entirely new function, right?

I can do that, yes.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8489512 [details] [diff] [review]
v1

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

(In reply to Peter Van der Beken [:peterv] from comment #5)
> (In reply to Bobby Holley (:bholley) from comment #3)
> > 1 - Why do we need a special JSNative to toString interface objects?
> > Shouldn't the inherited fun_toString give us the right answer, at least
> > in-content (and eventually, with the bug 787070, over Xrays as well)? 
> 
> Because we can't always use a function object, iirc mostly for instanceof
> (and so see
> http://hg.mozilla.org/mozilla-central/annotate/488d490da742/dom/bindings/
> BindingUtils.cpp#l465).

Ah I see. Is that in the spec? Is it a bug in SpiderMonkey?
 
> > 2 - If there is a reason for (1), it still seems wrong to pass the target's
> > toString JSFunction back to the Xray caller. Standard Xray semantics mint an
> > entirely new function, right?
> 
> I can do that, yes.

OK, cool. That'll avoid introducing the extra slot, right?

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8493710 [details] [diff] [review]
v2

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

::: dom/bindings/BindingUtils.cpp
@@ +1279,5 @@
> +      desc.object().set(wrapper);
> +      desc.setAttributes(0);
> +      desc.setGetter(JS_PropertyStub);
> +      desc.setSetter(JS_StrictPropertyStub);
> +      desc.value().setObject(*JS_GetFunctionObject(toString));

Please use FillPropertyDescriptor here.

Comment 9

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c47153a1dd1

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c47153a1dd1