Bug 151513 - REGRESSION(r192247): [GTK] ASSERTION FAILED: type == WebCore::ActionType || type == WebCore::CheckableActionType || type == WebCore::SeparatorType
Summary: REGRESSION(r192247): [GTK] ASSERTION FAILED: type == WebCore::ActionType || t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-20 14:12 PST by Michael Catanzaro
Modified: 2015-12-01 05:57 PST (History)
8 users (show)

See Also:


Attachments
Patch (4.76 KB, patch)
2015-11-30 04:19 PST, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-11-20 14:12:43 PST
Right-click inside a text entry:

ASSERTION FAILED: type == WebCore::ActionType || type == WebCore::CheckableActionType || type == WebCore::SeparatorType
../../Source/WebKit2/Shared/WebContextMenuItemData.cpp(57) : WebKit::WebContextMenuItemData::WebContextMenuItemData(WebCore::ContextMenuItemType, WebCore::ContextMenuAction, const WTF::String &, bool, bool)
1   0x7f430abf785d /home/mcatanzaro/src/jhbuild/install/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x1d) [0x7f430abf785d]
2   0x7f430f041cc9 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit22WebContextMenuItemDataC2EN7WebCore19ContextMenuItemTypeENS1_17ContextMenuActionERKN3WTF6StringEbb+0xd9) [0x7f430f041cc9]
3   0x7f430f4eb2cf /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit21WebContextMenuItemGtkC2ERKNS_22WebContextMenuItemDataE+0x6f) [0x7f430f4eb2cf]
4   0x7f430f509010 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZSt11make_uniqueIN6WebKit21WebContextMenuItemGtkEJRKNS0_22WebContextMenuItemDataEEENSt10_Unique_ifIT_E14_Single_objectEDpOT0_+0x50) [0x7f430f509010]
5   0x7f430f507f11 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_Z27webkitContextMenuItemCreateRKN6WebKit22WebContextMenuItemDataE+0x51) [0x7f430f507f11]
6   0x7f430f503e81 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_Z23webkitContextMenuCreateRKN3WTF6VectorIN6WebKit22WebContextMenuItemDataELm0ENS_15CrashOnOverflowELm16EEE+0x61) [0x7f430f503e81]
7   0x7f430f5d7841 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN21PageContextMenuClient29getCustomMenuFromDefaultItemsERN6WebKit7WebPageERKN7WebCore13HitTestResultERKN3WTF6VectorINS3_15ContextMenuItemELm0ENS7_15CrashOnOverflowELm16EEERNS8_INS0_22WebContextMenuItemDataELm0ESA_Lm16EEERNS7_6RefPtrIN3API6ObjectEEE+0x51) [0x7f430f5d7841]
8   0x7f430f418f1f /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZNK6WebKit14WebContextMenu21menuItemsWithUserDataERN3WTF6VectorINS_22WebContextMenuItemDataELm0ENS1_15CrashOnOverflowELm16EEERNS1_6RefPtrIN3API6ObjectEEE+0xaf) [0x7f430f418f1f]
9   0x7f430f418bef /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit14WebContextMenu4showEv+0x9f) [0x7f430f418bef]
10  0x7f430f43ec31 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(+0x3e89c31) [0x7f430f43ec31]
11  0x7f430f436108 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(+0x3e81108) [0x7f430f436108]
12  0x7f430f435ec1 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit7WebPage10mouseEventERKNS_13WebMouseEventE+0x1f1) [0x7f430f435ec1]
13  0x7f430f6b4cd6 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC22callMemberFunctionImplIN6WebKit7WebPageEMS2_FvRKNS1_13WebMouseEventEESt5tupleIJS3_EEJLm0EEEEvPT_T0_OT1_St14index_sequenceIJXspT2_EEE+0x96) [0x7f430f6b4cd6]
14  0x7f430f6b4c2c /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC18callMemberFunctionIN6WebKit7WebPageEMS2_FvRKNS1_13WebMouseEventEESt5tupleIJS3_EESt19make_index_sequenceILm1EEEEvOT1_PT_T0_+0x6c) [0x7f430f6b4c2c]
15  0x7f430f6ac6fa /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC13handleMessageIN8Messages7WebPage10MouseEventEN6WebKit7WebPageEMS5_FvRKNS4_13WebMouseEventEEEEvRNS_14MessageDecoderEPT0_T1_+0xba) [0x7f430f6ac6fa]
16  0x7f430f6a7fe0 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit7WebPage24didReceiveWebPageMessageERN3IPC10ConnectionERNS1_14MessageDecoderE+0x5a0) [0x7f430f6a7fe0]
17  0x7f430f43a79a /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit7WebPage17didReceiveMessageERN3IPC10ConnectionERNS1_14MessageDecoderE+0x17a) [0x7f430f43a79a]
18  0x7f430f43a7e4 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZThn16_N6WebKit7WebPage17didReceiveMessageERN3IPC10ConnectionERNS1_14MessageDecoderE+0x34) [0x7f430f43a7e4]
19  0x7f430eff6eb8 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC18MessageReceiverMap15dispatchMessageERNS_10ConnectionERNS_14MessageDecoderE+0x118) [0x7f430eff6eb8]
20  0x7f430f28b9fd /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit10WebProcess17didReceiveMessageERN3IPC10ConnectionERNS1_14MessageDecoderE+0x3d) [0x7f430f28b9fd]
21  0x7f430efe15f3 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC10Connection15dispatchMessageERNS_14MessageDecoderE+0x33) [0x7f430efe15f3]
22  0x7f430efdc096 /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC10Connection15dispatchMessageESt10unique_ptrINS_14MessageDecoderESt14default_deleteIS2_EE+0x166) [0x7f430efdc096]
23  0x7f430efe171b /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZN3IPC10Connection18dispatchOneMessageEv+0x11b) [0x7f430efe171b]
24  0x7f430efe41dd /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(+0x3a2f1dd) [0x7f430efe41dd]
25  0x7f430efe3ffd /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(+0x3a2effd) [0x7f430efe3ffd]
26  0x7f430ef41e4e /home/mcatanzaro/src/jhbuild/install/lib/libwebkit2gtk-4.0.so.37(_ZNKSt8functionIFvvEEclEv+0x3e) [0x7f430ef41e4e]
27  0x7f430ac1621a /home/mcatanzaro/src/jhbuild/install/lib/libjavascriptcoregtk-4.0.so.18(_ZN3WTF7RunLoop11performWorkEv+0x13a) [0x7f430ac1621a]
28  0x7f430ac59adc /home/mcatanzaro/src/jhbuild/install/lib/libjavascriptcoregtk-4.0.so.18(+0x1721adc) [0x7f430ac59adc]
29  0x7f430ac59ab8 /home/mcatanzaro/src/jhbuild/install/lib/libjavascriptcoregtk-4.0.so.18(+0x1721ab8) [0x7f430ac59ab8]
30  0x7f430ac59a91 /home/mcatanzaro/src/jhbuild/install/lib/libjavascriptcoregtk-4.0.so.18(+0x1721a91) [0x7f430ac59a91]
31  0x7f430ac59a38 /home/mcatanzaro/src/jhbuild/install/lib/libjavascriptcoregtk-4.0.so.18(+0x1721a38) [0x7f430ac59a38]
Comment 1 Michael Catanzaro 2015-11-28 13:22:54 PST
The problem is that the WebContextMenuItemGtk::WebContextMenuItemGtk(const WebContextMenuItemData& data) always fails if data is of type ContextMenuItemType::SubmenuType, because it chains up to the wrong WebContextMenuItemData constructor. There's no way to determine the right constructor to chain up to, so I added a new one accepting a WebContextMenuItemData and switched WebContextMenuItemGtk::WebContextMenuItemGtk(const WebContextMenuItemData& data) to use it instead:

