1322473 - Twitter account isn't removed when choosing to remove data

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Onno Ekker [:nONoNonO UTC+1]]

There's a new option to also remove account data when removing an account. This doesn't work for chat accounts, at least not for a Twitter account. When you choose that option all seems OK, the dialog shows Account removal succeeded, but after restarting the account is shown again.

Possible related messages from the error console:

TypeError: this.imAccount is null[Learn More]

TypeError: localDirectory is null[Learn More]  removeAccount.js:25:3
	onLoad chrome://messenger/content/removeAccount.js:25:3
	onload chrome://messenger/content/removeAccount.xul:1:1
	onRemoveAccount chrome://messenger/content/AccountManager.js:759:3
	oncommand chrome://messenger/content/AccountManager.xul:1:1
	MsgAccountManager chrome://messenger/content/accountUtils.js:255:9
	oncommand chrome://messenger/content/messenger.xul:1:1
 
TypeError: srcFile is null[Learn More]

Comment 1

[:aceman]

Are you sure this only happens when choosing "remove account data"?

Comment 2

[Onno Ekker [:nONoNonO UTC+1]]

I think on one computer I was able to remove my twitter account, on the other I wasn't able at all: whether I chose remove account data or not, the account was there again after restarting.

Comment 3

[:aceman]

Attachment: WIP patch

This was caused by bug 274452 implementing a new account removal dialog allowing to remove also files belonging to an account.

The problems:
1. The new dialog calls accountmanager.removeAccount() with the argument to also remove account files. That causes it to call account.incomingServer.removeFiles(). This function does not exist in IM accounts. It seems this was failing silently (without triggering the catch {} block in the caller to indicate removal failed). So I implement the function.

2. The new code also calls account.incomingServer.localPath, which is not implemented for IM, as it seems to not have any local storage. In this patch I chose to hide showing the local path in the dialog if it is null. But I'm open for proposals what to show here for IM. E.g. we could show the path to the logs, that we are going to remove if "remove message data" is chosen.

3. Bug 1308767 added calling account.incomingServer.forgetPassword which again is not implemented for IM accounts. But the passwords are removed as part of the account deletion so no feature is lost. Also this call was in a try{} catch{} block so no exception was thrown. Anyway, I add the function as a stub to not throw unnecessarily and add comments to note these findings.

Most of the changes done in chat/im files were suggested by aleth.

Comment 4

[aleth [:aleth]]

Comment on attachment 8817734 [details] [diff] [review]
WIP patch

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

