1776280 - Folder cache fails to cache values retrieved from the database

Status: RESOLVED FIXED

(MailNews Core :: Database, defect, P2)

Description

[Rachel Martin]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

Follow up to bug 1773822 comment #16. We noticed that hundreds of folders don't have the MRUTime property that add-on Quick-Folder-Move retrieves.

One would expect that after the first use of QFM the folder cache would hold all the retrieved attributes. That is not the case.

Comment 1

[Rachel Martin]

With this patch, QFM is much faster on second use. Inspection of folderCache.json shows no entries with missing MRUTime:
https://github.com/Betterbird/thunderbird-patches/blob/main/102/bugs/1776280-really-cache-retrieved-props.patch

We saw another problem: When a folder is removed from disk manually, the folder's entry stays in folderCache.json forever.

Comment 2

[Wayne Mery (:wsmwk)]

Perhaps not strictly a regression of bug 418551, but certainly related.

Comment 3

[Andrei Hajdukewycz [:sancus]]

(In reply to Rachel Martin from comment #1)

With this patch, QFM is much faster on second use. Inspection of folderCache.json shows no entries with missing MRUTime:
https://github.com/Betterbird/thunderbird-patches/blob/main/102/bugs/1776280-really-cache-retrieved-props.patch

Is there a reason that patch duplicates the if (NS_SUCCEEDED(rv)) which is already used just above? Genuinely asking as it looked weird to me but I'm obviously not knowledgeable about this code.

This patch needs BenC's review in any case.

Comment 4

[Ben Campbell]

Patch looks good to me: r+
(what's the best way to land it? I was going to put it through phab, but I couldn't upload it without blatting over the author attribution...)

The if (NS_SUCCEEDED(rv)) looks sane - it's required to avoid setting the cached value if the retrieval from folderInfo fails.

I've got a slight nagging sensation that caching the value upon read doesn't feel quite right... by rights, shouldn't the value already be cached, either by the corresponding nsMsgDBFolder::SetStringProperty() or via a migration (from panacea.dat - maybe https://bugzilla.mozilla.org/show_bug.cgi?id=1773822#c19 is relevant)?

But I can't see any particular downsides to this patch, so hey :-)

try run going here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c801fba877f1fd588efb93c198ee29d36d0948af

Comment 5

[Andrei Hajdukewycz [:sancus]]

Attachment: Bug 1776280 - Make folder cache store properties retrieved from DB

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a96377dc5770
Make folder cache store properties retrieved from DB. r=benc

Comment 7

[b1]

(In reply to Ben Campbell from comment #4)

I've got a slight nagging sensation that caching the value upon read doesn't feel quite right... by rights, shouldn't the value already be cached, either by the corresponding nsMsgDBFolder::SetStringProperty() or via a migration (from panacea.dat - maybe https://bugzilla.mozilla.org/show_bug.cgi?id=1773822#c19 is relevant)?

How about this theory:
panacea.dat contains a value of null, this is migrated to null in JSON, We still have 1700 occurrences of "MRMTime" : null, and 1800 of "LastPurgeTime" : null, in the cache. If then someone, like the "Quick Folder Move" add-on, mass-retrieves a property which is mostly null, an error is returned from here:
https://searchfox.org/comm-central/rev/74a9eb0f3cb0ba1bfee84ec01f3dc233d8d69346/mailnews/base/src/nsMsgFolderCache.cpp#53
since the null check fails. In turn the software goes to retrieve the value from the DB, opening a massive amount of folders, bug 1773822. And since even then the retrieved value isn't stored (this bug here), next time around, all the databases are opened again, leading to slowness in the add-on use or Recent Folders within TB itself, bug 1773401.

This theory isn't tested, one would have to retrieve a property which is known to be present but with a null value and see what happens in TB 91 and 102. If the theory is true, the fix in bug 1773822 is still useful. You might consider backing out this bug here in favour of a change to nsMsgFolderCache.cpp#53 permitting a null value to be returned. Or maybe it's beneficial to replace null values with real values.

Comment 8

[b1]

Please set the Milestone to 104.

Comment 9

[b1]

Coming to think of it, https://hg.mozilla.org/comm-central/rev/aba71e533211 appears to be a kludge that papered over an issue that was introduced by the changed "null retrieval" behaviour at nsMsgFolderCache.cpp#53.

Furthermore, please make bug 1773401 a duplicate of this bug here or declare it "fixed by bug 1776280" after checking with the reporter.

Comment 10

[b1]

See bug 1778888 for a continuation.

Comment 11

[Magnus Melin [:mkmelin]]

Time to uplift?

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9284638 [details] [diff] [review]
Bug 1776280 - Make folder cache store properties retrieved from DB

[Triage Comment]
Approved for esr102

Take this before bug 1778888

Comment 13

[Rob Lemley [:rjl]]

Thunderbird 102.2.0:
https://hg.mozilla.org/releases/comm-esr102/rev/0f5c26b4330a

Comment 14

[Axel Grude]

After this landed in ESR 102.2 performance is greatly enhanced and speed back to that of Tb 91.* even with my Add-on QuickFolders enabled. Good work!