WebContextMenuItemData::WebContextMenuItemData(const WebContextMenuItemData& data)
    : m_type(data.m_type)
    , m_action(data.m_action)
    , m_title(data.m_title)
{
    if (m_type == WebCore::SubmenuType)
        m_submenu = data.submenu();
    
    m_enabled = data.m_enabled;
    m_checked = data.m_checked;
}

That fixed the crash, but resulted in a submenu that is completely empty, which isn't good. Also, the GTK+ API test TestContextMenu is still crashing on the same assertion. This requires further investigation.

Also possibly problematic is the fact that our API allows removing a submenu (by passing NULL to webkit_context_menu_item_set_submenu) while leaving it with SubmenuType.
Comment 2 Carlos Garcia Campos 2015-11-30 04:19:09 PST
Created attachment 266243 [details]
Patch
Comment 3 WebKit Commit Bot 2015-11-30 04:20:45 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 4 Michael Catanzaro 2015-11-30 11:32:37 PST
Comment on attachment 266243 [details]
Patch

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

> Source/WebKit2/ChangeLog:15
> +        WebContextMenuItemGtk is SubmenuType if it has submenu itmes, but

itmes -> items

> Source/WebKit2/Shared/gtk/WebContextMenuItemGtk.cpp:128
> +    : WebContextMenuItemData(data.type() == SubmenuType ? ActionType : data.type(), data.action(), data.title(), data.enabled(), data.checked())

