Bug 37436 - [Chromium] Select popups assert when destroyed.
Summary: [Chromium] Select popups assert when destroyed.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 37605 37711
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-11 22:58 PDT by Jay Civelli
Modified: 2010-04-16 11:08 PDT (History)
6 users (show)

See Also:


Attachments
Fix assert and adds unit-tests (16.67 KB, patch)
2010-04-11 23:16 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Attempt at fixing style issues. (16.68 KB, patch)
2010-04-12 14:15 PDT, Jay Campan
no flags Details | Formatted Diff | Diff
Another attempt (16.78 KB, patch)
2010-04-15 11:37 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff
Fix Mac build (1.22 KB, patch)
2010-04-16 10:52 PDT, Jay Civelli
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jay Civelli 2010-04-11 22:58:42 PDT
Navigate to a page with a select.
Click on the select to open the popup then select an item to close it.
Navigate to a new page.

This causes an ASSERT to be triggered.
Comment 1 Jay Civelli 2010-04-11 23:15:28 PDT
Assert happens because PopupContainer::notifyPopupHidden() is called twice, once the popup is closed and once it is destroyed.
Comment 2 Jay Civelli 2010-04-11 23:16:06 PDT
Created attachment 53153 [details]
Fix assert and adds unit-tests
Comment 3 WebKit Review Bot 2010-04-11 23:23:59 PDT
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.
Comment 4 WebKit Review Bot 2010-04-11 23:30:42 PDT
Attachment 53153 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1550428
Comment 5 Adam Barth 2010-04-11 23:37:22 PDT
+ 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?
Comment 6 Jay Campan 2010-04-12 13:57:16 PDT
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.
Comment 7 Jay Campan 2010-04-12 14:15:41 PDT
Created attachment 53187 [details]
Attempt at fixing style issues.
Comment 8 WebKit Review Bot 2010-04-12 14:17:25 PDT
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.
Comment 9 WebKit Review Bot 2010-04-12 14:21:40 PDT
Attachment 53187 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1568437
Comment 10 David Levin 2010-04-14 07:24:05 PDT
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).
Comment 11 Dimitri Glazkov (Google) 2010-04-14 09:59:24 PDT
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?
Comment 12 Adam Barth 2010-04-14 10:57:58 PDT
> 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.
Comment 13 Adam Barth 2010-04-14 11:01:28 PDT
> 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 14 Dimitri Glazkov (Google) 2010-04-14 11:08:21 PDT
Comment on attachment 53187 [details]
Attempt at fixing style issues.

Sounds good.
Comment 15 WebKit Commit Bot 2010-04-14 13:21:45 PDT
Comment on attachment 53187 [details]
Attempt at fixing style issues.

Clearing flags on attachment: 53187

Committed r57599: <http://trac.webkit.org/changeset/57599>
Comment 16 WebKit Commit Bot 2010-04-14 13:21:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 WebKit Review Bot 2010-04-14 13:30:17 PDT
http://trac.webkit.org/changeset/57599 might have broken Chromium Mac Release
Comment 18 Eric Seidel (no email) 2010-04-14 13:46:19 PDT
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.
Comment 19 Jay Civelli 2010-04-15 11:37:51 PDT
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.
Comment 20 WebKit Review Bot 2010-04-15 11:42:39 PDT
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 21 Dimitri Glazkov (Google) 2010-04-15 12:56:00 PDT
Comment on attachment 53457 [details]
Another attempt

ok. Let's try again.
Comment 22 Dimitri Glazkov (Google) 2010-04-16 09:41:43 PDT
Oops, we forgot to reopen the bug.
Comment 23 WebKit Commit Bot 2010-04-16 10:18:24 PDT
Comment on attachment 53457 [details]
Another attempt

Clearing flags on attachment: 53457

Committed r57724: <http://trac.webkit.org/changeset/57724>
Comment 24 WebKit Commit Bot 2010-04-16 10:18:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Review Bot 2010-04-16 10:31:20 PDT
http://trac.webkit.org/changeset/57724 might have broken Chromium Mac Release
Comment 26 Jay Civelli 2010-04-16 10:52:16 PDT
Created attachment 53538 [details]
Fix Mac build
Comment 27 Dimitri Glazkov (Google) 2010-04-16 11:00:06 PDT
Comment on attachment 53538 [details]
Fix Mac build

ok.
Comment 28 Dimitri Glazkov (Google) 2010-04-16 11:05:46 PDT
Comment on attachment 53538 [details]
Fix Mac build

will land manually.
Comment 29 Dimitri Glazkov (Google) 2010-04-16 11:08:04 PDT
Comment on attachment 53538 [details]
Fix Mac build

Clearing flags on attachment: 53538

Committed r57727: <http://trac.webkit.org/changeset/57727>
Comment 30 Dimitri Glazkov (Google) 2010-04-16 11:08:12 PDT
All reviewed patches have been landed.  Closing bug.