Bug 73304 - Opening two popup menus by dispatchEvent() makes problems
: Opening two popup menus by dispatchEvent() makes problems
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Jing Zhao
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-29 03:38 PST by Jing Zhao
Modified: 2011-12-14 21:41 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.40 KB, patch)
2011-11-29 03:42 PST, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (3.83 KB, patch)
2011-11-29 07:09 PST, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (7.26 KB, patch)
2011-12-01 23:44 PST, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (19.21 KB, patch)
2011-12-04 22:08 PST, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (9.50 KB, patch)
2011-12-08 23:16 PST, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (19.97 KB, patch)
2011-12-11 21:23 PST, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (20.04 KB, patch)
2011-12-12 03:34 PST, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (20.17 KB, patch)
2011-12-13 18:09 PST, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (22.09 KB, patch)
2011-12-14 02:26 PST, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (3.10 KB, patch)
2011-12-14 21:07 PST, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (2.00 KB, patch)
2011-12-14 21:23 PST, Jing Zhao
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jing Zhao 2011-11-29 03:38:32 PST
Revert http://trac.webkit.org/changeset/101337.

The newly added test fast/forms/select-popup-crash.html causes its next test to crash on Linux Debug bot. I'd revert it and send out another fix.
Comment 1 Jing Zhao 2011-11-29 03:42:29 PST
Created attachment 116940 [details]
Patch
Comment 2 Jing Zhao 2011-11-29 07:09:20 PST
Created attachment 116962 [details]
Patch
Comment 3 Kent Tamura 2011-11-29 22:18:58 PST
Would you show the call stack of the crash please?
Comment 4 Jing Zhao 2011-11-30 00:04:17 PST
Reproduce the crash in Chromium Linux:
new-run-webkit-tests --debug fast/forms/select-popup-crash.html fast/forms/select-popup-pagekeys.html

Stack trace:
	base::debug::StackTrace::StackTrace() [0x714cb2]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x6d1a15]
	0x7f0c44c6faf0
	WebKit::WebViewImpl::popupClosed() [0x4c11cc]
	WebKit::ChromeClientImpl::popupClosed() [0x4d9e9b]
	WebCore::PopupContainer::notifyPopupHidden() [0xeb247c]
	WebCore::PopupListBox::hidePopup() [0xeb7298]
	WebCore::PopupListBox::abandon() [0xeb6540]
	WebCore::PopupContainer::hide() [0xeb2cd8]
	WebCore::PopupMenuChromium::hide() [0xeb3eac]
	WebCore::PopupMenuChromium::~PopupMenuChromium() [0xeb3d8d]
	WTF::RefCounted<>::deref() [0x4dbe92]
	WTF::derefIfNotNull<>() [0x165c380]
	WTF::RefPtr<>::operator=() [0x165c2d8]
	WebCore::RenderMenuList::~RenderMenuList() [0x16595b5]
	WebCore::RenderObject::arenaDelete() [0x166700d]
	WebCore::RenderObject::destroy() [0x1666e33]
	WebCore::Node::detach() [0xcee50f]
	WebCore::ContainerNode::detach() [0xc75a13]
	WebCore::Element::detach() [0xcd05b1]
	WebCore::HTMLFormControlElement::detach() [0xe59b5f]
	WebCore::ContainerNode::detach() [0xc759df]
	WebCore::Element::detach() [0xcd05b1]
	WebCore::ContainerNode::detach() [0xc759df]
	WebCore::Element::detach() [0xcd05b1]
	WebCore::ContainerNode::detach() [0xc759df]
	WebCore::Element::detach() [0xcd05b1]
	WebCore::ContainerNode::detach() [0xc759df]
	WebCore::Document::detach() [0xc8a75d]
	WebCore::Frame::setView() [0x12f073a]
	WebCore::Frame::createView() [0x12f2e15]
	WebKit::WebFrameImpl::createFrameView() [0x49ac7d]
	WebKit::FrameLoaderClientImpl::makeDocumentView() [0x4e6642]
	WebKit::FrameLoaderClientImpl::transitionToCommittedForNewPage() [0x4ea026]
	WebCore::FrameLoader::transitionToCommitted() [0x125864d]
	WebCore::FrameLoader::commitProvisionalLoad() [0x1257ade]
	WebCore::DocumentLoader::commitIfReady() [0x12398ce]
	WebCore::DocumentLoader::commitLoad() [0x123997c]
	WebCore::DocumentLoader::receivedData() [0x1239be2]
	WebCore::MainResourceLoader::addData() [0x126c9a3]
	WebCore::ResourceLoader::didReceiveData() [0x1280173]
	WebCore::MainResourceLoader::didReceiveData() [0x126ded2]
	WebCore::ResourceLoader::didReceiveData() [0x1280a82]
	WebCore::ResourceHandleInternal::didReceiveData() [0x4fcf1a]
	webkit_glue::WebURLLoaderImpl::Context::OnReceivedData() [0x1ae707c]
	(anonymous namespace)::RequestProxy::NotifyReceivedData() [0x1bb958c]
	base::internal::RunnableAdapter<>::Run() [0x1bbfc85]
	base::internal::InvokeHelper<>::MakeItSo() [0x1bbf678]
	base::internal::Invoker<>::Run() [0x1bbeae0]
	base::Callback<>::Run() [0x68e14f]
	MessageLoop::RunTask() [0x6a91a3]
	MessageLoop::DeferOrRunPendingTask() [0x6a92bd]
	MessageLoop::DoWork() [0x6a9adf]
	base::MessagePumpGlib::HandleDispatch() [0x70609d]
	(anonymous namespace)::WorkSourceDispatch() [0x7055d7]
	0x7f0c4a8cc8c2
	0x7f0c4a8d0748
	0x7f0c4a8d08fc
	base::MessagePumpGtk::RunOnce() [0x707905]
	base::MessagePumpGlib::RunWithDispatcher() [0x705d50]
	base::MessagePumpGlib::Run() [0x70617a]
	MessageLoop::RunInternal() [0x6a8e25]
