471130 - import Outlook Express (oexpress). folders appear but are empty

Status: RESOLVED FIXED

(Thunderbird :: Migration, defect)

Description

[Phil Lacy]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081225 Shredder/3.0b2pre

Same as Bug 455229 but patch is in place and still occurs however this is different in that folder is not empty in TB 2.

TB2 start up will populate the folder when clicked

Reproducible: Always

Steps to Reproduce:
1.TB3 import everyting oexpress
2.click on a folder and nothing shows up. no count no msgs.
3.exit open TB2 no count. click folders. count and msg appear as normal.
4.exit open TB3. that folder is now populated as normal.  Others still have problem.
5.repeat 3 to gain access to other folder msgs in TB3.




I have my own patch installed for importing but shouldn't affect this.  I don't know what the mechanism is to affect a populate when folder is clicked.
Please someone advise and I will debug.

Comment 1

[Phil Lacy]

on import the *.msf file is incorrect and TB2 corrects fixes it and TB3 doesn't.
There doesn't appear to be significant differences in the nsLocalMailFolder.cpp function nsMsgLocalMailFolder::UpdateFolder that is called on folderpane click.

if the offending *.msf folder is deleted TB3 with generate a proper one.  Somewhere in the import the *.msf file must be being created before the db file is in place.

Comment 2

[Phil Lacy]

Ouch this took a long time to track down. Need some advise on this
The TB2 stable release (version 2.0.0.18 (20081105)) works.

However this was changed in 1.92 of commandglue.js and that rev is not in the final release of this TB2. 	Bug 436718

This folder failure to open is caused by NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE.
 0x80550005 

The TB2 handles it in changefolderbyuri but it has been changed.

Comment 3

[Phil Lacy]

Rescind comment 2
the problem stems from fix for bug 437886

When a folder in folderpane is clicked this code is called
https://bugzilla.mozilla.org/attachment.cgi?id=353719&action=diff#a/mailnews/local/src/nsLocalMailFolder.cpp_sec1

and it use to have create=False. so the following code returned null db and the commandglue.js rebuilt the nullptr db.

https://bugzilla.mozilla.org/attachment.cgi?id=353719&action=diff#a/mailnews/db/msgdb/src/nsMsgDatabase.cpp_sec2

OEimport code has invalid db files.  Maybe the import can be fixed but nevertheless an invalid db is being left without being rebuilt.

Comment 4

[Phil Lacy]

fixed this for this problem but needs looking by the Bug 437886 people.

Comment 5

[Phil Lacy]

Attachment: rebuilds bad or out of date summary files

nsMsgLocalMailFolder::GetDatabaseWOReparse
in TB2 this function did an openfolderdb() with create=false and that caused it to return on err without filling in the mDatabase.

That null mDatabase then was in place when .js later did an UpdateFolder and caused a GetDatabaseWithReparse

In TB3 returned a bad mDatabase and of course was not reparsed.

Once in GetDatabaseWithReparse the test of out-of-date was removed for some reason.

This need to be put back in place or something done to rebuild invalid summary files.  the only way to do it is to delete the file and parse.

Comment 6

[Mark Banner (:standard8)]

The second part of your patch has been fixed in the last couple of days by bug 471307. As the fix is slightly different, does that negate the need for the first part of the patch, or do you still need that as well?

FWIW I think I've seen this importing from SeaMonkey 2.x into Thunderbird (but I'm not entirely convinced its the same bug).

Comment 7

[Phil Lacy]

Yes. A few comments re:

1. TB2 OpenFolderDB() passed in create=false which caused it to return and empty mDatabase with an error. So as a matter of upkeep we should take the same action here in case other callers to this function expect null db.

2. clicking a folder the *.js eventually gets here and if it get a mDatabase on error it sends it to UpdateFolder() and it only getdatabasewithreparse()'s a null mDatabase.

I was at least a week going back and forth with TB2 to figure out where this was.

