Justin Dolske [:Dolske] Reporter
	Description • 17 years ago

Satchel's nsFormHistory::Notify should check to see if the input field had markAsLoginManagerField() called on it (like nsFormFillController::StartSearch() does to see who to ask for autocomplete entries).

Currently, any values the user might enter will be stored, but they're never visible because the autocomplete menu is provided by login manager, not the form history. This may have some privacy implications, as usernames will be stored even if you've cleared stored logins (or not saved a login to begin with).

Note: passwords are never stored by the form manager.

	Justin Dolske [:Dolske] Reporter
	Comment 2 • 17 years ago

Comment on attachment 286206 [details] [diff] [review]
Patch for review, v.1

Eh, I'll r? Mano instead he's done the last few reviews to this code. :-)

	Asaf Romano (gone)
	Comment 3 • 17 years ago

Comment on attachment 286206 [details] [diff] [review]
Patch for review, v.1

>Index: toolkit/components/satchel/public/nsIFormFillController.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/satchel/public/nsIFormFillController.idl,v
>retrieving revision 1.3
>diff -u -p -8 -r1.3 nsIFormFillController.idl
>--- toolkit/components/satchel/public/nsIFormFillController.idl	16 May 2007 10:02:51 -0000	1.3
>+++ toolkit/components/satchel/public/nsIFormFillController.idl	25 Oct 2007 22:48:17 -0000
>@@ -71,9 +71,17 @@ interface nsIFormFillController : nsISup

rev the uuid.


