270343 - In <msgMail3PaneWindow.js>, "Warning: redeclaration of var msgFolder" and "Warning: reference to undefined property defaultServer.msgFolder"

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5) Gecko/20041116] (nightly) (W98SE)

{{
Warning: redeclaration of var msgFolder
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 277, Column: 19
Source Code:
               var msgFolder =
folder.QueryInterface(Components.interfaces.nsIMsgFolder);

Warning: reference to undefined property defaultServer.msgFolder
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 803
}}

Steps:
1. Open MailNews.

Comment 1

[Serge Gautherie (:sgautherie)]

|Warning: reference to undefined property defaultServer.msgFolder|
I don't know how to fix this one: helpwanted !

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-MAS) <msgMail3PaneWindow.js>

Please, check line 206 too:
|		var lastMessageLoaded = msgFolder.lastMessageLoaded;|
I wonder where this |msgFolder| was/is supposed to come from,
or maybe(!?) why this |if| block is not included at the end of the previous
one...

Comment 3

[[e-mail]]

(In reply to comment #2)
>Please, check line 206 too:
>|		var lastMessageLoaded = msgFolder.lastMessageLoaded;|
>I wonder where this |msgFolder| was/is supposed to come from,
>or maybe(!?) why this |if| block is not included at the end of the previous
>one...
Very interesting... I suggest the safest course of action would be to declare
and initialize msgFolder around line 130 so it's always available.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b-MAS) <msgMail3PaneWindow.js>

Av1, with comment 3 suggestion(s).

Neil:
*It's more line 140 than 130, isn't it ?
*Do you know how to fix the other warning ?

Comment 5

[[e-mail]]

I must have some changes which disrupt my line numbers.

At least the first part of your patch looks ok, I haven't read the rest yet.

As for the other warning I'm not sure what conditions you need to trigger it.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> As for the other warning I'm not sure what conditions you need to trigger it.

It's triggered by the following line
|user_pref("mail.server.server1.login_at_startup", true);|.
If I change the value to |false| the warning goes away.

NB: Same condition for bug 256447 warning which appears right after this one.


[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.5) Gecko/20041217] (release) (W98SE)

No bug(s):
+ (K) Regression.

Comment 7

[Serge Gautherie (:sgautherie)]

I guess that the Pop3 check starts before the UI has fully initialized !?

Comment 8

[[e-mail]]

Weird. I saw this once, and then it went away again...

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 168070 [details] [diff] [review]
(Av1b-MAS) <msgMail3PaneWindow.js>

David:
You can do the review too, if you have time... Thanks.

Comment 10

[[e-mail]]

Comment on attachment 168070 [details] [diff] [review]
(Av1b-MAS) <msgMail3PaneWindow.js>

>            var rerootingFolder = (uri == gCurrentFolderToReroot);
>+           var msgFolder = folder.QueryInterface(Components.interfaces.nsIMsgFolder);
>            if (rerootingFolder) {
Nit: keep the var rerootingFolder next to the if (rerootingFolder) it looks
neater that way.

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Av1c-MAS) <msgMail3PaneWindow.js> [Checked in: Comment 16]

Av1b-MAS, with comment 10 suggestion(s).

Keeping
{{
(Av1b-MAS) <msgMail3PaneWindow.js>	 patch		2004-12-06 15:55 PST   
3.82 KB 	neil.parkwaycc.co.uk: review+
}}

David:
Could you sr this for v1.8a6 ?

Comment 12

[David :Bienvenu]

Comment on attachment 170624 [details] [diff] [review]
(Av1c-MAS) <msgMail3PaneWindow.js>
[Checked in: Comment 16]

thx, sr=bienvenu, but I don't see any reason to put it in 1.8a6...

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 170624 [details] [diff] [review]
(Av1c-MAS) <msgMail3PaneWindow.js>
[Checked in: Comment 16]