(In reply to :aceman from comment #3)
> 2. The new code also calls account.incomingServer.localPath, which is not
> implemented for IM, as it seems to not have any local storage. In this patch
> I chose to hide showing the local path in the dialog if it is null. But I'm
> open for proposals what to show here for IM. E.g. we could show the path to
> the logs, that we are going to remove if "remove message data" is chosen.

The only local storage are the chat logs. Iirc chat logging is optional and there is some place in Preferences you can turn it off. When the log folder exists for an account, it would make sense to show it as you suggest, if the mail account equivalent is to open the profile subfolder with the mail data.

::: mail/components/im/imIncomingServer.js
@@ +48,5 @@
>    },
> +
> +  // Removes files created by this account.
> +  removeFiles: function() {
> +    Services.logs.deleteLogFolderForAccount(this.imAccount);

Is this called after the account deletion or before? Can one be sure there are no open connections to that account at this point?

Is there some provision for account deletion on the server? ("close my email account")

Comment 5

[:aceman]

(In reply to aleth [:aleth] from comment #4)
> The only local storage are the chat logs. Iirc chat logging is optional and
> there is some place in Preferences you can turn it off. When the log folder
> exists for an account, it would make sense to show it as you suggest, if the
> mail account equivalent is to open the profile subfolder with the mail data.

Ok. I can check if it exists. If there is no .localPath defined or no log folder (or they don't exist on disk)
I could hide the option to remove account data. Hopefully there is no server type that stores more data in other folders that are not covered by these 2 attributes.
 
> ::: mail/components/im/imIncomingServer.js
> @@ +48,5 @@
> >    },
> > +
> > +  // Removes files created by this account.
> > +  removeFiles: function() {
> > +    Services.logs.deleteLogFolderForAccount(this.imAccount);
> 
> Is this called after the account deletion or before? Can one be sure there
> are no open connections to that account at this point?

It is called inside account deletion at https://dxr.mozilla.org/comm-central/rev/29b034f30fcb12b0e252a33502ba44521f5768d9/mailnews/base/src/nsMsgAccountManager.cpp#557 . There is a LogoutOfServer and similar calls before that so any activity should be stopped. Can you check if that does what is needed on IM? (I do not see that function implemented for IM).
 
> Is there some provision for account deletion on the server? ("close my email
> account")

There isn't and that would be probably hard to do. We explicitly say the data on server is kept in the description: https://dxr.mozilla.org/comm-central/rev/29b034f30fcb12b0e252a33502ba44521f5768d9/mail/locales/en-US/chrome/messenger/removeAccount.dtd#14

You may want to improve the wording for IM, I've implemented it so that IM can have their own description (now it mentions the logs).

Comment 6

[aleth [:aleth]]

> > > +  removeFiles: function() {
> > > +    Services.logs.deleteLogFolderForAccount(this.imAccount);
> > 
> > Is this called after the account deletion or before? Can one be sure there
> > are no open connections to that account at this point?
> 
> It is called inside account deletion at
> https://dxr.mozilla.org/comm-central/rev/
> 29b034f30fcb12b0e252a33502ba44521f5768d9/mailnews/base/src/
> nsMsgAccountManager.cpp#557 . There is a LogoutOfServer and similar calls
> before that so any activity should be stopped. Can you check if that does
> what is needed on IM? (I do not see that function implemented for IM).

Thanks for the pointer. The sequence is trouble as it is, as removeFiles is called before the account is deleted in clearAllValues. That means the account may still be connected with data incoming. So the log folder will get deleted and immediately recreated if you are unlucky.

LogoutofServer calls shutdown which does nothing in imIncomingServer, maybe you can disconnect the account there (imIAccount.disconnect). The mail/ code isn't ideal here because disconnection should be async (it takes time) but this code is all synchronous.

Comment 7

[:aceman]

Attachment: patch v2

Thanks. Implemented also server.shutdown to disconnect from the server.

The option to remove message data is only shown when the log directory exists.

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8817813 [details] [diff] [review]
patch v2

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

I assume we purge the buddy list, etc. during the account deletion? That's the only other thing I can think of that's on disk. And maybe cached buddy icons?

::: chat/components/public/imILogger.idl
@@ +94,5 @@
>    // until the promise resolves. If the callback throws (or rejects), iteration
>    // will stop and the returned promise will reject with the same error.
>    jsval forEach(in imIProcessLogsCallback aCallback);
> +
> +  // Removes the folder storing all logs for aAccount.

I think you forgot to update this comment.

Comment 9

[Richard Marti (:Paenglab)]

What about changing "Remove message data" to "Remove conversation data" when a IM account is selected?

Comment 10

[aleth [:aleth]]

Comment on attachment 8817813 [details] [diff] [review]
patch v2

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

::: chat/components/public/imILogger.idl
@@ +97,5 @@
> +
> +  // Removes the folder storing all logs for aAccount.
> +  AUTF8String getLogFolderPathForAccount(in imIAccount aAccount);
> +
> +  // Removes the folder storing all logs for aAccount.

Add a comment that the account should be disconnected before calling.

::: chat/components/src/logger.js
@@ +813,5 @@
> +  getLogFolderPathForAccount: function(aAccount) {
> +    return getLogFolderPathForAccount(aAccount);
> +  },
> +
> +  deleteLogFolderForAccount: function(aAccount) {

This should probably throw an error if (!aAccount.disconnecting || !aAccount.disconnected) and a warning in the console if (aAccount.disconnecting).

@@ +816,5 @@
> +
> +  deleteLogFolderForAccount: function(aAccount) {
> +    let logPath = this.getLogFolderPathForAccount(aAccount);
> +    // Find all operations on files inside the log folder.
> +    let pendingTasks = [];

nit: pendingPromises is less confusing, a Task is something else ;)

@@ +817,5 @@
> +  deleteLogFolderForAccount: function(aAccount) {
> +    let logPath = this.getLogFolderPathForAccount(aAccount);
> +    // Find all operations on files inside the log folder.
> +    let pendingTasks = [];
> +    function checkLogFiles(promiseOperation, filePath, map) {

map is unused, you can leave it out.

@@ +824,5 @@
> +    }
> +    gFilePromises.forEach(checkLogFiles);
> +    // After all operations finish, remove the whole log folder.
> +    return Promise.all(pendingTasks)
> +                  .then(values => { OS.File.removeDir(logPath, { ignoreAbsent: true }); });

add a .catch() that Cu.reportErrors

Comment 11

[Richard Marti (:Paenglab)]

Comment on attachment 8817813 [details] [diff] [review]
patch v2

I'm minusing now the ui-r because of my comment 9 and both r? are also -. Using "messages" in this context feels wrong for me.

Comment 12

[:aceman]

(In reply to Patrick Cloke [:clokep] from comment #8)
> I assume we purge the buddy list, etc. during the account deletion? That's
> the only other thing I can think of that's on disk. And maybe cached buddy
> icons?

Where are those stored?

Comment 13

[:aceman]

Attachment: patch v3

Addressed all comments.

The removal of buddies is still needs to be addressed if needed.

Comment 14

[Richard Marti (:Paenglab)]

Comment on attachment 8819568 [details] [diff] [review]
patch v3

Yeah, I think this is the better description for IM accounts.

Comment 15

[aleth [:aleth]]

(In reply to :aceman from comment #12)
> (In reply to Patrick Cloke [:clokep] from comment #8)
> > I assume we purge the buddy list, etc. during the account deletion? That's
> > the only other thing I can think of that's on disk. And maybe cached buddy
> > icons?
> 
> Where are those stored?

The contacts are in a sql database. I'm pretty sure removing an account deletes the associated contacts.

Comment 16

[aleth [:aleth]]

Comment on attachment 8819568 [details] [diff] [review]
patch v3

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

Looks good otherwise! Have you tested this?

Since this should be fixed in 52 as well, do you plan to find some way to land a version of this without new strings in aurora, or will you ask for late l10n?

::: chat/components/src/logger.js
@@ +814,5 @@
> +    return getLogFolderPathForAccount(aAccount);
> +  },
> +
> +  deleteLogFolderForAccount: function(aAccount) {
> +    if (!aAccount.disconnecting || !aAccount.disconnected)

Should be && (my mistake)

Comment 17

[:aceman]

(In reply to aleth [:aleth] from comment #16)
> Looks good otherwise! Have you tested this?

Sure, but as I am not familiar with IM accounts much, it'd be fine if you hammered on it with some edge cases :)
 
> Since this should be fixed in 52 as well, do you plan to find some way to
> land a version of this without new strings in aurora, or will you ask for
> late l10n?

Yes, that's the plan. It should work without string changes, if you are content with the idea to use the already existing email account strings also for IM accounts.

Comment 18

[:aceman]

Attachment: patch v3.1

Comment 19

[aleth [:aleth]]

I tried the following:

Create freenode IRC account, join #ubuntu and #python, wait for some messages to show up. Have the connected chatroom open in the chat tab.
Now remove the account (Chat "show accounts" -> Properties -> Remove account from the dropdown)
The account is removed, I get "Removal succeeded".

But there is *always* an error message:

23:16:31.325 A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Mon Dec 19 2016 23:16:31 GMT+0100 (CET)
Full Message: TypeError: this._tree.rowCountChanged is not a function
Full Stack: cLTV__rebuild@chrome://messenger/content/chat/chat-messenger-overlay.js:1218:7
chatLogTreeView@chrome://messenger/content/chat/chat-messenger-overlay.js:1205:3
_showLogList@chrome://messenger/content/chat/chat-messenger-overlay.js:489:37
onListItemSelected/<@chrome://messenger/content/chat/chat-messenger-overlay.js:688:9
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:932:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7
get _worker/worker.onmessage@resource://gre/modules/PromiseWorker.jsm:231:9
onRemoveAccount@chrome://messenger/content/AccountManager.js:759:3
oncommand@chrome://messenger/content/AccountManager.xul:1:1
am_edit@chrome://messenger/content/chat/imAccounts.js:315:7
oncommand@chrome://messenger/content/chat/imAccounts.xul:1:1
 1 chat-messenger-overlay.js:1218

Comment 20

[:aceman]

(In reply to aleth [:aleth] from comment #19)
> The account is removed, I get "Removal succeeded".

Is it really removed, or just the message says so?

Did you choose to remove the conversation data?
 
> 23:16:31.325 A promise chain failed to handle a rejection. Did you forget to
> '.catch', or did you forget to 'return'?
> See
> https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/
> Promise

I filed a bug about this invalid URL.
 
> Date: Mon Dec 19 2016 23:16:31 GMT+0100 (CET)
> Full Message: TypeError: this._tree.rowCountChanged is not a function
> Full Stack:
> Full Stack: cLTV__rebuild@chrome://messenger/content/chat/chat-messenger-overlay.js:1218:7
> chatLogTreeView@chrome://messenger/content/chat/chat-messenger-overlay.js:1205:3
> _showLogList@chrome://messenger/content/chat/chat-messenger-overlay.js:489:37
> onListItemSelected/<@chrome://messenger/content/chat/chat-messenger-overlay.js:688:9

https://dxr.mozilla.org/comm-central/rev/b020cffa188fac078bff74c9e19c98b044ea4173/mail/components/im/content/chat-messenger-overlay.js#1218

Is the chatLogTreeView used also for displaying of active chatroom? (or only for old logs)

> onRemoveAccount@chrome://messenger/content/AccountManager.js:759:3
Why does the stack contain nothing from removeAccount.js?

> oncommand@chrome://messenger/content/AccountManager.xul:1:1
> am_edit@chrome://messenger/content/chat/imAccounts.js:315:7
> oncommand@chrome://messenger/content/chat/imAccounts.xul:1:1
>  1 chat-messenger-overlay.js:1218

Comment 21

[aleth [:aleth]]

(In reply to :aceman from comment #20)
> Is it really removed, or just the message says so?

Yes

> Did you choose to remove the conversation data?

Yes

> > Date: Mon Dec 19 2016 23:16:31 GMT+0100 (CET)
> > Full Message: TypeError: this._tree.rowCountChanged is not a function
> > Full Stack:
> > Full Stack: cLTV__rebuild@chrome://messenger/content/chat/chat-messenger-overlay.js:1218:7
> > chatLogTreeView@chrome://messenger/content/chat/chat-messenger-overlay.js:1205:3
> > _showLogList@chrome://messenger/content/chat/chat-messenger-overlay.js:489:37
> > onListItemSelected/<@chrome://messenger/content/chat/chat-messenger-overlay.js:688:9
> 
> https://dxr.mozilla.org/comm-central/rev/
> b020cffa188fac078bff74c9e19c98b044ea4173/mail/components/im/content/chat-
> messenger-overlay.js#1218
> 
> Is the chatLogTreeView used also for displaying of active chatroom? (or only
> for old logs)

It's the "Previous conversations" tree displayed for the active chatroom. Probably there is some race condition here with the account being removed before or during the chat log tree trying to update itself?

> > onRemoveAccount@chrome://messenger/content/AccountManager.js:759:3
> Why does the stack contain nothing from removeAccount.js?

I don't know. I just gave you STR ;)

Comment 22

[aleth [:aleth]]

Maybe the then() handler in onListItemSelected/<@chrome://messenger/content/chat/chat-messenger-overlay.js:688:9 gets called when the conversation is already closed, and then showLogList errors?

Comment 23

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #22)
> Maybe the then() handler in
> onListItemSelected/<@chrome://messenger/content/chat/chat-messenger-overlay.
> js:688:9 gets called when the conversation is already closed, and then
> showLogList errors?

Yes, looks like _showLogList should check if (document.getElementById("conversationsDeck").selectedPanel == document.getElementById("noConvScreen")), and/or the then() in line 688 should check whether document.getElementById("conversationsDeck").selectedPanel is still pointing at the same conversation the logs were fetched for.

Comment 24

[Onno Ekker [:nONoNonO UTC+1]]

(In reply to :aceman from comment #17)
> (In reply to aleth [:aleth] from comment #16)
> ...
> > Since this should be fixed in 52 as well, do you plan to find some way to
> > land a version of this without new strings in aurora, or will you ask for
> > late l10n?
> 
> Yes, that's the plan. It should work without string changes, if you are
> content with the idea to use the already existing email account strings also
> for IM accounts.

What is the plan? To land a new version without string changes, or to ask for late l10n? I hope the latter, because otherwise the new strings for IM won't make it until TB59 next year. But if you want to ask for l10n changes, I think you shouldn't wait too long and maybe ask for a preliminary review for the string changes...

Comment 25

[aleth [:aleth]]

Attachment: Ensure the selected conversation hasn't changed before updating the log list

In the interest of unblocking this, this patch fixes the issue from comment 23, which was only uncovered by this bug, but isn't directly related.

Comment 26

[:aceman]

Thanks. I wanted to ask if the method removing an IM account shouldn't close any open chats/log windows for that account?

Comment 27

[aleth [:aleth]]

(In reply to :aceman from comment #26)
> Thanks. I wanted to ask if the method removing an IM account shouldn't close
> any open chats/log windows for that account?

This already happens, that's what triggers the error in comment 19 as log results arrive for conversations that have already been closed.

Comment 28

[:aceman]

Comment on attachment 8819625 [details] [diff] [review]
patch v3.1

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

Ok, thanks. Then I understand we can move with this patch as is.

Comment 29

[Patrick Cloke [:clokep]]

Comment on attachment 8824745 [details] [diff] [review]
Ensure the selected conversation hasn't changed before updating the log list

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

We talked about this on IRC. It seems reasonable to me!

Comment 30

[Philip Chee]

Comment on attachment 8819625 [details] [diff] [review]
patch v3.1

rs=me for the /suite/changes.

Comment 31

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/7b3948ead9381fa9c7fd7b1ba0a76b06c4f7395a
Bug 1322473 - Ensure the selected conversation hasn't changed before updating the log list. r=clokep

https://hg.mozilla.org/comm-central/rev/b0e38b2324c339f3e765ab8f63acabf10cbaaa73
Bug 1322473 - Support removing IM accounts from the UI and implement forgetPassword() and removeFiles() for imIncomingServer. ui-r=Paenglab, r=aleth,mkmelin rs,a=Ratty

Comment 32

[aleth [:aleth]]

Resolved fixed, but this still requires some action for 52, hopefully before the next merge on Jan 23.

Comment 33

[Jorg K (CEST = GMT+2)]

(In reply to aleth [:aleth] from comment #32)
> Resolved fixed, but this still requires some action for 52, hopefully before
> the next merge on Jan 23.
Meaning? ;-)

Comment 34

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #33)
> (In reply to aleth [:aleth] from comment #32)
> > Resolved fixed, but this still requires some action for 52, hopefully before
> > the next merge on Jan 23.
> Meaning? ;-)

See comment 17.

Comment 35

[:aceman]

Yes, I'll try editing the patch.

Comment 36

[:aceman]

Attachment: patch for TB52

Can you see if this works for you in TB52? I kept all the code but reused existing strings also for the chat specific elements.

Comment 37

[aleth [:aleth]]

Comment on attachment 8824745 [details] [diff] [review]
Ensure the selected conversation hasn't changed before updating the log list

[Approval Request Comment]
User impact if declined: errors on removing log files (and conceivably some  other rare situations)
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): none

Comment 38

[aleth [:aleth]]

Comment on attachment 8826293 [details] [diff] [review]
patch for TB52

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

I can't test this at the moment due to laptop trouble, but it looks OK to me. Maybe Paenglab could take a look as it's mainly a UI decision whether the current strings are "good enough" to be clear.

Comment 39

[Jorg K (CEST = GMT+2)]

Comment on attachment 8824745 [details] [diff] [review]
Ensure the selected conversation hasn't changed before updating the log list

Sure, this goes with the TB 52 patch, right?

Weren't you going to do your own approvals/uplifts? But no problem, I already have two bugs for uplift in the queue.

Comment 40

[Richard Marti (:Paenglab)]

Comment on attachment 8826293 [details] [diff] [review]
patch for TB52

The labels aren't 100% optimal but "good enough". I haven't checked the other code which should be the same as for TB 53.

Comment 41

[Jorg K (CEST = GMT+2)]

Comment on attachment 8826293 [details] [diff] [review]
patch for TB52

OK, let's take this to TB 52.

Comment 42

[Jorg K (CEST = GMT+2)]

Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/66831b0dbb31647ef2bd7381063f7104ebf98cef
https://hg.mozilla.org/releases/comm-aurora/rev/b06fe5e65af2e86b972648d5ef47ee64d267494a

Note that imILogger.idl was changed here and I got an error when uplifting since the pretxnchangegroup.e_idl_no_uuid hook is still in place. So I had to force it.

Normally interface changes prohibit uplift. So this was intentional?

Comment 43

[:aceman]

Yeah, I forgot about that, but we probably can't fix the bug without those changes. Also the imILogger.idl and logger.js changes only add new methods so I'd think any existing callers/addons of the imILogger object shouldn't be affected.

Comment 44

[Jorg K (CEST = GMT+2)]

As far as I know, the UUID changes were only necessary for binary add-ons, right? Since we don't support those any more, added interfaces shouldn't be a problem.

Comment 45

[:aceman]

Yes, but we can still be nice to JS addons and not change e.g. function arguments in the middle of ESR beta. That is what I considered now and I think just adding new methods should be fine.

Comment 46

[Patrick Cloke [:clokep]]

(In reply to Jorg K (GMT+1) from comment #42)
> Aurora (TB 52):
> https://hg.mozilla.org/releases/comm-aurora/rev/
> 66831b0dbb31647ef2bd7381063f7104ebf98cef
> https://hg.mozilla.org/releases/comm-aurora/rev/
> b06fe5e65af2e86b972648d5ef47ee64d267494a
> 
> Note that imILogger.idl was changed here and I got an error when uplifting
> since the pretxnchangegroup.e_idl_no_uuid hook is still in place. So I had
> to force it.
> 
> Normally interface changes prohibit uplift. So this was intentional?

We don't normally lock down IDL changes except for on ESR. I thought we had these disabled but...maybe not. I agree with aleth that since this is just adding to the interface it should be fine.

Comment 47

[Jorg K (CEST = GMT+2)]

I think you meant Aceman, since he replied. Clearing NI for Aleth.