1925085 - Attempt to fix folder corruption bug 1890230 using backout of related changes on comm-esr128 since TB 122.

Status: NEW

(Thunderbird :: General, task)

Description

[Kai Engert [:KaiE:]]

This is an attempt to fix folder corruption bug 1890230 using backout of related changes since TB 122.

It's not yet clear if this approach will be taken, and whether it is safe to do.

The intention is to investigate/test this approach in parallel to the efforts in bug 1890230.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 1d6b847ef67f - Bug 1904799 - Change nsIDBFolderInfo.GetTransferInfo to return nsIPropertyBag2.

Comment 2

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 18ebd7d4ceac - Bug 1906835 - Always close backup database in nsMsgDBFolder::Shutdown

Comment 3

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset c7f634ccd3a7 - Bug 1872230 - Modernize nsMsgBodyHandler::GetNextFilterLine().

Comment 4

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 2da8c4a2cba4 - Bug 1900172 - Workaround nsIFile.fileSize stat cache after folder compaction.

Comment 5

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 5dd17b198727 - Bug 1888790 - Fix nsParseNewMailState to ignore "From " lines in message body.

Comment 6

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 8e2371878dbe - manually merged - Bug 1890448 - Rewrite folder compaction (2/2), Replace nsIMsgFolderCompactor.

Comment 7

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 5b31114107d5 - manually merged - Bug 1890448 - Rewrite folder compaction (1/2), Add nsIMsgPluggableStore.asyncCompact().

Comment 8

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 7f5d26c07e3e - Bug 1892866 - Follow-up, fix debug errors.

Comment 9

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 1e74c5719795 - manually merged - Bug 1892866 - Remove nsIImapService.fetchMessage() from public interface.

Comment 10

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 808b03c44b2b - Bug 1857450 - Write out the whole retrieved nntp article in one go.

Comment 11

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 9009daf0b2ad - Bug 1891937 - Fix crash in [@ nsMsgDBFolder::AddMarkAllReadUndoAction ].

Comment 12

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 1ddb01b156d9 - Bug 1889653 - Register a subfolder in memory before creating the storage for it.

Comment 13

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset ceccb46eba33 - Bug 1890453 - Add C++ shortcut URI accessor to nsIMsgFolder (and nsMsgDBFolder).

Comment 14

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset c09532b365ba - Bug 1890135 - Fix folder compaction to not fail on zero-size mbox files.

Comment 15

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 583b8b7291d6 - manually merged - Bug 1887778 - Fix mismatched folder locking in nsMsgDBFolder StartNewOfflineMessage()/EndNewOfflineMessage().

Comment 16

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 65b97226f41e - manually merged - Bug 1887047 - modernize nsIMsgCopyServiceListener.

Comment 17

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset e9f33b21057f - manually merged - Bug 1882282 - Make mailbox url.folder return null if it doesn't exist.

Comment 18

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 26b35edb35ff - manually merged - Bug 1880043 - IMAP download progress status and localization improvement.

Comment 19

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 7c318cb160c7 - Bug 1885812 - Release semaphore in early error return path.

Comment 20

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 8a74d750e2ec - Bug 1882971 Make the file stream used by Pop3 a buffered stream.

Comment 21

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 12ffdc0549a4 - manually merged - Bug 1872849 - Add protection against interleaved message writes to nsImapMailFolder.

Comment 22

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 2a9a8a1a695f - Bug 1880765 - follow-up, fix parameter.

Comment 23

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 98b5c2f16da5 - Bug 1880765 - tone down warning when unexpected status was found for parsing Berkeley Mail status header.

Comment 24

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset bbea0bbe317d - manually merged - Bug 1857450 - Ensure nsMsgBrkMBoxStore prevents multiple simultaneous writes to same mbox file.

Comment 25

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset b7bb63ac09cc - manually merged - Bug 1873282 - Use better fake keys for pseudo-offline IMAP copies.

Comment 26

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 096cfd237f70 - manually merged - Bug 1873282 - Fix setting references when parsing IMAP mailbox.

Comment 27

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 55d178a009b0 - Bug 1872232 - Fix crash in [@ nsOfflineStoreCompactState::OnStopRequest].

