Bug 110865 – [GTK] Closing inspector window crashes wk

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

[Claudio Saavedra]

This is WK1. Here's the stacktrace:

#0  0x00007ffff38ff41e in WebKit::InspectorFrontendClient::~InspectorFrontendClient (this=0xb0ed90, __in_chrg=<optimized out>)
    at ../../Source/WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:192
#1  0x00007ffff38ff47a in WebKit::InspectorFrontendClient::~InspectorFrontendClient (this=0xb0ed90, __in_chrg=<optimized out>)
    at ../../Source/WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:193
#2  0x00007ffff39001a2 in WTF::deleteOwnedPtr<WebCore::InspectorFrontendClient> (ptr=0xb0ed90)
    at ../../Source/WTF/wtf/OwnPtrCommon.h:63
#3  0x00007ffff4030ec1 in WTF::OwnPtr<WebCore::InspectorFrontendClient>::~OwnPtr (this=0xafed98, __in_chrg=<optimized out>)
    at ../../Source/WTF/wtf/OwnPtr.h:63
#4  0x00007ffff402ef7b in WebCore::InspectorController::~InspectorController (this=0xafed30, __in_chrg=<optimized out>)
    at ../../Source/WebCore/inspector/InspectorController.cpp:189
#5  0x00007ffff42a9282 in WTF::deleteOwnedPtr<WebCore::InspectorController> (ptr=0xafed30) at ../../Source/WTF/wtf/OwnPtrCommon.h:63
#6  0x00007ffff42a7e41 in WTF::OwnPtr<WebCore::InspectorController>::~OwnPtr (this=0xaff078, __in_chrg=<optimized out>)
    at ../../Source/WTF/wtf/OwnPtr.h:63
#7  0x00007ffff42a254d in WebCore::Page::~Page (this=0xaff020, __in_chrg=<optimized out>) at ../../Source/WebCore/page/Page.cpp:194
#8  0x00007ffff392dec3 in webkit_web_view_dispose (object=0x7f43d0) at ../../Source/WebKit/gtk/webkit/webkitwebview.cpp:1353
#9  0x00007fffee8380ea in g_object_run_dispose (object=0x7f43d0) at gobject.c:1062
#10 0x00007ffff0a7acb6 in gtk_widget_destroy (widget=0x7f43d0) at gtkwidget.c:4090
#11 0x00007ffff0790498 in gtk_bin_forall (container=0x8a6280, include_internals=0, callback=0x7ffff0a7abff <gtk_widget_destroy>, 
    callback_data=0x0) at gtkbin.c:180
#12 0x00007ffff096d746 in gtk_scrolled_window_forall (container=0x8a6280, include_internals=0, 
    callback=0x7ffff0a7abff <gtk_widget_destroy>, callback_data=0x0) at gtkscrolledwindow.c:1582
#13 0x00007ffff07f6c34 in gtk_container_foreach (container=0x8a6280, callback=0x7ffff0a7abff <gtk_widget_destroy>, callback_data=0x0)
    at gtkcontainer.c:2074
#14 0x00007ffff07f52eb in gtk_container_destroy (widget=0x8a6280) at gtkcontainer.c:1377
#15 0x00007ffff096c9b7 in gtk_scrolled_window_destroy (widget=0x8a6280) at gtkscrolledwindow.c:1265
#16 0x00007fffee833c60 in g_cclosure_marshal_VOID__VOID (closure=0x6102c0, return_value=0x0, n_param_values=1, 
    param_values=0x7fffffffc4a0, invocation_hint=0x7fffffffc3d0, marshal_data=0x7ffff096c7b7 <gtk_scrolled_window_destroy>)
    at gmarshal.c:85
#17 0x00007fffee83123d in g_type_class_meta_marshal (closure=0x6102c0, return_value=0x0, n_param_values=1, 
    param_values=0x7fffffffc4a0, invocation_hint=0x7fffffffc3d0, marshal_data=0x98) at gclosure.c:970
#18 0x00007fffee830bed in g_closure_invoke (closure=0x6102c0, return_value=0x0, n_param_values=1, param_values=0x7fffffffc4a0, 
    invocation_hint=0x7fffffffc3d0) at gclosure.c:777
#19 0x00007fffee84ed32 in signal_emit_unlocked_R (node=0x610760, detail=0, instance=0x8a6280, emission_return=0x0, 
    instance_and_params=0x7fffffffc4a0) at gsignal.c:3700
#20 0x00007fffee84dadb in g_signal_emit_valist (instance=0x8a6280, signal_id=3, detail=0, var_args=0x7fffffffc758) at gsignal.c:3328
#21 0x00007fffee84e023 in g_signal_emit (instance=0x8a6280, signal_id=3, detail=0) at gsignal.c:3384
#22 0x00007ffff0a87ed0 in gtk_widget_dispose (object=0x8a6280) at gtkwidget.c:10768
#23 0x00007fffee8380ea in g_object_run_dispose (object=0x8a6280) at gobject.c:1062
#24 0x00007ffff0a7acb6 in gtk_widget_destroy (widget=0x8a6280) at gtkwidget.c:4090
#25 0x00007ffff0790498 in gtk_bin_forall (container=0xafb000, include_internals=0, callback=0x7ffff0a7abff <gtk_widget_destroy>, 
    callback_data=0x0) at gtkbin.c:180
