1929105 - (imap-corruption-self-healing) IMAP code should handle local storage read errors by marking message not offline.

Status: VERIFIED FIXED

(MailNews Core :: General, defect)

Description

[Ben Campbell]

IMAP uses local storage (mbox, maildir) as a sort of cache, and will use the local copy in preference to streaming the message down from the server. Which is what you'd expect.

But it doesn't seem that this local storage access does much error handling.

If the IMAP code encounters an error when reading a message from local storage, probably the correct response is to invalidate the local copy of the message: clear .storeToken, set .offlineMessageSize to 0 and clear the Offline flag.
Ideally it'd also retry the operation, this time streaming the message from the server instead (and adding a new local copy to local storage, if appropriate).

The upshot is that if it fails to get a message from local store, it should automatically get it from the server instead, essentially performing an incremental "folder repair" on any borked messages.

Same principle applies to other server-based folder types (eg ews)

Comment 1

[Ben Campbell]

Attachment: Bug 1929105 - Respond to IMAP offline message read errors by discarding local copy. r=#thunderbird-reviewers

Comment 2

[Ben Campbell]

The easiest way to test this is probably:

1. set up an IMAP account with a few messages in it
2. exit TB
3. load the INBOX file into a text editor
4. insert a few characters into a message and save back to INBOX
5. run TB again, and click through the messages to view them

step 4 should have the effect of changing the offsets of all the messages following the one you edited. This means the DB storeTokens for those will now be wrong.
Those messages should now fail to load from the local store, because the message no longer begins exactly with "From ".
The first time you click on such a message, the message view should just be blank. This is the read failing, and the local copy should be discarded as a result.
If you click to another message then back to the first one, this time it should a) download from the server b) show up in the message display c) be added to the end of the mbox as a new (working!) local copy.

