Bug 151513 – REGRESSION(r192247): [GTK] ASSERTION FAILED: type == WebCore::ActionType || type == WebCore::CheckableActionType || type == WebCore::SeparatorType

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

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]

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

Created attachment 266243 [details]
Patch

[WebKit Commit Bot]

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

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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.

[Martin Robinson]

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.

[Carlos Garcia Campos]

Committed r192881: <http://trac.webkit.org/changeset/192881>