343182 - Password Manager asks multiple times to save a password

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Joey Minta]

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]]

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]]

Attachment: testcase

Comment 3

[Michael Wu [:mwu]]

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]]

Attachment: 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]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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]]

(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]]

(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]]

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]]

Attachment: 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]]

Bug 343264 is different from this. This bug is not caused by bug 221634, but by bug 257781.

Comment 13

[Johnny Stenback (:jst)]

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 [:bzbarsky]]

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 (they/them - no direct email)]

Checked in to trunk

Comment 16

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

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: [e-mail]]]

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