1109589 - Can't delete IRC account

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Hadi Rezaee]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; rv:11.0) like Gecko

Steps to reproduce:

1: Add an irc account. In my case, i added this server:
irc.eu.synirc.net
2: after you added the account and you're connected to the server, try going to the accounts list and delete the server.



Actual results:

The delete button does not work, so your account won't be deleted. error console error below:

Timestamp: 12/10/2014 4:45:12 PM
Error: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "this.conversations is undefined" {file: "resource://gre/components/irc.js" line: 1760}]'[JavaScript Error: "this.conversations is undefined" {file: "resource://gre/components/irc.js" line: 1760}]' when calling method: [prplIAccount::remove]
Source File: resource://gre/components/imAccounts.js
Line: 623


Expected results:

Account should be deleted.

Comment 1

[aleth [:aleth]]

STR: add Freenode account, connect, delete (no open conversations). Slightly different errors produced, but basically the same issue.

Timestamp: 10/12/2014 16:28:50
Warning: ReferenceError: reference to undefined property this.buddies
Source File: /components/irc.js
Line: 1768
Timestamp: 10/12/2014 16:28:50
Error: TypeError: this.buddies is undefined
Source File: /components/irc.js
Line: 1768
Timestamp: 10/12/2014 16:28:50
Error: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "this.buddies is undefined" {file: "components/irc.js" line: 1768}]'[JavaScript Error: "this.buddies is undefined" {file: "components/irc.js" line: 1768}]' when calling method: [prplIAccount::unInit]
Source File: components/imAccounts.js
Line: 651
Timestamp: 10/12/2014 16:28:50
Error: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "this.buddies is undefined" {file: "components/irc.js" line: 1768}]'[JavaScript Error: "this.buddies is undefined" {file: "/components/irc.js" line: 1768}]' when calling method: [prplIAccount::unInit]
Source File: /components/imAccounts.js
Line: 651

Comment 2

[Patrick Cloke [:clokep]]

What's happening here is that remove() gets called, then the account gets unInited, IRC's unInit calls quit() first, which causes us to try to tell the server things about conversations that have already been forgotten by "remove".

My inclination is that the account should be disconnected before being removed, but that's async...

Comment 3

[Patrick Cloke [:clokep]]

Attachment: WIP v1

This is the basic fix...but after appyling this I get a "this.imAccount is not defined" error inside of jsProtoHelper line 46. It seems to be coming when the socket is clearing it's buffer.

Comment 4

[aleth [:aleth]]

Comment on attachment 8534722 [details] [diff] [review]
WIP v1

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

::: chat/protocols/irc/irc.js
@@ +1766,5 @@
>    },
>  
>    remove: function() {
> +    // Ensure we're disconnected before cleaning up.
> +    this.disconnect();

If remove() should call disconnect(), that should be in jsProtoHelper too. Are we sure all other prpls do this? (If they don't, that seems like a bug.) Would it be better to call disconnect() before remove() is called, in imAccounts or whereever?

Since disconnect() is async, does this work without errors (what happens to the server response?)

It would also be cleaner to chain remove() on the disconnection completing, since that is async. Maybe put the remove/unInit/... stuff in an account-disconnected observer?

@@ +1776,5 @@
>    },
>  
>    unInit: function() {
> +    // Disconnect before freeing memory.
> +    this.disconnect();

I wonder if we didn't do this because this.disconnect starts a quit timer?

Comment 5

[aleth [:aleth]]

Attachment: accountdelete.diff

This seems simpler and cleaner to me.

It fixes this bug for IRC because if the account is already disconnecting when unInit is called, unInit doesn't call quit.

Comment 6

[Patrick Cloke [:clokep]]

Comment on attachment 8594394 [details] [diff] [review]
accountdelete.diff

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

Well that was simpler!

Comment 7

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/6d58afb3c8de

Comment 8

[aleth [:aleth]]

Attachment: accountdelete.diff v2

Added "|| this.connecting" before pushing.

[Approval Request Comment]
Low risk bugfix.

Comment 9

[Kent James (:rkent)]

Comment on attachment 8594449 [details] [diff] [review]
accountdelete.diff v2

https://hg.mozilla.org/releases/comm-aurora/rev/e4457a5dd5f7

Comment 10

[Kent James (:rkent)]

Comment on attachment 8594449 [details] [diff] [review]
accountdelete.diff v2

https://hg.mozilla.org/releases/comm-beta/rev/4231e422562a