90392 - Form submitted multiple times if 'Insecure' dialog dismissed by hitting Enter. [form sub]

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Basil Fritts]

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.

Comment 1

[Basil Fritts]

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.

Comment 2

[Tim Powell]

Test with build 2001071004 on Win2K to try to reproduce this bug.

Comment 3

[Tim Powell]

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.

Comment 4

[hirata masakazu]

Dupe of Bug 89888

*** This bug has been marked as a duplicate of 89888 ***

Comment 5

[Tim Powell]

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.

Comment 6

[Basil Fritts]

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

[Eric Pollmann]

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 8

[Eric Pollmann]

CC'ing Steve - potential stop ship bug.

Comment 9

[Eric Pollmann]

Attachment: reduced test case (insecure form submit warning must be on)

Comment 10

[selmer (gone)]

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

[Eric Pollmann]

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

[Eric Pollmann]

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

[Eric Pollmann]

*** Bug 89888 has been marked as a duplicate of this bug. ***

Comment 14

[Eric Pollmann]

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

[Eric Pollmann]

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

[Eric Pollmann]

Attachment: patch: only trap KEY_PRESS not KEY_UP on submit inputs (back out fix for bug 68846)

Comment 17

[Eric Pollmann]

I am not able to reproduce bug 68846, even after backing out this part of the 
fix.  CC'ing Blake... 

Comment 18

[Eric Pollmann]

Attachment: patch 2: Trap KEY_PRESS for ENTER and KEY_UP for SPACE

Comment 19

[Eric Pollmann]

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.

Comment 20

[Eric Pollmann]

Attachment: Patch 2 with cleanup, per jag's review...

Comment 21

[jag (Peter Annema)]

r=jag

Comment 22

[Johnny Stenback (:jst)]

sr=jst

Comment 23

[Eric Pollmann]

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...  :) )

Comment 24

[hirata masakazu]

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

[hirata masakazu]

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

[Eric Pollmann]

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

[hirata masakazu]

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.

Comment 28

[Basil Fritts]

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

[Eric Pollmann]

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

[Eric Pollmann]

Ahh, whew!  It's there.  :)  Basil, try: 

ftp://ftp.mozilla.org/pub/mozilla/nightly/2001-07-23-09-trunk

Comment 31

[Eric Pollmann]

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

[Eric Pollmann]

Checked in on the branch.

Comment 33

[Eric Pollmann]

Updating whiteboard so this doesn't look like rogue checkin.  ;)  [PDT gave
approval verbally at a meeting]

Comment 34

[selmer (gone)]

You did the right thing.  Updating to mention checkin has happened.

Comment 35

[selmer (gone)]

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

[Vladimir Ermakov]

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

Comment 37

[Kevin McCluskey (gone)]

Bulk reassigning form bugs to Alex

Comment 38

[Jaime Rodriguez, Jr.]

is this fixed on the 094 branch?

Comment 39

[lchiang]

gerardo or vladimir - pls verify.  I assume this is fixed for 094 since the fix
was in 092

Comment 40

[Vladimir Ermakov]

resolving

Comment 41

[Vladimir Ermakov]

verifying