Bug 37436 – [Chromium] Select popups assert when destroyed.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jay Civelli]

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.

[Jay Civelli]

Assert happens because PopupContainer::notifyPopupHidden() is called twice, once the popup is closed and once it is destroyed.

[Jay Civelli]

Created attachment 53153 [details]
Fix assert and adds unit-tests

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 53153 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1550428

[Adam Barth]

+ 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?

[Jay Campan]

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.

[Jay Campan]

Created attachment 53187 [details]
Attempt at fixing style issues.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 53187 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1568437

[David Levin]

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).

[Dimitri Glazkov (Google)]

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?

[Adam Barth]

> 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 Barth]

> 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!

[Dimitri Glazkov (Google)]

Comment on attachment 53187 [details]
Attempt at fixing style issues.

Sounds good.

[WebKit Commit Bot]

Comment on attachment 53187 [details]
Attempt at fixing style issues.

Clearing flags on attachment: 53187

Committed r57599: <http://trac.webkit.org/changeset/57599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57599 might have broken Chromium Mac Release

[Eric Seidel (no email)]

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.

[Jay Civelli]

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.

[WebKit Review Bot]

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.

[Dimitri Glazkov (Google)]

Comment on attachment 53457 [details]
Another attempt

ok. Let's try again.

[Dimitri Glazkov (Google)]

Oops, we forgot to reopen the bug.

[WebKit Commit Bot]

Comment on attachment 53457 [details]
Another attempt

Clearing flags on attachment: 53457

Committed r57724: <http://trac.webkit.org/changeset/57724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57724 might have broken Chromium Mac Release

[Jay Civelli]

Created attachment 53538 [details]
Fix Mac build

[Dimitri Glazkov (Google)]

Comment on attachment 53538 [details]
Fix Mac build

ok.

[Dimitri Glazkov (Google)]

Comment on attachment 53538 [details]
Fix Mac build

will land manually.

[Dimitri Glazkov (Google)]

Comment on attachment 53538 [details]
Fix Mac build

Clearing flags on attachment: 53538

Committed r57727: <http://trac.webkit.org/changeset/57727>

[Dimitri Glazkov (Google)]

All reviewed patches have been landed.  Closing bug.