Closed Bug 1591148 Opened 4 months ago Closed 4 months ago

Creating a new Chat account doesn't store the password in 71.0b1

Categories

(Thunderbird :: Instant Messaging, defect)

x86_64
All
defect
Not set

Tracking

(thunderbird71 fixed, thunderbird72 fixed)

VERIFIED FIXED
Thunderbird 72.0
Tracking Status
thunderbird71 --- fixed
thunderbird72 --- fixed

People

(Reporter: walts48, Assigned: aceman)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1562314 +++

While testing TB 71.0b rc (Buid1) with a fresh test profile on Ubuntu 18.04.3.

Click Chat to create a new Chat account
Select IRC
Click Next
Enter the Username
Enter irc.mozilla.org for the Server
Click Next
Enter a Password
Click Next
Click Next in the Advanced Options dialog
Notice the 'Character Set' is set to UTF-8
Click Finish to create the account and connect
Notice the 'Character Set' is set to false.

What happens:

Account connects with a prompt from NickServ to verify the nick is yours by entering the password.

What should happen:

Account should connect without the prompt.

Clicking Show Accounts > Properties opens the account settings, and I noticed the password isn't saved and 'Character Set' is set to false.

Error console shows "TypeError: this.imAccount is nullimIncomingServer.js:163:5"

Alice, can you please check for us when that broke.

Flags: needinfo?(alice0775)

I checked 70b4 and I do not believe it is broken there.

This might be related to bug 1587054?

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

I checked 70b4 and I do not believe it is broken there.

This might be related to bug 1587054?

It wasn't broken in 70.0b4 when I tested that release candidate.

Even if account creation doesn't store the correct details, you can correct them later, right? That's what I did. So it's not a show-stopper.

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

Even if account creation doesn't store the correct details, you can correct them later, right? That's what I did. So it's not a show-stopper.

Yes, the user that creates a new account can correct them later.

Users updating from 770.0b4 should see no problem. I didn't when I tested updating from 70.0b4 to the 71.0b1 candidate.

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

Alice, can you please check for us when that broke.

I tried to create chat with fake username/password/server.
"Character Set : false" instead "Password: ****" in Summary section of "Chat Account Wizard" dialog.
And the username/password would not appear in Password Manager.

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=a3a6442dde2fa53d263a561a8dc4845718f14318&tochange=7c55e731be1891f8e9d0dab13ca377fbd42bac6c
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6a36994d2e14869283ea43331eeeaedac5be9e7c&tochange=598d441e4ebaa93ab098d266035a396057c82129

Flags: needinfo?(alice0775)

Thank you so much, as always, Alice.

Flags: needinfo?(khushil324)
Regressed by: 1581152
No longer blocks: 1562438
Duplicate of this bug: 1562438

Sorry, looks like it's regressed by bug 1563122. Alex is away, so Khushil, can you please take a look.

Regressed by: 1563122
No longer regressed by: 1581152
Assignee: nobody → khushil324
Flags: needinfo?(khushil324)
Duplicate of this bug: 1587054
Attachment #9104541 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9104541 [details] [diff] [review]
Bug-1591148_store-password-chat-account.patch

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

::: mail/components/im/content/imAccountWizard.js
@@ +417,5 @@
>      // Strangely for <input type="number"> "checked" is also set.
> +    if (
> +      elt.getAttribute("type") != "number" &&
> +      elt.localName != "input" &&
> +      "checked" in elt

Won't this unnecessarily exclude <input type="checkbox"> ?

I suggest just enumerating <input  type="checkbox"> and <checkbox> to be on the safe side, instead of trying to whack all cases that have "checked" property but aren't real checkboxes (seem all <input> element do have this property).
Comment on attachment 9104541 [details] [diff] [review]
Bug-1591148_store-password-chat-account.patch

Thanks for finding the right spot. Aceman and I worked on this before, see bug 1562314 comment #17. Your patch would have worked, but only as long as <checkbox> was used and not <input type="checkbox">. Also, you could have removed the number check.

We'll post a new patch shortly.
Attachment #9104541 - Flags: review?(mkmelin+mozilla)
Attached patch 1591148.patchSplinter Review

This is what Aceman suggested via IRC.

Attachment #9104541 - Attachment is obsolete: true
Attachment #9104554 - Flags: review+
Comment on attachment 9104554 [details] [diff] [review]
1591148.patch

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

Thanks.

::: mail/components/im/content/imAccountWizard.js
@@ +414,5 @@
>      if ("selectedItem" in elt) {
>        return elt.selectedItem.value;
>      }
> +    // Strangely various input types also have "checked" set,
> +    // so we check explicitly.

"Strangely various input types also have a "checked" property defined, so we check for the expected elements explicitly.

@@ +417,5 @@
> +    // Strangely various input types also have "checked" set,
> +    // so we check explicitly.
> +    if (
> +      ((elt.getAttribute("type") == "checkbox" && elt.localName == "input") ||
> +        elt.localName == "checkbox") &&

elt.localName == "input" could come first on the line for nicer readability.
Attachment #9104554 - Flags: feedback+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f6f166242dfd
only use 'checked' on <checkbox> and <input type="checkbox"> to fix password storage on IM account creation. r=jorgk

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Assignee: khushil324 → acelists
Target Milestone: --- → Thunderbird 72.0
Attachment #9104554 - Flags: approval-comm-beta+

Tested this only a custom built Thunderbird and the issue is fixed. Thanks Jorg/aceman!

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