#26 0x00007ffff07f6c34 in gtk_container_foreach (container=0xafb000, callback=0x7ffff0a7abff <gtk_widget_destroy>, callback_data=0x0)
    at gtkcontainer.c:2074
#27 0x00007ffff07f52eb in gtk_container_destroy (widget=0xafb000) at gtkcontainer.c:1377
#28 0x00007ffff0a9b7e3 in gtk_window_destroy (widget=0xafb000) at gtkwindow.c:4702
#29 0x00007fffee833c60 in g_cclosure_marshal_VOID__VOID (closure=0x6102c0, return_value=0x0, n_param_values=1, 
    param_values=0x7fffffffcca0, invocation_hint=0x7fffffffcbd0, marshal_data=0x7ffff0a9b6ad <gtk_window_destroy>) at gmarshal.c:85
#30 0x00007fffee83123d in g_type_class_meta_marshal (closure=0x6102c0, return_value=0x0, n_param_values=1, 
    param_values=0x7fffffffcca0, invocation_hint=0x7fffffffcbd0, marshal_data=0x98) at gclosure.c:970
#31 0x00007fffee830bed in g_closure_invoke (closure=0x6102c0, return_value=0x0, n_param_values=1, param_values=0x7fffffffcca0, 
    invocation_hint=0x7fffffffcbd0) at gclosure.c:777
#32 0x00007fffee84ed32 in signal_emit_unlocked_R (node=0x610760, detail=0, instance=0xafb000, emission_return=0x0, 
    instance_and_params=0x7fffffffcca0) at gsignal.c:3700
#33 0x00007fffee84dadb in g_signal_emit_valist (instance=0xafb000, signal_id=3, detail=0, var_args=0x7fffffffcf58) at gsignal.c:3328
#34 0x00007fffee84e023 in g_signal_emit (instance=0xafb000, signal_id=3, detail=0) at gsignal.c:3384
#35 0x00007ffff0a87ed0 in gtk_widget_dispose (object=0xafb000) at gtkwidget.c:10768
#36 0x00007ffff0a9740a in gtk_window_dispose (object=0xafb000) at gtkwindow.c:2401
#37 0x00007fffee8380ea in g_object_run_dispose (object=0xafb000) at gobject.c:1062
#38 0x00007ffff0a7acb6 in gtk_widget_destroy (widget=0xafb000) at gtkwidget.c:4090
#39 0x00007ffff08d0900 in gtk_main_do_event (event=0xafd4a0) at gtkmain.c:1597
#40 0x00007ffff043df82 in _gdk_event_emit (event=0xafd4a0) at gdkevents.c:69
#41 0x00007ffff047a4c4 in gdk_event_source_dispatch (source=0x65f6f0, callback=0x0, user_data=0x0) at gdkeventsource.c:364
#42 0x00007fffee52323d in g_main_dispatch (context=0x63be20) at gmain.c:3054
#43 0x00007fffee523fa2 in g_main_context_dispatch (context=0x63be20) at gmain.c:3630
#44 0x00007fffee524192 in g_main_context_iterate (context=0x63be20, block=1, dispatch=1, self=0x67b830) at gmain.c:3701
#45 0x00007fffee5245c2 in g_main_loop_run (loop=0x8cdbe0) at gmain.c:3895
#46 0x00007ffff08d0254 in gtk_main () at gtkmain.c:1156
#47 0x0000000000405a61 in main (argc=1, argv=0x7fffffffd558) at ../../Tools/GtkLauncher/main.c:542

[Alberto Garcia]

Created attachment 205995 [details]
Patch

This happens because when we destroy priv->corePage in
webkit_web_view_dispose() we're triggering the deletion of
InspectorFrontendClient before it can clean up itself.

This can be fixed by waiting until the dispose method finishes before
actually destroying the corePage object. This way we ensure that the
webView's "destroy" signal is emitted first.

[Carlos Garcia Campos]

Comment on attachment 205995 [details]
Patch

I think it would be cleaner to delete the page in finalize instead of dispose. Since WebKitWebView uses the placement new syntax, you can make page a OwnPtr and it will be automatically deleted in finalize.

[Alberto Garcia]

