Closed Bug 1935331 Opened 1 year ago Closed 1 year ago

Error message "could not be compacted because writing to folder failed." when compacting (non-empty) folder with corrupted or unexpected local storage file

Categories

(MailNews Core :: General, defect, P2)

Product:
MailNews Core
Component:
General
Version:
Thunderbird 128
Type:
defect

Tracking

(thunderbird_esr128 fixed, thunderbird134 affected)

Status:
RESOLVED FIXED
Milestone:
135 Branch
Tracking Flags:
Tracking Status
thunderbird_esr128 --- fixed
thunderbird134 --- affected

People

(Reporter: KaiE, Assigned: benc)

References

Details

(Whiteboard: ! uplift at same time as bug 1941646 ---- Test instructions in comment 43)

Attachments

(2 files)

Bug 1935331 - Add a unit test to illustrate that nsIMsgPluggableStore.asyncCompact() throws a NS_MSG_ERROR_MALFORMED_MBOX with bad mbox files. r=#thunderbird-back-end-reviewers
48 bytes, text/x-phabricator-request
corey
: approval-comm-esr128+
Details | Review
Bug 1935331 - Fix compact of IMAP folder with corrupted local storage. r=BenC
48 bytes, text/x-phabricator-request
corey
: approval-comm-esr128+
Details | Review

Inspired by bug 1935124 I was looking into additional potential failure scenarios.

Steps:

  • create a new IMAP folder
  • copy one small message to that IMAP folder
  • ensure you have local synchronization enabled (sync & storage settings, keep messages on this computer)
  • quit TB
  • find the corresponding file for that folder in your profile directory and open that file in a text editor
  • insert additional text at the beginning, e.g. an additional line that starts with the word "test"
  • save the file, quit editor
  • start Thunderbird
  • right click that folder and select "compact"

Actual behavior:
The following error is shown:
"folder ... could not be compacted because writing to folder failed. Verify that you have enough disk space, and that you have write privileges to the file system, then try again."

Expected behavior:
For IMAP mbox, we could automatically discard the whole mbox storage and automatically repair the folder.

For Local Folders, we probably should give the user a better error message. We should tell the user that their storage is corrupted and that they should restore a backup.

For Local Folders, we probably should give the user a better error message. We should tell the user that their storage is corrupted and that they should restore a backup.

Let's consider this a separate issue. Currently, when manipulating a local folder in that way, nothing happens. No error message shown. No message shown. Both repair and compact are silent and don't change anything.

Summary: Error message when compacting folder with corrupted local storage → Error message when compacting folder with corrupted or unexpected local storage file

Alessandro shared a mailbox file.

The file doesn't start with the expected "From " text.
Instead, it immediately starts with the usual email headers, in this case with a "Delivered-To:" header.

What should we do?
It seems we need to be more relaxed.

Thunderbird 115 accepted such files.

I'd like to understand how it's possible the file in that format exists.

Is it
(a) because an older (stable) release version created it?
(b) because an experimental (daily/beta) version created it?
(c) a cause of random corruption?
(d) unknown cause

I'm guessing that (c) is unlikely, and I'm guessing that it isn't (a).

Is it (b) ?

I assume that repairing helps.
Is it sufficient to tell our testers to repair in this scenario?

Is it possible that Thunderbird might have created "local folders" in this (now considered broken) format?
If yes, how do we help our testers to get out of that situation, because repair won't help?

It's (a).

I only use this profile specifically with ESR versions and I've been carrying since pre-115.
That specific folder was created this year in March, so still in the 115 cycle.

Version: unspecified → Thunderbird 128

Thanks Alex.

I've inspected the many test profiles that I have.
I have several ones that haven't been used in the past 9-11 months, and I'm certain they have been touched with 115 (or older) last.

Besides two exceptions, all my regular IMAP folders always have the expected "From " lines at the front of each message.
I don't have any regular mailbox file that looks like the example that you gave me.

The expection is two trash folders in different profiles.
In both Trash folders, several messages are missing the "From " prefix lines, including missing at the begining of the file.

