Make stringification of DOM Xrays use Object.prototype.toString

RESOLVED FIXED in mozilla35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 8489512 [details] [diff] [review]
v1
Attachment #8489512 - Flags: review?(bobbyholley)
(Assignee)

Comment 1

4 years ago
Comment on attachment 8489512 [details] [diff] [review]
v1

Actually, this isn't right.
Attachment #8489512 - Attachment is obsolete: true
Attachment #8489512 - Flags: review?(bobbyholley)
(Assignee)

Comment 2

4 years ago
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.
Attachment #8489512 - Attachment is obsolete: false
Attachment #8489512 - Flags: review?(bobbyholley)
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?
(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).
(Assignee)

Comment 5

4 years ago
(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 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?
Attachment #8489512 - Flags: review?(bobbyholley) → review-
(Assignee)

Comment 7

4 years ago
Created attachment 8493710 [details] [diff] [review]
v2
Attachment #8489512 - Attachment is obsolete: true
Attachment #8493710 - Flags: review?(bobbyholley)
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.
Attachment #8493710 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/8c47153a1dd1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.