Comment 28

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 38d893017845 - Bug 1868705 - Fix nullptr dereference in StoreIndexer::OnStopRequest().

Comment 29

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 8822b10f77e9 - Bug 1868504 - Fix crash bug in mbox parsing.

Comment 30

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 222268ebb87b - manually merged - Bug 1719121 - Part 1/2, Confine "From " line handling to mbox-specific code.

Comment 31

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset f46382a0d7a9 - Bug 1719121 - Part 2/2, Update tests for mbox refactor.

Comment 32

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset a8d5067b54ea - manually merged - Bug 1891426 - Remove extra "From" line in test messages.

Comment 33

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset e2aa6040155b - partial - Bug 1888569 - Only include headers as needed.

Comment 34

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Bug 1925085 - Fixes required on top of backout.

Comment 35

[Kai Engert [:KaiE:]]

All attached patches are on top of comm-esr128.

Comment 36

[Kai Engert [:KaiE:]]

Attached are 33 backed out commits.
For the record, I'm counting 2211 commits on the comm-esr128 branch since bug 1719121 commit 222268ebb87b.

Comment 37

[Kai Engert [:KaiE:]]

Removed 2 unnecessary backouts.

Comment 38

[Kai Engert [:KaiE:]]

[Edited: This build is old. Please scroll down for a newer build.]

Below are links to experimental builds of 128.3.x, which revert the changes that were made to Thunderbird's Folder storage during the development of TB 128. (In particular, since bug 1719121).

If you are affected by bug 1890230, and are willing to test an alternative fix, you may try these builds. They will work with your regular profile, the one you're already using with Version 128.

Please be aware that this has some risk on its own. It's possible that there is an interdependency between other changes that were made to 128.x, which is broken by the changes in this experiment. So if you consider to try this, it might be good if you backup your Profile directory beforehand.

If you do test it, please give us feedback here:

• Does this version prevent new corruption issues from happening?
• Do you see any other functional side effects when using this build?

Thank you very much in advanced for testing and reporting back.

I suggest that you download, extract it to a separate place, and manually run it from the extracted directory.
This approach won't overwrite your installed version. However, it will require that every time you want to use this experimental version again, you must again run it manually from the extracted directory.

Download links:

Linux 64 bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Y8z_SNldSmi6YI8OM0B1Rg/runs/0/artifacts/public/build/target.tar.bz2

Windows 64 bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/RN--citoStC9Y4gJLUf1_g/runs/0/artifacts/public/build/target.zip

Windows 32 bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/ATzePuc_RqqDSGLFhi_kGA/runs/0/artifacts/public/build/target.zip

Mac OS:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/dC7AujHLQteC4Lk8ZTmLUA/runs/0/artifacts/public/build/target.dmg

(For technical users, here is the overall link to all details of this build:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=0a410752e0b6649baf261b71bd80a7c2d505a498 )

Comment 39

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 84f49037eb35 - Bug 1923765 - [ESR128] Modify tests so they pass on this branch.

Comment 40

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset d5033b970eb3 - Bug 1923765 - Test what happens if a new message arrives while the folder is locked.

Comment 41

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 4baf75331558 - Bug 1923765 - Clear the offline flag when trying to load messages with no storeToken.

Comment 42

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 11e3be3f0e27 - Bug 1923747 - Reject message transfer from nsImapProtocol to locked folders.

Comment 43

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset ee0b75b68723 - Bug 1720047 - Remove remaining nsIMsgDBHdr.messageOffset.

Comment 44

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset f74c7440c2f1 - Bug 1923541 - Use storeToken attribute instead of string property where appropriate.

Comment 45

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset f662b664701c - Bug 1923541 - Create a storeToken attribute on nsIMsgDBHdr.

Comment 46

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 1925085 - Backed out changeset 889ad2370718 - Bug 1878541 - Add migration code to delete nstmp files left over from failed folder compactions.

Comment 47

[Kai Engert [:KaiE:]]

Using a 32 bit build, we should test that working with 5 GB mailboxes works correctly.

Comment 48

[Kai Engert [:KaiE:]]

Here is an updated try build:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=146cbe883f0b90ab48221a1dc30d11b09cc75855

