252526 - Don't submit form when pressing Return on fileUpload "Browse..." button (can cause crash)

Status: RESOLVED WORKSFORME

(Core :: Layout: Form Controls, defect)

Description

[Wolfgang Schwarz]

When I hit the return key with focus on a file upload's "Browse..." button, I
expect the file selection dialog to appear. This happens, but at the same time,
the form in the document underneath gets submitted, which is not what I expect
-- after all, I just wanted to add a local file into that form.

Steps to reproduce:
1. Go to http://validator.w3.org/
2. Tab the focus to the "Browse..." button
3. Hit Return

I'm seeing this on Linux in both SeaMonkey and Firefox.

This is particularly troublesome because in a case I frequently use, it reliably
crashes Mozilla. The problem there is that the form is in a popup window, which
closes itself after the form submission. So when I hit Enter on the "Browse..."
button, the file selection dialog appears, while in the meantime the window that
opened the dialog disappears. On closing the dialog (either by "Cancel" or by
selecting a file), both Firefox and SeaMonkey crash. 

I'll attach a testcase to illustrate this.

Steps to reproduce:
1. Open the attachment
2 [details] [diff] [review]. Click the link (opens a new window)
3. Hit Return on the "Browse..." button in the new window
4. Click "Cancel" in the file dialog

Comment 1

[Wolfgang Schwarz]

Attachment: the crash testcase

Comment 2

[Wolfgang Schwarz]

Attachment: the crash testcase

oops, the first testcase didn't work. This one should.

Comment 3

[Wolfgang Schwarz]

ok, bugzilla doesn't like my form. I've uploaded it the crash testcase to
http://umsu.de/misc/_mozform.html. Apologies for the spam.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to not submit

Comment 5

[Boris Zbarsky [:bzbarsky]]

Dan, that patch fixes the testcase, but I'm sure the crash could be triggered by
just having a keypress handler on the button that submits.... The crash happens at:

(gdb) frame 0
#0  0x41a5cb8f in nsContentTreeOwner::SetBlurSuppression(int) (this=0x8b81e40, 
    aBlurSuppression=0)
    at
/home/bzbarsky/mozilla/debug/mozilla/xpfe/appshell/src/nsContentTreeOwner.cpp:590
590       return mXULWindow->SetBlurSuppression(aBlurSuppression);
(gdb) p mXULWindow
$1 = (class nsXULWindow *) 0x0

It looks to me like the whole file needs 

  NS_ENSURE_TRUE(mXULWindow, NS_ERROR_NOT_AVAILABLE);

or something scattered about...  Thoughts? 

Comment 6

[Boris Zbarsky [:bzbarsky]]

Patch checked in, but the underlying problem remains... danm?

Comment 7

[Phil Ringnalda (:philor)]

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

Comment 8

[Phil Ringnalda (:philor)]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Mike, Neil, see comment 5...

Comment 10

[[e-mail]]

By the way, I don't like the proposed solution. At first I came up with a harebrained scheme whereby the VK_RETURN event that gets dispatched to the button, where at line 1492 gets turned into a click event, which then gets handled by the button, which should then set the event status to consume the event, which should then get propagated back to the key event, would mean that it doesn't trigger its parent's form submission. However I realized that it would be easier if the anonymous text control's form was the file control's form, then file controls wouldn't need their own VK_RETURN handling. Or, given roc's filepicker patch, you could just disable the text control completely.

Comment 11

[José Jeria]

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

Comment 12

[[e-mail]]

(In reply to comment #5)
> The crash happens at:
>
> (gdb) frame 0
> #0  0x41a5cb8f in nsContentTreeOwner::SetBlurSuppression(int) (this=0x8b81e40, 
>     aBlurSuppression=0)
>     at
> /home/bzbarsky/mozilla/debug/mozilla/xpfe/appshell/src/nsContentTreeOwner.cpp:590
> 590       return mXULWindow->SetBlurSuppression(aBlurSuppression);
> (gdb) p mXULWindow
> $1 = (class nsXULWindow *) 0x0
> 
> It looks to me like the whole file needs 
> 
>   NS_ENSURE_TRUE(mXULWindow, NS_ERROR_NOT_AVAILABLE);
> 
> or something scattered about...  Thoughts? 
That's probably a good idea but you'll find that it will only defer the crash in this case (the caller's caller is a dead frame at this point).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Right.  In this case we definitely want to be doing that initial "patch to not submit".

Comment 14

[Daniel Veditz [:dveditz]]

This currently WFM on Mac, and I know we've rewritten large swaths of the frame code between then and now. Has this been fixed or has the problem just moved around a bit, requiring a different testcase to demonstrate?

Comment 15

[Daniel Holbert [:dholbert]]

Sounds like this became WORKSFORME sometime in the 17 years that have passed since it was filed, based on comment 14 and my retesting today.

(I went to https://validator.w3.org/#validate_by_upload+with_options ("Validate by File Upload" selected), hit Tab 6 times to focus the file-picker object, and then pressed Enter. This caused a file picker dialog to appear, and it did not cause the form to be submitted.)