GTK 3.24.30 changed its emoji data format in https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/3521. Now opening the emoji chooser results in this UI process crash: (gdb) bt full #0 0x00007efdc24474bb in raise () at /usr/lib/x86_64-linux-gnu/libc.so.6 #1 0x00007efdc2430867 in abort () at /usr/lib/x86_64-linux-gnu/libc.so.6 #2 0x00007efdc276ec7c in g_assertion_message_expr.cold () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0 #3 0x00007efdc27cf69f in g_assertion_message_expr () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0 #4 0x00007efdc27f0ad7 in g_variant_serialised_n_children () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0 #5 0x00007efdc27ebc27 in g_variant_n_children () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0 #6 0x00007efdc27e714c in g_variant_iter_init () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0 #7 0x00007efdc27e71ac in g_variant_iter_new () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0 #8 0x00007efdbefd5a44 in webkitEmojiChooserSetupEmojiSections (buttonBox=<optimized out>, emojiBox=0x563d619b35f0, chooser=0x563d608ee3f0) at ../Source/WebKit/UIProcess/API/gtk/WebKitEmojiChooser.cpp:500 vAdjustment = <optimized out> flowBox = 0x563d60ba48d0 sections = {{firstEmojiName = 0x7efdc1007162 "grinning face", title = 0x7efdc1007170 "Smileys & People", iconName = 0x7efdc1007181 "emoji-people-symbolic", canHaveVariations = true}, {firstEmojiName = 0x7efdc1007197 "selfie", title = 0x7efdc100719e "Body & Clothing", iconName = 0x7efdc10071ae "emoji-body-symbolic", canHaveVariations = true}, {firstEmojiName = 0x7efdc10071c2 "monkey", title = 0x7efdc10071c9 "Animals & Nature", iconName = 0x7efdc10071da "emoji-nature-symbolic", canHaveVariations = false}, {firstEmojiName = 0x7efdc10071f0 "grapes", title = 0x7efdc10071f7 "Food & Drink", iconName = 0x7efdc1007204 "emoji-food-symbolic", canHaveVariations = false}, {firstEmojiName = 0x7efdc1007218 "globe showing Europe-Africa", title = 0x7efdc1007234 "Travel & Places", iconName = 0x7efdc1007244 "emoji-travel-symbolic", canHaveVariations = false}, {firstEmojiName = 0x7efdc100725a "jack-o-lantern", title = 0x7efdc1007269 "Activities", iconName = 0x7efdc1007274 "emoji-activities-symbolic", canHaveVariations = false}, {firstEmojiName = 0x7efdc100728e "muted speaker", title = 0x7efdc100714c "Objects", iconName = 0x7efdc100729c "emoji-objects-symbolic", canHaveVariations = false}, {firstEmojiName = 0x7efdc10072b3 "ATM sign", title = 0x7efdc10072bc "Symbols", iconName = 0x7efdc10072c4 "emoji-symbols-symbolic", canHaveVariations = false}, {firstEmojiName = 0x7efdc10072db "chequered flag", title = 0x7efdc1007154 "Flags", iconName = 0x7efdc10072ea "emoji-flags-symbolic", canHaveVariations = false}} chooser = 0x563d608ee3f0 mainBox = 0x563d619b3330 searchEntry = <optimized out> stack = 0x563d61984b60 box = 0x563d619b3490 swindow = <optimized out> emojiBox = 0x563d619b35f0 buttonBox = 0x563d619b3750 vAdjustment = <optimized out> #9 webkitEmojiChooserConstructed(GObject*) (object=0x563d608ee3f0) at ../Source/WebKit/UIProcess/API/gtk/WebKitEmojiChooser.cpp:588 chooser = 0x563d608ee3f0 mainBox = 0x563d619b3330 searchEntry = <optimized out> stack = 0x563d61984b60 box = 0x563d619b3490 swindow = <optimized out> emojiBox = 0x563d619b35f0 buttonBox = 0x563d619b3750 vAdjustment = <optimized out> ............
Created attachment 434935 [details] WIP Patch Untested, imports all the changes from the upstream gtk-3-24 branch (adapted to WebKit style, of course). This needs a bit of work to keep using the old code paths for GTK versions older than 3.24.30, so runtime checks still need to be added and some of the removed code brough back before even considering the patch ready.
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 434984 [details] Emoji picker almost working again Well, with a couple of tweaks at least there is no crash, I can see the picker, and enter emojis (recents work, too!) All the emojis get stashed in the first group, though. This seems related to the criticals, so hopefully won't be hard to iron out. Then I'll add some code to switch things around depending on the GTK version at runtime and we'll be good to go :)
Created attachment 435626 [details] WIP Patch v2 Better version, still needs a bit of work. This includes the runtime check to choose whether to use the old or the new emoji data format. There are a couple of cases where I still need to re-add support for the old format, mostly around recently used emojis.
Created attachment 441015 [details] Patch
Comment on attachment 441015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441015&action=review Well it's worth being a little evil when you get to delete this much code. I can't think of anything better. No design awards for you this day, though. :P > Source/WebKit/UIProcess/API/gtk/WebKitEmojiChooser.cpp:43 > + GtkWidget* entry = gtk_entry_new(); I think this is leaked? Since you don't add it to the widget hierarchy, you have to unref it manually, right? Solution: GRefPtr<GtkWidget> entry = adoptGRef(g_object_ref_sink(gtk_entry_new())); > Source/WebKit/UIProcess/API/gtk/WebKitEmojiChooser.cpp:57 > + GSource* source = g_idle_source_new(); Avoid the manual g_source_unref(): GRefPtr<GSource> source = adoptGRef(g_idle_source_new());
Comment on attachment 441015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441015&action=review > Source/WebKit/ChangeLog:12 > + Instead of providing a copy of the emoji chooser widget, use GLib's > + introspection features and a little sprinkle of knowledge about how > + GTK works in order to obtain the type code of the GtkEmojiChooser > + widget included with GTK. This ensures that a working widget is > + always used, regardless of the current GTK version. Great idea! >> Source/WebKit/UIProcess/API/gtk/WebKitEmojiChooser.cpp:43 >> + GtkWidget* entry = gtk_entry_new(); > > I think this is leaked? Since you don't add it to the widget hierarchy, you have to unref it manually, right? Solution: > > GRefPtr<GtkWidget> entry = adoptGRef(g_object_ref_sink(gtk_entry_new())); This is equivalent to just GRefPtr<GtkWidget> entry = gtk_entry_new(); but the entry is not leaked because gtk_widget_destroy is used, which ignores the refs and calls g_object_run_dispose().
(In reply to Carlos Garcia Campos from comment #7) > Comment on attachment 441015 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441015&action=review > > > Source/WebKit/ChangeLog:12 > > + Instead of providing a copy of the emoji chooser widget, use GLib's > > + introspection features and a little sprinkle of knowledge about how > > + GTK works in order to obtain the type code of the GtkEmojiChooser > > + widget included with GTK. This ensures that a working widget is > > + always used, regardless of the current GTK version. > > Great idea! I'm glad you didn't hate it :) > >> Source/WebKit/UIProcess/API/gtk/WebKitEmojiChooser.cpp:43 > >> + GtkWidget* entry = gtk_entry_new(); > > > > I think this is leaked? Since you don't add it to the widget hierarchy, you have to unref it manually, right? Solution: > > > > GRefPtr<GtkWidget> entry = adoptGRef(g_object_ref_sink(gtk_entry_new())); > > This is equivalent to just GRefPtr<GtkWidget> entry = gtk_entry_new(); but > the entry is not leaked because gtk_widget_destroy is used, which ignores > the refs and calls g_object_run_dispose(). Yes, I went with a call to gtk_widget_destroy() because it looked more clear and explicit to me that the widget is not used for anything else than forcing the GtkEmojiChooser class to be registered with the type system. I will change the patch to use GRefPtr<GSource> for the idle source before landing, though.
Created attachment 441053 [details] Patch
(In reply to Carlos Garcia Campos from comment #7) > > GRefPtr<GtkWidget> entry = adoptGRef(g_object_ref_sink(gtk_entry_new())); > > This is equivalent to just GRefPtr<GtkWidget> entry = gtk_entry_new(); Um, yes, that's right. Maybe I should add a comment in GRefPtr.h to remind us how to handle floating refs, since clearly I can't keep it straight. > but > the entry is not leaked because gtk_widget_destroy is used, which ignores > the refs and calls g_object_run_dispose(). Sure it will be disposed, since objects can be disposed multiple times, but it obviously cannot be finalized when there is an outstanding ref. The documentation of gtk_widget_destroy() says: """ It's important to notice that gtk_widget_destroy() will only cause the widget to be finalized if no additional references, acquired using g_object_ref(), are held on it. In case additional references are in place, the widget will be in an "inert" state after calling this function; widget will still point to valid memory, allowing you to release the references you hold, but you may not query the widget's own state. """ So I'm pretty sure it will be leaked!
Comment on attachment 441053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441053&action=review > Source/WebKit/UIProcess/API/gtk/WebKitEmojiChooser.cpp:46 > + GtkWidget* entry = gtk_entry_new(); > + gtk_entry_set_input_hints(GTK_ENTRY(entry), GTK_INPUT_HINT_EMOJI); > + g_object_ref_sink(entry); So I would do: GRefPtr<GtkWidget> entry = gtk_entry_new();
(In reply to Michael Catanzaro from comment #11) > Comment on attachment 441053 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441053&action=review > > > Source/WebKit/UIProcess/API/gtk/WebKitEmojiChooser.cpp:46 > > + GtkWidget* entry = gtk_entry_new(); > > + gtk_entry_set_input_hints(GTK_ENTRY(entry), GTK_INPUT_HINT_EMOJI); > > + g_object_ref_sink(entry); > > So I would do: > > GRefPtr<GtkWidget> entry = gtk_entry_new(); No, we don't want that: using GRefPtr would drop the last ref when the lambda goes out of scope, but we want to actually drop the last ref in the idle source callback where currently gtk_widget_destroy() is done. Otherwise we will hit a critical warning because GTK is missing a null check and it tries to unref a null GVariant which gets populated in the next main loop iteration. There is a comment explaining that in the patch, let me know if it is not clear enough and I can try to reword it. Also, I will update POTFILES to remove “WebKitEmojiChooser.cpp” before landing, which is something Carlos García reminded me earlier in a private chat =)
(In reply to Michael Catanzaro from comment #10) > (In reply to Carlos Garcia Campos from comment #7) > > > GRefPtr<GtkWidget> entry = adoptGRef(g_object_ref_sink(gtk_entry_new())); > > > > This is equivalent to just GRefPtr<GtkWidget> entry = gtk_entry_new(); > > Um, yes, that's right. Maybe I should add a comment in GRefPtr.h to remind > us how to handle floating refs, since clearly I can't keep it straight. > > > but > > the entry is not leaked because gtk_widget_destroy is used, which ignores > > the refs and calls g_object_run_dispose(). > > Sure it will be disposed, since objects can be disposed multiple times, but > it obviously cannot be finalized when there is an outstanding ref. The > documentation of gtk_widget_destroy() says: > > """ > It's important to notice that gtk_widget_destroy() will only cause the > widget to be finalized if no additional references, acquired using > g_object_ref(), are held on it. In case additional references are in place, > the widget will be in an "inert" state after calling this function; widget > will still point to valid memory, allowing you to release the references you > hold, but you may not query the widget's own state. > """ > > So I'm pretty sure it will be leaked! Mmmh, right. I actually checked the implementation and basically it only calls into g_object_run_dispose() — I need to add a call to g_object_unref(), and given that there are no circular references (i.e. the widget is a dummy one, not added into any hierarchy) we can skip the gtk_widget_destroy() call. We still need to free the widget in the idle callback, though, so there is little incentive to use GRefPtr for it.
Created attachment 441130 [details] Patch for landing
Comment on attachment 441130 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=441130&action=review > Source/WebKit/UIProcess/API/gtk/WebKitEmojiChooser.cpp:62 > + }, entry, nullptr); Eh, I would still store ownership in a GRefPtr for as long as possible, then use entry.leakRef() when you need to transfer the ownership into the lambda. Up to you. If you want to land as-is, just set cq+ again.
Created attachment 441143 [details] Patch for landing
Last version of the patch adds the GRefPtr suggested by Michael, but the g_object_unref() is kept explicit inside the idle lambda callback for clarity :)
Committed r284132 (242952@main): <https://commits.webkit.org/242952@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441143 [details].