Closed Bug 1591148 Opened 5 years ago Closed 5 years ago

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

Categories

(Thunderbird :: Instant Messaging, defect)

Product:
Thunderbird
Component:
Instant Messaging
Platform:
x86_64
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird71 fixed, thunderbird72 fixed)

Status:
VERIFIED FIXED
Milestone:
Thunderbird 72.0
Tracking Flags:
Tracking Status
thunderbird71 --- fixed
thunderbird72 --- fixed

People

(Reporter: walts48, Assigned: aceman)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

1591148.patch
(deleted), patch
jorgk-bmo
: review+
aceman
: feedback+
jorgk-bmo
: approval-comm-beta+
Details | Diff | Splinter Review

+++ 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

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)
Attached patch Bug-1591148_store-password-chat-account.patch (obsolete) (deleted) — — Splinter Review
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.patch (deleted) — — Splinter 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: 5 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: