Last Comment Bug 343182 - Password Manager asks multiple times to save a password
: Password Manager asks multiple times to save a password
Status: RESOLVED FIXED
: fixed1.8.1, regression
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: 1.8 Branch
: All All
: -- normal with 1 vote (vote)
: mozilla1.8.1beta2
Assigned To: Michael Wu [:mwu]
:
Mentors:
http://webfile.nd.edu
Depends on:
Blocks: 345216 257781
  Show dependency treegraph
 
Reported: 2006-06-29 13:51 PDT by Joey Minta
Modified: 2006-07-19 17:33 PDT (History)
14 users (show)
mtschrep: blocking1.8.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (501 bytes, text/html)
2006-06-30 10:02 PDT, Michael Wu [:mwu]
no flags Details
Avoid resetting mNotifiedObservers in SetAttr (1.41 KB, patch)
2006-06-30 18:32 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Add NS_EARLYFORMSUBMIT_SUBJECT (9.58 KB, patch)
2006-07-07 16:59 PDT, Michael Wu [:mwu]
jst: review+
bzbarsky: superreview+
mbeltzner: approval1.8.1+
Details | Diff | Splinter Review

Description Joey Minta 2006-06-29 13:51:53 PDT
After typing in my name+password at http://webfile.nd.edu I get prompted multiple times for whether or not I want to save my password

Steps to reproduce:
1.) Go to http://webfile.nd.edu
2.) Type in name+password
3.) Firefox prompts whether you want it to remember the password.
4.) Choose 'Not Now'.
5.) Firefox prompts you again. (x4)

I've seen this on mac-trunk.  jwalden reports reproducing it on branch-linux and trunk-linux too.
Comment 1 Michael Wu [:mwu] 2006-06-29 23:27:24 PDT
Opps. This is most likely caused by my previous patch for bug 257781. Will be a good idea to fix this up so users aren't bothered by sites that submit() too many times.
Comment 2 Michael Wu [:mwu] 2006-06-30 10:02:25 PDT
Created attachment 227698 [details]
testcase
Comment 3 Michael Wu [:mwu] 2006-06-30 18:10:16 PDT
Ok, so this bug is two parts. One part is bug 343264. Without that fix, the http://webfile.nd.edu/ website can show the password save prompt 4 times. With that fix, it only shows the prompt twice (bug 343264 doubles the number of prompts normally shown, and shows at least one prompt if there shouldn't be any). The reason for showing twice is because that site sets the action of the form when it decides what you've entered is valid. Setting the action resets any pending submissions *and* the flag that indicates we've already notified the password manager. We're probably gonna have to be more clever about where we reset that flag..
Comment 4 Michael Wu [:mwu] 2006-06-30 18:32:04 PDT
Created attachment 227779 [details] [diff] [review]
Avoid resetting mNotifiedObservers in SetAttr

Scripts often set the action or target once they've validated your form input. In these cases, we don't want the password manager to be notified again. (thus possibly causing two prompts to save the password to be shown)
Comment 5 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-30 20:39:44 PDT
Blocking against beta2 as this is a pretty bad regression, but not so bad that we'll want to slip beta1 for it. If we can get it reviewed by beta1 codefreeze, all the better.

mwu: jst is on vacation, so you might want to try and find a different reviewer
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2006-06-30 21:53:52 PDT
For what it's worth, I'm more or less on vacation too (completely so in about 20 minutes).  That said, I have a few questions:

1)  Why are we setting mNotifiedObservers in ForgetCurrentSubmission()?  I
    assume there was a reason to add that to start with?  I guess we want that
    if the submission is canceled altogether, so that the next time the form is
    submitted we'll notify, right?  Doesn't this patch break that?
2)  How does this affect non-Firefox apps using these notifications (I'm
    guessing "the same as Firefox apps" in that everyone is getting doubled
    notifications right now, but the notifications are under-documented enough
    and the code complex enough that I'm not sure).
3)  If we're getting the action URI changed _after_ we've notified observers,
    aren't we lying about the action URI when we notify?  It's not clear to me
    that we want that API change, esp. on the 1.8 branch (this comment is more
    about the patch for bug 257781).  I realize that the Firefox and Seamonkey
    password managers doesn't care where we're submitting to, but are we sure
    we're not breaking other implementors of this API?

