Bug 73304 - Opening two popup menus by dispatchEvent() makes problems
: Opening two popup menus by dispatchEvent() makes problems
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-11-29 03:38 PST by
Modified: 2011-12-14 21:41 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-11-29 03:42:29 PST -------
Created an attachment (id=116940) [details]
Patch
------- Comment #2 From 2011-11-29 07:09:20 PST -------
Created an attachment (id=116962) [details]
Patch
------- Comment #3 From 2011-11-29 22:18:58 PST -------
Would you show the call stack of the crash please?
------- Comment #4 From 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 From 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 From 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 From 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 From 2011-12-01 23:44:10 PST -------
Created an attachment (id=117579) [details]
Patch
------- Comment #9 From 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 From 2011-12-02 00:01:12 PST -------
(From update of attachment 117579 [details])
Attachment 117579 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10695339
------- Comment #11 From 2011-12-02 00:21:07 PST -------
(From update of attachment 117579 [details])
Attachment 117579 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10725265
------- Comment #12 From 2011-12-02 08:00:47 PST -------
(From update of attachment 117579 [details])
Attachment 117579 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10711369
------- Comment #13 From 2011-12-02 14:16:15 PST -------
Anyway, I'll roll r101337 out. Crashing the next test is not good.
------- Comment #14 From 2011-12-02 14:50:32 PST -------
(From update of attachment 117579 [details])
Looks reasonable, but we can't break the other platforms.
------- Comment #15 From 2011-12-04 22:08:54 PST -------
Created an attachment (id=117841) [details]
Patch
------- Comment #16 From 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 From 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 From 2011-12-05 03:20:08 PST -------
(From update of attachment 117841 [details])
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 From 2011-12-08 23:16:20 PST -------
Created an attachment (id=118536) [details]
Patch
------- Comment #20 From 2011-12-08 23:33:50 PST -------
(From update of attachment 118536 [details])
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 From 2011-12-11 21:23:02 PST -------
Created an attachment (id=118729) [details]
Patch
------- Comment #22 From 2011-12-12 03:34:50 PST -------
Created an attachment (id=118766) [details]
Patch
------- Comment #23 From 2011-12-12 03:37:29 PST -------
Comparing with the second last patch, the last patch only updated the bug subject.
------- Comment #24 From 2011-12-13 16:30:20 PST -------
(From update of attachment 118766 [details])
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 From 2011-12-13 18:09:34 PST -------
Created an attachment (id=119128) [details]
Patch
------- Comment #26 From 2011-12-13 18:30:33 PST -------
(From update of attachment 119128 [details])
Looks good.
------- Comment #27 From 2011-12-13 23:14:07 PST -------
(From update of attachment 119128 [details])
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 From 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 From 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 From 2011-12-14 00:08:44 PST -------
Thanks. I'll change it to hasOpenedPopup().
------- Comment #31 From 2011-12-14 00:23:46 PST -------
(From update of attachment 119128 [details])
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 From 2011-12-14 02:26:59 PST -------
Created an attachment (id=119186) [details]
Patch
------- Comment #33 From 2011-12-14 03:39:55 PST -------
(From update of attachment 119186 [details])
ok
------- Comment #34 From 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 From 2011-12-14 18:07:38 PST -------
Please set commit-queue?.
------- Comment #36 From 2011-12-14 20:16:42 PST -------
(From update of attachment 119186 [details])
Clearing flags on attachment: 119186

Committed r102874: <http://trac.webkit.org/changeset/102874>
------- Comment #37 From 2011-12-14 20:16:48 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #38 From 2011-12-14 20:43:36 PST -------
Looks like I broke Mac and WinCE builds. Preparing a patch to fix them.
------- Comment #39 From 2011-12-14 21:07:52 PST -------
Reopening to attach new patch.
------- Comment #40 From 2011-12-14 21:07:57 PST -------
Created an attachment (id=119369) [details]
Patch
------- Comment #41 From 2011-12-14 21:10:04 PST -------
Mac build fix landed on http://trac.webkit.org/changeset/102879.
------- Comment #42 From 2011-12-14 21:10:38 PST -------
(From update of attachment 119369 [details])
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 From 2011-12-14 21:23:55 PST -------
Created an attachment (id=119370) [details]
Patch
------- Comment #44 From 2011-12-14 21:40:56 PST -------
(From update of attachment 119370 [details])
Clearing flags on attachment: 119370

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