(In reply to comment #2)
> I think it would be cleaner to delete the page in finalize instead
> of dispose. Since WebKitWebView uses the placement new syntax, you
> can make page a OwnPtr and it will be automatically deleted in
> finalize.

The problem is that the corePage pointer will still be != 0 in the
meantime, and that will produce a crash during the disposal of the
parent class:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff3d2f5e8 in WebCore::AXObjectCache::rootObject (this=0x0)
    at ../../Source/WebCore/accessibility/AXObjectCache.cpp:415
415         return getOrCreate(m_document->view());
#0  0x00007ffff3d2f5e8 in WebCore::AXObjectCache::rootObject (this=0x0)
    at ../../Source/WebCore/accessibility/AXObjectCache.cpp:415
#1  0x00007ffff3b8f2df in webkit_web_view_get_accessible (widget=0x5da2a0)
    at ../../Source/WebKit/gtk/webkit/webkitwebview.cpp:1416
#2  0x00007ffff1c34ee9 in gtk_container_accessible_real_remove_gtk (container=0x449320, widget=0x5da2a0, data=0x1a8a0e0)
    at gtkcontaineraccessible.c:137
[...]
#7  0x00007ffff1a2abb4 in gtk_container_remove (container=<optimized out>, widget=widget@entry=0x5da2a0)
    at gtkcontainer.c:1546
#8  0x00007ffff1bf6cc2 in gtk_widget_dispose (object=0x5da2a0) at gtkwidget.c:10254

A different alternative would be to run parent->dispose() first. That
seems to work fine it doesn't look like the rest of the code in that
method would be affected by that.

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > I think it would be cleaner to delete the page in finalize instead
> > of dispose. Since WebKitWebView uses the placement new syntax, you
> > can make page a OwnPtr and it will be automatically deleted in
> > finalize.
> 
> The problem is that the corePage pointer will still be != 0 in the
> meantime, and that will produce a crash during the disposal of the
> parent class:
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ffff3d2f5e8 in WebCore::AXObjectCache::rootObject (this=0x0)
>     at ../../Source/WebCore/accessibility/AXObjectCache.cpp:415
> 415         return getOrCreate(m_document->view());
> #0  0x00007ffff3d2f5e8 in WebCore::AXObjectCache::rootObject (this=0x0)
>     at ../../Source/WebCore/accessibility/AXObjectCache.cpp:415
> #1  0x00007ffff3b8f2df in webkit_web_view_get_accessible (widget=0x5da2a0)
>     at ../../Source/WebKit/gtk/webkit/webkitwebview.cpp:1416
> #2  0x00007ffff1c34ee9 in gtk_container_accessible_real_remove_gtk (container=0x449320, widget=0x5da2a0, data=0x1a8a0e0)
>     at gtkcontaineraccessible.c:137
> [...]
> #7  0x00007ffff1a2abb4 in gtk_container_remove (container=<optimized out>, widget=widget@entry=0x5da2a0)
>     at gtkcontainer.c:1546
> #8  0x00007ffff1bf6cc2 in gtk_widget_dispose (object=0x5da2a0) at gtkwidget.c:10254

hmm, unfortunate.

> A different alternative would be to run parent->dispose() first. That
> seems to work fine it doesn't look like the rest of the code in that
> method would be affected by that.

It could be easier, yes, adding a comment explaining why parent dispose is called first.

[Alberto Garcia]

Created attachment 208556 [details]
Patch

(In reply to comment #4)
> It could be easier, yes, adding a comment explaining why parent
> dispose is called first.

Here it is.

[Carlos Garcia Campos]

Comment on attachment 208556 [details]
Patch

Thanks

[Alberto Garcia]

Fixed in <http://trac.webkit.org/changeset/153991>.

[Gustavo Noronha (kov)]

Comment on attachment 208556 [details]
Patch

This appears to have caused a few API tests to fail:

(./Tools/gtk/../Scripts/../../WebKitBuild/Release/Programs/unittests/testloading:9117): GLib-CRITICAL **: g_hash_table_lookup_extended: assertion `hash_table != NULL' failed
(./Tools/gtk/../Scripts/../../WebKitBuild/Release/Programs/unittests/testhttpbackend:9159): GLib-CRITICAL **: g_hash_table_lookup_extended: assertion `hash_table != NULL' failed

[Alberto Garcia]

(In reply to comment #8)
> (./Tools/gtk/../Scripts/../../WebKitBuild/Release/Programs/unittests/testloading:9117): GLib-CRITICAL **: g_hash_table_lookup_extended: assertion `hash_table != NULL' failed
> (./Tools/gtk/../Scripts/../../WebKitBuild/Release/Programs/unittests/testhttpbackend:9159): GLib-CRITICAL **: g_hash_table_lookup_extended: assertion `hash_table != NULL' failed

I think this is because of putting the code after the parent's dispose call.

Going back to my original solution should fix this. I'll upload a new patch.

[Alberto Garcia]

Created attachment 208630 [details]
Patch

Here's the patch with the original fix.

[Carlos Garcia Campos]

I guess we need to roll out the patch first

[Alberto Garcia]

(In reply to comment #11)
> I guess we need to roll out the patch first

This one goes on top of the other already.

[Carlos Garcia Campos]

It doesn't seem to apply in EWS bots

[Carlos Garcia Campos]

reopening

[Alberto Garcia]

Created attachment 208658 [details]
Patch

Resubmitting patch for EWS processing.

[Alberto Garcia]

Fixed in <http://trac.webkit.org/changeset/154014>.

[Alberto Garcia]

*** Bug 48203 has been marked as a duplicate of this bug. ***