Bug 73189 – [Chromium] Assertion fails when opening two popup menus

• Component: Forms
• Status: RESOLVED WONTFIX

[Jing Zhao]

The assertion in WebViewImpl::popupOpened() fails when opening two popup menus.

See the test in the patch.

[Jing Zhao]

Created attachment 116703 [details]
Patch

[Kent Tamura]

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.

[Alexey Proskuryakov]

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

[Jing Zhao]

Created attachment 116875 [details]
Patch

[Jing Zhao]

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.

[Kent Tamura]

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.

[Kent Tamura]

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?

[Jing Zhao]

Created attachment 116889 [details]
Patch

[Jing Zhao]

All done. Please take another look.

[Kent Tamura]

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.

[Jing Zhao]

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

[Kent Tamura]

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.

[Jing Zhao]

Created attachment 116903 [details]
Patch

[Kent Tamura]

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

[Jing Zhao]

Created attachment 116905 [details]
Patch

[Kent Tamura]

Comment on attachment 116905 [details]
Patch

ok

[Jing Zhao]

Kent, thank you for your review!

[WebKit Review Bot]

Comment on attachment 116905 [details]
Patch

Clearing flags on attachment: 116905

Committed r101337: <http://trac.webkit.org/changeset/101337>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

Reverted r101337 for reason:

It's a wrong way to fix the problem

Committed r101852: <http://trac.webkit.org/changeset/101852>

[Kent Tamura]

We are discussing a correct fix in Bug 73304. This bug was obsoleted.