| Summary: | [GTK] Opening emoji chooser crashes UI process with GTK 3.24.30 | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||||||
| Component: | WebKitGTK | Assignee: | Adrian Perez <aperez> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | annulen, aperez, berto, bugs-noreply, cgarcia, ews-watchlist, gustavo, gyuyoung.kim, mcatanzaro, ryuan.choi, sergio | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | Linux | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Michael Catanzaro
2021-07-30 15:47:09 PDT
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]. |