(In reply to comment #12)
> (From update of attachment 170624 [details] [diff] [review] [edit])
> thx, sr=bienvenu, but I don't see any reason to put it in 1.8a6...

To fix comment 0 (warning in console) and comment 3 ("undefined" var usage) !?

'approval1.8a6=?':
Trivial U.I. code cleanup, no risk.

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-TB) <msgMail3PaneWindow.js> [Checked in: Comment 19]

"Same" as Av1c-MAS, but for ThunderBird.

I don't use TB: Could you test/review/check in this patch ? Thanks.

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 170624 [details] [diff] [review]
(Av1c-MAS) <msgMail3PaneWindow.js>
[Checked in: Comment 16]

a=asa for checkin to 1.8a6

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 170624 [details] [diff] [review]
(Av1c-MAS) <msgMail3PaneWindow.js>
[Checked in: Comment 16]


Check in: { 2005-01-09 14:13	neil%parkwaycc.co.uk	mozilla/ mailnews/
base/ resources/ content/ msgMail3PaneWindow.js      1.274 }

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 170628 [details] [diff] [review]
(Bv1-TB) <msgMail3PaneWindow.js>
[Checked in: Comment 19]

'approval1.8a6=?':
Trivial U.I. code cleanup, no risk.
This one should not affect v1.8a6, and it's MAS counterpart has already been
checked in: it can't hurt to stay in sync..

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 170628 [details] [diff] [review]
(Bv1-TB) <msgMail3PaneWindow.js>
[Checked in: Comment 19]

too late for non-critical fixes. please hold until we open for 1.8 Beta.
thanks.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 170628 [details] [diff] [review]
(Bv1-TB) <msgMail3PaneWindow.js>
[Checked in: Comment 19]


Check in: { 1.58	neil%parkwaycc.co.uk	2005-01-14 05:07 }

Comment 20

[[e-mail]]

The defaultServer.msgFolder problem, is a regression from bug 260447 - msgFolder
is not a property of defaultServer, perhaps you meant rootFolder instead?
>+                if (defaultServer && defaultServer.equals(currentServer) && 
>+                  !defaultServer.isDeferredTo &&
>+                  defaultServer.msgFolder == defaultServer.rootMsgFolder)

Comment 21

[[e-mail]]

That was actually quoted from mailWindowOverlay.js so in fact we have that error
in both Suite and Thunderbird's versions of the two files.

Comment 22

[David :Bienvenu]

Attachment: (Cv1) <mailWindowOverlay.js> [Checked in: Comment 24]

Comment 23

[[e-mail]]

Comment on attachment 173646 [details] [diff] [review]
(Cv1) <mailWindowOverlay.js>
[Checked in: Comment 24]

I'll take that as a "yes" ;-) I think there's still a case in the two copies of
msgMail3PaneWindow.js as mentioned in comment 0 and comment 1.

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 173646 [details] [diff] [review]
(Cv1) <mailWindowOverlay.js>
[Checked in: Comment 24]


Check in: { 2005-02-07 10:34	bienvenu%nventure.com }

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) <msgMail3PaneWindow.js> [Checkin: See comment 35]

Using Cv1 as a template...

I don't use TB: Could you test/(super-)review/check in this patch ? Thanks.

Comment 26

[David :Bienvenu]

Attachment: fix regression introduced by fixing warning. [Checked in: Comment 27]

fixing the warning breaks the downoading of pop3 mail on startup - since the
bogus code before always returned false for one of the conditions, I've removed
the whole if clause to get back to where we were before the warning was fixed.

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 173763 [details] [diff] [review]
fix regression introduced by fixing warning.
[Checked in: Comment 27]


Check in (without the |-w|): { 2005-02-08 10:25 bienvenu%nventure.com  
mozilla/ mailnews/ base/ resources/ content/ mailWindowOverlay.js	1.214 }

Comment 28

[Serge Gautherie (:sgautherie)]

