442248 - In <msgMail3PaneWindow.js>, "Error: gMsgFolderSelected is undefined"

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.1pre) Gecko/2008062703 Thunderbird/3.0a2pre] (nightly) (W2Ksp4)

No bug.


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.1pre) Gecko/2008062701 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

1. Go offline.
2. Create (dumb) IMAP account.
2r. Each opening of 3-pane window:
{{
Error: gMsgFolderSelected is undefined
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 253
}}

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008070402 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Venkman call stack is
{{
#0: function anonymous(event=XPComponent:{5}, folder=XPComponent:{133}) in <chrome://messenger/content/msgMail3PaneWindow.js> line 253
 1: performBiff / x-jsd:native-code
#2: function loadStartFolder(initialUri=null:null) in <chrome://messenger/content/msgMail3PaneWindow.js> line 842
#3: function (null)() in <chrome://messenger/content/msgMail3PaneWindow.js> line 750
}}

NB: (In Venkman)
Use 'Stop for Errors' (or a BreakPoint),
but not 'Stop for Exceptions',
otherwise the Inbox folder is not selected and this error doesn't show up.

I hope the patch for (POP3) bug 442251 will fix this too: let's wait and see.

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> I hope the patch for (POP3) bug 442251 will fix this too: let's wait and see.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071115 SeaMonkey/2.0a1pre] (SEA-WIN32-TBOX-trunk) (W2Ksp4)

I was wrong to write that :-<

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <msgMail3PaneWindow.js>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.1pre) Gecko/2008070203 Thunderbird/3.0a2pre] (nightly) (W2Ksp4)

From the comment 1 stack analysis, the code at these 3 places is nearly-enough identical, so I don't know why TB doesn't show this bug.


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071115 SeaMonkey/2.0a1pre] (SEA-WIN32-TBOX-trunk) (W2Ksp4)

Anyway, adding a |CheckOnline()| call/test fixes it for SM.

NB: Fwiw, I tried to change and move SM
|setTimeout("loadStartFolder(gStartFolderUri);", 0);|
to TB
|loadStartFolder(gStartFolderUri);|
but that didn't help.

PS: If you find a better/underlying fix, you're welcome...

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071103 Thunderbird/3.0a2pre] (nightly) (W2Ksp4)

> I don't know why TB doesn't show this bug.

But, as a hint, TB (offline/online) selects the imap account itself whereas SM selects the Inbox folder...

Comment 5

[Karsten Düsterloh]

Comment on attachment 329176 [details] [diff] [review]
(Av1) <msgMail3PaneWindow.js>

>Index: msgMail3PaneWindow.js
>===================================================================
>         if (!initialUri && isLoginAtStartUpEnabled && gLoadStartFolder
>             && !defaultServer.isDeferredTo &&
>-            defaultServer.rootFolder == defaultServer.rootMsgFolder)
>+            defaultServer.rootFolder == defaultServer.rootMsgFolder &&
>+            CheckOnline())
>           defaultServer.performBiff(msgWindow);

The Offline() check should be second.
Also, for enhanced readability, put each condition on its own line and embrace ;-) the biff call.

r=me with that.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) <msgMail3PaneWindow.js>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071502 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Av1, with comment 5 suggestion(s),
plus (while there):

*Add a comment for |var gLoadStartFolder|.
*Remove a useless |var folderTree|.
*Remove superfluous |var isLoginAtStartUpEnabled|.
*Merge if's !
 *and fix a few nits nearby...
*Add a missing |defaultAccount = null;| !

Comment 7

[Karsten Düsterloh]

Comment on attachment 330515 [details] [diff] [review]
(Av1a) <msgMail3PaneWindow.js>

Nice!

>Index: msgMail3PaneWindow.js
>===================================================================
>+        // |initialUri| is null on startup.
>+        if (initialUri)
>             startFolderResource = RDF.GetResource(initialUri);
>         else
>         {

r=me with braces around the initial if branch.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b) <msgMail3PaneWindow.js>

Av1a, with comment 7 suggestion(s).

Comment 9

[Karsten Düsterloh]

We *do* require sr as long as noone says differently.
As you were told repeatedly... ;-)

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> We *do* require sr as long as noone says differently.

