741858 - Change stored password doorhanger doesn't handle large password string very well

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, steps to reproduce:
- Tap on the Submit button, the remember password doorhanger pops up
- Tap on 'Remember'
- Tap on the Android back button
- Tap on the password input, type some text in there
- Tap on the Submit button, the change stored password doorhanger pops up

Expected result:
- The text input value that is shown is truncated just like in the remember password doorhanger popup

Actual result:
- The text input value that is shown is not truncated.

Tested in today's trunk build on the Samsung Galaxy Nexus.

Comment 1

[:Margaret Leibovic]

This is a also a toolkit login manager bug. I'll file another bug for that. Good find!

For the save login dialog, we call _sanitizeUsername:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/LoginManagerPrompter.js#236

but we don't do that for the change login dialog:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/LoginManagerPrompter.js#374

Comment 2

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #1)
> This is a also a toolkit login manager bug. I'll file another bug for that.

Filed bug 742396.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Martijn, does this affect the password/login if it gets cut off?  Or is this only a visual effect?  Trying to understand how critical this bug is for triage.

Comment 4

[Veeraya Pupatwibul]

Attachment: screenshot (on pc, not android)

there was a similar bug on firefox (for ubuntu) and that's how it looked like. i believe it should look like that on fennec too.

Comment 5

[:Margaret Leibovic]

(In reply to Naoki Hirata :nhirata from comment #3)
> Martijn, does this affect the password/login if it gets cut off?  Or is this
> only a visual effect?  Trying to understand how critical this bug is for
> triage.

Yes, this just looks bad. It makes it so that you potentially have to scroll the doorhanger notification to get to the buttons, but you would need a ridiculously long username.

Comment 6

[Martijn Wargers (dead)]

This is not a bug that users would encounter, I think.

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I have an incredibly long user name of [e-mail]  would that count?  :)  Just want to make sure that there's no login issues due to this.  I guess a better question is what's the upper limit of characters?

Comment 8

[Raphael "Gans"]

Hi. I'm a begginer here but i wanna work on this bug.
I'm confused if it's have solved or not. If not, where i look initially(in code)?

Comment 9

[Aaron Train [:aaronmt]]

Hey Raphael,

This bug is still open (it's marked NEW). I would most definitely have a look at the LoginManagerPrompter.js code that Margaret linked to in comment #1 as the place for change. If you have questions, feel free to hop on IRC @ irc.mozilla.org #mobile and talk to us.

Comment 10

[:Margaret Leibovic]

(In reply to Raphael "Gans" from comment #8)
> Hi. I'm a begginer here but i wanna work on this bug.
> I'm confused if it's have solved or not. If not, where i look initially(in
> code)?

Hi Raphael! (Or do you prefer Gans?)

As Aaron mentioned, I linked to the relevant code in comment 1. You basically want to do the exact same thing to the mobile version of LoginManagerPrompter.js that Veeraya did to fix bug 742396 for desktop.

Do you have a mobile build environment set up? As Aaron said, you can find us on IRC, but build instructions are also on our wiki here: https://wiki.mozilla.org/Mobile/Fennec/Android

Comment 11

[Raphael "Gans"]

Thanks for feedback! I prefer Gans, it's how my friends call me. ^^
I look at the other bug(bug 742396) and "LoginManagePrompter.js". I modified the code for sanitize the new password. I believe it's ok but I'm having troubles about the mobile build environment.

My (small) experience with android's environment is only with Eclipse/ADT Plugin. Because some problems with my Internet's connection, the download of NDK are impossible in the moment. May i execute the code on Eclipse? Or upload the patch without testing it? The last option isn't good but i'm only question about to understand the process here, at Bugzilla. ;)

Oh, i forgot to say before: I'm sorry about my terrible english. I'm brazilian and don't speak english very well. =)

Thanks for answers!

Comment 12

[Raphael "Gans"]

Thanks for feedback! I prefer Gans, it's how my friends call me. ^^
I look at the other bug(bug 742396) and "LoginManagePrompter.js". I modified the code for sanitize the new password. I believe it's ok but I'm having troubles about the mobile build environment.

My (small) experience with android's environment is only with Eclipse/ADT Plugin. Because some problems with my Internet's connection, the download of NDK are impossible in the moment. May i execute the code on Eclipse? Or upload the patch without testing it? The last option isn't good but i'm only question about to understand the process here, at Bugzilla. ;)

Oh, i forgot to say before: I'm sorry about my terrible english. I'm brazilian and don't speak english very well. =)

Thanks for answers!

Comment 13

[Mark Capella [:capella]]

Attachment: Patch (v1)

This small change tests out and works nice on my Galaxy device.

fyi, the DIFF will show that I modifed both the _showChangeLoginNotification() and the _showChangeLoginDialog() member functions, as the _sanitizeUsername() call was currently in place for both the _showSaveLoginNotification() and the _showSaveLoginDialog() functions.

I mention this because though the changes look trivial, my testing may have only exercised the changed portion in the notification doorhanger routine.

Comment 14

[Mark Capella [:capella]]

More fyi, I hacked the code to force display of the _showChangeLoginDialog() routine and it works also, sort of. The sanitized 30-char name + "..." shows up but takes two lines in the dialog.

Comment 15

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #13)
> Created attachment 652335 [details] [diff] [review]
> Patch (v1)
> 
> This small change tests out and works nice on my Galaxy device.
> 
> fyi, the DIFF will show that I modifed both the
> _showChangeLoginNotification() and the _showChangeLoginDialog() member
> functions, as the _sanitizeUsername() call was currently in place for both
> the _showSaveLoginNotification() and the _showSaveLoginDialog() functions.

Nice catch! That's something I didn't notice when I filed bug 742396, so that problem still exists in the toolkit version of _showChangeLoginDialog (although I'm not sure if there are still toolkit apps that go through that code path).

Comment 16

[:Margaret Leibovic]

Comment on attachment 652335 [details] [diff] [review]
Patch (v1)

This looks good, but I have a few style nits to adhere to our JS style conventions.

>diff --git a/mobile/android/components/LoginManagerPrompter.js b/mobile/android/components/LoginManagerPrompter.js

>@@ -329,20 +329,22 @@ LoginManagerPrompter.prototype = {
>+            var displayUser = this._sanitizeUsername(aOldLogin.username);

s/var/let

>             notificationText  = this._getLocalizedString(
>                                           "passwordChangeText",
>-                                          [aOldLogin.username]);
>+                                          [displayUser]);
>+        }
>         else

Put the else on the same line as the brace, and surround the else block in braces as well.

>@@ -387,20 +389,22 @@ LoginManagerPrompter.prototype = {

>+            var displayUser = this._sanitizeUsername(aOldLogin.username);
>             dialogText  = this._getLocalizedString(
>                                     "passwordChangeText",
>-                                    [aOldLogin.username]);
>+                                    [displayUser]);
>+        }
>         else
>             dialogText  = this._getLocalizedString(
>                                     "passwordChangeTextNoUser");

Same comments apply to this hunk as well.

Comment 17

[Mark Capella [:capella]]

Attachment: Patch (v2)

As corrected, and pushing to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=46901461801b

Comment 18

[Mark Capella [:capella]]

Onwards and upwards to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5228384d4da8

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5228384d4da8