Closed
Bug 90392
Opened 23 years ago
Closed 23 years ago
Form submitted multiple times if 'Insecure' dialog dismissed by hitting Enter. [form sub]
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
People
(Reporter: basil, Assigned: alexsavulov)
References
()
Details
(Whiteboard: [PDT+] fixed on branch [which one?])
Attachments
(4 files)
168 bytes,
text/html
|
Details | |
662 bytes,
patch
|
Details | Diff | Splinter Review | |
1.34 KB,
patch
|
Details | Diff | Splinter Review | |
1.36 KB,
patch
|
Details | Diff | Splinter Review |
When submitting insecure forms and the insecure form alert pops, if I click on OK, the alert clears; if I hit enter, the alert pops again. On some form submissions, like the Yahoo! mail URL above, hitting enter multiple times does not clear the alert. On others, like the poll on http://home.netscape.com/, the alert clears after two strikes of the enter key. I know that each strike of the enter key is sending the same information, because this bug in another web email client will send as many copies of mail as the number of times I hit enter. I am seeing this bug on build 2001071104 for Win32. I believe I began seeing it Monday (20010709xx), but I may have begun seeing it with Friday's build (20010706xx). This appears to be a regression.
Reporter | ||
Comment 1•23 years ago
|
||
bug 76605 and bug 89888 seem to be similar issues; possible dupes. Since this bug is seen on Mac builds, changing platform and OS to all.
OS: Windows 98 → All
Hardware: PC → All
Comment 2•23 years ago
|
||
Test with build 2001071004 on Win2K to try to reproduce this bug.
Comment 3•23 years ago
|
||
Confirming... Bugzilla generated the mid-air collision with my first submit as expected after I pressed Enter the second time to the insecure submission prompt. To reproduce: 1 Login to bugzilla if not already 2 Set the browser to show warning when posting to insecure pages: Edit->Preferences->Privacy and Security->SSL and check "Sending form data from an insecure page to an insecure page. 3 Add comments and click Commit button 4 Press enter to the insecure form submission warning (don't click ok) 5 Press enter again when it reappears Expected: The insecure form submission should only appear once and be dismissed when enter is pressed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 4•23 years ago
|
||
Dupe of Bug 89888 *** This bug has been marked as a duplicate of 89888 ***
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → DUPLICATE
Comment 5•23 years ago
|
||
Reopening... Although this does show up as duplicate behavior, I believe this bug is more general and better explains the problem. The problem is not specific to just Bugzilla; it is visible anytime you do an insecure submission. If anything, bug 89888 should be marked a duplicate of this one.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Reporter | ||
Comment 6•23 years ago
|
||
After looking more closely at bug 76605, it is very likely a different issue. Bug 76605 is almost certainly page-related and possibly even invalid, whereas this issue happens on all pages that have insecure form submission.
Comment 7•23 years ago
|
||
Wow, didn't notice this bug until just now... It is easily reproducible for any form submission with a branch build. The action of pressing Enter to dismiss warning dialogs is very common user behavior. Double submission of forms is extrememly severe as it could result in actions taking place twice (online purchases, ...) Grabbing this one. I'll try to come up with a quick fix for the branch!
Comment 9•23 years ago
|
||
Comment 10•23 years ago
|
||
Do you have a patch coming soon? This is worth working on, but we need to see the actual fix to make an evaluation for taking it. Do we need any other people helping on this?
Comment 11•23 years ago
|
||
No patch yet - I am in the middle of a clobber build on Window (unfortunately I had clobbered before I noticed this bug...) I am not seeing this problem on Linux (which is too bad because I was going to use my Linux build to debug while my Windows build finishes...) I'll continue work on this in an hour or so after my build finishes and I grab a bite to eat... :) My hope/guess is that I'll be able to come up with a couple-line patch that disables form submit requests when one is currently being processed - which might fix this problem *fingers crossed*.
Comment 12•23 years ago
|
||
CC'ing some people who might know why (presumably) the keypress event in a modal dialog is getting sent to a button in the main window.
Comment 13•23 years ago
|
||
*** Bug 89888 has been marked as a duplicate of this bug. ***
Comment 14•23 years ago
|
||
First two attempts at a patch failed. The problem is not that the keypress comes through *during* the first submit, but that it comes immediately after. Thus, from the perspective of the form submission logic, this bug is indistinguishable from bug 72906 (rapid double click on submit causes two submits). We will probably need to do something like: 1) Find out why this rogue keypress event gets to the submit element to begin with, and stop it from happening. This seems like the "right" thing to do. I don't really know where to begin here other than going through checkins. I've gone as far back as 24-June-2001 and still see the problem. Tom or Chris, do you have any ideas here? 2) Prevent submit that occurs within x time of previous submit from causing an actual submit. If x time happened to be the doubleclick interval, we would also be fixing bug 72906. 3) Don't allow the insecure form submit warning dialog to be dismissed with a keypress. I also don't have a clue where to begin here David, Ben, ??? any XUL people know if this is practical?
Comment 15•23 years ago
|
||
Okay, nevermind - figured it out: nsHTMLInputElement.cpp: 1.175 blakeross%telocity.com Jun 19 23:20 Keypress event bubbles up to alerts, meaning alerts were dismissed without the user's consent (68846). r=kerz sr=ben a=asa Since this warning dialog is dismissed on Enter->KEY_DOWN and the form submit is triggered by Enter->KEY_UP, this was just bad, bad luck... I'll have to look at bug 68846, but submitting form on KEY_UP instead of KEY_PRESS seems like a bad idea to me...
Comment 16•23 years ago
|
||
Comment 17•23 years ago
|
||
I am not able to reproduce bug 68846, even after backing out this part of the fix. CC'ing Blake...
Comment 18•23 years ago
|
||
Comment 19•23 years ago
|
||
Patch 2 makes our HTML control behaviour (button clicks, checkboxes, radios, and image inputs) consistent with our XUL dialog behavior, and consistent with what IE does. It will activate the control on KEY_PRESS (immediately) when Enter is pressed, but on KEY_UP (waiting until the key is released) when the spacebar is pressed. This fixes 90392 (it prevents multiple form submit) and bug 68846 (it prevents dialogs from being automatically dismissed), and does not have any regressions. (The first patch caused the spacebar to no longer activate HTML controls at all. It is also pretty small -> easy to understand. The problem was an impedance mismatch between HTML controls and XUL dialogs. XUL dialogs were emulating IE's behaviour, but HTML controls were always activating on KEY_UP, regardless of which key was used. That would be fine if the space bar was used to activate the control, but when Enter was used to dismiss the dialog box, the KEY_PRESS would come first, dismissing the dialog, then the KEY_UP would come later, submitting the form again. The patch fixes this problem by making HTML control respond to the same key events as the XUL dialogs, thereby preventing them from receiving an event not consumed by the HTML control and acting on it.
Summary: Insecure form submission alert not cleared by hitting enter. → Form submitted multiple times if 'Insecure' dialog dismissed by hitting Enter.
Whiteboard: PDT → PDT, fix in hand, need r=/sr=/a=
Comment 20•23 years ago
|
||
Comment 21•23 years ago
|
||
r=jag
Comment 22•23 years ago
|
||
sr=jst
Comment 23•23 years ago
|
||
Checked in on the trunk. This morning's trunk build should have the fix and can be banged around to see if there are any regressions caused by it... (My assertion earlier "does not have any regressions" should have read "does not have any *known* regressions", typing slower than the brain works... :) )
Whiteboard: PDT, fix in hand, need r=/sr=/a= → PDT, fix in hand, need branch approval
Comment 24•23 years ago
|
||
In 2001072304 trunk for Mac, hitting return does not bring up another security warning any more, but it in fact submits another one WITHOUT asking. Therefore, hitting return is still captured by the form as submit in addition to OK to the warning, and this second submission somehow bypasses the security warning.
Comment 25•23 years ago
|
||
Too quick. Sorry about my previous posting. I may have hit return too hard so it produced chattering? I could not reproduce double submission this time.
Comment 26•23 years ago
|
||
Hirata, yes, a multiple-submission can still be produced by holding down the Enter key until the auto-repeat starts kicking in. This would seem to me to be a less common problem. (Remember, without this fix, simply pressing the Enter key quickly will cause a multiple submit.) Also, note that holding down the Enter key until the auto-repeat starts kicking in will also cause multiple- submission in IE. ??? Yes, this is a lesser-of-two-evils kind of thing. One way to get around this would be to make the Enter key also trigger on KEY_UP for both form submission and XUL dialogs. I looked around the XUL dialog code and do not know how that would be accomplished - could be a big change, could be small, any XUL guru care to comment? Note that this change would make us behave differently than IE. Another way to get around this would be to disable multiple submission altogether, or at least pop up a warning dialog (a special one that can't be dismissed by holding down Enter) if one was about to occur. This would also be a larger change, but I will be working on it today - and will report if I make any progress... In the meantime, in my opinion, this fix is at least better than our current behavior, and it should go in for the branch.
Comment 27•23 years ago
|
||
I agree that the current fix is OK. What I was bewildered about was the last time I tried submitting some bug with the latest build, I got midair collision with only one security warning after hitting Enter a bit too long. That seemed wrong. But I have not been able to reproduce that particular result, because you can not try this kind of behavior too many times.
Reporter | ||
Comment 28•23 years ago
|
||
Still seeing this bug in Win32 build 2001072303-trunk. Is it possible that the fix didn't get checked in before the Win32 build?
Comment 29•23 years ago
|
||
It was checked in at 2001-07-23 at 3:30AM, so it won't be in 2001072303. I thought at least the trunk was still doing respins at 7AM?!
Comment 30•23 years ago
|
||
Ahh, whew! It's there. :) Basil, try: ftp://ftp.mozilla.org/pub/mozilla/nightly/2001-07-23-09-trunk
Comment 31•23 years ago
|
||
Hirata, if you want to test this a bunch of times without messing up a bug, try the 'reduced test case' on this bug report. Each time you submit the form, presuming that nobody else is messing with the test case at the exact same time, the Request ID should increase by one. FWIW, Internet Explorer and all previous versions of NS 6 (and I think Nav 4.x?) dismiss that dialog immediately if you hold down the Enter key until the auto repeat kicks in, or press Enter twice quickly, which are essentially the same thing.
Comment 32•23 years ago
|
||
Checked in on the branch.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 33•23 years ago
|
||
Updating whiteboard so this doesn't look like rogue checkin. ;) [PDT gave approval verbally at a meeting]
Whiteboard: PDT, fix in hand, need branch approval → PDT+
Comment 34•23 years ago
|
||
You did the right thing. Updating to mention checkin has happened.
Whiteboard: PDT+ → PDT+ fixed on branch
Comment 35•23 years ago
|
||
Oops, I need to reopen this so it shows up on the PDT+ radar. Sorry for the spam. Changing from nsBranch to vbranch so it gets looked at.
Comment 36•23 years ago
|
||
This is fixed, please resolve I can verify on on build 2001-07-24-05-0.9.2 windows 98 build and 2001-07-24-05-0.9.2 Linux RedHat 6.2 build
Updated•23 years ago
|
Assignee: pollmann → alexsavulov
Status: REOPENED → NEW
Comment 37•23 years ago
|
||
Bulk reassigning form bugs to Alex
Assignee | ||
Updated•23 years ago
|
Summary: Form submitted multiple times if 'Insecure' dialog dismissed by hitting Enter. → Form submitted multiple times if 'Insecure' dialog dismissed by hitting Enter. [form sub]
Comment 38•23 years ago
|
||
is this fixed on the 094 branch?
Whiteboard: PDT+ fixed on branch → [PDT+] fixed on branch [which one?]
Comment 39•23 years ago
|
||
gerardo or vladimir - pls verify. I assume this is fixed for 094 since the fix was in 092
Comment 40•23 years ago
|
||
resolving
Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•