684601 - window.toString.call() with native JS Object

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Tom S [:evilpie]]

We should try to figure out some way to make toString behave consistent with Object.prototoype.toString. I don't fully understand the necessary of them, because most of time they are not needed, and i think we already unwrap some of them.

Testcase:
window.toString.call(new Number()) should be '[object Number]', but is '[xpconnect wrapped native prototype]'. Which is very nasty, because it does not even follow the '[object ' + [[Class]] + ']' pattern.

Comment 1

[Tom S [:evilpie]]

Attachment: shadowing patch

This is a patch i wrote a while back, that just shadows this problem. I showed this to gal on irc, but he proposed some different solution, that i don't remember anymore :O

Comment 2

[Boris Zbarsky [:bzbarsky]]

Well, a good first question is what should happen here per spec....

Comment 3

[Cameron McCormack (:heycam)]

Per spec, window.toString.call(new Number()) should indeed return "[object Number]", since window.toString should be the one from Object.prototype; Window doesn't define its own.

It looks like `window.toString != Object.prototype.toString` however.

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Cameron McCormack (:heycam) from comment #3)
> Per spec, window.toString.call(new Number()) should indeed return "[object
> Number]", since window.toString should be the one from Object.prototype;
> Window doesn't define its own.
> 
> It looks like `window.toString != Object.prototype.toString` however.

So what happens here is that the window is actually a native wrapper with a reference to the native chrome window. It has a toString method (also native) XPC_WN_Shared_ToString (xpcwrappednativejsops.cpp) and works fine with other native wrappers, but for objects like that number it falls back to ToStringGuts (same file). This function is not really supposed to be called on a none wrapper object, like a Number instance for example, and simply returns "[xpconnect wrapped native prototype]".
My guess is that this part could be changed to call Object.prototype.toString on it (maybe with some condition...), but don't really know what is this string originally stands for. I mean is there a case where it's the expected return value, or is it just a 'this should not really happen' branch?

Comment 5

[Boris Zbarsky [:bzbarsky]]

The simplest way to invoke XPC_WN_Shared_ToString with a non-XPCWN object as |this| is to do window.__proto__.toString().

In which case, "[xpconnect wrapped native prototype]" is sort of a purposeful return value.

Not sure what the DOM spec says nowadays about what window.__proto__.toString() should return.

Comment 6

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky (:bz) from comment #5)
> In which case, "[xpconnect wrapped native prototype]" is sort of a
> purposeful return value.

Right, this makes sense, then the question is if it would be a good enough solution to fake the behavior of the Object.prototype.toString function here? Or we want a solution for the window.toString != Object.prototype.toString problem as well.

In the first case we could check the 'this' object and either return this const string or simulate the Object.prototype.toString on the 'this' object (we would get the 'this' object as an optional 2nd arg).

In the second case however it's a more complicated problem (at my knowledge level at least)...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Long-term, we probably just want to nuke the XPConnect toString hook on DOM classes.  In my opinion.

That will make toString on DOM prototypes kinda useless like it is on native JS objects, but it's what the spec seems to call for....

Now one issue with that approach is that the XPC toString actually does something interesting in debug builds.  I suppose we could keep it in debug builds if we wanted.

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Long-term, we probably just want to nuke the XPConnect toString hook on DOM
> classes.  In my opinion.

I can ifdef it out in DefinePropertyIfFound. But I'm not sure I can identify if the object is from a DOM class easily. There are a set of interfaces the object implements so it is possible probably (slow and not nice at all), do we need this toString method for not DOM objects? Or can I just ifdef it out?
 
> Now one issue with that approach is that the XPC toString actually does
> something interesting in debug builds.  I suppose we could keep it in debug
> builds if we wanted.

It would confuse me a bit at some bug debugging that I get totally different results for a toString... If not many ppl need it maybe it would be better to link this to a mozconfig flag, like some other debugging helper features. What do you think?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> do we need this toString method for not DOM objects?

It's very useful when debugging, at least.  So generally yes.

> But I'm not sure I can identify if the object is from a DOM class easily

