Closed
Bug 256766
Opened 20 years ago
Closed 20 years ago
Chatzilla displays a bogus users line with (null) instead of the user counts for @ + and %
Categories
(Other Applications :: ChatZilla, defect)
Other Applications
ChatZilla
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mcsmurf, Assigned: rginda)
References
Details
Attachments
(2 files)
1.01 KB,
patch
|
Details | Diff | Splinter Review | |
898 bytes,
patch
|
shaver
:
review+
brendan
:
approval-aviary+
brendan
:
approval1.7.5+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
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.
Reporter | ||
Comment 2•20 years ago
|
||
Looks like this comes from Bug 254067, maybe window.opener.foo needs to be used (just a guess so)?!
Comment 3•20 years ago
|
||
This sucks much. I can't believe they've /deliberately/ broken |foo instanceof Array| when foo is definately a normal Array.
Updated•20 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Comment 4•20 years ago
|
||
> 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•20 years ago
|
||
(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.
Assignee | ||
Comment 6•20 years ago
|
||
I don't think brendan is suggesting that the fix would be part of the js engine. Just a chatzilla workaround.
Comment 7•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
(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).
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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
(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•20 years ago
|
||
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•20 years ago
|
||
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 :)
Attachment #157223 -
Flags: review?(shaver)
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
Attachment #157223 -
Flags: review?(shaver) → review+
Attachment #157223 -
Flags: approval1.7.x?
Comment 17•20 years ago
|
||
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?).
Depends on: 255081
Comment 18•20 years ago
|
||
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
Attachment #157223 -
Flags: approval1.7.x?
Attachment #157223 -
Flags: approval1.7.x+
Attachment #157223 -
Flags: approval-aviary+
Comment 19•20 years ago
|
||
Marking fix per Silver's comment 17. /be
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Flags: blocking1.8a4?
Updated•20 years ago
|
Product: Core → Other Applications
You need to log in
before you can comment on or make changes to this bug.
Description
•