I think you should add this check to the constructor above, as well...

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:212
> +    item->priv->menuItem = std::make_unique<WebContextMenuItemGtk>(ActionType, ContextMenuItemBaseApplicationTag, String::fromUTF8(label));

...so that you don't need to do this.
Comment 5 Carlos Garcia Campos 2015-12-01 00:05:42 PST
Comment on attachment 266243 [details]
Patch

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

>> Source/WebKit2/Shared/gtk/WebContextMenuItemGtk.cpp:128
>> +    : WebContextMenuItemData(data.type() == SubmenuType ? ActionType : data.type(), data.action(), data.title(), data.enabled(), data.checked())
> 
> I think you should add this check to the constructor above, as well...

I don't think so, the constructor above receives a type, the caller should provide the right type. We internally don't use the submenu type,l so we only need to check it when constructing from an external source like WebContextMenuItemData, but not from our own implementation

>> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:212
>> +    item->priv->menuItem = std::make_unique<WebContextMenuItemGtk>(ActionType, ContextMenuItemBaseApplicationTag, String::fromUTF8(label));
> 
> ...so that you don't need to do this.

I prefer to make this explicit. This is building a WebContextMenuItemGtk, which doesn't use SubmenuType.
Comment 6 Martin Robinson 2015-12-01 05:40:31 PST
Comment on attachment 266243 [details]
Patch

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

>>> Source/WebKit2/Shared/gtk/WebContextMenuItemGtk.cpp:128
>>> +    : WebContextMenuItemData(data.type() == SubmenuType ? ActionType : data.type(), data.action(), data.title(), data.enabled(), data.checked())
>> 
>> I think you should add this check to the constructor above, as well...
> 
> I don't think so, the constructor above receives a type, the caller should provide the right type. We internally don't use the submenu type,l so we only need to check it when constructing from an external source like WebContextMenuItemData, but not from our own implementation

There should probably be an assertion there instead. The assertion can also serve as a type of documentation for what is going on. Can you do that before landing?

>>> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuItem.cpp:212
>>> +    item->priv->menuItem = std::make_unique<WebContextMenuItemGtk>(ActionType, ContextMenuItemBaseApplicationTag, String::fromUTF8(label));
>> 
>> ...so that you don't need to do this.
> 
> I prefer to make this explicit. This is building a WebContextMenuItemGtk, which doesn't use SubmenuType.

I agree with Michael here. I don't understand the motivation for having constructors that differ so much in behavior. On the other hand, I don't think it should block the patch. It's a somewhat minor point.
Comment 7 Carlos Garcia Campos 2015-12-01 05:57:40 PST
Committed r192881: <http://trac.webkit.org/changeset/192881>