1589552 - Conversations list assumes all elements represent a contact

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Patrick Cloke [:clokep]]

The list of conversations seems to assume that all conversations are backed by a contact, this is definitely false for conversations backed by a prplIConvChat room.

In particular this breaks a prplIConvChat sending the update-conv-title notification, but I don't see any reason why this shouldn't be allowed.

In particular this refers to the code in chat-contact.js, and chat-group.js. This also has some relationship to conv-imconv.js, but it isn't 100% clear to me what that represents (vs. chat-contact.js).

As far as I can tell this was also broken in the XBL binding (before this was converted to a custom element), but the diff is a bit hard to follow. I suspect this was also broken in Instantbird, but it isn't really worth going back and checking.

Comment 1

[Patrick Cloke [:clokep]]

Looking into this more, it seems that when a conversation element in that list is "updated" (e.g. if the conversation title is changed), then the resulting code follows a path to assume it is a private conversation, as opposed to a chat. This happens in the chat-group code (https://searchfox.org/comm-central/rev/dbc116c2b1bcb8d1c50af5d45b750af2a174a9e3/mail/components/im/content/chat-group.js#155-156) where it removes the "contact", then re-adds the "contact". By calling addContact without a tag makes it assume it is a private conversation, see the definition of addContact: https://searchfox.org/comm-central/rev/dbc116c2b1bcb8d1c50af5d45b750af2a174a9e3/mail/components/im/content/chat-group.js#94-102

The whole idea of depending on a tag to know if this is a chat or a private conversation seems super broken.

Comment 2

[Patrick Cloke [:clokep]]

Ugh, so I can't seem to reproduce this by hacking the IRC code, but I can reproduce this with the Matrix code I'm working on, so I'm not really sure what's going on.

Comment 3

[Patrick Cloke [:clokep]]

OK, I was able to reproduce this:

1. Patch the IRC code to change the channel name on JOIN (this seems to have to do more than change the case, it also seems to require actually re-ordering the channels). I used the following patch:

diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
--- a/chat/protocols/irc/ircBase.jsm
+++ b/chat/protocols/irc/ircBase.jsm
@@ -219,16 +219,18 @@ var ircBase = {
           this.normalize(this._nickname)
         ) {
           // If we join, clear the participants list to avoid errors with
           // repeated participants.
           conversation.removeAllParticipants();
           conversation.left = false;
           conversation.joining = false;
 
+          channelName = channelName.split("").reverse().join("");
+
           // Update the channel name if it has improper capitalization.
           if (channelName != conversation.name) {
             conversation._name = channelName;
             conversation.notifyObservers(null, "update-conv-title");
           }
 
           // If the user parted from this room earlier, confirm the rejoin.
           if (conversation._rejoined) {

2. Compile and such.
3. Set-up two auto-joined rooms for an IRC account: #testib and #zzzzz.
4. Connect the account and let the auto-join happen.
5. You'll see errors and the conversation title will go "undefined".

Errors are:

JavaScript error: chrome://messenger/content/chat/chat-contact.js, line 147: TypeError: this.contact.preferredBuddy is undefined
JavaScript error: file:///Users/pcloke/mozilla/mozilla-central/obj-x86_64-apple-darwin18.7.0-tb/dist/Thunderbird%20Daily.app/Contents/Resources/components/imConversations.js, line 557: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "this.contact.preferredBuddy is undefined" {file: "chrome://messenger/content/chat/chat-contact.js" line: 147}]'[JavaScript Error: "this.contact.preferredBuddy is undefined" {file: "chrome://messenger/content/chat/chat-contact.js" line: 147}]' when calling method: [nsIObserver::observe]

Comment 4

[Patrick Cloke [:clokep]]

Attachment: patch

Florian helped me understand what's going on here a bit and had a suggestion that might clarify this code a bit. The "tag" in this code refers to the HTML (previously XUL) tag to use, not the "tag on the contact". I'm still unsure if this regressed or not though.

Comment 5

[Patrick Cloke [:clokep]]

Attachment: A stack of the error

Comment 6

[Patrick Cloke [:clokep]]

Attachment: Patch v1

This persists the tag type of the element to ensure that conversations stay conversations and contacts stay contacts.

I tested this by using the patching of ircBase.jsm as I stated above. Then joining rooms and ensuring things still updated properly.

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 9102619 [details] [diff] [review]
Patch v1

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

Adding Magnus as a reviewed -- he might have more time and he reviewed the conversation of this to custom elements.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9102619 [details] [diff] [review]
Patch v1

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

Rather bitrotted, but I don't think this is very good. You already have the "is" attribute so not much point adding some other attribute. (Besides that, I don't like adding custom attributes unless really needed.)

Let me add a suggestion.

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: bug1589552_addcontact.patch

Something like this. I'm not familiar with the larger context here. Feel free to do what you want with it.

Comment 10

[Patrick Cloke [:clokep]]

(In reply to Magnus Melin [:mkmelin] from comment #8)

You already have the "is" attribute so not much point adding some other attribute.

From talking on IRC, this was added in bug 1585302, which was not authored when I started writing this patch.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 9102619 [details] [diff] [review]
Patch v1

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

I agree that not adding a tagName attribute is better.

Comment 12

[Patrick Cloke [:clokep]]

I didn't look deeply into this, but it generally seems to work. Unfortunately it seems like something is not getting uninitialized properly though:

1. The rename happens correctly.
2. The chat tab ends up having no selected conversation.
3. A few errors are thrown in the console.

I'll need to copy down the errors again... but something odd is going on.

Comment 13

[Patrick Cloke [:clokep]]

So building on comment 12, the console errors I get are:

JavaScript error: chrome://messenger/content/chat/chat-conversation.js, line 83: TypeError: this.conv is undefined
JavaScript error: file:///Users/pcloke/mozilla/mozilla-central/obj-x86_64-apple-darwin18.7.0-tb/dist/Thunderbird%20Daily.app/Contents/Resources/components/imConversations.js, line 557: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "this.conv is undefined" {file: "chrome://messenger/content/chat/chat-conversation.js" line: 83}]'[JavaScript Error: "this.conv is undefined" {file: "chrome://messenger/content/chat/chat-conversation.js" line: 83}]' when calling method: [nsIObserver::observe]
prpl-irc: [Exception... "[JavaScript Error: "this.conv is undefined" {file: "chrome://messenger/content/chat/chat-conversation.js" line: 83}]'[JavaScript Error: "this.conv is undefined" {file: "chrome://messenger/content/chat/chat-conversation.js" line: 83}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///Users/pcloke/mozilla/mozilla-central/obj-x86_64-apple-darwin18.7.0-tb/dist/Thunderbird%20Daily.app/Contents/Resources/components/imConversations.js :: notifyObservers :: line 557"  data: yes]
JavaScript error: chrome://chat/content/conversation-browser.js, line 425: TypeError: this.docShell is null

I've been struggling to debug this, so if anyone has suggestions I'd love to hear them!

Comment 14

[Jorg K (CEST = GMT+2)]

Drive-by comment: this.conv doesn't look right. Mostly it's this._conv in the file, like there are a few this._conv.title.

Comment 15

[Patrick Cloke [:clokep]]

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

Drive-by comment: this.conv doesn't look right. Mostly it's this._conv in the file, like there are a few this._conv.title.

It seems that conv is just an alias for _conv: https://searchfox.org/comm-central/rev/95aa507b6227d1f8cfd774a625d2256478d8a9e2/mail/components/im/content/chat-conversation.js#1761-1781

Seems like we should just use conv everywhere, but that's a separate issue...

Comment 16

[Khushil Mistry [:khushil324]]

Hello Patrick and Magnus,
I have few questions over here.

With the IRC patch, you are trying to update the name of chat-imconv-richlistitem which will trigger

conversation.notifyObservers(null, "update-conv-title");

which will lead here: https://searchfox.org/comm-central/source/mail/components/im/content/chat-imconv.js#116-118
updateContactPosition function is for updating the rank of the contact-richlistitem, not for the imconv-richlistitem.

We are not providing the feature of updating the name of the chat-imconv-richlistitem in Chat: If you right-click on contact-item, you will see a context menu with options of deleting the item or updating the name. But if you right-click on the imconv-richlistitem, you will see an option of closing the conversation only: https://searchfox.org/comm-central/source/mail/components/im/content/chat-messenger.js#108-116

So what you are doing is something not supported by the Chat. If we want to have a feature where we can update the name of imconv-richlistitem, we need to have another function or tweak the addContact function a bit. Currently, the updating of the contact-richlistitem name is also broken. I will take care of that during the work of this bug. But the main question is, do we want to support the update functionality of the imconv-richlistitem? But looking at the context-menu code, I think we were not supporting this feature.

Comment 17

[Patrick Cloke [:clokep]]

I believe that we expect both IMs and chats to be able to have their name changed. I think we've just never been in a situation (before Matrix) where the name of a chat can actually change after being created.

I'm unsure what this has to do with the context menu code though -- I think this is the code to add an alias to a contact?

Comment 18

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #17)

I believe that we expect both IMs and chats to be able to have their name changed. I think we've just never been in a situation (before Matrix) where the name of a chat can actually change after being created.

Now I get that, Thanks. I will now look into it deeply. Magnus's patch is working but showing the errors you mentioned in comment #13 are there. I will start debugging them.

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: Bug-1589552_tagName-in-addContact-chat-0.patch

Here, the issue was related to re-selecting the previously selected item. The removeContact function call changes the selected item through item.destroy call: https://searchfox.org/comm-central/source/mail/components/im/content/chat-imconv.js#246-250
Because of this, the correct item was not getting selected and this.conv related issues were there.

Comment 20

[Patrick Cloke [:clokep]]

Comment on attachment 9132906 [details] [diff] [review]
Bug-1589552_tagName-in-addContact-chat-0.patch

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

::: mail/components/im/content/chat-group.js
@@ +160,3 @@
>            list.selectedItem = newItem;
> +        } else {
> +          list.selectedItem = selectedItem;

Isn't `list.selectedItem` still set to `selectedItem`, why do we need to set it again here?

Comment 21

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #20)

Isn't list.selectedItem still set to selectedItem, why do we need to set
it again here?

Yes, else condition is not needed. Removing it and submiiting a patch in a while.

Comment 22

[Khushil Mistry [:khushil324]]

Attachment: Bug-1589552_tagName-in-addContact-chat-1.patch

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3511b452ac30
add tagName param in addContact function in chat-group.js. r=clokep