Things to note:
Auto-discarding local messages like this does not contribute to the expunge counter (as we don't really have any certainty about the size of a damaged message), so an immediate compact will bail without doing anything. No big deal - once some (working) messages have been deleted, the next compact will clean things up including the damaged message we discarded, leaving the new, good copy only.

You can also trigger the message-too-big sanity check by inserting a bunch of extra lines into a message. The read error will trigger when 10% more bytes are read than the .offlineMessageSize was indicating. Again, the error should be caught (the display will stop at the point the error triggered rather than being blank), and the message will be discarded, so next attempt to access should result in it being re-downloaded.

Alternatively, you can programatically screw up .storetoken values on msgHdrs.

Comment 3

[Kai Engert [:KaiE:]]

I've applied the patch to comm-esr128 locally to my local build.

Using a corrupted IMAP folder (end markers not being found, some junk displayed at end of message),
the self healing works for me in the following way,

When clicking corrupted message A, it's still displayed with the junk at the end.
Then I click another message B.
Then I go back and again click message A.
Now A is displayed correctly.

Comment 4

[Ben Campbell]

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

When clicking corrupted message A, it's still displayed with the junk at the end.

A bit unsightly, but not too much we can do there - the error doesn't trigger until the docshell has already received (and displayed) most of the message. We don't have any control over what the docshell does at that point.
Ideally, I'd like the docshell to handle the error by retrying the request, so the new copy would immediately be downloaded, but hey.
(I'm sure there's some way to configure an error handler in the docshell which could do this, but I'm not sure it's worth the bother - in any case you wouldn't want more than one retry, as the error could be due to some reason which caused the request to always fail - eg bad disk, bad network, server down, whatever).

Comment 5

[Kai Engert [:KaiE:]]

Attachment: Bug 1929105 - Automatically reload the displayed email message if reading an offline copy failed. r=aleca,BenC

Comment 6

[Kai Engert [:KaiE:]]

Attachment: esr128-part1-mark-offline.patch

Ben's patch merged to esr128

Comment 7

[Kai Engert [:KaiE:]]

Attachment: esr128-part2-reload.patch

Kai's patch merged to esr128

Comment 8

[Kai Engert [:KaiE:]]

Attachment: esr128-part2-reload.patch

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1929105 - Automatically reload the displayed email message if reading an offline copy failed. r=aleca,BenC,KaiE

Depends on D228621.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/533bf3e32420
Respond to IMAP offline message read errors by discarding local copy. r=kaie
https://hg.mozilla.org/comm-central/rev/574f520e017d
Automatically reload the displayed email message if reading an offline copy failed. r=aleca,BenC,KaiE

Comment 11

[Kai Engert [:KaiE:]]

Attachment: 1929105-128-discard.patch

Comment 12

[Kai Engert [:KaiE:]]

Attachment: 1929105-128-reload.patch

Comment 13

[Kai Engert [:KaiE:]]

Attachment: Bug 1929105 - Tolerate NS_MSG_ERROR_MBOX_MALFORMED in gtest TestMsgMboxRead. r=#thunderbird-reviewers

Comment 14

[Kai Engert [:KaiE:]]

This needs follow-up patches to fix the tests.

I'll look at the simple gtest failure.

The second failures is
https://treeherder.mozilla.org/logviewer?job_id=482633074&repo=comm-central&lineNumber=15147
which is related to new test
mail/base/test/browser/browser_reloadBadMessage.js

Apparently that test triggers loading of URIs that "not same document load",
and is rejected by gecko's internal checks.

To understand this better I'd need to find out what kind of URIs are being loaded here.

Comment 15

[Kai Engert [:KaiE:]]

imap://user@localhost:34065/fetch%3EUID%3E%5EINBOX%3E1
imap://user@localhost:34065/fetch%3EUID%3E%5EINBOX%3E2
imap://user@localhost:34065/fetch%3EUID%3E%5EINBOX%3E3
imap://user@localhost:34065/fetch%3EUID%3E%5EINBOX%3E4

Comment 16

[Ben Campbell]

Attachment: Bug 1929105 - Fix empty-mbox gtests in TestMsgMboxRead. r=#thunderbird-reviewers

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/470a95a50c32
Fix empty-mbox gtests in TestMsgMboxRead. r=kaie

Comment 18

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1929105 follow-up - Fix test tripping over horrible IMAP code. r=#thunderbird-reviewers

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/db474dce6ae1
follow-up - Fix test tripping over horrible IMAP code. r=tobyp

Comment 21

[Kai Engert [:KaiE:]]

We need to reopen, because something isn't right.

I'm investigating regression bug 1931324. It fails, because it is unable to clone an URI. That shouldn't happen.

I've tried to debug why the clone is failing, but when I'm slowly stepping through the code, I don't get the failure. Only if I set a breakpoint on
/home/user/moz/comm-esr128/obj-thunder-debug/dist/include/nsIURIMutator.h:49
I run into the NS_FAILED condition.

  [[nodiscard]] nsresult InitFromURI(T* aURI)
  {
    nsCOMPtr<nsIURI> clone;
    nsresult rv = aURI->Clone(getter_AddRefs(clone));
->  if (NS_FAILED(rv)) {
      return rv;
    }
    mURI = static_cast<T*>(clone.get());
    return NS_OK;
  }

Could there be an unsafe multithreaded race?

I see that the fix for this bug adds checked that we're running on the main thread.
So maybe there isn't yet sufficient protection.

Comment 22

[Kai Engert [:KaiE:]]

Please disregard my previous comment 21.

I wasn't cc'ed on bug 1931324.
That's why I didn't see the comments over there, that the bug was fixed.
I unnecessarily spent time today trying to investigate it...

With that, a remaining regression is bug 1931508.

Comment 23

[Intermittent Failures Robot]

12 failures in 4227 pushes (0.003 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 12

Platform and build breakdown:

• windows11-64-2009-shippable-qr: 2
  • opt: 2
• linux1804-64-qr: 2
  • debug: 1
  • opt: 1
• windows11-32-2009-shippable-qr: 2
  • opt: 2
• windows11-32-2009-qr: 2
  • opt: 1
  • debug: 1
• windows11-64-2009-qr: 2
  • debug: 1
  • opt: 1
• linux1804-64-shippable-qr: 2
  • opt: 2

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1929105&startday=2024-11-11&endday=2024-11-17&tree=all

Comment 24

[Geoff Lankow (:darktrojan)]

Comment on attachment 9436777 [details]
Bug 1929105 - Respond to IMAP offline message read errors by discarding local copy. r=#thunderbird-reviewers

[Approval Request Comment]
This patch has a bunch of follow-ups to fix tests and edge cases, including in two other bugs. I did a try run of it on beta that you can copy. I don't think the order is important ultimately, but it might be.

Regression caused by (bug #):
User impact if declined: messages that are corrupted by folder compact remain that way unless the user intervenes
Testing completed (on c-c, etc.): landed late last week
Risk to taking this patch (and alternatives if risky): there's some risk, that's why I want it on beta for as long as possible before going to ESR.

Comment 25

[Corey Bryant]

Comment on attachment 9436777 [details]
Bug 1929105 - Respond to IMAP offline message read errors by discarding local copy. r=#thunderbird-reviewers

[Triage Comment]
Approved for beta

Comment 26

[Corey Bryant]

Comment on attachment 9437615 [details]
Bug 1929105 - Automatically reload the displayed email message if reading an offline copy failed. r=aleca,BenC,KaiE

[Triage Comment]
Approved for beta

Comment 27

[Corey Bryant]

Comment on attachment 9437805 [details]
Bug 1929105 - Fix empty-mbox gtests in TestMsgMboxRead. r=#thunderbird-reviewers

[Triage Comment]
Approved for beta

Comment 28

[Corey Bryant]

Comment on attachment 9437843 [details]
Bug 1929105 follow-up - Fix test tripping over horrible IMAP code. r=#thunderbird-reviewers

[Triage Comment]
Approved for beta

Comment 29

[Corey Bryant]

Thunderbird 133.0b5:
https://hg.mozilla.org/releases/comm-beta/rev/56f3a2cfd774
https://hg.mozilla.org/releases/comm-beta/rev/6a7f22e73226
https://hg.mozilla.org/releases/comm-beta/rev/b89600ebed47
https://hg.mozilla.org/releases/comm-beta/rev/4504d2f76120

Comment 30

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Confirming this issue as verified fixed using 133.0b5-build2(20241118140457) with macOS 14, Ubuntu 24 and WIndows 11 using the STR from comment 2.

Comment 31

[Toby Pilling [:tobyp]]

Ben, could you please request this for uplift to 128.5.0?

Comment 32

[Ben Campbell]

Comment on attachment 9437668 [details] [diff] [review]
1929105-128-discard.patch

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is part of the "self-healing" change to automatically detect corrupted messages and re-download them from IMAP.
• User impact if declined: Users will be left with bad messages unless they perform an IMAP folder repair. NOTE: this also needs the patch in Bug 1931508 to be uplifted
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 33

[Ben Campbell]

Comment on attachment 9437668 [details] [diff] [review]
1929105-128-discard.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Users will be left with bad messages unless they perform an IMAP folder repair. NOTE: this also needs the patch in Bug 1931508 to be uplifted
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): lowish

Comment 34

[Ben Campbell]

Comment on attachment 9437669 [details] [diff] [review]
1929105-128-reload.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Users will be left looking at broken messages that should now be working.
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): low

Comment 35

[Ben Campbell]

Comment on attachment 9437805 [details]
Bug 1929105 - Fix empty-mbox gtests in TestMsgMboxRead. r=#thunderbird-reviewers

A test-only tweak to handle the new logic.
Regression caused by (bug #):
User impact if declined: none
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): low

Comment 36

[Ben Campbell]

Comment on attachment 9437843 [details]
Bug 1929105 follow-up - Fix test tripping over horrible IMAP code. r=#thunderbird-reviewers

A test-only tweak to handle the new logic.

Regression caused by (bug #):
User impact if declined: none
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): low

Comment 37

[Geoff Lankow (:darktrojan)]

Attachment: 1929105-test-imap-cleanup.diff

[Approval Request Comment]
See comment 36. ESR needed a slightly different version of this patch to avoid test failure.

Comment 38

[Geoff Lankow (:darktrojan)]

I've done a try run of the patches in this bug and related bugs. There were some slight changes (as mentioned in the whiteboard of this bug) and the patch I've just added instead of the one that landed on central.

Comment 39

[Corey Bryant]

Comment on attachment 9437668 [details] [diff] [review]
1929105-128-discard.patch

[Triage Comment]
Approved for esr128

Comment 40

[Corey Bryant]

Comment on attachment 9437669 [details] [diff] [review]
1929105-128-reload.patch

[Triage Comment]
Approved for esr128

Comment 41

[Corey Bryant]

Comment on attachment 9437805 [details]
Bug 1929105 - Fix empty-mbox gtests in TestMsgMboxRead. r=#thunderbird-reviewers

[Triage Comment]
Approved for esr128

Comment 42

[Corey Bryant]

Comment on attachment 9438665 [details] [diff] [review]
1929105-test-imap-cleanup.diff

[Triage Comment]
Approved for esr128

Comment 43

[Corey Bryant]

Thunderbird 128.5.0esr:
https://hg.mozilla.org/releases/comm-esr128/rev/a6e39e9e612a
https://hg.mozilla.org/releases/comm-esr128/rev/d1d23997da43
https://hg.mozilla.org/releases/comm-esr128/rev/1f5de97a0f6a
https://hg.mozilla.org/releases/comm-esr128/rev/695a38b3fd9d