527674 - password echo displays more stars than chars after delete

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Axel Hecht [:Pike]]

I typed in 3 chars of a password, deleted the last char, typed another char and had 4 stars.

I just checked, the password is submitted fine, just the stars don't match.

BuildID 20091109085301, on an n810.

Might be a regression from bug 514212?

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #0)
> Might be a regression from bug 514212?

yes and no. I can reproduce this without password echoing. However, without password echoing it takes two deletes from the same caret position, with password echoing you can also trigger this with one delete at the end of the string.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this adds a few asserts to make sure we never do this again

Comment 3

[[e-mail]]

Comment on attachment 411777 [details] [diff] [review]
patch

>+static inline nsIDOMNode* GetTextNode(nsISelection *selection, 
>+                            nsEditor *editor) {
already_AddRefed<nsIDOMNode> is probably a better return type. (You need to use selNode.forget() instead of selNode.get() in this case.)

>+  NS_ASSERTION(selection, "passed in selection is null");
Not really necessary; you know who the callers are.

>+          selNode == nsnull) {
Nit: !selNode

>+#define ASSERT_PASSWORD_LENGTHS_EQUAL() \
This has two callers, so I'd prefer most of it to be a function. The condition could stay as part of the define though.

>+  if (mFlags & nsIPlaintextEditor::eEditorPasswordMask){ \
Nit: space between ) and {

>+    res = mEditor->GetTextSelectionOffsets(selection, start, end); \
>+    if (NS_FAILED(res)) return res; \
Don't need this.

>@@ -942,6 +983,19 @@ nsTextEditRules::WillDeleteSelection(nsI
>     PRUint32 start, end;
>     mEditor->GetTextSelectionOffsets(aSelection, start, end);
>     NS_ENSURE_SUCCESS(res, res);
>+    nsCOMPtr<nsILookAndFeel> lookAndFeel = do_GetService(kLookAndFeelCID);
>+
>+    if (lookAndFeel->GetEchoPassword()) {
>+      HideLastPWInput();
>+      mLastStart = start;
>+      mLastLength = start == end ? -1 : start - end;
I don't think this is right for a delete. We should just set mLastLength to 0 to indicate that this wasn't an insert.

> NS_IMETHODIMP nsTextEditRules::Notify(class nsITimer *) {
>-  return HideLastPWInput();
>+  nsresult res = HideLastPWInput();
In fact I think we should set mLastLength to 0 here too, since HideLastPWInput will get called again on the next insert or delete.

>+  if (mLastLength <= 0) {
(So this just becomes !mLastLength)

>+  nsCOMPtr<nsIDOMNode> selNode = GetTextNode(selection, mEditor);
>+  nsCOMPtr<nsIDOMCharacterData> nodeAsText(do_QueryInterface(selNode));
>+  if (nodeAsText)
There's not much point doing anything if you can't find the text node. In particular it's replacing the data that mucks around with the selection, which makes it pointless bothering to restore the selection.

>-  PRUint32             mLastStart, mLastLength;
>+  PRInt32              mLastStart, mLastLength;
(And this change isn't necessary.)

Comment 4

[[e-mail]]

(In reply to comment #3)
> >+#define ASSERT_PASSWORD_LENGTHS_EQUAL() \
> This has two callers, so I'd prefer most of it to be a function.
I just realised you could use mEditor->GetTextLength instead, which would make this short enough to work as a define rather than a function.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.2

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Stuart: I agree this sucks, but do you really think it blocks?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/da736df98b8c

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5a96cb6d26bf

Comment 10

[Aakash Desai [:aakashd]]

verified FIXED on build:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2b6pre) Gecko/20091228 Namoroka/3.6b6pre Fennec/1.0a4pre