Bug 126981 – [GTK] UI process crashes when closing the window right after printing with javascript

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Created attachment 221150 [details]
Test case

This is a similar situation to bug #126977, but in this case the web process doesn't crash. The UI process crashes because when the page is closed, the web view is destroyed before the print operation has actually finished. I haven't been able to reproduce it with the unit tests included in patch attached to bug #126977, but I can always reproduce it with the attached test case.

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff215f6f4 in g_object_unref (_object=0x99e9c0) at gobject.c:3068
3068	  g_return_if_fail (G_IS_OBJECT (object));
(gdb) bt
#0  0x00007ffff215f6f4 in g_object_unref (_object=0x99e9c0) at gobject.c:3068
#1  0x00007ffff5c1a1e2 in drawPagesForPrintingCompleted(OpaqueWKError const*, OpaqueWKError const*, void*) ()
   from WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.25
#2  0x00007ffff5ca43e7 in void WebKit::invalidateCallbackMap<WTF::RefPtr<WebKit::GenericCallback<OpaqueWKError const*, API::Error*> > >(WTF::HashMap<unsigned long, WTF::RefPtr<WebKit::GenericCallback<OpaqueWKError const*, API::Error*> >, WTF::IntHash<unsigned long>, WTF::HashTraits<unsigned long>, WTF::HashTraits<WTF::RefPtr<WebKit::GenericCallback<OpaqueWKError const*, API::Error*> > > >&) () from WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.25
#3  0x00007ffff5c93b7f in WebKit::WebPageProxy::resetState() () from WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.25
#4  0x00007ffff5c93db7 in WebKit::WebPageProxy::close() () from WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.25
#5  0x00007ffff5c35f06 in webkit_web_view_base_finalize(_GObject*) () from WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.25
#6  0x00007ffff215f88a in g_object_unref (_object=0x7da840) at gobject.c:3171
#7  0x00007ffff2174e1f in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7fffffffd228) at gsignal.c:3270
#8  0x00007ffff2175322 in g_signal_emit (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>) at gsignal.c:3368
#9  0x00007ffff5d56a3d in WebKit::WebPageProxy::didReceiveMessage(IPC::Connection*, IPC::MessageDecoder&) ()
   from WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.25
#10 0x00007ffff71461bb in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection*, IPC::MessageDecoder&) ()
   from WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.25
#11 0x00007ffff5cacba2 in WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection*, IPC::MessageDecoder&) ()
   from WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.25
#12 0x00007ffff714036b in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::MessageDecoder, std::default_delete<IPC::MessageDecoder> >) ()
   from WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.25
#13 0x00007ffff71404b3 in IPC::Connection::dispatchOneMessage() () from WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.25
#14 0x00007ffff1321f06 in WTF::RunLoop::performWork() () from WebKit/WebKitBuild/Release/.libs/libjavascriptcoregtk-3.0.so.0
#15 0x00007ffff132ea39 in WTF::RunLoop::queueWork(WTF::RunLoop*) () from WebKit/WebKitBuild/Release/.libs/libjavascriptcoregtk-3.0.so.0
#16 0x00007ffff1e684b5 in g_main_dispatch (context=0x65ab00) at gmain.c:3068
#17 g_main_context_dispatch (context=context@entry=0x65ab00) at gmain.c:3643
#18 0x00007ffff1e68818 in g_main_context_iterate (context=0x65ab00, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3714
#19 0x00007ffff1e68c1a in g_main_loop_run (loop=0x904c50) at gmain.c:3908
#20 0x00007ffff3c05e85 in gtk_main () at gtkmain.c:1195
#21 0x0000000000408311 in main ()

[Carlos Garcia Campos]

Created attachment 221152 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Gustavo Noronha (kov)]

Comment on attachment 221152 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221152&action=review

> Source/WebKit2/ChangeLog:17
> +        The UI process crashes because when the page is closed, the web
> +        view is destroyed before the print operation has actually
> +        finished. We were connecting to the destroy signal of the view to
> +        be notified when it's destroyed to unref the print
> +        operation. That's wrong because it assumes that the print
> +        operation is destroyed in the finished callback. Use a weak pointer
> +        instead, to make sure the web view pointer is set to NULL when the
> +        view is destroyed and emit the finished callback always so that
> +        the user can clean up the operation even when the web view has
> +        been closed.

I don't understand the "That's wrong because it assumes that the print operation is destroyed in the finished callback." bit in this description, perhaps you meant destroy callback? I think you should mention the unref too, how about "That's wrong because we can't be sure the print operation is destroyed by the unref in the callback for destroy, and it may try to use the now invalid webView pointer."?

I think readability would be improved if you made the lines longer, too.

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 221152 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221152&action=review
> 
> > Source/WebKit2/ChangeLog:17
> > +        The UI process crashes because when the page is closed, the web
> > +        view is destroyed before the print operation has actually
> > +        finished. We were connecting to the destroy signal of the view to
> > +        be notified when it's destroyed to unref the print
> > +        operation. That's wrong because it assumes that the print
> > +        operation is destroyed in the finished callback. Use a weak pointer
> > +        instead, to make sure the web view pointer is set to NULL when the
> > +        view is destroyed and emit the finished callback always so that
> > +        the user can clean up the operation even when the web view has
> > +        been closed.
> 
> I don't understand the "That's wrong because it assumes that the print operation is destroyed in the finished callback." bit in this description, perhaps you meant destroy callback? I think you should mention the unref too, how about "That's wrong because we can't be sure the print operation is destroyed by the unref in the callback for destroy, and it may try to use the now invalid webView pointer."?

Nope, I mean finished callback, but I agree it's quite confusing. The thing is that a print operation can be created directly by the user or by a web view when print() is called from javascript. In this case we do:

g_signal_connect(printOperation.leakRef(), "finished", G_CALLBACK(g_object_unref), 0);

When the web view is destroyed we were releasing a ref of the print operation that we hadn't taken, assuming finish is not going to be emitted and the caller calls unref in the finish callback like web view does. Instead of that, we should make sure that finish is called, and never release a ref we haven't taken.

> I think readability would be improved if you made the lines longer, too.

That's how my emacs auto fills changelog files.

[Carlos Garcia Campos]

Committed r162057: <http://trac.webkit.org/changeset/162057>