1592505 - XMPP message window ignores "return" so no messages can be sent

Status: VERIFIED FIXED

(Chat Core :: General, defect)

Description

[Ari Maniatis]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

• upgrade to Thunderbird 71.0b1 on OSX
• create new instant message window for XMPP connection
• type message of any length
• hit return

Actual results:

Nothing happens. Message is not sent.

Expected results:

Message should be sent. There is no workaround I could find. All add-ons disabled. Reproduced on both iMac and Macbook Pro.

Option-return successfully adds a new line.

Comment 1

[Patrick Cloke [:clokep]]

Thanks for filing this! I had noticed it yesterday and didn't have time to file a bug yet.

Another interesting thing to note -- I saw this only with XMPP accounts. IRC accounts seem fine.

Comment 2

[Patrick Cloke [:clokep]]

I confirmed this is broken in TB71b1 (and b2). This worked fine in TB70bn, since I was using XMPP last week without an issue.

I se the following in the error console:

NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [prplIConversation.prepareForSending] imConversations.js:479
    sendMsg jar:file:///Applications/Thunderbird.app/Contents/Resources/omni.ja!/components/imConversations.js:479
    sendMsg chrome://messenger/content/chat/chat-conversation.js:565
    inputKeyPress chrome://messenger/content/chat/chat-conversation.js:1008
    inputKeyPress self-hosted:869

My guesses here are bug 1563122 (or maybe bug 1554633 -- but that landed in TB69, so that doesn't make sense).

Comment 3

[Alessandro Castellani [:aleca]]

I'm looking into this, but I don't think those bugs are what caused the regression.
I remember working on those and testing with both IRC and XMPP accounts, chatting with Kai before submitting the patches and everything worked as expected.

I don't know if it's related, but whenever I open a conversation, this error shows up:

JavaScript error: chrome://chat/content/conversation-browser.js, line 852: NotSupportedError: 'conversation-browser' has already been defined as a custom element

Comment 4

[Patrick Cloke [:clokep]]

Thanks for looking into this aleca. I talked to florian about it and he thinks it is likely from bug 1557506. I'll look into that soon!

He suggested:

10:23:43 AM - clokep: florian: Any ideas what could cause bug 1592505? I feel like it can't be UI due to it working for IRC, but breaking for XMPP.
10:28:31 AM - florian: IRC does https://searchfox.org/comm-central/source/chat/protocols/irc/irc.js#269
10:28:40 AM - florian: all the other prpls use https://searchfox.org/comm-central/source/chat/modules/jsProtoHelper.jsm#663
10:29:34 AM - florian: my guess is bug 1557506
10:30:04 AM - florian: previously this could return null
10:30:37 AM - florian: I think the NS_ERROR_FAILURE means failing to convert null to the Array<AString> type
10:30:53 AM - clokep: Ah, interesting. Thanks!
10:30:59 AM - clokep: That sounds likely. :)
10:31:11 AM - clokep: Do you know how to handle a situation where it can return Array<AString> or null?
10:33:10 AM - florian: for native code I think there's a Maybe<> type
10:33:16 AM - florian: but it doesn't seem to exist in idl
10:33:21 AM - florian: maybe just a '?' at the end?
10:33:53 AM - florian: ah, that seems to be webidl syntax rather than xpidl
10:34:54 AM - florian: ah, there are a few .idl files with a '?' suffix after the type
10:34:56 AM - florian: worth a try I think

Comment 5

[Patrick Cloke [:clokep]]

I confirmed this is regressed by 1557506. I backed out the portion of the patch that changed this part of the IDL. Will look a little bit more to see if there's a better way to mark the return value as optional.

Comment 6

[Patrick Cloke [:clokep]]

Ben, it seems like you've been handling most of this for the TB side. Do you know if there's a way to specify in idl that a return value is an Array, but might be null?

Comment 7

[Patrick Cloke [:clokep]]

Attachment: Patch v1

This backs out a portion of bug 1557506. Going to wait to see if Ben has a better idea though before requesting review.

Comment 8

