108284 - better error message when 0 passed and expected object or null

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[John Bandhauer]

akkana ran into a case where she was getting a conversion failure when passing 0 
(from JS) to a method that expected an nsISupport*. xpcconvert will convert null 
or undefined to a C++ nsnull AND it will accept an object. Every other type will 
result in a conversion error. Perhaps we should change the logic a bit so that 
the other JS types are considered. We *don't* want to call JS_ValueToObject 
becasue that has the potential of creating new JSObjects to represent numbers 
and the like. But, we could check for 0 if its a number or false if its boolean 
and yield an nsnull. I'm thinking this will do more good than bad. Thoughts?

Comment 1

[Mike Shaver (:shaver emeritus)]

So, I'm normally pretty conservative about this sort of thing, for reasons of
future-proofing and general stick-in-the-mudness, but I think I'm OK with this.

Two questions, though:

- With the addition of variant support, will there ever be a case of automatic
variant creation, where passing a number or an object will have different
semantics?  If so, I don't think we should go this route, because even if we
don't do the additional 0-to-null conversion for variants, we will breed confusion.

- Do the additional checks here only occur in the case where we would currently
be failing?  That is: will there be any performance penalty to
"correctly-written" code?

I'm still a little icked-out about borrowing C/C++'s pointer/integer equivalence
into our nice tidy language, but I think I can *probably* live with it. 
(|false| is a bit more of a stretch, since you can't do that in C++ either, and
I see the value here as being primarily related to C++ brainprint-compatibility.)

Comment 2

[John Bandhauer]

shaver: Thanks for the thoughts.

The variant issue is a good one to raise. In the current code if a param is 
declared as nsIVariant then xpconnect will try to build an xpcvariant 
to contain your value regardless of what you pass. So, passing null gives you a 
non-null variant object that is set as 'empty'. This is unlike what happens if 
the param is declared as nsISupports* (where you get null). And so this would be 
odder still if we added this additional conversion because in the variant case 
you'd get a variant holding the number 0 or 'empty' if you passed 0 or null, but 
in the nsISupports case you'd get null either way.

On your second question... I'd be proposing extra work only in the case that 
would have been an error.

I'm glad you brought up the compatibility question. I'd overlooked it this time 
(though I'm usually right on top of that). I'm not happy with gratuitous new 
features whose lack of support in previous versions people will find at their 
own peril. When the gain is minimal then we should really question such things.

I have to agree that for people who don't come from a C/C++ this conversion is 
not so natural.

So, I think I'm going to back off from my (tentative) suggestion. Instead, I 
propose a new error code and message that will note the 0 case (not bothering 
with "0" or false) and give a conversion error diagnostic that offers a clue 
that if the coder meant null they should use null. This is a trivial fix. I'll 
attach a patch. If others agree then we can morph this bug.

Comment 3

[John Bandhauer]

Attachment: proposed fix to just give a better error message

Comment 4

[Akkana Peck]

That would be perfect.  As the one whose JS ignorance inspired this bug, I was
glad to have the difference pointed out to me, and if JS could have pointed out
the difference on its own without my having to ask jband, so much the better. 
No need to try to make JS behave like C.

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 56363 [details] [diff] [review]
proposed fix to just give a better error message

That's the stuff. sr=shaver.

Comment 6

[John Bandhauer]

dbradley: review?

Comment 7

[David Bradley]

Comment on attachment 56363 [details] [diff] [review]
proposed fix to just give a better error message

r=dbradley

Looks fine to me.

Comment 8

[John Bandhauer]

fix checked in to trunk

Comment 9

[Phil Schwartau]

Marking Verified -