256766 - Chatzilla displays a bogus users line with (null) instead of the user counts for @ + and %

Status: RESOLVED FIXED

(Other Applications Graveyard :: ChatZilla, defect)

Description

[Frank Wein [:mcsmurf]]

To reproduce you need a current CVS trunk build, so get one, then join a irc
server (here irc.mozilla.org) and join some channel then. Then when you look at
the upper right of the Chatzilla window, you get 
Users  	58,10,0,0, (null)@, (null)%, (null)+
The 10 would need to stand before the @ instead of the (null), same for the two 0. 
This regressed somewhere before 2004-08-07 and 2004-08-05 i think. Probably this
is not a ChatZilla Bug, but i couldn't find a bug for this yet, so filling a new
one for triage of this problem, feel free to dupe/move to another component :)

Comment 1

[James Ross]

This is, I'm reliably informed, because "someone broke instanceof". The gist is
that "foo instanceof Array" is no longer working if |foo| comes from a different
window... which is exactly what's going on here (output window code vs. chrome
code).

This should be marked dependant or dup of the relevant bug... when I find it. :)

For those really interested, the issue is that
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/irc/xul/content/output-window.js&mark=373-375#361
calls
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/irc/js/lib/message-manager.js&mark=199#192
and that check fails so you end up with the replacemen parameter list just
containing a isngle item - the toString() of the actual list.

Comment 2

[Frank Wein [:mcsmurf]]

Looks like this comes from Bug 254067, maybe window.opener.foo needs to be used
(just a guess so)?!

Comment 3

[James Ross]

This sucks much. I can't believe they've /deliberately/ broken |foo instanceof
Array| when foo is definately a normal Array.

Comment 4

[Brendan Eich [:brendan]]

> This sucks much. I can't believe they've /deliberately/ broken |foo instanceof
> Array| when foo is definately a normal Array.

No one broke any such thing, as should be clear from reading bug 254067.

foo instanceof top.Array or fooFromTop instanceof Array, where fooFromTop is a
reference to an Array constructed in another window, is what we broke, in
keeping with ECMA sanity, future security, and other testable
object/prototype/constructor relationships.

Sorry it broke chatzilla, but the fix shouldn't be hard.

/be

Comment 5

[James Ross]

(In reply to comment #4)
> foo instanceof top.Array or fooFromTop instanceof Array, where fooFromTop is a
> reference to an Array constructed in another window, is what we broke, in
> keeping with ECMA sanity, future security, and other testable
> object/prototype/constructor relationships.

No use defending it now, I'd already read the bug and knew what had changed.

> Sorry it broke chatzilla, but the fix shouldn't be hard.

You're welcome to fix it, then - I don't know enough about the mysterious JS
engine to fix this.

Comment 6

[Robert Ginda]

I don't think brendan is suggesting that the fix would be part of the js engine.
 Just a chatzilla workaround.

Comment 7

[Brendan Eich [:brendan]]

Silver, if you want the JS fix backed out, make a better case than whining like
a nine year old.  If it's all mysterious and you can only act hard done by, take
it elsewhere, find more sympathetic marks.  Around here, unintended breakage
happens, and the right fix is not always to go back to the status quo ante, or
act like a brat until someone else does.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: possible fix

There are other instanceof uses in extensions/irc/js/lib that might need
similar fixing -- rginda, can you say which do not?

/be

Comment 9

[James Ross]

(In reply to comment #7)
> Silver, if you want the JS fix backed out

I never suggested that or thought about that as an option.

I suggested you fix it because *you* are the one here that knows what SM
provides that might be useful, like __parent__ (which seems to be undocumented
and deprecated, but that's neither here nor there).

Comment 10

[Mike Shaver (:shaver emeritus)]

Would it be better to check |typeof(params) == "object" && "length" in params|?
 That's our recommended "looks like a duck" Array-detection pattern, IIRC.

Comment 11

[Brendan Eich [:brendan]]

shaver: 'length' in params might lie -- the code wanted to know whether params
was an Array, not an object with a length property, and it seems better not to
relax that constraint.

Silver: sorry I misread you.  __parent__ is not deprecated, AFAIK.  Did someone
say it was?  It's peculiar to SpiderMonkey, but then so was the joining of all
constructors, in an instanceof sense, across window objects.

Anyone: what other instanceof usage in js/lib should change?

/be

Comment 12

[timeless]

brendan: mighty funny. jsengine strict warnings complain when you access .__parent__

>xpcshell.exe -w -s
js> a={}
typein:1: strict warning: assignment to undeclared variable a
[object Object]
js> a.__parent__
typein:2: strict warning: deprecated __parent__ usage
[object global]
js> build()
built on Aug 18 2004 at 08:30:02

believe me, we're familiar w/ that here, as we use it a lot and that warning
gets annoying while using venkman.

Comment 13

[James Ross]

(In reply to comment #11)
> Silver: sorry I misread you.  __parent__ is not deprecated, AFAIK.  Did
> someone say it was?

Not in SpiderMonkey, but then I couldn't find any SM-specific reference to it
(which is where the undocumented comment came from). The (deprecated) comment
came from a page about Rhino (http://www.mozilla.org/rhino/overview.html) which
says "The deprecated features are the __proto__ and __parent__ properties, and
the constructors With, Closure, and Call." (about 1/4th of the way down). I
guess these are only deprecated in Rhino, then, from your reaction (which is
good, as they're very useful :) ).

Comment 14

[Brendan Eich [:brendan]]

Attachment: remove strict warning for get of __proto__/__parent__

For crying out loud, *I* added strict warnings deprecating __proto__ and
__parent__ (maybe the Rhino mafia was pressuring me ;-).

This removes the strict warnings for the get case.

I'm likely to check it into the 1.8a4 trunk with very little encouragement from
timeless.

/be

Comment 15

[timeless]

Comment on attachment 157223 [details] [diff] [review]
remove strict warning for get of __proto__/__parent__

well, that'd certainly make some of us happy especially if you landed it on 1.7
branch :)

Comment 16

[Mike Shaver (:shaver emeritus)]

Comment on attachment 157223 [details] [diff] [review]
remove strict warning for get of __proto__/__parent__

Let's not have this fight in front of the children. r=shaver

Comment 17

[James Ross]

This was fixed in the landing of ChatZilla 0.9.65 (bug 255081).

I'll leave the bug open, though, as the JS engine strict warning patch is still
waiting for approval for 1.7.x landing (has it landed on trunk, either?).

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 157223 [details] [diff] [review]
remove strict warning for get of __proto__/__parent__

This patch was checked into 1.7 and aviary branches already -- stealthed as
part of a larger jsobj.c patch.  Sorry I forgot to note the approvals.

/be

Comment 19

[Brendan Eich [:brendan]]

Marking fix per Silver's comment 17.

/be