498819 - Exception in nsIMsgFolder.msgDatabase when "no msf" condition. After it, rebuild-Index is not invoked, Account Central is displayed for all mail folders ("0x80550006 [nsIMsgFolder.msgDatabase]" at dbViewWrapper.js :: DBViewWrapper_open :: line 762)

Status: VERIFIED FIXED

(MailNews Core :: Database, defect)

Description

[WADA:World Anti-bad-Duping Agency]

If "no .msf condition" exists, internal rebuild-index is not invoked properly.

[ Build ]
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090615 Shredder/3.0b3pre

[Steps to reproduce]
(1) Delete XXX.msf for mail folder XXX, and restart Tb.
    (emulation of problem like bug 498814, Bug 498817)
(2) XXX.msf of file_size=0 is created
(3) Mouse over on XXX => next exception. XXX.msf is deleted.
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80550006 [nsIMsgFolder.msgDatabase]"  nsresult: "0x80550006 (<unknown>)"
> location: "JS frame :: chrome://messenger/content/mailWidgets.xml :: parseFolder :: line 1951"  data: no]
(4) Left click of folder XXX
    => Thread pane is not displayed (Same as display or accout)
    => next exception.
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80550006 [nsIMsgFolder.msgDatabase]"  nsresult: "0x80550006 (<unknown>)"
> location: "JS frame :: file:///C:/wada/DOWNLOAD/@Mozilla/Thunderbird/@06-15/thunderbird/modules/dbViewWrapper.js :: DBViewWrapper_open :: line 762"  data: no]
(5) XXX.msf is not created.

To recover form above, manual Rebuild-Index followed by restart of Tb was required.

Comment 1

[WADA:World Anti-bad-Duping Agency]

I couldn't  see above exception with 2009/6/08 build.
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090608 Shredder/3.0b3pre
This bug is one of regressions by fix for bug 474701?
(See dependency tree for meta Bug 497199)

Comment 2

[WADA:World Anti-bad-Duping Agency]

After the exception, next phenomenon also occurs.
> Account Central is displayed for all mail folders. 
It's same symptom as bug 498145 which is listed in dependency tree for Bug 497199.
Putting 497199 in Blocks: field. Please remove if it's wrong.

Comment 3

[WADA:World Anti-bad-Duping Agency]

Phenomenon of "no automatic rebuild-index" and "Account Central is displayed for all mail folders" were also observed after next failure of (b).
(outdated msf condition, forced by mail folder file length change).
If manual Rebuild-Index is executed after (a) before left-click of the folder, i.e. before failure of (b), no problem occurred.

(a) On mouse over at folder pane
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80550005 [nsIMsgFolder.msgDatabase]"  nsresult: "0x80550005 (<unknown>)"
> location: "JS frame :: chrome://messenger/content/mailWidgets.xml :: parseFolder :: line 1951"  data: no]

(b) Upon left-click of folder at folder pane.
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80550005 [nsIMsgFolder.msgDatabase]"  nsresult: "0x80550005 (<unknown>)" 
> location: "JS frame :: file:///C:/wada/DOWNLOAD/@Mozilla/Thunderbird/@06-15/thunderbird/modules/dbViewWrapper.js :: DBViewWrapper_open :: line 762"  data: no]

Comment 4

[David :Bienvenu]

I'll look at this - I'm also seeing that rebuild index from the properties dialog doesn't cause the view to refresh itself, if the folder is open. Perhaps that's related.

Comment 5

[David :Bienvenu]

marking blocker.

Comment 6

[David :Bienvenu]

Attachment: patch explaining the problem

this patch shows what's going on - we're failing to open the db, and do all the setup we do once we've opened the db. This patch sets a flag when that happens and makes us figure out all that stuff again after we get the folderLoaded notification. It does fix the bug, but I need to do a patch where we don't duplicate all that code.

This patch doesn't fix the rebuild index case, though I think it's most of the way there...

Comment 7

[David :Bienvenu]

Attachment: proposed fix with mozmill test

basically, I extracted out the code that sets up the view from info in the db so that we can use it in the delayed load case as well. I'm not sure I factored out the code correctly - a lot of the virtual folder stuff probably doesn't apply so much, but it probably doesn't hurt to have it factored out either.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 385495 [details] [diff] [review]
proposed fix with mozmill test

While I'm not going to hold you to my potentially excessive level of commenting, I think some commenting is in order :). Your change to shouldShowMessagesForFolderImmediately definitely requires that the comment for the method be updated to be consistent with the code, even if your solution is just to nuke the verbose explanation of the logic.

