Bug 30365

Summary: empty optgroup crashes browser upon form submit
Product: WebKit Reporter: Derek Jones <derek.jones>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, kuchhal, mitz
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.6   
Attachments:
Description Flags
form snippet
none
Crash report
none
Crash fix and a layout test for it.
ap: review+, ap: commit-queue-
new patch without tabs.
eric: review+, commit-queue: commit-queue-
new patch without tabs and with proper EOLs none

Derek Jones
Reported 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>
Attachments
form snippet (189 bytes, text/html)
2009-10-14 13:22 PDT, Derek Jones
no flags
Crash report (38.00 KB, text/plain)
2009-10-14 13:34 PDT, Derek Jones
no flags
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-
new patch without tabs. (3.29 KB, patch)
2009-10-23 11:51 PDT, Rahul Kuchhal
eric: review+
commit-queue: commit-queue-
new patch without tabs and with proper EOLs (3.28 KB, patch)
2009-10-23 17:55 PDT, Rahul Kuchhal
no flags
Derek Jones
Comment 1 2009-10-14 13:34:59 PDT
Created attachment 41187 [details] Crash report (if it's helpful)
Rahul Kuchhal
Comment 2 2009-10-22 16:16:34 PDT
Created attachment 41693 [details] Crash fix and a layout test for it.
Alexey Proskuryakov
Comment 3 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.
Rahul Kuchhal
Comment 4 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?
Rahul Kuchhal
Comment 5 2009-10-23 11:51:08 PDT
Created attachment 41740 [details] new patch without tabs.
Eric Seidel (no email)
Comment 6 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+.
Eric Seidel (no email)
Comment 7 2009-10-23 17:23:28 PDT
Comment on attachment 41740 [details] new patch without tabs. LGTM too.
WebKit Commit Bot
Comment 8 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
Eric Seidel (no email)
Comment 9 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.
Rahul Kuchhal
Comment 10 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.
WebKit Commit Bot
Comment 11 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>
WebKit Commit Bot
Comment 12 2009-10-26 12:20:14 PDT
All reviewed patches have been landed. Closing bug.
mitz
Comment 13 2010-01-03 09:07:33 PST
mitz
Comment 14 2010-01-03 09:21:01 PST
Also occurs with <select><hr></select>
Note You need to log in before you can comment on or make changes to this bug.