1926832 - Notify the user if a mail storage folder seems corrupted and should be repaired

Status: RESOLVED WONTFIX

(MailNews Core :: Database, enhancement)

Description

[Kai Engert [:KaiE:]]

If a mailbox is corrupted, it should be repaired.

Users might not be aware of the need to repair,
and they might not know HOW to repair.

Suggestion is to:
(1) implement the backend code to detect corruption, and start by reporting it in logs
(2) implement a user visible notification when corruption of a folder is detected
(3) enhance the notification with a button that triggers repairing

Comment 1

[Kai Engert [:KaiE:]]

(4) add a telemetry probe, so we know how frequently users are affected by corruption

Comment 3

[Kai Engert [:KaiE:]]

Let's handle the backend part in bug 1926810, and use this bug here for the UI part.

Comment 4

[Toby Pilling [:tobyp]]

Ben is this something that you can do or would you like UI help?

Comment 5

[Ben Campbell]

I'm not yet totally convinced we should be popping up UI here.

Bug 1926810 adds a couple of things:

1. It throws an error when trying to read a message from an mbox which doesn't start exactly on a "From " separator
2. It reports cases where this happens via telemetry (and also where the mbox read obviously overruns the expected size - a check added in Bug 1924291)

It'll be interesting to see what 2 reports.

For 1, IMAP code at least should catch mbox read errors and mark the borked message as offline (I just wrote that up in Bug 1929105).

Comment 6

[Francesco]

It throws an error when trying to read a message from an mbox which doesn't start exactly on a "From " separator

How does the user experience this? The message is not displayed? The preview goes blank or the previous message display remains?

Comment 7

[Ben Campbell]

(In reply to Francesco from comment #6)

It throws an error when trying to read a message from an mbox which doesn't start exactly on a "From " separator

How does the user experience this? The message is not displayed? The preview goes blank or the previous message display remains?

It's not just display - it's anytime you're reading from an mbox. For example, copying a message to another folder: The main source of problems we've had recently is where a storeToken is pointing to rubbish. Previously, copying that message would have happily written out whatever random data at that location into the new copy, and so the problems spread. You end up with a well-stored message (i.e. correct "From " separator, correct offlineMessageSize) that consists of garbage. This should stop that happening.

For message display: off the top of my head I'm not sure. From memory I think it goes blank, but I could be wrong. In any case, the IMAP code has never really handled such errors in any rigorous way, so Bug 1929105 is there to improve that state of affairs.

Comment 8

[Kai Engert [:KaiE:]]

Ben, are you saying that you expect that everything will self-repair automatically?

If it doesn't self-repair, then users need to know what action they should take. Especially users who have never heard about the folder repair functionality.

Comment 9

[Ben Campbell]

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

Ben, are you saying that you expect that everything will self-repair automatically?

If it doesn't self-repair, then users need to know what action they should take. Especially users who have never heard about the folder repair functionality.

Yes. If we can detect that a message is borked, then clearing the Offline flag (and clearing storeToken and offlineMessageSize to be tidy), should have the effect of making sure it is fetched from the IMAP server next time it's accessed. The old (damaged) version of the message is still in the mbox file, but nothing is pointing at it and it'll be garbage-collected next compaction. A new copy of the message will be added to the mbox as part of being fetched from the server.

Comment 10

[betterbird.project+18]

Note that currently mailbox parse errors are just getting lost here:
https://searchfox.org/comm-central/rev/b022a46917103ccf292217e8f5a65ba8fbeca2fe/mailnews/local/src/nsMsgBrkMBoxStore.cpp#1422-1426

The CTOR of MboxMsgInputStream detects the parse error, but the function returns NS_OK anyway. We've added this patch to dump out information when a parse error happens.

Running with this patch, we haven't seen any real parse errors, but moving an incoming POP message with a filter to a large folder, creates a fake parse error every time. Here is some redacted output:

Mbox parsing error at offset 127049244(127049244) on D:\MAIL<snip>\Mail\Local Folders\XXX nsMsgBrkMBoxStore.cpp:1364
Mbox parsing error for folder mailbox://nobody@Local%20Folders/XXX looking for key=201966, date=<snip>, subject='<snip>', messageID=<snip> nsMsgDBFolder.cpp:811

The message in question displays totally fine in the target folder, the offset into the file is 933,750,395. So the offset retrieved from the storeToken here
https://searchfox.org/comm-central/rev/b022a46917103ccf292217e8f5a65ba8fbeca2fe/mailnews/base/src/nsMsgDBFolder.cpp#749
is totally wrong, it's in fact the offset of the Inbox from which the message got moved.

As far as we understand POP processing, a message moved by a filter shouldn't be incorporated into the POP inbox at all but streamed straight into the target folder. In any case, the message can be found in the POP inbox at offset 127049244.

This looks like a smoking gun, since it moved messages carry a stale storeToken with them that points into the source folder, some bizarre errors are imaginable.

Comment 11

[betterbird.project+18]

We filed bug 1930511 for the "move message by filter issue".

Comment 12

[betterbird.project+18]

We noticed many fake parse errors at offset 12345678 when body-searching a locally downloaded newsgroup. Looks like this offset was stored in the .msf from then time when this code was active (now removed):
https://hg.mozilla.org/comm-central/rev/8c44f53caece#l1.52
https://hg.mozilla.org/comm-central/rev/d9da220889f6#l8.32 (removal)
A repair of the .msf (redownload of the NG) fixed the issue.

Comment 13

[Kai Engert [:KaiE:]]

I think we have moved away from the plan to notify the user.
I suggest we close this bug as wontfix, please comment if you disagree.

We have implemented self-healing for individual corrupted messages in IMAP folders, and we're working on automatic folder repair in bug 1935331.

Comment 14

[Toby Pilling [:tobyp]]

It seems that bug 1935331 is closed so I can't comment there - but we would be happy to implement a method to notify a user that a folder repair is advisable and allow them to trigger the repair at a time that suits them. Would this help Ban?

Comment 15

[Ben Campbell]

(In reply to Toby Pilling [:tobyp] from comment #14)

It seems that bug 1935331 is closed so I can't comment there - but we would be happy to implement a method to notify a user that a folder repair is advisable and allow them to trigger the repair at a time that suits them. Would this help Ban?

I don't really have any opinion on popping up notifications to the user (and tbh, I don't know about the GUI side of things).
But bug 1935331 added a 'folder-needs-repair' event, which triggers an automatic repair, so I guess that's where put code to ask the user first?

Comment 16

[Magnus Melin [:mkmelin]]

Agree with Kai comment 13. -> WONTFIX