(The previous messages end with the correct and expected trailing MIME separator line, then, the next following line is already the first email header line of the next mesage. There's no empty separator line in between.)

One of those Trash folders is in my primary work profile. Earlier Trash messages are missing the From, recent messages have it.

The other profile hasn't been used since November 2022.

Another observation regarding the file that Alex has sent to me.

The message file contains the same message twice, and nothing else is inside the file.

The file starts with the message (no From header), and ends correctly with the MIME boundary.

No empty separator line.
The next line immediately contains a "From - $date" line, following by two X-Mozilla-Status lines.
Then the exact same message follows again.

The msf file lists only one message, according to the dump tool I'm using.

The message has a storeToken and correctly points to the "From " line of the second entry.

We probably don't understand what caused this file structure.

However, it seems fair to conclude that in some situations, our code produced such files.
If those files can exist, I think we must do better than showing the error "cannot write to disk" that we currently show.

I think if the first message is missing the "From" line, but only of an IMAP folder, we should trigger an repair of the whole folder.

Please advise if you agree, and if yes, whether you want the (potentially slow) repair to happen silently, or whether you want to inform the user of the need to repair the folder.

Ben, Geoff, what do you think, see comment 7 ?

Flags: needinfo?(benc)
Flags: needinfo?(geoff)

(In reply to Kai Engert [:KaiE:] from comment #5)

The other profile hasn't been used since November 2022.

I can see that profile was used with a comm-central build, so could potentially have used bad code.

(In reply to Kai Engert [:KaiE:] from comment #5)

One of those Trash folders is in my primary work profile. Earlier Trash messages are missing the From, recent messages have it.

I had disabled "offline sync" for this account a few days ago.
I re-enabled it.
I waited for TB to download all the messages.
When done, I inspected the Trash file again, it was still missing the "From " at the beginning (no surprise).

Then I restarted, and told TB to compact the Trash folder.
As expected, I got the message from comment 0 ("could not be compacted because writing to folder failed").

This confirms, folders in that state can happen in production.
We need a sane way to handle them.

IMHO, either auto-repair, or tell the user to repair the folder.

Are there any drawbacks on silently triggering folder auto repair?
What happens if we trigger it and the user immediately shuts down the application?

It re-downloads all the mail in the folder, which usually would not be desired.

If the user shuts down while compaction is running, the compaction process is aborted.
In my past testing, it seemed to like it is somehow remembered that there's a need for compact, and I saw that compact was automatically re-attempted after restart.
I don't know if there is indeed a systematic guarantee that compact will be re-initiated.

(In reply to Kai Engert [:KaiE:] from comment #6)

Another observation regarding the file that Alex has sent to me.

The message file contains the same message twice, and nothing else is inside the file.

The file starts with the message (no From header), and ends correctly with the MIME boundary.

No empty separator line.
The next line immediately contains a "From - $date" line, following by two X-Mozilla-Status lines.
Then the exact same message follows again.

The msf file lists only one message, according to the dump tool I'm using.

The message has a storeToken and correctly points to the "From " line of the second entry.

I think this is probably a result of the new "self-healing" code which re-downloads messages if they can't be read from local storage.
So:

  1. old mbox file has no leading "From " line
  2. Alex goes to display that message
  3. new malformed-mbox code fails the read because it doesn't start with a "From " line.
  4. the "bad" message is marked not offline
  5. next time, the system sees the message is not offline and downloads a new copy, appends it to the mbox and correctly sets the storeToken.

So that's all good.... except...
Compaction fails because of the missing leading "From " line, so the bad message never gets cleared up.

I think there should be a relatively easy fix.
Compaction is a a sequential scan of the mbox file, rather than the usual random-access by which messages are usually read. So it seems OK to loosen the malformed-mbox check (the leading "From " requirement) for compaction.

I think the way to do it would be to have a "compaction" mode on MboxParser, where it goes into eExpectHeaderLine state instead of eMalformed state here: https://searchfox.org/comm-central/source/mailnews/base/src/MboxMsgInputStream.cpp#364
But it should probably only do that for the very first message in the mbox... as the rest of the messages must - by definition - have a "From " separator.

The special compaction mode would be set up in the nsMsgBrkMBoxStore::AsyncScan() function, which is what compaction and local (not imap) folder repair use to iterate through the mbox.

I don't mind trying this, but I've got a few other things to finish up first.

Flags: needinfo?(benc)

I think this is probably a result of the new "self-healing" code which re-downloads messages if they can't be read from local storage.

I'm not sure that's the case.
That folder is an old one that I haven't accessed since March of this year, and when I got the compaction error dialog I purposely avoided accessing the folder and loading the message to avoid accidental changes or self healing.

Ben, your suggestion from comment 14 wouldn't result in an acceptable remedy.

In the example folder that I have, I have "many messages without from" at the beginning of the mbox file.

With your suggestion to ignore the missing initial from line, the compact code would incorrectly treat the set of many initial messages as one big message, and would write that set as one new message into the compact-result mbox file. And you'd probably record that big size of the expected size of the message in msf.

Next, if the user clicked on that first message, we'd display that set of messages as one big message (including all the headers from the second message onwards), and our code wouldn't detect "bad read", because it would match the size recorded in the msf (as written by compact code).

This way, you'd lose the ability to notice that the folder storage is incorrect as a whole, and we'd lose the knowledge that a folder repair is justified.

Flags: needinfo?(benc)

(In reply to Kai Engert [:KaiE:] from comment #17)

In the example folder that I have, I have "many messages without from" at the beginning of the mbox file.

Oh.

OK, that's a bad mbox file, and there's no safe way to parse it if there aren't "From " lines separating the messages.
(You can use header lines as a heuristic to detect a new message, but has lots of potential to screw up valid messages and would likely do more harm than good).

Alex: how/when was this mbox file generated? It sounds like it's been around for a while? But I don't know of any point in TB history where we were generating mbox files without "From " lines.
Then again, the mbox data was being written in different ways all over the place, so I guess it's not surprising that there might have been a code path that didn't write a "From " line? If that's the case, the good news is that it's likely confined to IMAP (and not the standard code path, something more obscure, or this would be more common). And because it's IMAP, Kai's suggestion in Comment 7 would likely be the way forward...

Flags: needinfo?(benc) → needinfo?(alessandro)

how/when was this mbox file generated?

March 2024, during the 115 cycle.
This is from the profile I use exclusively with ESR.

Flags: needinfo?(alessandro)

Ben, we have two different example mboxes.

Alessandro's example has just one message.

It's my example mbox (a Trash folder) that has many messages.

(In reply to Kai Engert [:KaiE:] from comment #20)

It's my example mbox (a Trash folder) that has many messages.

So how/when was that one generated? I'm assuming it's IMAP, right?
I'm trying to figure out if it was a long-standing issue, or if it happened with the new mbox code (which would be worrying - the whole idea was to factor out the mbox code so that writing was done in only one place, and correctly, so it should be impossible to write to an mbox without the code automatically inserting "From " markers).

(In reply to Alessandro Castellani [:aleca] from comment #19)

how/when was this mbox file generated?

March 2024, during the 115 cycle.
This is from the profile I use exclusively with ESR.

OK, so that case must have been pre-mbox-refactor, which means there were code paths then that generated borked mbox files.
Poop.
On the bright side, it was probably just wrong for some specific code path for IMAP, so the comment 7 suggestion would do the trick.

The easiest fix would be just to make sure the compactor fail with an identifiable error code (probably NS_MSG_ERROR_MBOX_MALFORMED), and catch it here:
https://searchfox.org/comm-central/source/mailnews/base/src/FolderCompactor.cpp#717
The existing errors are just handled by telling the folder to pop up an error dialog. We could just pop up a message to tell the user we can't compact the folder until they perform a repair (a bit of a cop out).
I don't quite know how we ask the user and then safely kick off a folder repair within the context of the compaction... I suspect it'd be cleaner to do that further up the stack (as a general rule, I think doing any interactive GUI stuff from the C++ side tends to be a bad thing)

So, first off, how widespread an issue is this likely to be (do we have any other reports coming in?). Is it enough to just show a message prompting the user to do an IMAP repair themselves?

Also: do we know of any cases of this having happened in local folders?

In that case, I think the only option would be to add a second Folder Compactor, which used a combination of current offsets from the database and heuristics to guess at the message boundaries...

(In reply to Ben Campbell from comment #21)

(In reply to Kai Engert [:KaiE:] from comment #20)

It's my example mbox (a Trash folder) that has many messages.

So how/when was that one generated? I'm assuming it's IMAP, right?

The messages in that folder, which are missing From, are from 2023-10 and 2024-04

I've edited/shortened the previous comment to remove unnecessary quotes

(In reply to Kai Engert [:KaiE:] from comment #23)

(In reply to Ben Campbell from comment #21)

(In reply to Kai Engert [:KaiE:] from comment #20)

It's my example mbox (a Trash folder) that has many messages.

So how/when was that one generated? I'm assuming it's IMAP, right?

Yes, IMAP.

(In reply to Ben Campbell from comment #21)

The easiest fix would be just to make sure the compactor fail with an identifiable error code (probably NS_MSG_ERROR_MBOX_MALFORMED), and catch it here:
https://searchfox.org/comm-central/source/mailnews/base/src/FolderCompactor.cpp#717
The existing errors are just handled by telling the folder to pop up an error dialog. We could just pop up a message to tell the user we can't compact the folder until they perform a repair (a bit of a cop out).

Note that we need a fix for the stable 128 branch, and it's difficult to introduce new user interface strings, because the need for localization makes that complicated. We usually only add new strings in the primary development phases.

The risk is that any new string might be presented to users in english language, which they might not understand. This is on top of the general issue that users might have no idea how to perform the repair.

So, instead of saying "repair this folder" (and hope that they will find out how to do that), I think we should either automatically trigger the repair (we could have a simple message popup that says "corruption detected, will automatically repair folder), or, if we aren't comfortable triggering it automatically, we could bring up a prompt with very simple wording. "Corruption in local storage detected. Do you want to repair? ok/cancel".

I don't quite know how we ask the user and then safely kick off a folder repair within the context of the compaction... I suspect it'd be cleaner to do that further up the stack (as a general rule, I think doing any interactive GUI stuff from the C++ side tends to be a bad thing)

The compact code could post a new global event (that we define), plus a global handler that will watch and catch that event, and act upon it. The event would contain information about the affected folder.

So, first off, how widespread an issue is this likely to be (do we have any other reports coming in?).

I'm currently following a few bug reports where users still complain about confusing behavior and corruption. I need to check which bug it had - but yes - in one bug, someone complained they also saw exactly that message.

I think it doesn't matter whether it's one-thousand or 100-thousand. If Alessandro, myself, and one more user is affected, we should assume we need to help users to get this sorted.

Is it enough to just show a message prompting the user to do an IMAP repair themselves?

See above, I would prefer to assist the user in getting the repair triggered, either automatically, or with an offering "yes/cancel".

P2 ?

Priority: -- → P2
See Also: → 1900750
Summary: Error message when compacting folder with corrupted or unexpected local storage file → Error message "could not be compacted because writing to folder failed." when compacting folder with corrupted or unexpected local storage file

(In reply to Kai Engert [:KaiE:] from comment #26)

The compact code could post a new global event (that we define), plus a global handler that will watch and catch that event, and act upon it. The event would contain information about the affected folder.

I agree with everything you wrote, but I just wanted to emphasise that at the lower levels the issue should be detected and passed up as an error code (NS_MSG_ERROR_MBOX_MALFORMED is the likely candidate), and that error should be handled where all the other compaction errors are https://searchfox.org/comm-central/source/mailnews/base/src/FolderCompactor.cpp#717 - so that's where the global event would be generated. I want to keep anything gui-related out of the lower level stuff.

I'll have a look at how to get that error code generated if someone else wants to have a go at the event handler to run the repair?

Late to the party, but are we sure we don't already have NS_MSG_ERROR_MBOX_MALFORMED? Telemetry reports that there are compactions with that as the result status. We only have specific error messages about NS_ERROR_FILE_NO_DEVICE_SPACE and NS_MSG_FOLDER_BUSY and everything else is a generic message. (This should be changed.) The only way to know is if logging happens to be enabled. (This should also be logged to the error console.)

Flags: needinfo?(geoff)
Assignee: nobody → benc
Status: NEW → ASSIGNED

(In reply to Geoff Lankow (:darktrojan) from comment #29)

Late to the party, but are we sure we don't already have NS_MSG_ERROR_MBOX_MALFORMED?

Yes - you're right - the lowlevel compact code already spits out NS_MSG_ERROR_MBOX_MALFORMED if the mbox is borked. Hooray!

That patch I just uploaded adds a test to confirm that and adds an explicit NS_MSG_ERROR_MBOX_MALFORMED case to the error handling where the invitation to repair would be triggered.
So I'm going to leave it at that and let someone else deal with UI and whatnot :-)

Assignee: benc → nobody
Status: ASSIGNED → NEW
Assignee: nobody → benc
Status: NEW → ASSIGNED

If we know it's an IMAP folder we're compacting, and we come across what we think is a corrupt message, clearing the offline flag should be enough to trigger a new download once compaction is complete, no?

I do not know if it helps, I came across this error since I updated to 128.5.2esr and start receiving this error when I compact the root folder, so I rightclick on my IMAP name and it gives the error when it hits the trash can, the trash can is empty.
If I fill the trash can, it compacts when I compact the same way, if I empty the trash can, and compact the same way, NO error.
If I leave TB and go back, and then compact, I get the error.
If I just rightclick on the trash can folder it gives no error.

I have 6 different mail profiles on this pc, 6 different providers. All the accounts have this same problem when I press on compact. Not all of those 6 can be corrupted after 1 update?

(In reply to Geoff Lankow (:darktrojan) from comment #32)

If we know it's an IMAP folder we're compacting, and we come across what we think is a corrupt message, clearing the offline flag should be enough to trigger a new download once compaction is complete, no?

No.

Downloading a new copy of a corrupted message doesn't resolve the issue, because it doesn't fix the bad data at the beginning at the mailbox file.

As long as we have non-"From " data at the beginning of the file, compact continues to fail with that error message.

(In reply to Geoff Lankow (:darktrojan) from comment #32)

If we know it's an IMAP folder we're compacting, and we come across what we think is a corrupt message, clearing the offline flag should be enough to trigger a new download once compaction is complete, no?

Compact doesn't complete.

It aborts when it sees that bad data.

(In reply to Geoff Lankow (:darktrojan) from comment #32)

If we know it's an IMAP folder we're compacting, and we come across what we think is a corrupt message, clearing the offline flag should be enough to trigger a new download once compaction is complete, no?

Like Kai said. The compaction can't compact such a borked mbox.
Without message separators you just can't safely parse an mbox file. I wrote the parsing to be pretty tolerant of all kinds of atrocities, but this one is just a step too far (it's not even an mbox file).

Background:

The compaction currently works using nsIMsgPluggableStore.asyncScan().
asyncScan() iterates through any messageStore (mbox or maildir) and invokes a listener callback for each message found.
The compactor uses that callback to decide whether to write that message to the new mbox, or to discard it.
(asyncScan() is also used by the local folder parse routine - to (re)build the database from a msgStore).

Alternative fix:

We could rewrite the compaction to work "inside out", instead of using asyncScan(). That is, we get a list of messages from the database, and read each one in turn, writing it out to the new mbox file. As long as the the messages are sorted by file offset, read access to the source mbox file is still sequential (I want to keep performance in mind). And whenever we hit a bad message (no "From " at the start), we can flag it as not offline and let it be re-downloaded next time it's accessed.
This might even give a speed boost, because we skip reading messages we're ditching (we don't have to scan them to find where the next one starts).
The old compactors kind of did this. But not deliberately. Just because they were a code jumble, tangling up all the different systems.

I kind of think we should do this: I think it's slightly more robust, and it fixes this problem as a side effect.

But we probably shouldn't do it right now. It'd be an easy change (the compaction code is nicely decoupled and easy to work with these days), but it's still a non-trivial change.

So I think we should:

  1. maybe do this after the holiday slowdown.
  2. implement the repair-folder hack as a workaround until then.

Regarding Ben's suggestion from comment 37 to do a repair based on the existing msf/index file.

My initial reaction is fear of new regressions. We know we have scenarios were the index is file is corrupt, I'm worried that changing the logic to use the index file as the primary source of information could introduce new angles for failures.

Have we ever seen a local folder or pop3 file which didn't have "from " line separators?

(In reply to Kai Engert [:KaiE:] from comment #38)

Regarding Ben's suggestion from comment 37 to do a repair based on the existing msf/index file.

My initial reaction is fear of new regressions. We know we have scenarios were the index is file is corrupt, I'm worried that changing the logic to use the index file as the primary source of information could introduce new angles for failures.

If a message has a bad storeToken, then we don't have a viable local copy of that message anyway. In such cases, the only thing we can do is mark the message not offline so it'll be redownloaded. Same as repair folder, but with the actual downloading only required for bad messages, and deferred until the next time the message is needed (or a more general offline-sync operation happens).

(not quite true: in this case with the mbox missing "From " separators, we do have valid storeTokens, but because we're lacking "From " separators we can't distinguish between valid and invalid messages anyway).

(In reply to Kai Engert [:KaiE:] from comment #39)

Have we ever seen a local folder or pop3 file which didn't have "from " line separators?

Not that I know of (comment #22).
But that is the question which could drastically alters the priority here.
I'm hopeful it's just an IMAP issue: In the old code, everything was responsible for writing out it's own "From " lines. Since we see the issue on IMAP, I'm assuming there was some obscure IMAP code path that didn't write "From " lines. It's unlikely that an IMAP-specific code path would have been used by other protocols. The protocols are all rather.... bespoke.

(In reply to Ben Campbell from comment #31)

That patch I just uploaded adds a test to confirm that and adds an explicit NS_MSG_ERROR_MBOX_MALFORMED case to the error handling where the invitation to repair would be triggered.
So I'm going to leave it at that and let someone else deal with UI and whatnot :-)

Done. We don't have good strings to reuse.
Maybe it's fine to simply trigger automatic repair.
Implemented in https://phabricator.services.mozilla.com/D232415

Test instructions.

prepare and reproduce bug, use a build without the fix

use imap account, account settings, sync, keep messages on local computer
create a new imap folder
copy 10 messages to that folder
click folder
wait a few moments for folder to sync
quit tb

go to profile directory, imapmail, edit file for that folder
at the beginning, add an additional line containing "test"
save, quit editor

start tb
click that folder
the first message, ensure that it loads (will auto repair)

click the last message
delete it

right click folder
compact

you will get error message -> bug

folder display might be empty -> bug
click another folder, then go back, message is visible again

quit tb

if you want, start tb again, try compact again, same problem

start newer tb with the fix

open error console
ensure debug category is clicked

right click compact
-> no error message
-> error console should report "caught folder-needs-repair"
-> folder will be refreshed and show message list again

click on trash icon in error console

again right click compact
-> no report in error console

quit tb
inspect file, must start with "From " line

Whiteboard: Test instructions in comment 43
Keywords: checkin-needed-tb
status-thunderbird134: --- → affected
status-thunderbird_esr128: --- → affected
Target Milestone: --- → 135 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/be3ebae65e60
Add a unit test to illustrate that nsIMsgPluggableStore.asyncCompact() throws a NS_MSG_ERROR_MALFORMED_MBOX with bad mbox files. r=darktrojan
https://hg.mozilla.org/comm-central/rev/f1bfc518afe7
Fix compact of IMAP folder with corrupted local storage. r=BenC

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
Duplicate of this bug: 1938498

I suspect this has upset comm/mail/base/test/browser/browser_repairFolder.js on Windows
https://treeherder.mozilla.org/jobs?repo=comm-central&selectedTaskRun=XaFlJsKGRCC-jVTGBZl4iQ.0

No longer duplicate of this bug: 1938498

(In reply to Magnus Melin [:mkmelin] from comment #48)

I suspect this has upset comm/mail/base/test/browser/browser_repairFolder.js on Windows
https://treeherder.mozilla.org/jobs?repo=comm-central&selectedTaskRun=XaFlJsKGRCC-jVTGBZl4iQ.0

Is this still a problem? I don't see this failure on latest builds.

Flags: needinfo?(mkmelin+mozilla)

Are we ready to uplift to 128 ?

Summary: Error message "could not be compacted because writing to folder failed." when compacting folder with corrupted or unexpected local storage file → Error message "could not be compacted because writing to folder failed." when compacting (non-empty) folder with corrupted or unexpected local storage file

I haven't seen the failure recently.

Flags: needinfo?(mkmelin+mozilla)

There was no beta exposure yet. And the next beta branch switch is in 3 days, on Monday.

I guess we want to wait a little longer, until this gets some beta testing.

See Also: → 1926832

There's one detail I didn't think about earlier. We might want to limit this automatic behavior to IMAP folders, because a repair of a corrupted local folder could potentially result in dataloss?

Ben, Magnus, what do you think?

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(benc)

The check could be added before sending the folder-needs-repair event.

I wouldn't think there is dataloss? Probably still best to repair local folders as well. Without repair, the folder does have inaccessible data from the point of view of the user, and there's not much users in general could do to fix that.

Flags: needinfo?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #56)

I wouldn't think there is dataloss? Probably still best to repair local folders as well.

I remember we had seen issues where a repair of a corrupted folder resulted in messages being lost.
I believe the risk of dataloss is real.

Without repair, the folder does have inaccessible data from the point of view of the user, and there's not much users in general could do to fix that.

No, it is possible that the user cannot compact the folder (because of this issue at the beginning), but besides that, the user may be able to access the messages in the folder (because for many messages the index might point to a valid message).

In that situation, repairing would be fatal, because repairing destroys the messages.

Without repairing, if the user notices that messages are gone, the user might use manual inspection to read or recover the old messages, that are still present in the file.

Because this bug is already fixed in 135, and c-c has already moved to 136, I will not reopen, but rather file a follow-up bug.

I'll not ask for uplift until we're certain my comment 57 is wrong or irrelevant, OR, we have a fix for my concern in comment 57.

Blocks: 1941646

(In reply to Kai Engert [:KaiE:] from comment #57)

No, it is possible that the user cannot compact the folder (because of this issue at the beginning), but besides that, the user may be able to access the messages in the folder (because for many messages the index might point to a valid message).

Just to clarify, this is the case where a message starts half-way through another one, leaving an invalid mbox file eg:

From MAILER-DAEMON Fri Jul  8 12:08:34 2011
From: Author <author@example.com>
To: Recipient <recipient@example.com>
Subject: Sample message 1
 
This is the body.
Another line starts here... but...From MAILER-DAEMON Fri Jul  8 12:08:34 2011
From: Author <author@example.com>
To: Recipient <recipient@example.com>
Subject: Sample message 2
 
This is the second body.

In this case, repair cannot detect the second message - a correct reading of this mbox will yield just one single message (the whole thing).
The database (probably) contains the offsets of both messages. So we potentially know where the both messages begin.

Repair doesn't use the database (that's the whole idea - it builds a database from the mbox when the db is missing or damaged).
But in theory compact could use the offsets in the database (if we trust them!)

But:

The compaction is failing because the mbox file is invalid (because there is no "From " at the start of the file).
Local repair works by scanning through the mbox file.
So local repair will also fail.

IMAP repair is fine as it just ditches the mbox file and re-downloads.

So yeah, I guess we should only generate the "folder-needs-repair" event for IMAP folders. Sigh.
In any case I don't think we've seen any cases of this situation arising on local folders anyway.

Flags: needinfo?(benc)
See Also: → 1941770
Whiteboard: Test instructions in comment 43 → ! uplift at same time as bug 1941646 ---- Test instructions in comment 43

Comment on attachment 9443076 [details]
Bug 1935331 - Add a unit test to illustrate that nsIMsgPluggableStore.asyncCompact() throws a NS_MSG_ERROR_MALFORMED_MBOX with bad mbox files. r=#thunderbird-back-end-reviewers

[Approval Request Comment]
Regression caused by (bug #): folder compact work
User impact if declined: error shown for a corrupted folder, when rather we should auto-fix the issue
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): In certain corruption scenarios (when inconsistent data is found at the beginning of a local IMAP storage/cache file), we trigger an automatic repair of that file. Because this triggers an automatic download of the whole folder from the IMAP server, this can have a noticeable effect for users. But we believe this is better than simply showing an error message and doing nothing. The risk is that users with larger mailbox will experience a re-download and bandwidth use in that scenario.

Attachment #9443076 - Flags: approval-comm-esr128?

However, for users who have disabled full IMAP sync, we'll only re-download the headers for the folder.

Please uplift this at the same time as bug 1941646, to ensure this action is limited to IMAP folders. We don't want to auto-repair local folders.

Note that one of the patches is almost a no-op, but it's necessary as a base patch for the second patch.

Attachment #9444217 - Flags: approval-comm-esr128?

Comment on attachment 9443076 [details]
Bug 1935331 - Add a unit test to illustrate that nsIMsgPluggableStore.asyncCompact() throws a NS_MSG_ERROR_MALFORMED_MBOX with bad mbox files. r=#thunderbird-back-end-reviewers

[Triage Comment]
Approved for esr128

Attachment #9443076 - Flags: approval-comm-esr128? → approval-comm-esr128+

Comment on attachment 9444217 [details]
Bug 1935331 - Fix compact of IMAP folder with corrupted local storage. r=BenC

[Triage Comment]
Approved for esr128

Attachment #9444217 - Flags: approval-comm-esr128? → approval-comm-esr128+
See Also: → 1959858
See Also: → 1960105
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: