1331210 - Followup issues with account data deletion completeness, post Bug 274452.

Status: NEW

(MailNews Core :: Account Manager, defect)

Description

[alta88]

Some issues on Tb 53.0ab, linux.

1. If an account is a default, its data is not deleted despite the user checking the box. Testing discovered this by having an existing feed account (the default), creating a gmail imap account (which became the default), and subsequent delete of the imap account, which did not remove any files/prefs/passwords (nor indicate it didn't despite being removed from folderpane).  This is wrong; the data should be deleted.

2. In [profD]/ImapMail and /News folders, an account such as imap.gmail.com also has an imap.gmail.com.msf file created.  These are erroneously not also deleted with account data.  So dupes happen if creating the name again.

3. In a /News folder, in addition to .msf, an account like news.mozilla.org will have a newsrc-news.mozilla.org created. It is not deleted either, causing dupes if the account is recreated.  This should check the account pref for newsrc.

4. In /ImapMail and /News, if the only/last account is removed, those folders should also be removed.

5. Why is account.incomingServer.forgetPassword() not used instead of the strange Services.logins loop, which seems cargo culted. If forgetPassword() has an issue, it should be fixed there.

6. If the server key of the account being deleted matches pref mail.account.lastKey, then the pref value should be decremented by 1 to at least avoid some account/server key mismatching.

Note that implementation of bug 274452 is a great improvement and is appreciated.

Comment 1

[:aceman]

Thanks for thoroughly trying out the functionality.

(In reply to alta88 from comment #0)
> 1. If an account is a default, its data is not deleted despite the user
> checking the box. Testing discovered this by having an existing feed account
> (the default), creating a gmail imap account (which became the default), and
> subsequent delete of the imap account, which did not remove any
> files/prefs/passwords (nor indicate it didn't despite being removed from
> folderpane).  This is wrong; the data should be deleted.

Interesting. I should try this.
 
> 2. In [profD]/ImapMail and /News folders, an account such as imap.gmail.com
> also has an imap.gmail.com.msf file created.  These are erroneously not also
> deleted with account data.  So dupes happen if creating the name again.

Do you know what are these files for?
 
> 4. In /ImapMail and /News, if the only/last account is removed, those
> folders should also be removed.

Interesting idea. Is that worth it?
 
> 5. Why is account.incomingServer.forgetPassword() not used instead of the
> strange Services.logins loop, which seems cargo culted. If forgetPassword()
> has an issue, it should be fixed there.

Yes, this should have been fixed in bug 1308767. What precise account type still does not work for you?
You mention the Services.logins loop, but that should not exist in TB53. Is there another one you see?
 
> 6. If the server key of the account being deleted matches pref
> mail.account.lastKey, then the pref value should be decremented by 1 to at
> least avoid some account/server key mismatching.

Why? We intentionally want that the next created account gets a new incremental key number, so that account numbers never repeat in the history of the profile. That is the only purpose of that pref. Why should it be ever decremented?
 
> Note that implementation of bug 274452 is a great improvement and is
> appreciated.

Thanks. In that bug I merely exposed the already existing backend in the UI. If some account types do not remove their files properly, we should polish those now. But at least some files for some accounts are deleted.

Comment 2

[alta88]

(In reply to :aceman from comment #1)
> Thanks for thoroughly trying out the functionality.
> 
> (In reply to alta88 from comment #0)
> > 1. If an account is a default, its data is not deleted despite the user
> > checking the box. Testing discovered this by having an existing feed account
> > (the default), creating a gmail imap account (which became the default), and
> > subsequent delete of the imap account, which did not remove any
> > files/prefs/passwords (nor indicate it didn't despite being removed from
> > folderpane).  This is wrong; the data should be deleted.
> 
> Interesting. I should try this.
>  
> > 2. In [profD]/ImapMail and /News folders, an account such as imap.gmail.com
> > also has an imap.gmail.com.msf file created.  These are erroneously not also
> > deleted with account data.  So dupes happen if creating the name again.
> 
> Do you know what are these files for?
>  

Some server folders have .msf created, not important why or if even necessary here, just to make sure they get deleted. Although a followup could see if they are necessary in fact.

> > 4. In /ImapMail and /News, if the only/last account is removed, those
> > folders should also be removed.
> 
> Interesting idea. Is that worth it?
>  

I obviously think so, it's not optimal cleanup otherwise.  Empty folders should not hang around, plus if needed they're created on demand anyway.

> > 5. Why is account.incomingServer.forgetPassword() not used instead of the
> > strange Services.logins loop, which seems cargo culted. If forgetPassword()
> > has an issue, it should be fixed there.
> 
> Yes, this should have been fixed in bug 1308767. What precise account type
> still does not work for you?
> You mention the Services.logins loop, but that should not exist in TB53. Is
> there another one you see?
>  

I only looked at the patch code, if that has been superseded by a later bug to use forgetPassword() then great.

> > 6. If the server key of the account being deleted matches pref
> > mail.account.lastKey, then the pref value should be decremented by 1 to at
> > least avoid some account/server key mismatching.
> 
> Why? We intentionally want that the next created account gets a new
> incremental key number, so that account numbers never repeat in the history
> of the profile. That is the only purpose of that pref. Why should it be ever
> decremented?

If you create key 4, account key 4 and server key 4, then delete the account/server, why should the next keys be 5, thus leaving 4 unused.  There should be no remnant of 4 and so it should be reused.  It feels sloppy to me otherwise.  
>  
> > Note that implementation of bug 274452 is a great improvement and is
> > appreciated.
> 
> Thanks. In that bug I merely exposed the already existing backend in the UI.
> If some account types do not remove their files properly, we should polish
> those now. But at least some files for some accounts are deleted.

Comment 3

[:aceman]

(In reply to alta88 from comment #2)
> If you create key 4, account key 4 and server key 4, then delete the
> account/server, why should the next keys be 5, thus leaving 4 unused.  There
> should be no remnant of 4 and so it should be reused.  It feels sloppy to me
> otherwise.  

There still remain references to account 4 even after its deletion. See bug 485839 where this lastKey facility was intentionally introduced to be strictly monotonic increasing.

Comment 4

[alta88]

Ah. The classic data leak into disparate silos problem. Nevermind on 6 then.

Btw, it's wrong that feeds accounts are placed in /Mail, they should be in a [profD]/Feeds folder. Any interest in fixing this while here?

Comment 5

[:aceman]

I'd say that should have a separate bug.

Comment 7

[Joe Hecht]

Attachment: same problem

Thanks from warrovade1982.