>+NS_IMETHODIMP
>+nsFormFillController::IsLoginManagerField(nsIDOMHTMLInputElement *aInput,
>+                                          PRBool *aResult)
>+{
>+  PRInt32 dummy;
>+  if (mPwmgrInputs.Get(aInput, &dummy))
>+    *aResult = PR_TRUE;
>+  else
>+    *aResult = PR_FALSE;
>+

nit: make that *aResult = mPwmgrInputs.Get(aInput, &dummy) ! nsnull;

>+  return NS_OK;
>+}
>+
>+
> ////////////////////////////////////////////////////////////////////////
> //// nsIAutoCompleteInput
> 
> NS_IMETHODIMP
> nsFormFillController::GetPopup(nsIAutoCompletePopup **aPopup)
> {
>   *aPopup = mFocusedPopup;
>   NS_IF_ADDREF(*aPopup);
>Index: toolkit/components/satchel/src/nsStorageFormHistory.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp,v
>retrieving revision 1.19
>diff -u -p -8 -r1.19 nsStorageFormHistory.cpp
>--- toolkit/components/satchel/src/nsStorageFormHistory.cpp	20 Jul 2007 21:55:19 -0000	1.19
>+++ toolkit/components/satchel/src/nsStorageFormHistory.cpp	25 Oct 2007 22:48:17 -0000
>@@ -199,16 +199,21 @@ nsFormHistory::FormHistoryEnabled()
> 
>     nsCOMPtr<nsIPrefBranch2> branchInternal =
>       do_QueryInterface(gFormHistory->mPrefBranch);
>     branchInternal->AddObserver(PREF_FORMFILL_ENABLE, gFormHistory, PR_TRUE);
> 
>     gPrefsInitialized = PR_TRUE;
>   }
> 
>+  if (!gFormHistory->mFormController) {
>+    gFormHistory->mFormController =
>+      do_GetService("@mozilla.org/satchel/form-fill-controller;1");
>+  }
>+
>   return gFormHistoryEnabled;
> }
> 
> 
> ////////////////////////////////////////////////////////////////////////
> //// nsIFormHistory2
> 
> NS_IMETHODIMP
>@@ -351,40 +356,50 @@ nsFormHistory::Notify(nsIDOMHTMLFormElem
> 
>   PRUint32 length;
>   elts->GetLength(&length);
>   for (PRUint32 i = 0; i < length; ++i) {
>     nsCOMPtr<nsIDOMNode> node;
>     elts->Item(i, getter_AddRefs(node));
>     nsCOMPtr<nsIDOMHTMLInputElement> inputElt = do_QueryInterface(node);
>     if (inputElt) {
>-      // Filter only inputs that are of type "text" without autocomplete="off"
>+      // Ignore inputs that are not type=text
>       nsAutoString type;
>       inputElt->GetType(type);
>       if (!type.LowerCaseEqualsLiteral("text"))
>         continue;
> 
>-      // TODO: If Login Manager marked this input, don't save it. The login
>-      // manager will deal with remembering it.
>-
>+      // Ignore inputs that have autocomplete=off set.
>       nsAutoString autocomplete;
>       inputElt->GetAttribute(kAutoComplete, autocomplete);
>-      if (!autocomplete.LowerCaseEqualsLiteral("off")) {
>-        // If this input has a name/id and value, add it to the database
>-        nsAutoString value;
>-        inputElt->GetValue(value);
>-        if (!value.IsEmpty()) {
>-          nsAutoString name;
>-          inputElt->GetName(name);
>-          if (name.IsEmpty())
>-            inputElt->GetId(name);
>-          if (!name.IsEmpty())
>-            AddEntry(name, value);
>-        }
>-      }
>+      if (autocomplete.LowerCaseEqualsLiteral("off"))
>+        continue;
>+
>+      // Ignore input that don't have a value.
>+      nsAutoString value;
>+      inputElt->GetValue(value);
>+      if (value.IsEmpty())
>+        continue;
>+
>+      // Ignore inputs being handled by the login manager.
>+      // (don't save usernames if login manager might)
>+      PRBool isUser;
>+      nsresult rv =
>+        gFormHistory->mFormController->IsLoginManagerField(inputElt, &isUser);

er, gFormHistory == this by chance?

	Justin Dolske [:Dolske] Reporter
	Comment 5 • 17 years ago

Gavin noted that there are other spots where gFormHistory is used instead of |this|, so I'll stay consistent with that. Seems like this is just kind of a mess with the asserts in the constructor/destructor, too.

	Justin Dolske [:Dolske] Reporter
	Comment 6 • 17 years ago

Hmm. So, while working up a test case I realized this isn't a perfect fix. I think it would be a step forward to complete review on this patch so far and check it in, but the bug as a whole isn't 100% fixed (and I'm not inclined to spend more time on it for FF3)...

It helps when you already have a login saved -- existing or new usernames won't be stored in the form manager. So, we're better off there.

But if you don't have any logins for the site stored, pwmgr will have bailed out early during page load to avoid trudging through each form. Thus the username fields never get marked, thus satchel doesn't know not to save them.

We could try to do the marking via nsIFormSubmitObserver, but that would mean somehow forcing the pwmgr's observer to run before satchel's. Or pwmgr could call nsFormHistory::RemoveEntriesForName(), but there's still the ordering problem.  This also raises a UX question -- if you're not saving any logins (in password manager, by choice), should you still be able to autocomplete just the username? Should it depend if you've clicked pwmgr's "never for this site" button?

	Asaf Romano (gone)
	Comment 7 • 17 years ago

Hrm, this affects UE either way. We should still complete user-names after removing all passwords IMO.

	Asaf Romano (gone)
	Comment 8 • 17 years ago

Comment on attachment 288265 [details] [diff] [review]
Patch for review, v.2.1

->beltzner, i guess.

	Justin Dolske [:Dolske] Reporter
	Comment 9 • 17 years ago

Comment on attachment 288265 [details] [diff] [review]
Patch for review, v.2.1

(updating attachment descriptions, I had originally incorrectly assumed the first review was a "r+ with these nits fixed.")

	Justin Dolske [:Dolske] Reporter
	Comment 10 • 17 years ago

Bailing out from this bug for now... The cost/benefit isn't worth it at this point in the FF3 cycle.

	Asaf Romano (gone)
	Comment 11 • 17 years ago

It was... I didn't realize the UE impact this has before reading comment 6.