1897303 - Folder cache entries not removed on account removal

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Sean Burke [:leftmostcat]]

When deleting an account, the entries relating to its folders are not removed from folderCache.json. For EWS work, this causes properties representing local state to be incorrectly retained when deleting and recreating an account.

We would expect that https://searchfox.org/comm-central/source/mailnews/base/src/nsMsgDBFolder.cpp#3279 would address this, but that doesn't seem to be happening.

Comment 1

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Hello Sean,

Is there something QA can manually do in order to reproduce this issue?

If not, could you please update the affected version in Tracking Flags(if that applies)?

Thank you in advance!

Comment 2

[Wayne Mery (:wsmwk)]

(In reply to Sean Burke [:leftmostcat] from comment #0)

When deleting an account, the entries relating to its folders are not removed from folderCache.json. For EWS work, this causes properties representing local state to be incorrectly retained when deleting and recreating an account.

But doesn't impact non-EWS users?

Comment 3

[Brendan Abolivier [:babolivier]]

(In reply to Wayne Mery (:wsmwk) from comment #2)

(In reply to Sean Burke [:leftmostcat] from comment #0)

When deleting an account, the entries relating to its folders are not removed from folderCache.json. For EWS work, this causes properties representing local state to be incorrectly retained when deleting and recreating an account.

But doesn't impact non-EWS users?

I've just taken a look at this. Folder entries aren't removed from the folder cache regardless of the account type (I can see folders from an IMAP account I've deleted in my test profile's folder cache). For EWS, this creates a bigger problem than other protocols, because we use the folder properties (which are copied to the folder cache) to store a token allowing us to tell the server which changes we've seen to the message list for a given folder, so we don't bother syncing changes we've already seen. As a result, removing an EWS account then adding it again leaves all folders empty (because we think we've already seen all the messages that are available).

This is not as big an issue with other protocols, because they use a different logic to figure out what to sync. However, it might affect users in a less harmful way by retaining e.g. flags, properties, etc that might have been set on one (now deleted) account and applying them to another (new) one.

Comment 4

[Brendan Abolivier [:babolivier]]

(In reply to Sean Burke [:leftmostcat] from comment #0)

When deleting an account, the entries relating to its folders are not removed from folderCache.json. For EWS work, this causes properties representing local state to be incorrectly retained when deleting and recreating an account.

We would expect that https://searchfox.org/comm-central/source/mailnews/base/src/nsMsgDBFolder.cpp#3279 would address this, but that doesn't seem to be happening.

This searchfox link points to nsMsgDBFolder::DeleteStorage(), but it doesn't look like this is called at all during the account deletion process. Nor is nsMsgDBFolder::RecursiveDelete() which performs the actual removal from the cache.

We already perform some actions on individual folders when deleting an account: https://searchfox.org/comm-central/rev/1016048a635bd062b826bfe767c86acaeadd004a/mailnews/base/src/nsMsgAccountManager.cpp#562-570
So we might be able to update that block to also remove each folder from the cache.

Comment 5

[Brendan Abolivier [:babolivier]]

Attachment: Bug 1897303 - Remove folders from the folder cache on account removal. r=#thunderbird-back-end-reviewers

Comment 6

[Brendan Abolivier [:babolivier]]

Actually I just remembered there's a test that needs fixing before this can land.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9b970f296967
Remove folders from the folder cache on account removal. r=heather

Comment 8

[Magnus Melin [:mkmelin]]

This is perma failing (crashing) comm/mail/test/browser/account/browser_accountTelemetry.js
https://treeherder.mozilla.org/jobs?repo=comm-central&selectedTaskRun=GXg5MEpXTCeIL7w7XhpKxg.0

#0  nsMsgAccountManager::RemoveFolderFromCache (this=this@entry=0x7fffc60ddec0, aFolder=0x7fffc7c48340)
    at /home/magnus/Code/tb/mozilla/comm/mailnews/base/src/nsMsgAccountManager.cpp:640
#1  0x00007fffee1e0bbc in nsMsgAccountManager::RemoveIncomingServer (this=0x7fffc60ddec0, aServer=0x7fffb22f9c40, aRemoveFiles=false)
    at /home/magnus/Code/tb/mozilla/comm/mailnews/base/src/nsMsgAccountManager.cpp:583
#2  0x00007fffee1e1158 in nsMsgAccountManager::RemoveAccount (this=0x7fffc60ddec0, aAccount=0x7fffb5056ab0, aRemoveFiles=false)
    at /home/magnus/Code/tb/mozilla/comm/mailnews/base/src/nsMsgAccountManager.cpp:746
#3  0x00007fffe947d106 in NS_InvokeByIndex () at /home/magnus/Code/tb/mozilla/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:101

I had understood MOZ_TRY as basically doing NS_ENSURE_SUCCESS. If causing crashes for things like this we should not be using it.

Comment 9

[Magnus Melin [:mkmelin]]

This is also causing the perma comm/mail/services/sync/test/unit/test_server_tracker.js failure

Comment 10

[Brendan Abolivier [:babolivier]]

(In reply to Magnus Melin [:mkmelin] from comment #8)

This is perma failing (crashing) comm/mail/test/browser/account/browser_accountTelemetry.js
https://treeherder.mozilla.org/jobs?repo=comm-central&selectedTaskRun=GXg5MEpXTCeIL7w7XhpKxg.0

#0  nsMsgAccountManager::RemoveFolderFromCache (this=this@entry=0x7fffc60ddec0, aFolder=0x7fffc7c48340)
    at /home/magnus/Code/tb/mozilla/comm/mailnews/base/src/nsMsgAccountManager.cpp:640
#1  0x00007fffee1e0bbc in nsMsgAccountManager::RemoveIncomingServer (this=0x7fffc60ddec0, aServer=0x7fffb22f9c40, aRemoveFiles=false)
    at /home/magnus/Code/tb/mozilla/comm/mailnews/base/src/nsMsgAccountManager.cpp:583
#2  0x00007fffee1e1158 in nsMsgAccountManager::RemoveAccount (this=0x7fffc60ddec0, aAccount=0x7fffb5056ab0, aRemoveFiles=false)
    at /home/magnus/Code/tb/mozilla/comm/mailnews/base/src/nsMsgAccountManager.cpp:746
#3  0x00007fffe947d106 in NS_InvokeByIndex () at /home/magnus/Code/tb/mozilla/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:101

I had understood MOZ_TRY as basically doing NS_ENSURE_SUCCESS. If causing crashes for things like this we should not be using it.

Ugh, sorry, I completely missed this one.

MOZ_TRY does return an error like NS_ENSURE_SUCCESS. Looking at logs and reproducing locally, it seems like folderPath is null here: https://searchfox.org/comm-central/rev/898aba590fb8b29599ec1669cda1691c51c027ea/mailnews/base/src/nsMsgAccountManager.cpp#640

Thus accessing it results in a crash. This happens during cleanup when we try to remove the accounts created by the tests. I'll investigate.

Comment 11

[Brendan Abolivier [:babolivier]]

(In reply to Magnus Melin [:mkmelin] from comment #9)

This is also causing the perma comm/mail/services/sync/test/unit/test_server_tracker.js failure

Ah, indeed - I had managed to work around this issue but forgot to apply that workaround when removing the root folder. The fix is fairly simple, but I'll wait to see if the cause for the crash is similar enough that I can lump both fixes in the same patch.

Comment 12

[Kai Engert [:KaiE:]]

Backed out, reopening.
https://hg-edge.mozilla.org/comm-central/rev/e59cab1e3a393b2e38446d7357d2d694ee691daf

Comment 13

[Brendan Abolivier [:babolivier]]

Attachment: Bug 1897303 - Follow-up: fix failures observed in tests. r=#thunderbird-back-end-reviewers

IM/chat accounts have a partial, synthetic representation of their folders that
doesn't map to on-disk paths; so they should be excluded from anything to do
with the folder cache.

This patch also loosens up the error handling when removing an account's root
folder from the cache, which was an oversight from the original patch (and also
lowers the logs from errors to warnings, in order to avoid triggering
assertions).

Comment 14

[Brendan Abolivier [:babolivier]]

I've updated the initial patch to fix the test failures and crash.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cf66e941f024
Remove folders from the folder cache on account removal. r=heather

Comment 16

[Magnus Melin [:mkmelin]]

Looks like comm/mail/base/test/browser/browser_statusFeedback.js is perma-failing due to this

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: Bug 1897303 - revert mail/base/test/browser/browser_statusFeedback.js changes that are causing failure. r=babolivier

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2a18a1ddda98
revert mail/base/test/browser/browser_statusFeedback.js changes that are causing failure. r=babolivier

Comment 19

[Magnus Melin [:mkmelin]]

Still failing so backed out in full:
https://hg.mozilla.org/comm-central/rev/25e6e66fa6e78f33b74150c579ca8f407c65a020
https://hg.mozilla.org/comm-central/rev/e7d593c1c538a6ff8a46e72a8a3e577f65e63753

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e5e1a031b5cf
Backed out changeset cf66e941f024

Comment 21

[Intermittent Failures Robot]

1 failures were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 1

Table

	no_variant
linux1804-64-qr--/opt	1

For more details, see:

https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1897303&startday=2025-04-21&endday=2025-04-27&tree=all

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8b032f3ad1ad
Remove folders from the folder cache on account removal. r=heather