Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: hadirezaei, Unassigned)

Tracking

Thunderbird Tracking Flags

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed)

Details

(Whiteboard: [1.6-blocking])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [1.6-blocking]

Comment 1

5 years ago
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
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...
Posted patch WIP v1 (obsolete) — Splinter Review
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.
Attachment #8534722 - Flags: feedback?(aleth)

Comment 4

5 years ago
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?
Attachment #8534722 - Flags: feedback?(aleth) → feedback+

Comment 5

4 years ago
Posted patch accountdelete.diff (obsolete) — Splinter Review
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.
Attachment #8594394 - Flags: review?(clokep)
Comment on attachment 8594394 [details] [diff] [review]
accountdelete.diff

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

Well that was simpler!
Attachment #8594394 - Flags: review?(clokep) → review+

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6

Comment 8

4 years ago
Added "|| this.connecting" before pushing.

[Approval Request Comment]
Low risk bugfix.
Attachment #8534722 - Attachment is obsolete: true
Attachment #8594394 - Attachment is obsolete: true
Attachment #8594449 - Flags: review+
Attachment #8594449 - Flags: approval-comm-beta?
Attachment #8594449 - Flags: approval-comm-aurora?
Comment on attachment 8594449 [details] [diff] [review]
accountdelete.diff v2

https://hg.mozilla.org/releases/comm-aurora/rev/e4457a5dd5f7
Attachment #8594449 - Flags: approval-comm-aurora? → approval-comm-aurora+
Comment on attachment 8594449 [details] [diff] [review]
accountdelete.diff v2

https://hg.mozilla.org/releases/comm-beta/rev/4231e422562a
Attachment #8594449 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.