Indeed, sorry, didn't pay attention on this one :-(

> As you were told repeatedly... ;-)

Comment 11

[Serge Gautherie (:sgautherie)]

IIRC:
{{
[10:03]	<NeilAway>	sgautherie: actually I meant to look up my original rewrite of loadStartFolder
}}

Comment 12

[[e-mail]]

Attachment: Original rewrite

Comment 13

[[e-mail]]

Attachment: Proposed patch [Checkin: Comment 16]

* Removed useless folder tree variable
* Switched from RDF resources to URI strings
* If only news accounts, select news server account central by default
* Removed initialUri check when performing biff, since
  isLoginAtStartupEnabled can't be set if initialUri is.

Comment 14

[Karsten Düsterloh]

Comment on attachment 334878 [details] [diff] [review]
Proposed patch
[Checkin: Comment 16]

>+++ b/mailnews/base/resources/content/msgMail3PaneWindow.js	Thu Aug 21 13:33:40 2008 +0100
> function loadStartFolder(initialUri)
> {
...
>     var startFolderResource = null;

This one can go as well.

>+                if (inboxFolder)
>+                    initialUri = inboxFolder.URI;

No need to heed the 4 space indent in the leaf here.

>+        if (isLoginAtStartUpEnabled && gLoadStartFolder &&
>+            !defaultServer.isDeferredTo &&
>             defaultServer.rootFolder == defaultServer.rootMsgFolder)

Probably worth to put each && clause on its own line for enhanced readability?

Comment 15

[[e-mail]]

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

Sorry, we don't need this.

Comment 16

[[e-mail]]

Pushed changeset dd10c695faf9 to comm-central.

Comment 17

[Serge Gautherie (:sgautherie)]

V.Fixed between
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080907003401 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
and
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080908013336 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 334878 [details] [diff] [review]
Proposed patch
[Checkin: Comment 16]

(I'm checking for the parts of my patch which still apply.)

>+        SelectFolder(initialUri);
> 
>         // Perform biff on the server to check for new mail, except for imap
>-        // or a pop3 account that is deferred or deferred to,
>-        // or the case where initialUri is non-null (non-startup)
>-        if (!initialUri && isLoginAtStartUpEnabled && gLoadStartFolder
>-            && !defaultServer.isDeferredTo &&
>+        // or a pop3 account that is deferred or deferred to.
>+        if (isLoginAtStartUpEnabled && gLoadStartFolder &&
>+            !defaultServer.isDeferredTo &&
>             defaultServer.rootFolder == defaultServer.rootMsgFolder)
>           defaultServer.performBiff(msgWindow);
>-
>-        SelectFolder(startFolder.URI);

Does |performBiff()| need to be executed after |SelectFolder()| only ?
Or could it (still) be done before ... in the try+if block ?

Comment 19

[[e-mail]]

(In reply to comment #18)
> Does |performBiff()| need to be executed after |SelectFolder()| only ?
I think it's better this way.

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <msgMail3PaneWindow.js> additional doc and fix [Checkin: Comment 23]

Av1b, unbitrotted/merged,

and extended:
*Remove superfluous variables.
*Add/Update documentation.

Comment 21

[[e-mail]]

Comment on attachment 338872 [details] [diff] [review]
(Av2) <msgMail3PaneWindow.js> additional doc and fix
[Checkin: Comment 23]

>           if (gNotifyDefaultInboxLoadedOnStartup && (folder.flags & 0x1000))
Hmm, this 0x1000 should be nsMsgFolderFlags.Inbox - I thought I'd changed all of these in bug 436044...

>                 var inboxFolder = rootMsgFolder.getFolderWithFlags(0x1000);
Whoa, I missed another :-(

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> this 0x1000 should be nsMsgFolderFlags.Inbox

I filed bug 455549.

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 338872 [details] [diff] [review]
(Av2) <msgMail3PaneWindow.js> additional doc and fix
[Checkin: Comment 23]

"Hunk #3 succeeded at 765 with fuzz 2 (offset 2 lines)."

http://hg.mozilla.org/comm-central/rev/42f6a631882a