Opening two popup menus by dispatchEvent() makes problems

[Jing Zhao]

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.

[Jing Zhao]

Created attachment 116940 [details]
Patch

[Jing Zhao]

Created attachment 116962 [details]
Patch

[Kent Tamura]

Would you show the call stack of the crash please?

[Jing Zhao]

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]

[Kent Tamura]

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?

[Jing Zhao]

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

[Kent Tamura]

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

[Jing Zhao]

Created attachment 117579 [details]
Patch

[Jing Zhao]

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

[Early Warning System Bot]

Comment on attachment 117579 [details]
Patch

Attachment 117579 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10695339

[Gyuyoung Kim]

Comment on attachment 117579 [details]
Patch

Attachment 117579 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10725265

[Gustavo Noronha (kov)]

Comment on attachment 117579 [details]
Patch

Attachment 117579 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10711369

[Kent Tamura]

Anyway, I'll roll r101337 out. Crashing the next test is not good.

[Eric Seidel (no email)]

Comment on attachment 117579 [details]
Patch

Looks reasonable, but we can't break the other platforms.

[Jing Zhao]

Created attachment 117841 [details]
Patch

[Jing Zhao]

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.

[Kent Tamura]

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

[Kent Tamura]

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.

[Jing Zhao]

Created attachment 118536 [details]
Patch

[Kent Tamura]

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.

[Jing Zhao]

Created attachment 118729 [details]
Patch

[Jing Zhao]

Created attachment 118766 [details]
Patch

[Jing Zhao]

Comparing with the second last patch, the last patch only updated the bug subject.

[Kent Tamura]

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.

[Jing Zhao]

Created attachment 119128 [details]
Patch

[Kent Tamura]

Comment on attachment 119128 [details]
Patch

Looks good.

[Johnny(Jianning) Ding]

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.

[Jing Zhao]

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.

[Kent Tamura]

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

[Jing Zhao]

Thanks. I'll change it to hasOpenedPopup().

[Kent Tamura]

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

[Jing Zhao]

Created attachment 119186 [details]
Patch

[Kent Tamura]

Comment on attachment 119186 [details]
Patch

ok

[Jing Zhao]

Hi Kent,

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

Thanks,
Jing

[Kent Tamura]

Please set commit-queue?.

[WebKit Review Bot]

Comment on attachment 119186 [details]
Patch

Clearing flags on attachment: 119186

Committed r102874: <http://trac.webkit.org/changeset/102874>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jing Zhao]

Looks like I broke Mac and WinCE builds. Preparing a patch to fix them.

[Jing Zhao]

Reopening to attach new patch.

[Jing Zhao]

Created attachment 119369 [details]
Patch

[Ryosuke Niwa]

Mac build fix landed on http://trac.webkit.org/changeset/102879.

[Ryosuke Niwa]

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.

[Jing Zhao]

Created attachment 119370 [details]
Patch

[Kent Tamura]

Comment on attachment 119370 [details]
Patch

Clearing flags on attachment: 119370

Committed r102883: <http://trac.webkit.org/changeset/102883>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.