It's based on 128.3.3 - with the fixes from that version, but also backing out the additional changes related to storage.

(In reply to Kai Engert (:KaiE:) from comment #47)

Using a 32 bit build, we should test that working with 5 GB mailboxes works correctly.

I confirmed that's working with this build.

Linux 64 bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/A0ueR37_T56kIxmZaAgvCg/runs/0/artifacts/public/build/target.tar.bz2

Linux 32 bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/O5nmaOTDTzeQM8oTGsoGJA/runs/0/artifacts/public/build/target.tar.bz2

Windows 64 bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/ZBR3S4dHRBKzHSIflrS9lw/runs/0/artifacts/public/build/target.zip

Windows 32 bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/EVqTKbRpSH-YzFiY3aUaNQ/runs/0/artifacts/public/build/target.zip

Mac OS:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/T9mNkVy2TSGYcPuxOo8ZUg/runs/0/artifacts/public/build/target.dmg

Comment 49

[Kai Engert [:KaiE:]]

Based on new information, the backout strategy no longer seems viable to me (without extra work).

The old code, which this backout would restore, expects that we have meta information available that accurately records the sizes of messages, as it uses those sizes to determine how much data to read from the mbox files.

The latest storage code (as used in released 128.x) no longer assures that the meta information has the same accurate sizes as in the past, it stores different sizes.

As a result, when using mbox files that were created using 128.x, with a backout build from here, the code operates on truncated messages.

I can reproduce this. When receiving a message with 128.x, then switching to the backout build and using "save message", the resulting file is truncated.

Comment 50

[Francesco]

Is this really true? That would mean an mbox file processed by 128 can't be used in 115 or earlier any more? Has that incompatibility been published?

Comment 51

[Kai Engert [:KaiE:]]

Let's ask Ben, maybe I misunderstood something.

Comment 52

[Kai Engert [:KaiE:]]

(But in general, we don't support downgrading to an earlier major release.)

Comment 53

[Francesco]

Well, while that's true, there are many situations where people need to use the -allow-downgrade switch. Usually a new version makes some changes to the profile, we've seen cases where the security databases don't survive a downgrade, there have been irreversible address book upgrades (where in case of downgrade you had to manually restore backed up address book files), but that gigabytes of "standard" mbox data become invalid or at least need to have their indexes rebuilt, appears to be a new situation. As far as I know mbox/msf data so far has been downward compatible for the last 10 years, certainly back down to TB 38 or so.

Comment 54

[Ben Campbell]

(In reply to Kai Engert (:KaiE:) from comment #51)

Let's ask Ben, maybe I misunderstood something.

No, you're basically right.

In the old code, .messageSize was overloaded with three different meanings:

1. for IMAP, it was the size of the message on the server
2. for local folders it was the size of the message as encoded in the mbox file (for IMAP, this value was instead stored in .offlineMessageSize)
3. it is the size of the non-mbox-encoded raw RFCx22 message (eg the size of the message as displayed in the gui)

1 and 3 are mostly the same meanings, except that local copies of messages usually have some extra headers added (X-Mozilla-Status/Status2/Keys), and so tend to be larger.

2 was the meaning that the code relied upon.
2 and 3 are subtlety different because mbox encoding adds the "From " separator line and also adds extra '>' characters in the message body to escape lines beginning with "From ".
This meant that every place that dealt with local copies of messages needed to implement it's own mbox encoding/decoding, and this was very inconsistent. It also made a lot of maildir (and potentially other storage backends) nigh-on impossible to implement properly.

So the aim of the new code was to decouple the mbox details from the rest of the codebase and keep all the mbox-specific encoding/decoding in one place (the mbox message store). So, given that the encoded length was definitely an mbox-specific detail, code outside the mbox shouldn't have to deal with it.

So I went with meaning 3 for .messageSize the raw RFCx22 size, which has the same value regardless of the storage backend.

I didn't anticipate that it would be an issue. All the storage backends (including mbox) must be capable of knowing when the end of a message has been encountered without having an explicit size passed in from outside, so meaning 3 seemed like the "correct" (and storage-agnostic) use of .messageSize.

The upshot being that .messageSize values written out by the newer code are likely to be a little less than the mbox-encoded-length of the message.