Closed
Bug 741858
Opened 13 years ago
Closed 12 years ago
Change stored password doorhanger doesn't handle large password string very well
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 17
People
(Reporter: martijn.martijn, Assigned: capella)
References
()
Details
(Keywords: testcase, Whiteboard: [good first bug][mentor=margaret][lang=js])
Attachments
(4 files)
474 bytes,
text/html
|
Details | |
21.51 KB,
image/png
|
Details | |
2.62 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
2.98 KB,
patch
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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
Whiteboard: [good first bug][mentor=margaret][lang=js]
Comment 2•13 years ago
|
||
(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.
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•13 years ago
|
||
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•13 years ago
|
||
(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.
Reporter | ||
Comment 6•13 years ago
|
||
This is not a bug that users would encounter, I think.
I have an incredibly long user name of nhirata.bugzilla@gmail.com 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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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!
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•13 years ago
|
||
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.
Attachment #652335 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 14•13 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
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.
Attachment #652335 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 17•12 years ago
|
||
As corrected, and pushing to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=46901461801b
Assignee | ||
Comment 18•12 years ago
|
||
Onwards and upwards to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5228384d4da8
Comment 19•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•