Closed Bug 1592505 Opened 4 months ago Closed 4 months ago

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

Categories

(Chat Core :: General, defect, blocker)

defect
Not set
blocker

Tracking

(thunderbird71 fixed, thunderbird72 fixed)

VERIFIED FIXED
Instantbird 72
Tracking Status
thunderbird71 --- fixed
thunderbird72 --- fixed

People

(Reporter: ari, Assigned: clokep)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.

Severity: normal → blocker
OS: Unspecified → macOS
Hardware: Unspecified → x86_64

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.

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

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(alessandro)
Keywords: regression

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
Flags: needinfo?(alessandro)

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

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.

Regressed by: 1557506
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Component: Instant Messaging → General
Product: Thunderbird → Chat Core

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?

Flags: needinfo?(benc)
Attached patch Patch v1 (obsolete) — Splinter Review

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

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)
Attachment #9106310 - Flags: feedback-

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

Attached patch Patch v2 (obsolete) — Splinter Review

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!

Attachment #9106310 - Attachment is obsolete: true
Flags: needinfo?(benc)
Attachment #9106378 - Flags: review?(mkmelin+mozilla)
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"?
Attachment #9106378 - Flags: review?(mkmelin+mozilla) → review+
OS: macOS → All
Hardware: x86_64 → All

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

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!

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)

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

Attachment #9106714 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9106714 [details] [diff] [review]
Alternate solution

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

LGTM, r=mkmelin
Attachment #9106714 - Flags: review?(mkmelin+mozilla) → review+

Pushed by clokep@gmail.com:
https://hg.mozilla.org/comm-central/rev/ebfcb7d8cf3e
Require prepareForSending to return a message. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 72
Attachment #9106378 - Attachment is obsolete: true
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).
Attachment #9106714 - Flags: approval-comm-beta?
Attachment #9106714 - Flags: approval-comm-beta? → approval-comm-beta+

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

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.