Somebody on that Bug 437886 ought to check old TB2 code for all opens that had the create=false as it may have expected a mDatabase too.

I can modify this patch to include the || NS_ERROR_FILE_TARGET_DOES_NOT_EXIST or not. Whatever the others decide.

Also, I was going to find out about the importing of bad db's.  Maybe they don't get imported invalid with this fix.  But I say they shouldn't make a summary file anyway.  Especially since that may change.

Comment 8

[Phil Lacy]

Yes. A few comments re:

1. TB2 OpenFolderDB() passed in create=false which caused it to return and empty mDatabase with an error. So as a matter of upkeep we should take the same action here in case other callers to this function expect null db.

2. clicking a folder the *.js eventually gets here and if it get a mDatabase on error it sends it to UpdateFolder() and it only getdatabasewithreparse()'s a null mDatabase.

I was at least a week going back and forth with TB2 to figure out where this was.

Somebody on that Bug 437886 ought to check old TB2 code for all opens that had the create=false as it may have expected a mDatabase too.

I can modify this patch to include the || NS_ERROR_FILE_TARGET_DOES_NOT_EXIST or not. Whatever the others decide.

Also, I was going to find out about the importing of bad db's.  Maybe they don't get imported invalid with this fix.  But I say they shouldn't make a summary file anyway.  Especially since that may change.

Comment 9

[Phil Lacy]

I did an import with TB2 and it makes malformed summary files too.  I'm going to see about getting rid of summary files on import.

To clarify I meant to say in the above:
"Somebody on that Bug 437886 ought to check old TB2 code for all opens that had
the create=false as it may have expected a *NULL* mDatabase too."

Sorry about repeat now I now what to do in a midair collision.

Comment 10

[Phil Lacy]

Attachment: clarification. no effective change of other patch.

Again, to make it clear I added the comment that this function expects null db on error and changed the code to make that sense.
I did a quick scan of TB2 mailnews code and couldn't find another instance of this function being called with create=false.

Mark shouldn't someone change the .idl to advise the db is null on error. they added several comments in the original bug but not that notice.

I left the added test in the other function.  That other bug is closed.

Comment 11

[David :Bienvenu]

if the target doesn't exist, what's the point of trying to transfer info from the old db?

Comment 12

[Phil Lacy]

I had no real purpose to put it in except it was put there by others and I didn't have a reason to remove it or knew the consequences of removing it.  I was just needing the test for missing summary.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Adding some flags-n-stuff so that this doesn't get lost.

Comment 14

[David :Bienvenu]

Comment on attachment 355515 [details] [diff] [review]
clarification. no effective change of other patch.

maybe jcranmer can look at this...
is this the same case as when the user explicitly deletes the .msf file?

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

I actually meant blocking-thunderbird3+; fixing.

Comment 16

[Phil Lacy]

to comment 14:
I think Bug 471682 is related and told them this patch may fix it.

Comment 17

[Phil Lacy]

comment 14. isn't related or at least doesn't fix that bug

Comment 18

[Kent James (:rkent)]

I'm wondering if the change in the behavior of OpenFolderDB was intentional, compared to previous cases when aCreate was false (that is, that the mDatabase is updated instead of not changed, as described in comment 5, when the summary file is out of date). Wouldn't it be better to correct it there, rather than search through to find cases that were broken by the change? It also seems like that change recreated the problem that bug 437886 was trying to solve, that the return parameters are difficult to access in js when an error is returned.

Perhaps the change was done so that there was some method to access an out-of-date database. In any case, I think we need to rethink the changes in bug 437886 now that we see the regressions that are happening.

Comment 19

[Wayne Mery (:wsmwk)]

I see symptoms of this for a local folder that receives mail filtered from imap.  Should I stick with this bug, bug 471682, or should I file a new one?

Comment 20

[Phil Lacy]

Here's an option.

leave the bad db in and send back to js.
Fix the updatefolder to handle bad msf files.  That makes more sense since a db with bad or good summary file still exists and should not be null.

