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]
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.
Created attachment 266243 [details] Patch
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 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 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 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.
Committed r192881: <http://trac.webkit.org/changeset/192881>