(In reply to comment #27)
> (From update of attachment 173763 [details] [diff] [review] [edit])
> 
> Check in (without the |-w|): { 2005-02-08 10:25 bienvenu%nventure.com  
> mozilla/ mailnews/ base/ resources/ content/ mailWindowOverlay.js	1.214 }

With the mailnews/ part too.

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 173679 [details] [diff] [review]
(Dv1) <msgMail3PaneWindow.js> [Checkin: See comment 35]

[Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE)

No review from <[e-mail]> since "2005-02-07" :-(

Could you review/check in this patch ? Thanks.

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 170628 [details] [diff] [review]
(Bv1-TB) <msgMail3PaneWindow.js>
[Checked in: Comment 19]

[Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE)

Welcomed on branch too.

Comment 31

[Scott MacGregor]

Comment on attachment 170628 [details] [diff] [review]
(Bv1-TB) <msgMail3PaneWindow.js>
[Checked in: Comment 19]

please don't nominate JS warnings for .0.1. This is a security respin intended
for critical security bugs and not JS warnings.

Comment 32

[Asa Dotzler [:asa]]

Serge, in line with what Scott said, please don't nominate javascript warnings
for inclusion in any release. If you can get them in during the open checkin
periods, that's great. If not, then please wait until the next open period. 

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 173679 [details] [diff] [review]
(Dv1) <msgMail3PaneWindow.js> [Checkin: See comment 35]

[Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE)

I'm puzzled by the fact that TB doesn't complain with either |msgFolder| or
|rootFolder|...
I can't tell which one it best.

Comment 34

[Scott MacGregor]

Comment on attachment 173679 [details] [diff] [review]
(Dv1) <msgMail3PaneWindow.js> [Checkin: See comment 35]

I think this patch is obsolete now. Looks like it's been checked in already.

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 173679 [details] [diff] [review]
(Dv1) <msgMail3PaneWindow.js> [Checkin: See comment 35]

(In reply to comment #34)
> (From update of attachment 173679 [details] [diff] [review] [edit])
> I think this patch is obsolete now. Looks like it's been checked in already.

Right:
/mail/ part was later included in bug 270743;
/mailnews/ part was later included in bug 304176.

As a sidenote,
I'm still "sad" that so many of my patches simply rot in BugZilla,
while other people (you and Neil in this case) 1) have to double my work (monthes later), 2) are able to get their patchs in :-/

Comment 36

[Scott MacGregor]

(In reply to comment #35)
> (From update of attachment 173679 [details] [diff] [review] [edit])
> (In reply to comment #34)
> > (From update of attachment 173679 [details] [diff] [review] [edit] [edit])
> > I think this patch is obsolete now. Looks like it's been checked in already.
> 
> Right:
> /mail/ part was later included in bug 270743;
> /mailnews/ part was later included in bug 304176.
> 
> As a sidenote,
> I'm still "sad" that so many of my patches simply rot in BugZilla,
> while other people (you and Neil in this case) 1) have to double my work
> (monthes later), 2) are able to get their patchs in :-/

maybe we can get you focused on something that's going to have more impact that these JS warnings that we don't even see. They are so low on our list that they tend to be more of a nuisance to have to deal with. And in one case, one of them snuck in and caused a really bad regression in Thunderbird 1.5. That would be a win win for everyone if we could do that :)

Comment 37

[Serge Gautherie (:sgautherie)]

(In reply to comment #36)
> maybe we can get you focused on something that's going to have more impact that
> these JS warnings that we don't even see. They are so low on our list that they
> tend to be more of a nuisance to have to deal with. And in one case, one of
> them snuck in and caused a really bad regression in Thunderbird 1.5. That would
> be a win win for everyone if we could do that :)

At least, I have stopped preparing patches (for such JS warning bugs)...

Comment 38

[[e-mail]]

(In reply to comment #36)
>And in one case, one of them snuck in and caused a really bad regression
Well in this case I'm blaming David for forgetting to set javascript.options.strict to true and thus catching his error :-P

Comment 39

[[e-mail]]

(In reply to comment #35)
>/mail/ part was later included in bug 270743;
>/mailnews/ part was later included in bug 304176.
Indeed, these regressions might well have been avoided had people paid more attention to JavaScript warnings in the first place.