As I understand the patch, the problem in a nutshell is that if the database does not exist, we do not get a database.  We also get an exception, so even if the exception did not kill us, the following logic block which must run to set variables will not run.

As far as I can tell, in the "database does not exist" case, there is no information benefit from deferring retrieving the information until the folder loads.  Our net benefit is that we can retrieve the dbFolderInfo which will have the default values available for us.

I'd be tempted to just explicitly encode the defaults in the dbViewWrapper so we can avoid the additional state permutation unless either of the following is true:

1) The rebuild index case will provide us with valid user-data propagated from the previous database that would not be available to us before that time.

2) Async database loading would require us to defer getting at the dbfolder info until the database is loaded, which is obviously async.  (This seems likely and reasonable.)

If either of those is true, I would suggest that we use _underlyingFolders == null as the equivalent invariant to _delayLoad.  Fewer variables to get into odd states.

Your mozmill test claims that it tests rebuilding the index but does not appear to do so.

I'm not sure the mozmill test actually needs to be a mozmill test.  Especially without the index rebuilding, the test could easily be an xcpshell test.  I could see an argument for rebuild index having a mozmill test since it makes sense to test the button actually kicks off the index rebuilding.  However, in that case I think more assertions and such would be required in that case.

Comment 9

[David :Bienvenu]

There are two cases where we don't get a db - one where the db is missing, but one where it's just not valid (e.g., the local mail folder has changed). In the latter case, we can usually pull all the interesting meta data out (e.g., the current sort, view flags, etc).

Yeah, I haven't got to the rebuild index case - it's less critical than the missing/invalid .msf file case, because you can click away and click back, whereas with a missing .msf file, you're effectively hosed forever w/o some sort of change.

Comment 10

[David :Bienvenu]

I'll tweak the patch to use _underlyingFolders - less variables === good.

Comment 11

[David :Bienvenu]

Attachment: fix addressing comments

I've added some comments, and got rid of the extra variable. I also tweaked the moztest to demonstrate that we can restore the sort order from an invalid db.

I do intend to fix the rebuild index issue, and add it to the moz-test, but this is higher priority...

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 385521 [details] [diff] [review]
fix addressing comments

Rich review:
http://reviews.visophyte.org/r/385521/

Exported review:

Nice touch on also checking the sort propagation!


on file: mailnews/base/src/dbViewWrapper.js line 794
>     try {
>       msgDatabase = this.displayedFolder.msgDatabase;
>     } catch (e) {}

Could you add your comment on the bug of "There are two cases where we don't
get a db - one where the db is missing, but
one where it's just not valid (e.g., the local mail folder has changed)."
here?


on file: mailnews/base/src/dbViewWrapper.js line 809
>     if (this.shouldShowMessagesForFolderImmediately()) {
>       this._enterFolder();
>     }

You no longer need the braces here since you killed the variable.


on file: mailnews/base/src/dbViewWrapper.js line 917
>               this._underlyingFolders == nsnull ||

While this might actually work out, I suggest using null instead of nsnull.


on file: mailnews/base/src/dbViewWrapper.js line 920
>   prepareToLoadView:

Please rename this to _prepareToLoadView since outside callers should not call
this.

Also, one more comment here, please.  Something like "load information on the
view from the message database's dbFolderInfo".  It may seem silly when the
code is visible but I'm close to having some documentation tooling available,
and even a concise summary is invaluable to find ones way around that.

Comment 13

[David :Bienvenu]

Attachment: address second round of comments...

carrying forward r+, requesting sr for the dbviewwrapper.js changes from Standard8.

It is nice to have a test for handling invalid/out of date .msf files since it's  just about everyone's favorite thing to break :-)

Comment 14

[David :Bienvenu]

fix checked in - I'm going to leave the rebuild index issue for an other bug (maybe already filed? If not, I'll file a new one)

Comment 15

[WADA:World Anti-bad-Duping Agency]

Checked with next build.
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090630 Shredder/3.0b3pre
Even if ".msf" is deleted and exception is detected, thread pane was not corrupted. Simply "no mail display at thread pane for the folder" was observed, and "manual rebuild-index" was effective recovery procedure.
=> VERIFIED/FIXED

I haven't opened separate bug for "automatic rebuild-index". Please file new one.

Comment 16

[WADA:World Anti-bad-Duping Agency]

After delete of .msf and restart of Tb, Tb 2009/7/06 build executed internal rebuild-index sucessfuly and thread pane was properly displayed. 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090706 Shredder/3.0b3pre
Which bug has resolved "rebuild-index" part?

Note: Next problem still remains.
 - Manual rebuild-index clears thread pane display.
   Folder re-open is required to get proper thread pane display gain.