Bug 30365 - empty optgroup crashes browser upon form submit
Summary: empty optgroup crashes browser upon form submit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-10-14 13:22 PDT by Derek Jones
Modified: 2010-01-03 09:21 PST (History)
4 users (show)

See Also:


Attachments
form snippet (189 bytes, text/html)
2009-10-14 13:22 PDT, Derek Jones
no flags Details
Crash report (38.00 KB, text/plain)
2009-10-14 13:34 PDT, Derek Jones
no flags Details
Crash fix and a layout test for it. (3.27 KB, patch)
2009-10-22 16:16 PDT, Rahul Kuchhal
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
new patch without tabs. (3.29 KB, patch)
2009-10-23 11:51 PDT, Rahul Kuchhal
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
new patch without tabs and with proper EOLs (3.28 KB, patch)
2009-10-23 17:55 PDT, Rahul Kuchhal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Derek Jones 2009-10-14 13:22:35 PDT
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>
Comment 1 Derek Jones 2009-10-14 13:34:59 PDT
Created attachment 41187 [details]
Crash report

(if it's helpful)
Comment 2 Rahul Kuchhal 2009-10-22 16:16:34 PDT
Created attachment 41693 [details]
Crash fix and a layout test for it.
Comment 3 Alexey Proskuryakov 2009-10-22 19:57:40 PDT
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.
Comment 4 Rahul Kuchhal 2009-10-23 11:50:21 PDT
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?
Comment 5 Rahul Kuchhal 2009-10-23 11:51:08 PDT
Created attachment 41740 [details]
new patch without tabs.
Comment 6 Eric Seidel (no email) 2009-10-23 14:02:06 PDT
Comment on attachment 41740 [details]
new patch without tabs.

If you want this patch cq+'d it will also need an r+.
Comment 7 Eric Seidel (no email) 2009-10-23 17:23:28 PDT
Comment on attachment 41740 [details]
new patch without tabs.

LGTM too.
Comment 8 WebKit Commit Bot 2009-10-23 17:33:01 PDT
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
Comment 9 Eric Seidel (no email) 2009-10-23 17:36:00 PDT
--- /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.
Comment 10 Rahul Kuchhal 2009-10-23 17:55:19 PDT
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.
Comment 11 WebKit Commit Bot 2009-10-26 12:20:07 PDT
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>
Comment 12 WebKit Commit Bot 2009-10-26 12:20:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 mitz 2010-01-03 09:07:33 PST
<rdar://problem/7129422>
Comment 14 mitz 2010-01-03 09:21:01 PST
Also occurs with <select><hr></select>