But 472446 depends on this fix and there may be more.

Comment 21

[Kent James (:rkent)]

Phil. I'm going to investigate preparing a patch to restore some of the old behaviour that was changed inadvertently in bug 437886. We had some discussions today on the maildev IRC channel, and it does not appear that these changes were intentional. (BTW it would be great if you could hang out on the mozilla maildev channel - you've got some great ideas and insights). All sorts of regressions are popping up, and it seems safer to restore the previous behavior as much as possible, rather than find and fix the places that the changes affect.

Comment 22

[Kent James (:rkent)]

I'm going to recommend at this point that Phil's patch be updated to only include the first part, and then be accepted. As I said in comment 21, I hope to prepare a more extensive patch to fix some other issues, but Phil's patch (when modified) is simple, clearly correct and fixes a regression from bug 437886. I'll want to do a more extensive patch, but we should fix this simple regression now.

Here's a brief analysis for the reviewers:

In openFolderDB, code was changed from:

  rv = msgDB->Open(folderPath, aCreate, aLeaveInvalidDB);
  if (NS_FAILED(rv) && (rv != NS_MSG_ERROR_FOLDER_SUMMARY_MISSING
    && rv != NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE || !aCreate))
    return rv;

to:

  rv = msgDB->Open(folderPath, PR_FALSE, aLeaveInvalidDB);
  if (NS_FAILED(rv) && rv != NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE)
    return rv;

nsLocalMailFolder::GetDatabaseWOReparse uses aCreate == false, so previously the test for returning the rv was just NS_FAILED(rv), but now it is:

NS_FAILED(rv) && rv != NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE

The msgDB->Open will return NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE when it detects that the database is invalid. Previously, OpenFolderDB returned immediately on that error, without changing the XPCOM returned value, so mDatabase was not affected in GetDatabaseWOReparse. But now if falls through to the section that sets the return value, so mDatabase is being set to the invalid database. OpenFolderDB must function this way, as that is now the only way to get access to an invalid database, which is sometimes needed for data recovery. But users of OpenFolderDB must now protect against this if they do not want the invalid database. GetDatabaseWOReparse does not do that currently, and Phil's patch adds the fix.

GetDatabaseWOReparse is the only place I could find in the old code where this issue occurs.

So Phil, can you please redo your patch to just include the first part, and resubmit it for review? Thanks for your work on this!

Comment 23

[Phil Lacy]

I can do that tonight. As a final comment from me though. It has been noted that there may be a condition where a caller wants the database back to extract out useful information and reparse.  (I believe that is correct.)
But I felt these .msf files were not just out-of-date, they were corrupt. So maybe (in future revisions) the deeper code should be returning another error message then out-of-date. I see where out-of-date was taken as in indication 'messages-may-be-coming-in-and-it-will-be-reparsed-by-other-handlers-so-do-nothing-here' type of thing.

Comment 24

[Phil Lacy]

Attachment: removed fix from 471307

Comment 25

[Phil Lacy]

Attachment: fixed the whitespace thing

Comment 26

[Kent James (:rkent)]

Comment on attachment 357596 [details] [diff] [review]
fixed the whitespace thing

I've already given my analysis and recommendation, and technically I'm not a reviewer, so I'll ask Bienvenu to look at this.

Comment 27

[David :Bienvenu]

I'll try this today. A comment, however - rkent asked me over irc if we want to put "invalid" databases in the db cache. I think we don't, because unless the caller deals with the invalid db, that invalid db will stay in the cache and all subsequent callers will get the invalid db directly from the cache, and think it's fine. I've been seeing that happening to me on the trunk. I don't know if that's related to the issue this patch is trying to address or not...

Comment 28

[Kent James (:rkent)]

The cache is only the second place that you usually look for the database. The first place is mDatabase on the folder object. This bug is about getting invalid databases in that object.

In spite of the many reports of database corruption problems, the number of reproducible issues has been few and far between. I think the issue solved in this bug is likely the mother of all database corruption problems as a regression from bug 437886.

