Bug 30365 – empty optgroup crashes browser upon form submit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Derek Jones]

Created attachment 41184 [details]
form snippet

An empty optgroup in a multi select will cause a browser crash when the form is submitted.

Reproduce with (attached file contains HTML snippet required to reproduce):

<form action="test_optgroup.html">
<div>
	<select id="foo" name="bar">
    	<optgroup label="foo" ></optgroup>
	</select>
</div>
<div>
	<input type="submit" value="submit" />
</div>
</form>

[Derek Jones]

Created attachment 41187 [details]
Crash report

(if it's helpful)

[Rahul Kuchhal]

Created attachment 41693 [details]
Crash fix and a layout test for it.

[Alexey Proskuryakov]

Comment on attachment 41693 [details]
Crash fix and a layout test for it.

+	https://bugs.webkit.org/show_bug.cgi?id=30365

Tab here, please use spaces.

The comment above the code that you modified is misguiding - as we now know, this case can legitimately happen if the select only contains optgroup children. And the comment doesn't suggest any criteria to verify if it's still relevant - maybe we have already fixed that? The comment should mention the case with optgroups, and then say that this should be the only such case. Maybe you could add an assertion to this effect and run layout tests to check if the assertion is hit.

r=me as is, with only the tab fixed, but this code could benefit from a little more attention.

[Rahul Kuchhal]

I always forget to check for tabs and it looks like prepare-ChangeLog inserts them. Sorry about that, I am attaching a new one without tabs.

Can it be put into the commit queue now?

[Rahul Kuchhal]

Created attachment 41740 [details]
new patch without tabs.

[Eric Seidel (no email)]

Comment on attachment 41740 [details]
new patch without tabs.

If you want this patch cq+'d it will also need an r+.

[Eric Seidel (no email)]

Comment on attachment 41740 [details]
new patch without tabs.

LGTM too.

[WebKit Commit Bot]

Comment on attachment 41740 [details]
new patch without tabs.

Rejecting patch 41740 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11511 test cases.
fast/forms/select-empty-optgroup.html -> failed

Exiting early after 1 failures. 6555 tests run.
124.76s total testing time

6554 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
2 test cases (<1%) had stderr output

[Eric Seidel (no email)]

--- /tmp/layout-test-results/fast/forms/select-empty-optgroup-expected.txt      2009-10-23 17:32:56.000000000 -0700
+++ /tmp/layout-test-results/fast/forms/select-empty-optgroup-actual.txt        2009-10-23 17:32:56.000000000 -0700
@@ -1,4 +1,4 @@
-This tests submits on an empty select. If successful, there should not be a crash.
-
-
-
+This tests submits on an empty select. If successful, there should not be a crash.
+
+
+

Looks like line ending differences.

[Rahul Kuchhal]

Created attachment 41769 [details]
new patch without tabs and with proper EOLs

I had run the layout test using Chromium layout script. Didn't realize it is different than WebKit run script. Sorry about that.

[WebKit Commit Bot]

Comment on attachment 41769 [details]
new patch without tabs and with proper EOLs

Clearing flags on attachment: 41769

Committed r50083: <http://trac.webkit.org/changeset/50083>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

<rdar://problem/7129422>

Also occurs with <select><hr></select>