Also, could we move this bug to Core?  This code (and bug) is not exactly Firefox-specific.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2006-06-30 22:02:30 PDT
I suspect one way to resolve issue #3 and this bug might be to have two separate categories -- "pre-submit" observers that are notified before we fire onsubmit and might get an incorrect URL (password manager is a good example here) and submit observers (which work like submit observers have always worked).  Or will that not work because password manager wouldn't catch submit() calls?
Comment 8 Michael Wu [:mwu] 2006-06-30 22:20:26 PDT
(In reply to comment #7)
> I suspect one way to resolve issue #3 and this bug might be to have two
> separate categories -- "pre-submit" observers that are notified before we fire
> onsubmit and might get an incorrect URL (password manager is a good example
> here) and submit observers (which work like submit observers have always
> worked).  Or will that not work because password manager wouldn't catch
> submit() calls?
> 
That was my other option when implementing the original patch to do early notification of password managers, but the other approach was simpler and seemed to work fine. Also, the submit() calls would be caught by the ordinary form submit category. (we want to support the new presubmit observers alongside the old ones in the password manager)

If that approach is chosen, we might want to back out the patch for 257781 if this problem is considered severe enough.
Comment 9 Michael Wu [:mwu] 2006-06-30 22:22:00 PDT
(In reply to comment #6)
> For what it's worth, I'm more or less on vacation too (completely so in about
> 20 minutes).  That said, I have a few questions:
> 
> 1)  Why are we setting mNotifiedObservers in ForgetCurrentSubmission()?  I
>     assume there was a reason to add that to start with?  I guess we want that
>     if the submission is canceled altogether, so that the next time the form is
>     submitted we'll notify, right?  Doesn't this patch break that?
Yes, if the script modifies target/action and then decides not to submit.

> 2)  How does this affect non-Firefox apps using these notifications (I'm
>     guessing "the same as Firefox apps" in that everyone is getting doubled
>     notifications right now, but the notifications are under-documented enough
>     and the code complex enough that I'm not sure).
Not sure, I'll have to check.

> 3)  If we're getting the action URI changed _after_ we've notified observers,
>     aren't we lying about the action URI when we notify?  It's not clear to me
>     that we want that API change, esp. on the 1.8 branch (this comment is more
>     about the patch for bug 257781).  I realize that the Firefox and Seamonkey
>     password managers doesn't care where we're submitting to, but are we sure
>     we're not breaking other implementors of this API?
> 
True, I didn't notice that.

> Also, could we move this bug to Core?  This code (and bug) is not exactly
> Firefox-specific.
> 
Will do.
Comment 10 Michael Wu [:mwu] 2006-07-05 19:19:15 PDT
Comment on attachment 227779 [details] [diff] [review]
Avoid resetting mNotifiedObservers in SetAttr

This patch is no good - it'll break the prompt asking the user if he/she really wants to submit in some cases. The current early notify changes, however, are safe AFAICT.
Comment 11 Michael Wu [:mwu] 2006-07-07 16:59:23 PDT
Created attachment 228486 [details] [diff] [review]
Add NS_EARLYFORMSUBMIT_SUBJECT

This adds NS_EARLYFORMSUBMIT_SUBJECT for observers who want to be notified of a submit ASAP without needing a valid actionURL.
Comment 12 Michael Wu [:mwu] 2006-07-07 17:02:36 PDT
Bug 343264 is different from this. This bug is not caused by bug 221634, but by bug 257781.
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2006-07-11 16:18:49 PDT
Comment on attachment 228486 [details] [diff] [review]
Add NS_EARLYFORMSUBMIT_SUBJECT

- In nsHTMLFormElement::SetAttr():

+    // Don't forget we've notified the password manager already if the
+    // page sets the action/target in the during submit. (bug 343182)

Remove "in the";

Other than that this looks good to me. The one thing to test here would be how this change interacts with the code that listens for form submit notifications in nsSecureBrowserUI.cpp.

With that, r=jst
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2006-07-16 11:40:33 PDT
Comment on attachment 228486 [details] [diff] [review]
Add NS_EARLYFORMSUBMIT_SUBJECT

sr=bzbarsky.  This looks like it restores the old behavior of NS_FORMSUBMIT_SUBJECT, right?

For NS_EARLYFORMSUBMIT_SUBJECT you'll run into trouble if the action URI is changed and then submit doesn't happen, in that you'll no longer get notifications for that form.  But I guess that's OK for that edge case and for our consumers of the subject.
Comment 15 Robert Strong [:rstrong] (use needinfo to contact me) 2006-07-17 17:19:09 PDT
Checked in to trunk
Comment 16 Mike Beltzner [:beltzner, not reading bugmail] 2006-07-19 12:32:15 PDT
Comment on attachment 228486 [details] [diff] [review]
Add NS_EARLYFORMSUBMIT_SUBJECT

a=drivers, please land on the 1.8.1branch
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-07-19 17:33:48 PDT
mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp 	1.1.2.12
mozilla/toolkit/components/satchel/src/nsFormHistory.cpp 	1.26.4.7
mozilla/toolkit/components/passwordmgr/base/nsPasswordManager.cpp 	1.65.2.8
mozilla/extensions/wallet/src/nsWalletService.cpp 	1.159.6.1
mozilla/content/html/content/src/nsHTMLFormElement.cpp 	1.186.4.3
mozilla/content/html/content/public/nsIFormSubmitObserver.h 	1.13.18.1

Note You need to log in before you can comment on or make changes to this bug.