|Summary:||empty optgroup crashes browser upon form submit|
|Product:||WebKit||Reporter:||Derek Jones <derek.jones>|
|Component:||New Bugs||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||commit-queue, eric, kuchhal, mitz|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.6|
Description Derek Jones 2009-10-14 13:22:35 PDT
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
Comment 14 mitz 2010-01-03 09:21:01 PST
Also occurs with <select><hr></select>