Comment 5 Kent Tamura 2011-11-30 01:26:02 PST
Thanks.

The patch looks hacky. IMO, If PopupContainer::showPopup() fails to open a popup, we should not  proceed the following process.  Now I think r101337 was wrong.

Is webwidget created in ChromeClientImpl::popupOpened() correctly deleted for the second popup?
Comment 6 Jing Zhao 2011-11-30 02:05:35 PST
(In reply to comment #5)
> Thanks.
> 
> The patch looks hacky. IMO, If PopupContainer::showPopup() fails to open a popup, we should not  proceed the following process.  Now I think r101337 was wrong.
> 
> Is webwidget created in ChromeClientImpl::popupOpened() correctly deleted for the second popup?

Yes it's deleted as it was.

There are two ways to fix the problem. One is this patch together with 101337 , which only effects popup open and close in WebViewImpl, ensuring no more than one popup is shown or hidden. If there are more than one popups, they are created but not shown.

Another way to fix the problem is like you said not to create the PopupContainer. We need to add an API in ChromeClientImpl to tell if there is an opened popup. This way needs more code change.
Comment 7 Kent Tamura 2011-12-01 01:40:12 PST
(In reply to comment #6)
> There are two ways to fix the problem. One is this patch together with 101337 , which only effects popup open and close in WebViewImpl, ensuring no more than one popup is shown or hidden. If there are more than one popups, they are created but not shown.

The current code doesn't expect craeted-but-not-shown state.  I'm afraid it causes other problems like this bug, or will cause problems in the future.

Another question. If
 - A <select> popup is created and shown,
 - Another <select> popup is created and not shown (r101337),
 - The first <select> popup is closed, then
 - The second <select> is clicked again,
What happens?
Comment 8 Jing Zhao 2011-12-01 23:44:10 PST
Created attachment 117579 [details]
Patch
Comment 9 Jing Zhao 2011-12-01 23:47:14 PST
(In reply to comment #7)
> Another question. If
>  - A <select> popup is created and shown,
>  - Another <select> popup is created and not shown (r101337),
>  - The first <select> popup is closed, then
>  - The second <select> is clicked again,
> What happens?

Yes this is a good case. Both <select>s will be hidden in this case, which is wrong, because the second <select> should be shown.

I tried the other way in the new patch. Could you take a look?
Comment 10 Early Warning System Bot 2011-12-02 00:01:12 PST
Comment on attachment 117579 [details]
Patch

Attachment 117579 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10695339
Comment 11 Gyuyoung Kim 2011-12-02 00:21:07 PST
Comment on attachment 117579 [details]
Patch

Attachment 117579 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10725265
Comment 12 Gustavo Noronha (kov) 2011-12-02 08:00:47 PST
Comment on attachment 117579 [details]
Patch

Attachment 117579 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10711369
Comment 13 Kent Tamura 2011-12-02 14:16:15 PST
Anyway, I'll roll r101337 out. Crashing the next test is not good.
Comment 14 Eric Seidel 2011-12-02 14:50:32 PST
Comment on attachment 117579 [details]
Patch

Looks reasonable, but we can't break the other platforms.
Comment 15 Jing Zhao 2011-12-04 22:08:54 PST
Created attachment 117841 [details]
Patch
Comment 16 Jing Zhao 2011-12-04 22:13:25 PST
The new patch should fix the build on qt, gtk, efl and win, but popupIsVisible() simply returns false, so the new test may fail on these platforms.

Should I write different descriptions in different ChangeLogs?

Also I'm not sure if I break mac build.
Comment 17 Kent Tamura 2011-12-05 03:15:23 PST
(In reply to comment #9)
> (In reply to comment #7)
> > Another question. If
> >  - A <select> popup is created and shown,
> >  - Another <select> popup is created and not shown (r101337),
> >  - The first <select> popup is closed, then
> >  - The second <select> is clicked again,
> > What happens?
> 
> Yes this is a good case. Both <select>s will be hidden in this case, which is wrong, because the second <select> should be shown.

I found the latest WebKit + Safari had a similar problem.  So changing non-Chromium code is reasonable.
Comment 18 Kent Tamura 2011-12-05 03:20:08 PST
Comment on attachment 117841 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117841&action=review

> Source/WebCore/ChangeLog:3
> +        [Chromium] Fix assertion fails when opening two popup menus.

Please update this line.  This is not a Chromium-specific issue now.

> Source/WebCore/page/ChromeClient.h:305
> +        virtual bool popupIsVisible() = 0;

Please add a comment so that developers for non-Chromium ports can know what this is.

> Source/WebKit2/ChangeLog:14
> +        By using element.dispatchEvent(), a user written script can open two
> +        popup menus, which causes the assertion in WebViewImpl::popupOpened()
> +        fail.
> +
> +        Add a popupIsVisible() method in ChromeClientImpl and a wrapper in
> +        Chrome. In RenderMenuList::showPopup(), check if there is an opened
> +        popup menu before opening a new popup menu.

You don't need to copy the content of WebCore/ChangeLog.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:659
> +    return false;

You should add notImplemented().

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:549
> +    return false;

ditto.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:857
> +    return false;

ditto.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:717
> +    return false;

ditto.
Comment 19 Jing Zhao 2011-12-08 23:16:20 PST
Created attachment 118536 [details]
Patch
Comment 20 Kent Tamura 2011-12-08 23:33:50 PST
Comment on attachment 118536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118536&action=review

> Source/WebCore/page/ChromeClient.h:306
> +        virtual bool popupIsVisible() const { return false; }

Please do not add the implementation. This should be a pure virtual function.
We should implement this function for non-Chromium platforms too, and it's hard for non-Chromium developers to find this function if this function has the implementation here.

> Source/WebKit/chromium/src/ChromeClientImpl.h:187
> +    virtual bool popupIsVisible() const;

nit: We had better append OVERRIDE.
Comment 21 Jing Zhao 2011-12-11 21:23:02 PST
Created attachment 118729 [details]
Patch
Comment 22 Jing Zhao 2011-12-12 03:34:50 PST
Created attachment 118766 [details]
Patch
Comment 23 Jing Zhao 2011-12-12 03:37:29 PST
Comparing with the second last patch, the last patch only updated the bug subject.
Comment 24 Kent Tamura 2011-12-13 16:30:20 PST
Comment on attachment 118766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118766&action=review

The code looks good.  I don't set r+ yet because of some nits.

> Source/WebCore/ChangeLog:12
> +        popup menus, which causes the assertion in WebViewImpl::popupOpened()
> +        fail.
> +
> +        Add a popupIsVisible() method in ChromeClientImpl and a wrapper in

These sentences contains non-WebCore classes.  Please explain WebCore changes in WebCore/ChangeLog.

> Source/WebCore/ChangeLog:25
> +        * loader/EmptyClients.h:
> +        (WebCore::EmptyChromeClient::popupIsVisible):
> +        * page/Chrome.cpp:
> +        (WebCore::Chrome::popupIsVisible):
> +        * page/Chrome.h:
> +        * page/ChromeClient.h:
> +        * rendering/RenderMenuList.cpp:
> +        (WebCore::RenderMenuList::showPopup):

We had better write what is changed for each of files/functions.

> Source/WebCore/loader/EmptyClients.h:143
> +    virtual bool popupIsVisible() const { return false; }

Please add OVERRIDE.

> LayoutTests/ChangeLog:14
> +        By using element.dispatchEvent(), a user written script can open two
> +        popup menus, which causes the assertion in WebViewImpl::popupOpened()
> +        fail.
> +
> +        Add a popupIsVisible() method in ChromeClientImpl and a wrapper in
> +        Chrome. In RenderMenuList::showPopup(), check if there is an opened
> +        popup menu before opening a new popup menu.

I don't think you need to write this again in LayoutTests/ChangeLog.
Comment 25 Jing Zhao 2011-12-13 18:09:34 PST
Created attachment 119128 [details]
Patch
Comment 26 Kent Tamura 2011-12-13 18:30:33 PST
Comment on attachment 119128 [details]
Patch

Looks good.
Comment 27 Johnny(Jianning) Ding 2011-12-13 23:14:07 PST
Comment on attachment 119128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119128&action=review

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:944
> +    return m_webView->selectPopup();

The return value should be a boolean. please change to "return !!m_webView->selectPopup();"

Drive-by contents, I am not a reviewer, but have some comments for the patch.

(1) RenderMenuList already has a data member: m_popupIsVisible to indicate its visibility status,  The new method named popupIsVisible may make people by contrast with m_popupIsVisible. I think it might be better to name it hasAnotherOpenedPopup
(2) The last patch missed the WebKit mac port, please change that port as well.
Comment 28 Jing Zhao 2011-12-13 23:27:33 PST
I agree popupIsVisible is probably not a good name..

Kent, are you OK with the new name Johnny suggested, hasAnotherOpenedPopup? If you are OK with it, I'll update it in all files.
Comment 29 Kent Tamura 2011-12-13 23:38:33 PST
(In reply to comment #28)
> I agree popupIsVisible is probably not a good name..
> 
> Kent, are you OK with the new name Johnny suggested, hasAnotherOpenedPopup? If you are OK with it, I'll update it in all files.

'Another' is meaningless in Chrome/ChromeClient. 'hasOpenedPopup()', 'hasVisiblePopup()', or something?
Comment 30 Jing Zhao 2011-12-14 00:08:44 PST
Thanks. I'll change it to hasOpenedPopup().
Comment 31 Kent Tamura 2011-12-14 00:23:46 PST
Comment on attachment 119128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119128&action=review

> LayoutTests/ChangeLog:9
> +        * fast/forms/select-popup-crash-expected.txt: Added.
> +        * fast/forms/select-popup-crash.html: Added.

BTW, I'd like to move them to fast/forms/select/.
Comment 32 Jing Zhao 2011-12-14 02:26:59 PST
Created attachment 119186 [details]
Patch
Comment 33 Kent Tamura 2011-12-14 03:39:55 PST
Comment on attachment 119186 [details]
Patch

ok
Comment 34 Jing Zhao 2011-12-14 18:06:37 PST
Hi Kent,

Could you set commit-queue+ on this patch? Or should I wait for another reviewer?

Thanks,
Jing
Comment 35 Kent Tamura 2011-12-14 18:07:38 PST
Please set commit-queue?.
Comment 36 WebKit Review Bot 2011-12-14 20:16:42 PST
Comment on attachment 119186 [details]
Patch

Clearing flags on attachment: 119186

Committed r102874: <http://trac.webkit.org/changeset/102874>
Comment 37 WebKit Review Bot 2011-12-14 20:16:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Jing Zhao 2011-12-14 20:43:36 PST
Looks like I broke Mac and WinCE builds. Preparing a patch to fix them.
Comment 39 Jing Zhao 2011-12-14 21:07:52 PST
Reopening to attach new patch.
Comment 40 Jing Zhao 2011-12-14 21:07:57 PST
Created attachment 119369 [details]
Patch
Comment 41 Ryosuke Niwa 2011-12-14 21:10:04 PST
Mac build fix landed on http://trac.webkit.org/changeset/102879.
Comment 42 Ryosuke Niwa 2011-12-14 21:10:38 PST
Comment on attachment 119369 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119369&action=review

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:70
> +#import <WebCore/NotImplemented.h>

Please revert this change as I've already made it.
Comment 43 Jing Zhao 2011-12-14 21:23:55 PST
Created attachment 119370 [details]
Patch
Comment 44 Kent Tamura 2011-12-14 21:40:56 PST
Comment on attachment 119370 [details]
Patch

Clearing flags on attachment: 119370

Committed r102883: <http://trac.webkit.org/changeset/102883>
Comment 45 Kent Tamura 2011-12-14 21:41:05 PST
All reviewed patches have been landed.  Closing bug.