Closed Bug 1560014 Opened 1 year ago Closed 1 year ago

nsMsgAccountManager::RemoveAccount allows removing the same account multiple times

Categories

(MailNews Core :: Account Manager, task)

task
Not set
normal

Tracking

(thunderbird68 fixed, thunderbird69 fixed)

RESOLVED FIXED
Thunderbird 69.0
Tracking Status
thunderbird68 --- fixed
thunderbird69 --- fixed

People

(Reporter: aceman, Assigned: aceman)

Details

Attachments

(1 file, 1 obsolete file)

When we remove an account in nsMsgAccountManager::RemoveAccount()
we check if the account was actually in the list (https://searchfox.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp#590), but still proceed deleting its data.

We have such a situation in mozmill/account/test-account-tree.js test where we try to remove the same account twice. It succeeds, but produces some warnings:
[7975, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file comm/mailnews/base/util/nsMsgDBFolder.cpp, line 2953
[7975, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, (nsresult)(((uint32_t)(1) << 31) | ((uint32_t)(16 + 69) << 16) | ((uint32_t)(22)))) failed with result 0x80004005: file comm/mailnews/local/src/nsLocalMailFolder.
[7975, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file comm/mailnews/base/util/nsMsgDBFolder.cpp, line 2953
[7975, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file comm/mailnews/base/util/nsMsgDBFolder.cpp, line 2953

I'll see if we can throw an error in this case as it looks like an error that the caller hangs onto an account object that already has its data removed and even manages to pass it again to RemoveAccount().

Attached patch 1560014.patch (obsolete) — Splinter Review

This makes removeAccount() return error (throw in JS) if the removed account doesn't exist anymore.

Also, after the method is called, I null out the object that was passed in so that we do not hold on to the object, while many of the internal state was already cleared out. I don't do that if after the call the function ends soon and the variable is local and is destroyed automatically. This was already done at some places in tests.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=252906107&revision=45c74cf3c3eb2c60eda2414afc7cf30119a12881

Attachment #9073394 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9073394 [details] [diff] [review]
1560014.patch

Review of attachment 9073394 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=mkmelin
Attachment #9073394 - Flags: review?(mkmelin+mozilla) → review+

Thanks.

Keywords: checkin-needed
Comment on attachment 9073394 [details] [diff] [review]
1560014.patch

Review of attachment 9073394 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +587,5 @@
>    nsresult rv = LoadAccounts();
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    bool accountRemoved = m_accounts.RemoveElement(aAccount);
> +  if (MOZ_UNLIKELY(!accountRemoved))

What's `MOZ_UNLIKELY`, only used once in C-C so far.

https://searchfox.org/comm-central/source/mozilla/mfbt/Likely.h#17

It hints the compiler that the expression is unlikely to be true so it can optimize the prediction in the CPU of which branch will be taken (the false one in this case). If this prediction is true, it will be executed faster. So this optimizes the code to run faster in the common case (where this if() will evaluate to false (==account is in the list)).
Yes, we use it rarely explicitly in c-c, but m-c does use it often. Also, it is hidden in all those NS_ENSURE_* macros, which are used for error checking and those are expected to be successful in the common case.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/70cded2918d3
Make nsMsgAccountManager::RemoveAccount() return error when given an already removed account. r=mkmelin

Status: NEW → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 69.0
Attachment #9073394 - Flags: approval-comm-beta+
Comment on attachment 9073394 [details] [diff] [review]
1560014.patch

Another one dependent on bug 1096006 :-(
Attachment #9073394 - Flags: approval-comm-beta+

There is no logical dependence, but I see there may a be code dependence changing the same hunk in test-account-manager-helpers.js.

Lots of hunks failed:

$ hg transplant -e -s ../comm-central/comm/ 70cded2918d3
searching for changes
applying 70cded2918d3
patching file mail/test/mozmill/account/test-account-deletion.js
Hunk #1 FAILED at 18
Hunk #2 FAILED at 77
Hunk #3 FAILED at 99
3 out of 3 hunks FAILED -- saving rejects to file mail/test/mozmill/account/test-account-deletion.js.rej
patching file mail/test/mozmill/account/test-account-tree.js
Hunk #2 FAILED at 153
1 out of 2 hunks FAILED -- saving rejects to file mail/test/mozmill/account/test-account-tree.js.rej
patching file mail/test/mozmill/account/test-mail-account-setup-wizard.js
Hunk #1 FAILED at 39
1 out of 1 hunks FAILED -- saving rejects to file mail/test/mozmill/account/test-mail-account-setup-wizard.js.rej
patching file mail/test/mozmill/shared-modules/test-account-manager-helpers.js
Hunk #1 FAILED at 206
1 out of 1 hunks FAILED -- saving rejects to file mail/test/mozmill/shared-modules/test-account-manager-helpers.js.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a920efa6f820
Backed out changeset 70cded2918d3 for merge conflicts with backout of AM in pref tab. a=backout

Please rebase the patch.

Status: RESOLVED → REOPENED
Flags: needinfo?(acelists)
Resolution: FIXED → ---
Attachment #9073394 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Keywords: checkin-needed
Attachment #9073731 - Flags: review+
Attachment #9073731 - Flags: approval-comm-beta+
Comment on attachment 9073731 [details] [diff] [review]
1560014.patch rebased

Review of attachment 9073731 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/test/unit/test_imapAuthMethods.js
@@ +137,5 @@
>  
>  function deleteIMAPServer(incomingServer) {
>    if (!incomingServer)
>      return;
> +//  MailServices.accounts.removeIncomingServer(incomingServer, false); // TODO cleanup files = true fails

BTW, why remove this call?

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ccd0d7d9d7e5
Make nsMsgAccountManager::RemoveAccount() return error when given an already removed account. r=mkmelin DONTBUILD

Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED

(In reply to Jorg K (GMT+2) from comment #13)

Comment on attachment 9073731 [details] [diff] [review]
1560014.patch rebased

Review of attachment 9073731 [details] [diff] [review]:

::: mailnews/imap/test/unit/test_imapAuthMethods.js
@@ +137,5 @@

function deleteIMAPServer(incomingServer) {
if (!incomingServer)
return;
+// MailServices.accounts.removeIncomingServer(incomingServer, false); // TODO cleanup files = true fails

BTW, why remove this call?

The following .removeAccount() call should also remove its server so this looked superfluous (even dangerous, removing a server that is still pointed to by an account). I'm assuming the default account holds that 'incomingServer'.

Sadly, the test is disabled, so hard to check it.

You need to log in before you can comment on or make changes to this bug.