[Magnus Melin [:mkmelin]]

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

The old way of doing it is going away, so keeping it isn't an option.
I don't know if there is a way to handle null, but the code can be changed to not need that case. (Adjust callers)

Comment 9

[Patrick Cloke [:clokep]]

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

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

The old way of doing it is going away, so keeping it isn't an option.
I don't know if there is a way to handle null, but the code can be changed
to not need that case. (Adjust callers)

Fair enough. Florian and I talked about whether this case needs to be able to return an empty array or not. I'll have to check the callers and see if that case makes sense.

Comment 10

[Patrick Cloke [:clokep]]

Attachment: Patch v2

Looks like the only caller of this already treated null and empty arrays the same. So the "real" fix here was to change the base implementation to return [] instead of null.

I also realized that one of our tests wasn't testing what we thought it was. We were modifying a message which had the test "pass", but it wasn't actually testing the behavior of using the returned string. This test starting failing with my changes.

Sorry for the bugspam Ben!

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9106378 [details] [diff] [review]
Patch v2

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

LGTM, thx! r=mkmelin with the "instead" removed or explained

::: chat/components/public/prplIConversation.idl
@@ +50,5 @@
>  
>    /**
>     * Preprocess messages before they are sent (eg. split long messages).
>     *
> +   * @returns the message(s) to use instead or an empty array if no changes are to be made.

What do you mean "instead"?

Comment 12

[Patrick Cloke [:clokep]]

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

::: chat/components/public/prplIConversation.idl
@@ +50,5 @@

/**
* Preprocess messages before they are sent (eg. split long messages).
*

• • @returns the message(s) to use instead or an empty array if no changes are to be made.

What do you mean "instead"?

I mean instead of the input message. The function takes an input message, allows transforms to be made to it and returns a new list of messages to use instead of the input message (or an empty array if the input should be used unchanged). I was having trouble simplifying that explanation down for the comment, but the old explanation was wrong since it implied only a single message could be returned. Would the following make more sense:

@returns the modified text to send or an empty array if the input should be used unchanged.

Comment 13

[Patrick Cloke [:clokep]]

Florian and I discussed this on IRC and we came to the conclusion that this code would probably be less confusing if we got rid of the "empty" case and have the protocol always have to return the text to send. This means that default implementation in jsProtoHelper would essentially pass through the input. This gets rid of the "or" in the above comment, which seems less confusing!

Comment 14

[Magnus Melin [:mkmelin]]

Sounds like a good idea.

If you're not planning to do it in this bug, how about

@returns the messages that have been preprocessed (excluding the ones that have not been changed)

Comment 15

[Patrick Cloke [:clokep]]

Attachment: Alternate solution

This is the alternative solution suggested by Florian where we require prepareForSending to return something, even if it just passes through the input.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9106714 [details] [diff] [review]
Alternate solution

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

LGTM, r=mkmelin

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ebfcb7d8cf3e
Require prepareForSending to return a message. r=mkmelin

Comment 18

[Patrick Cloke [:clokep]]

Comment on attachment 9106714 [details] [diff] [review]
Alternate solution

[Approval Request Comment]
Regression caused by (bug #): bug 1557506
User impact if declined: Users are unable to send XMPP (and any other protocol besides IRC) messages
Testing completed (on c-c, etc.): I tested this on a manually built Thunderbird, might be good to let it bake on nightly for a day or two
Risk to taking this patch (and alternatives if risky): The risk is that we'll break sending IRC messages. I did test this (and even ensured the splitting of long messages still works).

Comment 19

[Jorg K (CEST = GMT+2)]

TB 71 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/9be1474307f448e3db3e564e6f3efb0a3b7957ef

Comment 20

[Ari Maniatis]

I've tested build3 on OSX and the problem is resolved. Thanks.

https://ftp.mozilla.org/pub/thunderbird/candidates/71.0b3-candidates/build3/mac/en-GB/Thunderbird%2071.0b3.dmg

Comment 21

[Patrick Cloke [:clokep]]

Thanks for verifying! (And for reporting this initially.)