GNOME Bugzilla – Bug 782043
control-center may segfault when new account addition is cancelled
Last modified: 2017-08-17 19:33:18 UTC
This is from current git master: 5a01034dbd0a2f40b3e7425173c17288a35492d5 How to reproduce: 1. Open gnome-control-center -> Online accounts. 2. Below 'Add an account' select 'Google' 3. Before the page is loaded, click close and press the back button So that the home page of g-c-c is shown. 4. Wait for a few seconds. Result: gnome-control-center segfaults. libasan output: sadiq@rose:~/jhbuild/checkout/gnome-control-center$ jrun Gtk-Message: Failed to load module "canberra-gtk-module" Gtk-Message: Failed to load module "canberra-gtk-module" Gtk-Message: Failed to load module "canberra-gtk-module" Gtk-Message: Failed to load module "canberra-gtk-module" ASAN:DEADLYSIGNAL ================================================================= ==2210==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f5cdb669293 bp 0x7ffc6f827950 sp 0x7ffc6f827910 T0) #0 0x7f5cdb669292 in g_type_check_instance_cast /home/sadiq/jhbuild/checkout/glib/gobject/gtype.c:4057 #1 0x7f5ce53312b1 in GOA_OAUTH2_PROVIDER /media/sadiq/Main/Software/src/gnome/gnome-online-accounts/src/goabackend/goaoauth2provider.h:34 #2 0x7f5ce5332982 in on_web_view_decide_policy /media/sadiq/Main/Software/src/gnome/gnome-online-accounts/src/goabackend/goaoauth2provider.c:845 #3 0x7f5cd79e77c8 in webkit_marshal_BOOLEAN__OBJECT_ENUM (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x8b17c8) #4 0x7f5cdb641d23 in g_closure_invoke /home/sadiq/jhbuild/checkout/glib/gobject/gclosure.c:804 #5 0x7f5cdb6602a4 in signal_emit_unlocked_R /home/sadiq/jhbuild/checkout/glib/gobject/gsignal.c:3635 #6 0x7f5cdb65f676 in g_signal_emit_valist /home/sadiq/jhbuild/checkout/glib/gobject/gsignal.c:3401 #7 0x7f5cdb65fb28 in g_signal_emit /home/sadiq/jhbuild/checkout/glib/gobject/gsignal.c:3447 #8 0x7f5cd78d8fad in webkitWebViewMakePolicyDecision(_WebKitWebView*, WebKitPolicyDecisionType, _WebKitPolicyDecision*) (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x7a2fad) #9 0x7f5cd78bc9f3 in PolicyClient::decidePolicyForResponse(WebKit::WebPageProxy&, WebKit::WebFrameProxy&, WebCore::ResourceResponse const&, WebCore::ResourceRequest const&, bool, WTF::Ref<WebKit::WebFramePolicyListenerProxy>&&, API::Object*) (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x7869f3) #10 0x7f5cd76d0a14 in WebKit::WebPageProxy::decidePolicyForResponse(unsigned long, WebCore::SecurityOriginData const&, WebCore::ResourceResponse const&, WebCore::ResourceRequest const&, bool, unsigned long, WebKit::UserData const&) (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x59aa14) #11 0x7f5cd76d0ac1 in WebKit::WebPageProxy::decidePolicyForResponseSync(unsigned long, WebCore::SecurityOriginData const&, WebCore::ResourceResponse const&, WebCore::ResourceRequest const&, bool, unsigned long, WebKit::UserData const&, bool&, unsigned long&, WebKit::DownloadID&) (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x59aac1) #12 0x7f5cd7a0a294 in void IPC::handleMessage<Messages::WebPageProxy::DecidePolicyForResponseSync, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long, WebCore::SecurityOriginData const&, WebCore::ResourceResponse const&, WebCore::ResourceRequest const&, bool, unsigned long, WebKit::UserData const&, bool&, unsigned long&, WebKit::DownloadID&)>(IPC::Decoder&, IPC::Encoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long, WebCore::SecurityOriginData const&, WebCore::ResourceResponse const&, WebCore::ResourceRequest const&, bool, unsigned long, WebKit::UserData const&, bool&, unsigned long&, WebKit::DownloadID&)) (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x8d4294) #13 0x7f5cd7a02282 in WebKit::WebPageProxy::didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder, std::default_delete<IPC::Encoder> >&) (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x8cc282) #14 0x7f5cd7639c60 in IPC::MessageReceiverMap::dispatchSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder, std::default_delete<IPC::Encoder> >&) (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x503c60) #15 0x7f5cd76f8c7a in WebKit::WebProcessProxy::didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder, std::default_delete<IPC::Encoder> >&) (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x5c2c7a) #16 0x7f5cd76353da in IPC::Connection::dispatchSyncMessage(IPC::Decoder&) (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x4ff3da) #17 0x7f5cd76354a4 in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x4ff4a4) #18 0x7f5cd7636387 in IPC::Connection::dispatchOneMessage() (/home/sadiq/jhbuild/install/lib/libwebkit2gtk-4.0.so.37+0x500387) #19 0x7f5cd6ee8f84 in WTF::RunLoop::performWork() (/home/sadiq/jhbuild/install/lib/libjavascriptcoregtk-4.0.so.18+0xd84f84) #20 0x7f5cd6f1fa08 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) (/home/sadiq/jhbuild/install/lib/libjavascriptcoregtk-4.0.so.18+0xdbba08) #21 0x7f5cdb356d58 in g_main_dispatch /home/sadiq/jhbuild/checkout/glib/glib/gmain.c:3230 #22 0x7f5cdb357c20 in g_main_context_dispatch /home/sadiq/jhbuild/checkout/glib/glib/gmain.c:3895 #23 0x7f5cdb357e0a in g_main_context_iterate /home/sadiq/jhbuild/checkout/glib/glib/gmain.c:3968 #24 0x7f5cdb357eda in g_main_context_iteration /home/sadiq/jhbuild/checkout/glib/glib/gmain.c:4029 #25 0x7f5cdb956aae in g_application_run /home/sadiq/jhbuild/checkout/glib/gio/gapplication.c:2381 #26 0x560569bf6faf in main /home/sadiq/jhbuild/checkout/gnome-control-center/shell/main.c:57 #27 0x7f5cdad662b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0) #28 0x560569bf6da9 in _start (/media/sadiq/Temp/jhbuild/install/bin/gnome-control-center+0x110da9) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV /home/sadiq/jhbuild/checkout/glib/gobject/gtype.c:4057 in g_type_check_instance_cast ==2210==ABORTING backtrace from gdb: Thread 1 "gnome-control-c" received signal SIGSEGV, Segmentation fault. 0x00007fffec120293 in g_type_check_instance_cast (type_instance=0x6210002769d0, iface_type=106309032197792) at /home/sadiq/jhbuild/checkout/glib/gobject/gtype.c:4057 4057 node = lookup_type_node_I (type_instance->g_class->g_type); (gdb) thread apply all bt full
+ Trace 237413
Thread 1 (Thread 0x7ffff7ea3280 (LWP 2272))
Thanks
Mohammed, if you're interested in working on this, looking at the valgrind output when reproducing the problem should make it easier to solve.
Created attachment 357762 [details] [review] online-accounts: Destroy account dialog when disposing The current GNOME Online Accounts panel delegates destroying the account provider editor dialog to the default path, but this is flawed since the dialog may have a reference taken by the GoaProvider. Fix that by explicitly destroying the dialog when the panel disposes.
This patch must be applied on gnome-3-24 as well.
Review of attachment 357762 [details] [review]: Thanks for the patch, Georges! While the fix is spot on, the reasoning is a bit off. The GoaProvider doesn't take any references on the GtkDialog. They are related by this call but neither of them takes a reference on the other: goa_provider_add_account (provider, ..., dialog, ...); (Pedantically speaking, goa_provider_add_account should take a reference on 'dialog' as long as the method is active. However, it is one of those strange things that works asynchronously, but actually runs a blocking recursive main loop, so it doesn't matter.) The problem is that when the panel dies, the providers die with it, but not the dialog. So when WebKitWebView::decide-policy is emitted, the signal handler has an invalid pointer. That causes the crash. This is why the old GoaAddAccountDialog used to keep a reference on the GoaProviders. But a reference like that would stop the crash, but we'd still have a memory leak due to not destroying the dialog. ::: panels/online-accounts/cc-online-accounts-panel.c @@ +506,3 @@ object_class->finalize = cc_goa_panel_finalize; object_class->constructed = cc_goa_panel_constructed; + object_class->dispose = cc_goa_panel_dispose; Why not use the existing finalize method? :) If we were to be really pedantic about destroying ref-counted objects in dispose, then the existing finalize should be renamed to dispose, but that's not relevant to this patch.
Created attachment 357828 [details] [review] online-accounts: Fix crash after aborting web view and changing panels I tweaked the patch and the rewrote the commit message.
(In reply to Debarshi Ray from comment #4) > Review of attachment 357762 [details] [review] [review]: > ::: panels/online-accounts/cc-online-accounts-panel.c > @@ +506,3 @@ > object_class->finalize = cc_goa_panel_finalize; > object_class->constructed = cc_goa_panel_constructed; > + object_class->dispose = cc_goa_panel_dispose; > > Why not use the existing finalize method? :) Ok, I see why you had to gtk_widget_destroy the dialog in dispose. Since panel->edit_account_dialog is part of the panel's template, it gets NULLified by the time it reaches finalize. Specifically in GtkWidget's default destroy signal handler, which is run during the panel's dispose phase but after the panel's dispose method. I am going to repeat that I don't like having the dialog be part of the panel's template. We should split it out into a separate class after the 3.26 cycle is over. It is very non-idiomatic to have a composite template widget also define a separate top-level as a template child. Usually, template children don't need to be destroyed separately because that happens automatically when the main widget dies. But if a template child is a separate top-level, we do need to do that, as has been shown here. Secondly, they need to be specifically destroyed in dispose, not finalize. A lot of code out there doesn't differentiate between dispose and finalize, simply because there isn't any possibility of circular references. People are going to be badly tripped up by the subtle, barely noticeable distinction between dispose and finalize here. So, while a separate class for the dialog might lead to a bit more typing, I think it is the right thing to do here.
Created attachment 357831 [details] [review] online-accounts: Fix crash after aborting web view and changing panels Moved it back to dispose, added a comment and an explanation in the commit message.
Comment on attachment 357831 [details] [review] online-accounts: Fix crash after aborting web view and changing panels Pushed to both master and gnome-3-24.