Open Bug 1787609 Opened 1 month ago Updated 12 days ago

getMsgHdrForMsgId() may cause massive loss of MSF files since it potentially opens all folders in the profile hitting the limit of open files

Categories

(MailNews Core :: General, defect, P1)

Thunderbird 102

Tracking

(thunderbird_esr102+ affected, thunderbird105 affected)

UNCONFIRMED
Tracking Status
thunderbird_esr102 + affected
thunderbird105 --- affected

People

(Reporter: b1, Unassigned, NeedInfo)

References

Details

(Keywords: perf)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/104.0.5112.102 Safari/537.36 Edg/104.0.1293.70

Steps to reproduce:

getMsgHdrForMsgId() may cause massive loss of MSF files since it potentially opens all folders in the profile hitting the limit of open files.

As we've seen in bug 1773822, opening too many folder databases leads to the loss of the MSF files which can't be opened any more and are then considered invalid and are deleted.

https://github.com/Betterbird/thunderbird-patches/blob/main/102/bugs/1787609-fix-findMsgIdInFolder.patch
Turned out that CloseDBIfFolderNotOpen() didn't try hard enough, hence we added the ForceClosed().
The change in nsMsgDatabase::CheckForErrors() avoids blowing away databases needlessly due to "too many open files".

BTW, with ~1800 folders, some of them rather big, getMsgHdrForMsgId() will query all the databases for the given message ID if started with an non-existent message ID. This makes the application unresponsive for seconds/minutes. This should be up for a rewrite. With the patch at least it doesn't obliterate the MSF files any more. On Windows there is a ~500 open file limit, so for us ~1300 MSFs were deleted.

Several users are reporting serious version 102 performance issues, so we will want to sort this out.

Severity: -- → S2
Keywords: perf
Priority: -- → P1
See Also: → 1788901

Why is the new argument needed? if folderOpen is evaluated as false, then SetMsgDatabase(nullptr) is not enough to close the DB? If not and mDatabase->ForceClosed() is needed too, why don't we want to call it also at all other callsites (unconditionally)?

-nsresult nsMsgDBFolder::CloseDBIfFolderNotOpen() {
+NS_IMETHODIMP nsMsgDBFolder::CloseDBIfFolderNotOpen(bool aForceClosed) {
nsresult rv;
nsCOMPtr<nsIMsgMailSession> session =
do_GetService(NS_MSGMAILSESSION_CONTRACTID, &rv);
NS_ENSURE_SUCCESS(rv, rv);
bool folderOpen;
session->IsFolderOpenInWindow(this, &folderOpen);
if (!folderOpen &&

  •  !(mFlags & (nsMsgFolderFlags::Trash | nsMsgFolderFlags::Inbox)))
    
  •  !(mFlags & (nsMsgFolderFlags::Trash | nsMsgFolderFlags::Inbox))) {
    
  • if (aForceClosed && mDatabase) mDatabase->ForceClosed();
    SetMsgDatabase(nullptr);
  • }
    return NS_OK;
    }
Flags: needinfo?(b2)

(In reply to :aceman from comment #4)

Why is the new argument needed? if folderOpen is evaluated as false, then SetMsgDatabase(nullptr) is not enough to close the DB? If not and mDatabase->ForceClosed() is needed too, why don't we want to call it also at all other callsites (unconditionally)?

Good questions we cannot answer. Observed behaviour is a) calling "force closed" unconditionally breaks mail/components/extensions/test/xpcshell/test_ext_messages.js and b) not calling "force closed" and just using the hunk in MailUtils.jsm doesn't achieve the required result, the MSF files still get deleted. You can easily test that when trying to open an article via a message ID when the article is not on your system and therefore all folders are opened (see bug 37653 for enabling patch).

Flags: needinfo?(b2)

Correct, the test fails like this (with closing the DB unconditionally):

5:03.19 INFO xpcshell-local.ini:comm/mail/components/extensions/test/xpcshell/test_ext_messages.js | Starting test_move_copy_delete
5:03.19 INFO (xpcshell/head.js) | test test_move_copy_delete pending (2)
5:03.19 INFO (xpcshell/head.js) | test run_next_test 4 finished (2)
5:03.19 INFO "Extension attached"
10:00.48 pid:21844 JavaScript error: chrome://messenger/content/parent/ext-mail.js, line 1839: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMsgDBHdr.getProperty]
5:03.59 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMsgDBHdr.getProperty]" {file: "chrome://messenger/content/parent/ext-mail.js" line: 1839}]"

The line 1839 is the msgHdr.getProperty() call in the block:
function convertMessage(msgHdr, extension) {
if (!msgHdr) {
return null;
}

let composeFields = Cc[
"@mozilla.org/messengercompose/composefields;1"
].createInstance(Ci.nsIMsgCompFields);

let junkScore = parseInt(msgHdr.getProperty("junkscore"), 10) || 0;

The test does not call convertMessage() directly. It could use some investigation why it is called and whether there is not some wrong assumption of the folder having the DB still open.

Flags: needinfo?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.