Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Password Manager asks multiple times to save a password

RESOLVED FIXED in mozilla1.8.1beta2

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Joey Minta, Assigned: mwu)

Tracking

(Blocks: 1 bug, {fixed1.8.1, regression})

1.8 Branch
mozilla1.8.1beta2
fixed1.8.1, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
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.
Assignee: nobody → michael.wu
Flags: blocking-firefox2?
Target Milestone: --- → Firefox 2 beta1
(Assignee)

Comment 2

11 years ago
Created attachment 227698 [details]
testcase
(Assignee)

Comment 3

11 years ago
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..
Depends on: 343264
(Assignee)

Comment 4

11 years ago
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)
Attachment #227779 - Flags: superreview?(bugmail)
Attachment #227779 - Flags: review?(jst)
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
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2

Updated

11 years ago
Blocks: 221634
(Assignee)

Updated

11 years ago
Attachment #227698 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #227779 - Flags: review?(jst) → review?(bzbarsky)

Comment 6

11 years ago
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

11 years ago
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?
(Assignee)

Comment 8

11 years ago
(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.
(Assignee)

Comment 9

11 years ago
(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.
(Assignee)

Updated

11 years ago
Component: Password Manager → Layout: Form Controls
Flags: review?(bzbarsky)
Flags: blocking-firefox2+
Product: Firefox → Core
Target Milestone: Firefox 2 beta2 → mozilla1.8.1beta2
(Assignee)

Updated

11 years ago
Flags: blocking1.8.1?
(Assignee)

Updated

11 years ago
Attachment #227779 - Flags: review?(bugmail)
QA Contact: password.manager → layout.form-controls
Flags: blocking1.9a1?
(Assignee)

Comment 10

11 years ago
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.
Attachment #227779 - Attachment is obsolete: true
Attachment #227779 - Flags: superreview?(bugmail)
Attachment #227779 - Flags: review?(bugmail)
(Assignee)

Updated

11 years ago
Keywords: relnote

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Comment 11

11 years ago
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.
Attachment #228486 - Flags: superreview?(bugmail)
Attachment #228486 - Flags: review?(jst)
(Assignee)

Comment 12

11 years ago
Bug 343264 is different from this. This bug is not caused by bug 221634, but by bug 257781.
Blocks: 257781
No longer blocks: 221634
No longer depends on: 343264
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
Attachment #228486 - Flags: review?(jst) → review+

Comment 14

11 years ago
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.
Attachment #228486 - Flags: superreview?(bugmail) → superreview+
Checked in to trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Attachment #228486 - Flags: approval1.8.1?
(Assignee)

Updated

11 years ago
Blocks: 345216
Keywords: relnote
Comment on attachment 228486 [details] [diff] [review]
Add NS_EARLYFORMSUBMIT_SUBJECT

a=drivers, please land on the 1.8.1branch
Attachment #228486 - Flags: approval1.8.1? → approval1.8.1+
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
Flags: blocking1.9a1?
Keywords: fixed1.8.1
Version: Trunk → 1.8 Branch
You need to log in before you can comment on or make changes to this bug.