Comment 29

[David :Bienvenu]

I agree with what you say - but I think the api should make it really hard to end up with an invalid database - you should have to be promising to fix it if it's invalid, by passing in a parameter to OpenDatabase. In other words, for local folders, only nsLocalMailFolder::GetDatabaseWithReparse should really be getting back the invalid db. bug 437886 made it a lot easier to get invalid db's, which is what led to so many problems.

I ran with this patch - after the first import from OE, I got a couple of assertions opening the imported inbox, and didn't see any messages. The next time I clicked on the inbox, I did see the message (I may have had to restart to see it). So I need to play with this a bit more.

Comment 30

[David :Bienvenu]

Comment on attachment 357596 [details] [diff] [review]
fixed the whitespace thing

this is the right thing to do - I haven't been able to reproduce the assertions I saw the first time. I'll land this - thx for the patch!

Comment 31

[Kent James (:rkent)]

Oh I agree that the api needs work. That's why I keep talking about the need for a larger bug. And the api inconsistencies are what make this code hard to work with and fragile - which is also why I would prefer we put off any major changes until post-beta2 if possible.

For example, the _MISSING error from ::Open can mean:

1) the .msf file did not exist, but I created one (from Open) and it is now open
2) the .msf file does not exist, and I did not open it (from Open) (because you did not set aCreate)
3) the .msf file existed but was corrupt or invalid so I deleted it, and it is not open (from Open)

So you cannot predict from that return status code directly either the prior condition of the database, nor the post condition (open or closed). The meaning is highly dependent both on which routine you call (Open, OpenMDB, or OpenFolderDB) and which call parameters that you gave it.

::Open is the most problematic of the routines, yet it was not the one touched in bug 437886.

Comment 32

[David :Bienvenu]

an other complication is that we want to retain information from "invalid" db's with your backup code, which means we can't simply blow away invalid db's whenever we find them, which historically was a great simplifying assumption.

Comment 33

[Kent James (:rkent)]

I'm willing to rethink that (trying to recover info from the message database). I'm beginning to think that a better architecture might be to maintain message metadata in a separate database, and truly have a message summary database that can be blown away at will. But it's not just true message metadata (independent of the message content) at issue. DBFolderInfo is also currently part of the .msf file isn't it, as well as things that are harder to characterize like keywords (which depending on the IMAP server, and/or the number of keywords, can be recoverable from the message).

Even before I got involved, there were various partially implemented mechanisms to recover information from the message database.

Comment 34

[David :Bienvenu]

yes, you're right that we want to maintain the db folder info stuff at the very least. I wouldn't give up on recovering info from the message database myself.

Comment 35

[David :Bienvenu]

thx for the patch, Phil.

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 357596 [details] [diff] [review]
fixed the whitespace thing

>+    if (NS_FAILED(rv))
>+      mDatabase = nsnull;   // comment to this function above states failure expects null db returned
>+    if (mDatabase)


It looks like this could have used an |else|...

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #31)
> 
> So you cannot predict from that return status code directly either the prior
> condition of the database, nor the post condition (open or closed). The meaning
> is highly dependent both on which routine you call (Open, OpenMDB, or
> OpenFolderDB) and which call parameters that you gave it.

Splitting _MISSING out into three separate codes sounds like it might be easy to fix, in which case it may not be worth waiting for a bigger cleanup to do.  In any case, filing a spinoff bug for one or multiple cleanups would be a great way to be sure we don't lose track of this.

Comment 38

[Kent James (:rkent)]

(In reply to comment #37)
>
> In any case, filing a spinoff bug for one or multiple cleanups would be a great
> way to be sure we don't lose track of this.

I have bug 459680 open (and blocking) which serves as a placeholder for database issues that I am working on, but it does not really fully reflect the nature of the issues, just one symptom. So I am not losing track of this, mostly because some method to reliably preserve message metadata is critical to the class of applications I am interested in.