The assertion in WebViewImpl::popupOpened() fails when opening two popup menus. See the test in the patch.
Created attachment 116703 [details] Patch
Comment on attachment 116703 [details] Patch If a user clicks a point outside of opening popup-menu, platform-specific code should close the popup-menu and dispatch mouse events.
> If a user clicks a point outside of opening popup-menu, platform-specific code should close the popup-menu and dispatch mouse events. It would be nice to an ASSERT for this, which should be easy to do by modifying this patch.
Created attachment 116875 [details] Patch
To clarify, the assertion failure only happens when script simulates the clicks, but not by user generated clicks. The difference is that script can directly dispatch the mousedown event to the select element, but user generated clicks can't do that. This happens on multiple platforms. We can open the test page to verify: Chrome Mac: Tab crash Chrome Windows: Two popups appear Chrome Linux: Two popups appear Safari 5.1 Mac: The second popup didn't appear. When two popups appear, the assertion would fail. With the last patch, the second popup won't appear in any platform.
Comment on attachment 116875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116875&action=review > Source/WebKit/chromium/ChangeLog:7 > + Assertion fails when opening two popup menus > + https://bugs.webkit.org/show_bug.cgi?id=73189 > + > + Reviewed by NOBODY (OOPS!). > + You should mention the detail of the problem and how to fix it in ChangeLog. > Source/WebKit/chromium/src/WebViewImpl.cpp:965 > + if (popupContainer->popupType() == WebCore::PopupContainer::Select) This 'if' needs {} because the content has two physical lines. > LayoutTests/fast/forms/select-popup-crash.html:9 > +<script src="script-tests/listbox-selection-2.js"></script> Please do not have a separated .js file. select-popup-crash.js should be folded into select-popup-crash.html.
Comment on attachment 116875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116875&action=review > LayoutTests/fast/forms/script-tests/select-popup-crash.js:1 > +description('<select> selection test for opening two popup menus.'); The test is very redundant. Dispatching two mousedown events on two <select>s is enough, right?
Created attachment 116889 [details] Patch
All done. Please take another look.
Comment on attachment 116889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116889&action=review > LayoutTests/fast/forms/select-popup-crash.html:13 > + parent.innerHTML = '<select id="sl1" size=5>' This <select> doesn't open a popup menu. Please confirm that this test crashes without your WebViewImpl.cpp change.
(In reply to comment #10) > (From update of attachment 116889 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116889&action=review > > > LayoutTests/fast/forms/select-popup-crash.html:13 > > + parent.innerHTML = '<select id="sl1" size=5>' > > This <select> doesn't open a popup menu. On what platform did you try it? I tested it in Chrome 17.0.942.0 dev on Linux and it opens two popup menus. > Please confirm that this test crashes without your WebViewImpl.cpp change. Yes I confirmed the crash (only in debug mode).
Comment on attachment 116889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116889&action=review >>> LayoutTests/fast/forms/select-popup-crash.html:13 >>> + parent.innerHTML = '<select id="sl1" size=5>' >> >> This <select> doesn't open a popup menu. >> Please confirm that this test crashes without your WebViewImpl.cpp change. > > On what platform did you try it? I tested it in Chrome 17.0.942.0 dev on Linux and it opens two popup menus. Really? <select> with size >= 2 never show popup menus unless ENABLE_NO_LISTBOX_RENDERING is enabled.
Created attachment 116903 [details] Patch
Comment on attachment 116903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116903&action=review > LayoutTests/fast/forms/select-popup-crash.html:42 > + // Determine the item height. > + var sl1 = document.getElementById('sl1'); > + var sl2 = document.getElementById('sl2'); > + These lines are not needed. > LayoutTests/fast/forms/select-popup-crash.html:54 > + mouseDownOnSelect("sl2"); > + We had better show a 'PASS' message; debug("PASS if the test didn't crash.");
Created attachment 116905 [details] Patch
Comment on attachment 116905 [details] Patch ok
Kent, thank you for your review!
Comment on attachment 116905 [details] Patch Clearing flags on attachment: 116905 Committed r101337: <http://trac.webkit.org/changeset/101337>
All reviewed patches have been landed. Closing bug.
Reverted r101337 for reason: It's a wrong way to fix the problem Committed r101852: <http://trac.webkit.org/changeset/101852>
We are discussing a correct fix in Bug 73304. This bug was obsoleted.