We could add a flag to nsIXPCScriptable for this (or rather try to repurpose one of the existing flags, since we're out of flags).

Comment 10

[Boris Zbarsky [:bzbarsky]]

> It's very useful when debugging, at least.

Debugging extensions, so can't be linked to build-time stuff.

Comment 11

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Not sure what the DOM spec says nowadays about what
> window.__proto__.toString() should return.

"[object Object]", since window.__proto__ should be a plain object.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Attachment: Fix proposal

So I reused the WANT_CONVERT flag since that was not used by any class, so it's now IS_DOM_CLASS. And I added an extra about:config flag (dom.XPCToStringForDOMClasses) if someone wants to use the xpc version of toString for dom classes too (for debugging). 

Since we don't support prototypes per webidl as Boris pointed it out on irc, the window.__proto__.toString() returns now [object XPC_WN_ModsAllowed_NoCall_Proto_JSClass]  Which is not the best. The proper fix for this issue would be to support proper prototypes per webidl, but I think for that we should open a new bug. And actually since the new binding will solve this problem, maybe it's a won't fix for now...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 562737 [details] [diff] [review]
Fix proposal

I'd really like peterv to look over the nsIXPCSCriptable changes...

Another thing I just realized.  nsIClassInfo already has a DOM_OBJECT flag.  Is there a reason to not just use that here?  Does involve getting the scriptable helper, etc, but for prototype setup maybe that;s ok?

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 562737 [details] [diff] [review]
Fix proposal

>+PRBool NeedXPCToStringForDOMClasses()
>+{
>+    PRBool result = PR_FALSE;
>+    nsCOMPtr<nsIPrefBranch> prefs;
>+    nsCOMPtr<nsIPrefService> pserve(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+    if (pserve)
>+        pserve->GetBranch("", getter_AddRefs(prefs));
>+    if (prefs)
>+        prefs->GetBoolPref("dom.XPCToStringForDOMClasses", &result);    
>+    return result;

Could this use mozilla::Preferences?

>+            bool overwriteToString = 
>+                (scriptableInfo && scriptableInfo->GetFlags().IsDOMClass()) 
>+                    ? NeedXPCToStringForDOMClasses() 
>+                    : true;

This can be !scriptableInfo || !scriptableInfo->GetFlags().IsDOMClass() || NeedXPCToStringForDOMClasses(), no?

Comment 15

[Gabor Krizsanits (INACTIVE)]

Attachment: Bug 684601 - window.toString.call() with native JS Object

(In reply to Boris Zbarsky (:bz) from comment #13)
> Another thing I just realized.  nsIClassInfo already has a DOM_OBJECT flag. 
> Is there a reason to not just use that here?  Does involve getting the
> scriptable helper, etc, but for prototype setup maybe that;s ok?

I have not realized that I can get a reference to an nsIClassInfo here... so tried this approach and it simplifies the patch a lot. It works, but the raw casting of xpc_GetJSPrivate worries me a bit... Is there any guarding condition I should use before that here?

I have not modified anything in nsIXPCSCriptable in this version so I have not put peterv on the review list yet, but feel free to do so.


(In reply to Ms2ger from comment #14)
> Could this use mozilla::Preferences?
Thanks for pointing it out! I was not aware of that api...

> This can be !scriptableInfo || !scriptableInfo->GetFlags().IsDOMClass() ||
> NeedXPCToStringForDOMClasses(), no?
It's a matter of taste I guess... I thought it's easier to understand the other version at first read, but since you are asking this question probably it's not the case. I used this style in the new version since it's shorter.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 563038 [details] [diff] [review]
Bug 684601 - window.toString.call() with native JS Object

>+            XPCWrappedNativeProto* self =
>+                (XPCWrappedNativeProto*) xpc_GetJSPrivate(obj);
>+
>+            bool overwriteToString = !self || !self->ClassIsDOMObject() 
>+                || mozilla::Preferences::GetBool("dom.XPCToStringForDOMClasses", PR_FALSE);

'using namespace mozilla;' and 'Preferences::GetBool("dom.XPCToStringForDOMClasses", PR_FALSE)', please.

As for clearer... It's more that I find foo ? bar : true a pretty weird pattern. I think it came out slightly better, though.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 563038 [details] [diff] [review]
Bug 684601 - window.toString.call() with native JS Object

I don't think this is right.  In particular, nothing really guarantees that |obj| is an XPConnect proto as far as I can tell.

But the good news is that you don't need that.  You have a XPCNativeScriptableInfo (possibly null).  I believe for DOM objects the GetCallback from that actually QIs to nsIClassInfo.

Again, please have Peter review changes to this stuff.

Comment 18

[Gabor Krizsanits (INACTIVE)]

Attachment: 3rd attempt

Thanks for both reviews, I hope this time it'll be better.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 564476 [details] [diff] [review]
3rd attempt

>--- a/js/src/xpconnect/src/xpcwrappednativejsops.cpp
>+++ b/js/src/xpconnect/src/xpcwrappednativejsops.cpp
>+                nsCOMPtr<nsIClassInfo> classInfo;
>+                nsresult rv = NS_OK;
>+                classInfo = do_QueryInterface(scriptableInfo->GetCallback());
>+                if (classInfo && NS_FAILED(rv = classInfo->GetFlags(&flags)))
>+                    return Throw(rv, ccx);

I would write this as

if (nsCOMPtr<nsIClassInfo> classInfo = do_QueryInterface(scriptableInfo->GetCallback())) {
  nsresult rv = classInfo->GetFlags(&flags)
  if (NS_FAILED(rv)) {
    return Throw(rv, ccx);
  }
}

(Unless your reviewer disagrees, and with 4-space indent.)

>+            bool overwriteToString = !(flags & nsIClassInfo::DOM_OBJECT) 
>+                || Preferences::GetBool("dom.XPCToStringForDOMClasses", PR_FALSE);

s/PR_FALSE/false/ here, and || should be at the end of the line.

Comment 20

[Gabor Krizsanits (INACTIVE)]

(In reply to Ms2ger from comment #19)

Lesson learned: re-read our coding standards. But... 

> 
> if (nsCOMPtr<nsIClassInfo> classInfo =
> do_QueryInterface(scriptableInfo->GetCallback())) {
>   nsresult rv = classInfo->GetFlags(&flags)
>   if (NS_FAILED(rv)) {
>     return Throw(rv, ccx);
>   }
> }
> 

Would you mind instead of 
if () {

}

using 

if()
{

}

here, since the whole file is using this pattern I don't want to break that only here... In general, if a whole file using different patterns than the coding standard, which one leads? I personally don't like mixing different styles in one file, but I'll follow whatever standard we have in these cases.

Comment 21

[Jeff Walden [:Waldo]]

Yes, do indeed follow local "style".  (XPConnect, blech.)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 564476 [details] [diff] [review]
3rd attempt

r=me

Comment 23

[Tom S [:evilpie]]

push

Comment 24

[Gabor Krizsanits (INACTIVE)]

(In reply to Tom Schuster (evilpie) from comment #23)
> push

This patch is not ready to be pulled yet. I still need a conformation from peterv that we are doing the right thing here and then I will file an updated, cleaned up patch that can be pushed.

Comment 25

[Tom S [:evilpie]]

*Ping* I think bholley should just look at this.

Comment 26

[Bobby Holley (:bholley)]

These files were moved, renamed, and re-styled several months ago. In particular, comment 20 is no longer valid.

I'm happy to be the reviewer here if you don't need peterv's DOM expertise, though in that case bz's review is probably Sufficient. Feel free to flag me for review on a rebased patch.

Comment 27

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #26)
> These files were moved, renamed, and re-styled several months ago. In
> particular, comment 20 is no longer valid.
> 
> I'm happy to be the reviewer here if you don't need peterv's DOM expertise,
> though in that case bz's review is probably Sufficient. Feel free to flag me
> for review on a rebased patch.

I'm also happy to rebase the patch, or make any changes on it I have to, but since bc asked my clearly twice to ask peterv to take a look at it (whether or not this is the right thing we do here), I'm going to take his request seriously and wait for peterv's opinion.

Comment 28

[Tom S [:evilpie]]

This is still not working right. If necessary I can rebase this patch.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 564476 [details] [diff] [review]
3rd attempt

I don't think this needs a separate review from Peter.  Just land it?

Comment 30

[Gabor Krizsanits (INACTIVE)]

So I wanted to land this patch after an update, but it turned out on try that it has a side effect... The chrome window returns [object ChromeWindow] pre-patch but [object Window] post-patch. And some tests expects ChromeWindow ( http://mxr.mozilla.org/mozilla-central/source/docshell/test/navigation/NavigationUtils.js#111 ). So now I'm trying to figure out why is this happening, or how to fix it... Boris, what do you think?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Presumably the JSClass for that object should have ChromeWindow as its name?

Comment 32

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky (:bz) from comment #31)
> Presumably the JSClass for that object should have ChromeWindow as its name?

According to it's scriptableInfo yes. I'm not sure where is that "Window" coming from and got lost in the debugging so far...

Comment 33

[Gabor Krizsanits (INACTIVE)]

Ok I got this. So it was the nsOuterWindowProxy::obj_toString that returned that [object Window] string... I think I can just change that to return [object ChromeWindow] if the proxy was created for a chrome window. Except if an outer window that was chrome once can somehow turn into a non-chrome window or the other way around... Since the proxy has no idea about the object it proxies to, if this state can change dynamically then I have no idea how to fix this. (pushed my patch to try again to see what happens...)

Comment 34

[Gabor Krizsanits (INACTIVE)]

Attachment: no XPC_WN_Shared_ToString for DOM objects

Could you take a second look at this patch since I had to modify it? The changes in the nsGlobalGlobalWindow.cpp is the new stuff. As in my previously comment described I have to create different proxy class for chrome outher windows to return different value from the obj_toString method than for the non-chrome ones.
It's green on try.

Comment 35

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 666920 [details] [diff] [review]
no XPC_WN_Shared_ToString for DOM objects

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

::: dom/base/nsGlobalWindow.cpp
@@ +625,5 @@
>  
> +class nsChromeOuterWindowProxy : public nsOuterWindowProxy
> +{
> +public:
> +  nsChromeOuterWindowProxy() {}

Add

: nsOuterWindowProxy()

I think

@@ +627,5 @@
> +{
> +public:
> +  nsChromeOuterWindowProxy() {}
> +
> +  JSString *obj_toString(JSContext *cx, JSObject *wrapper);

Add 'virtual' here for clarity, please.

@@ +635,5 @@
> +
> +JSString *
> +nsChromeOuterWindowProxy::obj_toString(JSContext *cx, JSObject *proxy)
> +{
> +    JS_ASSERT(js::IsProxy(proxy));

MOZ_ASSERT in dom/

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +255,5 @@
>  
>      if (!found) {
>          if (reflectToStringAndToSource) {
>              JSNative call;
> +            PRUint32 flags = 0;

uint32_t

@@ +262,5 @@
> +                nsCOMPtr<nsIClassInfo> classInfo;
> +                nsresult rv = NS_OK;
> +                classInfo = do_QueryInterface(scriptableInfo->GetCallback());
> +                if (classInfo && NS_FAILED(rv = classInfo->GetFlags(&flags)))
> +                    return Throw(rv, ccx);

if (scriptableInfo) {
    nsCOMPtr<nsIClassInfo> classInfo = do_QueryInterface(scriptableInfo->GetCallback());
    if (classInfo) {
        nsresult rv = classInfo->GetFlags(&flags);
        if (NS_FAILED(rv))
            return Throw(rv, ccx);
    }
}

@@ +266,5 @@
> +                    return Throw(rv, ccx);
> +            }
> +
> +            bool overwriteToString = !(flags & nsIClassInfo::DOM_OBJECT) 
> +                || Preferences::GetBool("dom.XPCToStringForDOMClasses", false);

Trailing whitespace

@@ +269,5 @@
> +            bool overwriteToString = !(flags & nsIClassInfo::DOM_OBJECT) 
> +                || Preferences::GetBool("dom.XPCToStringForDOMClasses", false);
> +
> +            if(id == rt->GetStringID(XPCJSRuntime::IDX_TO_STRING)
> +                && overwriteToString) 

more

Comment 36

[Gabor Krizsanits (INACTIVE)]

(In reply to :Ms2ger from comment #35)
> Comment on attachment 666920 [details] [diff] [review]
> Add
> 
> : nsOuterWindowProxy()
> 
> I think

Is that necessary if it takes 0 args?
 
> more

I'll fix the rest locally.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 666920 [details] [diff] [review]
no XPC_WN_Shared_ToString for DOM objects

With Ms2ger's comments addressed, r=me

Comment 38

[Gabor Krizsanits (INACTIVE)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5a4513c759b9

Comment 39

[Jeff Walden [:Waldo]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #36)
> (In reply to :Ms2ger from comment #35)
> > Comment on attachment 666920 [details] [diff] [review]
> > Add
> > 
> > : nsOuterWindowProxy()
> > 
> > I think
> 
> Is that necessary if it takes 0 args?

Not really.  Omitting an initializer for a base class or member field causes that thing to be default-initialized.  Default initialization for an object which isn't POD (I can't imagine nsOuterWindowProxy is, but I haven't checked) is just calling the default constructor.

I'd assume the rationale for visibly calling the base class constructor, then, is simply explicitness and clarity.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5a4513c759b9

Should this have a test?

Comment 41

[Tom S [:evilpie]]

Yes, we should not forget about a testcase!