Bug 228664 – [GTK] Opening emoji chooser crashes UI process with GTK 3.24.30

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

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>
............

[Adrian Perez]

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.

[EWS Watchlist]

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

[Adrian Perez]

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 :)

[Adrian Perez]

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.

[Adrian Perez]

Created attachment 441015 [details]
Patch

[Michael Catanzaro]

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());

[Carlos Garcia Campos]

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().

[Adrian Perez]

(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.

[Adrian Perez]

Created attachment 441053 [details]
Patch

[Michael Catanzaro]

(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!

[Michael Catanzaro]

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();

[Adrian Perez]

(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 =)

[Adrian Perez]

(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.

[Adrian Perez]

Created attachment 441130 [details]
Patch for landing

[Michael Catanzaro]

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.

[Adrian Perez]

Created attachment 441143 [details]
Patch for landing

[Adrian Perez]

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 :)

[EWS]

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].