Closed Bug 1589552 Opened 1 year ago Closed 9 months ago

Conversations list assumes all elements represent a contact

Categories

(Thunderbird :: Instant Messaging, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 76.0

People

(Reporter: clokep, Assigned: khushil324)

References

Details

Attachments

(2 files, 4 obsolete files)

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.

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.

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.

Status: NEW → UNCONFIRMED
Ever confirmed: false

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) {
  1. Compile and such.
  2. Set-up two auto-joined rooms for an IRC account: #testib and #zzzzz.
  3. Connect the account and let the auto-join happen.
  4. 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]

Blocks: 1347533
Attached patch patch (obsolete) — Splinter Review

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.

Assignee: nobody → clokep
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached file A stack of the error
Attached patch Patch v1 (obsolete) — Splinter Review

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.

Attachment #9102602 - Attachment is obsolete: true
Attachment #9102619 - Flags: review?(florian)
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.
Attachment #9102619 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9102619 - Flags: review?(mkmelin+mozilla) → review-
Attached patch bug1589552_addcontact.patch (obsolete) — Splinter Review

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

Attachment #9103230 - Flags: feedback?(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 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.
Attachment #9102619 - Flags: review?(florian)

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.

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!

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.

(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...

Assignee: clokep → 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.

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?

(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.

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.

Attachment #9102619 - Attachment is obsolete: true
Attachment #9103230 - Attachment is obsolete: true
Attachment #9103230 - Flags: feedback?(clokep)
Attachment #9132906 - Flags: review?(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?

(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.

Attachment #9132906 - Attachment is obsolete: true
Attachment #9132906 - Flags: review?(clokep)
Attachment #9133099 - Flags: review?(clokep)
Attachment #9133099 - Flags: review?(clokep) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/3511b452ac30
add tagName param in addContact function in chat-group.js. r=clokep

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 76.0
You need to log in before you can comment on or make changes to this bug.