Summary: | [Chromium] Select popups assert when destroyed. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jay Civelli <jcivelli> | ||||||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, dglazkov, eric, jcampan, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Bug Depends on: | 37605, 37711 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Jay Civelli
2010-04-11 22:58:42 PDT
Assert happens because PopupContainer::notifyPopupHidden() is called twice, once the popup is closed and once it is destroyed. Created attachment 53153 [details]
Fix assert and adds unit-tests
Attachment 53153 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/tests/PopupMenuTest.cpp:33: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
WebKit/chromium/tests/PopupMenuTest.cpp:35: Alphabetical sorting problem. [build/include_order] [4]
WebKit/chromium/tests/PopupMenuTest.cpp:37: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
WebKit/chromium/tests/PopupMenuTest.cpp:39: Alphabetical sorting problem. [build/include_order] [4]
WebKit/chromium/tests/PopupMenuTest.cpp:179: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 5 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 53153 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1550428 + WebKit/chromium/tests/PopupMenuTest.cpp WebKit doesn't hasn't traditionally had unit tests like this. How is this test built? How is it run? Why can't we test this with a LayoutTest? dgalzkov suggested to write unit-test for the select popup there. These tests are built and run on the Chromium bots, see webkit_unit_tests on the Chromium water-fall. I think it's hard to write layout tests that test the select popup behavior properly as it requires simulating focus/keyboard/mouse events and it needs to be able to test if the select popup is showing or not. Created attachment 53187 [details]
Attempt at fixing style issues.
Attachment 53187 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/tests/PopupMenuTest.cpp:33: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
WebKit/chromium/tests/PopupMenuTest.cpp:35: Alphabetical sorting problem. [build/include_order] [4]
WebKit/chromium/tests/PopupMenuTest.cpp:37: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
WebKit/chromium/tests/PopupMenuTest.cpp:39: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 4 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 53187 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1568437 Comment on attachment 53187 [details]
Attempt at fixing style issues.
Not an in depth review, but it seems this doesn't merit one at present since it appears to break the chromium build.
Please fix the chromium build issue and update a new patch (or explain why the build break really isn't caused by this change).
I am totally ok with the unit tests. It's very hard and round-hole-square-pegish to attempt to test this code through DRT. Unit test is the right tool for the job here. Jay tells me he can't repro the build break. It looks like an include path failure. Adam, Eric, can this be an EWS issue? > I am totally ok with the unit tests.
I learned at the WebKit meeting that we have a bunch of these sorts of tests and we're thinking about adding more in the future. Looking at EWS now.
> Adam, Eric, can this be an EWS issue?
That's entirely possible. Maybe we should try landing and watching the tree to make sure its a false positive? If it does turn out to be a false positive, can you file a bug for the EWS failure and CC me? Thanks!
Comment on attachment 53187 [details]
Attempt at fixing style issues.
Sounds good.
Comment on attachment 53187 [details] Attempt at fixing style issues. Clearing flags on attachment: 53187 Committed r57599: <http://trac.webkit.org/changeset/57599> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/57599 might have broken Chromium Mac Release Looks like the EWS bots already told us (twice) that this would break Chromium. :( The fix is probably simple, but if no one is around we should roll this out ASAP. Created attachment 53457 [details]
Another attempt
This patch was reverted previously as it broke the Mac build.
I fixed the compilation problems but the tests are not working yet on Mac and Linux (they assert when creating a font).
So I made them Windows only for now.
Attachment 53457 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/tests/PopupMenuTest.cpp:33: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
WebKit/chromium/tests/PopupMenuTest.cpp:35: Alphabetical sorting problem. [build/include_order] [4]
WebKit/chromium/tests/PopupMenuTest.cpp:37: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
WebKit/chromium/tests/PopupMenuTest.cpp:39: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 4 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 53457 [details]
Another attempt
ok. Let's try again.
Oops, we forgot to reopen the bug. Comment on attachment 53457 [details] Another attempt Clearing flags on attachment: 53457 Committed r57724: <http://trac.webkit.org/changeset/57724> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/57724 might have broken Chromium Mac Release Created attachment 53538 [details]
Fix Mac build
Comment on attachment 53538 [details]
Fix Mac build
ok.
Comment on attachment 53538 [details]
Fix Mac build
will land manually.
Comment on attachment 53538 [details] Fix Mac build Clearing flags on attachment: 53538 Committed r57727: <http://trac.webkit.org/changeset/57727> All reviewed patches have been landed. Closing bug. |