126981 2014-01-14 05:47:49 -0800 [GTK] UI process crashes when closing the window right after printing with javascript 2014-01-14 23:52:40 -0800 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Gtk P2 Normal --- 1 cgarcia webkit-unassigned commit-queue gustavo ltilve mrobinson oldest_to_newest 967783 0 221150 cgarcia 2014-01-14 05:47:49 -0800 Created attachment 221150 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 () 967786 1 221152 cgarcia 2014-01-14 05:55:04 -0800 Created attachment 221152 Patch 967788 2 commit-queue 2014-01-14 05:57:35 -0800 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 967806 3 221152 gustavo 2014-01-14 06:56:10 -0800 Comment on attachment 221152 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. 967858 4 cgarcia 2014-01-14 09:11:05 -0800 (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. 968194 5 cgarcia 2014-01-14 23:52:40 -0800 Committed r162057: <http://trac.webkit.org/changeset/162057> 221150 2014-01-14 05:47:49 -0800 2014-01-14 05:47:49 -0800 Test case print_test.html text/html 229 cgarcia PGh0bWw+CjxoZWFkPgo8dGl0bGU+IFByaW50IHRlc3QhIDwvdGl0bGU+CjwvaGVhZD4KPGJvZHk+ CjxoMT4gUHJpbnQgdGVzdCEgPC9oMT4gPGJyIC8+Cjxmb3JtPjxpbnB1dCB0eXBlPSJidXR0b24i IHZhbHVlPSIgUHJpbnQgdGhpcyBwYWdlICIKb25jbGljaz0idyA9IHdpbmRvdy5vcGVuKCk7dy5w cmludCgpO3cuY2xvc2UoKTtyZXR1cm4gZmFsc2U7IiAvPjwvZm9ybT4KPC9ib2R5Pgo8L2h0bWw+ Cg== 221152 2014-01-14 05:55:04 -0800 2014-01-14 06:56:10 -0800 Patch wk2-printing-ui-crash2.diff text/plain 3960 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCA3YzNiZDJjLi44YzUyODIwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjYg QEAKIDIwMTQtMDEtMTQgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgogCisgICAgICAgIFtHVEtdIFVJIHByb2Nlc3MgY3Jhc2hlcyB3aGVuIGNsb3NpbmcgdGhlIHdp bmRvdyByaWdodCBhZnRlciBwcmludGluZyB3aXRoIGphdmFzY3JpcHQKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEyNjk4MQorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBVSSBwcm9jZXNzIGNyYXNoZXMg YmVjYXVzZSB3aGVuIHRoZSBwYWdlIGlzIGNsb3NlZCwgdGhlIHdlYgorICAgICAgICB2aWV3IGlz IGRlc3Ryb3llZCBiZWZvcmUgdGhlIHByaW50IG9wZXJhdGlvbiBoYXMgYWN0dWFsbHkKKyAgICAg ICAgZmluaXNoZWQuIFdlIHdlcmUgY29ubmVjdGluZyB0byB0aGUgZGVzdHJveSBzaWduYWwgb2Yg dGhlIHZpZXcgdG8KKyAgICAgICAgYmUgbm90aWZpZWQgd2hlbiBpdCdzIGRlc3Ryb3llZCB0byB1 bnJlZiB0aGUgcHJpbnQKKyAgICAgICAgb3BlcmF0aW9uLiBUaGF0J3Mgd3JvbmcgYmVjYXVzZSBp dCBhc3N1bWVzIHRoYXQgdGhlIHByaW50CisgICAgICAgIG9wZXJhdGlvbiBpcyBkZXN0cm95ZWQg aW4gdGhlIGZpbmlzaGVkIGNhbGxiYWNrLiBVc2UgYSB3ZWFrIHBvaW50ZXIKKyAgICAgICAgaW5z dGVhZCwgdG8gbWFrZSBzdXJlIHRoZSB3ZWIgdmlldyBwb2ludGVyIGlzIHNldCB0byBOVUxMIHdo ZW4gdGhlCisgICAgICAgIHZpZXcgaXMgZGVzdHJveWVkIGFuZCBlbWl0IHRoZSBmaW5pc2hlZCBj YWxsYmFjayBhbHdheXMgc28gdGhhdAorICAgICAgICB0aGUgdXNlciBjYW4gY2xlYW4gdXAgdGhl IG9wZXJhdGlvbiBldmVuIHdoZW4gdGhlIHdlYiB2aWV3IGhhcworICAgICAgICBiZWVuIGNsb3Nl ZC4KKworICAgICAgICAqIFVJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFByaW50T3BlcmF0aW9uLmNw cDoKKyAgICAgICAgKGRyYXdQYWdlc0ZvclByaW50aW5nQ29tcGxldGVkKToKKworMjAxNC0wMS0x NCAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CisKICAgICAgICAg W0dUS10gVUkgcHJvY2VzcyBjcmFzaGVzIHdoZW4gdGhlIHdlYiBwcm9jZXNzIGNyYXNoZXMgd2hp bGUgcHJpbnRpbmcKICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTEyNjk3NwogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0 ay9XZWJLaXRQcmludE9wZXJhdGlvbi5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJ L2d0ay9XZWJLaXRQcmludE9wZXJhdGlvbi5jcHAKaW5kZXggMDNmNjNjNy4uNTMxOGRkMCAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9ndGsvV2ViS2l0UHJpbnRPcGVy YXRpb24uY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFBy aW50T3BlcmF0aW9uLmNwcApAQCAtNjUsMTMgKzY1LDcgQEAgZW51bSB7CiB9OwogCiBzdHJ1Y3Qg X1dlYktpdFByaW50T3BlcmF0aW9uUHJpdmF0ZSB7Ci0gICAgfl9XZWJLaXRQcmludE9wZXJhdGlv blByaXZhdGUoKQotICAgIHsKLSAgICAgICAgZ19zaWduYWxfaGFuZGxlcl9kaXNjb25uZWN0KHdl YlZpZXcsIHdlYlZpZXdEZXN0cm95ZWRJZCk7Ci0gICAgfQotCiAgICAgV2ViS2l0V2ViVmlldyog d2ViVmlldzsKLSAgICBndWxvbmcgd2ViVmlld0Rlc3Ryb3llZElkOwogICAgIFByaW50SW5mbzo6 UHJpbnRNb2RlIHByaW50TW9kZTsKIAogICAgIEdSZWZQdHI8R3RrUHJpbnRTZXR0aW5ncz4gcHJp bnRTZXR0aW5nczsKQEAgLTgyLDExICs3Niw2IEBAIHN0YXRpYyBndWludCBzaWduYWxzW0xBU1Rf U0lHTkFMXSA9IHsgMCwgfTsKIAogV0VCS0lUX0RFRklORV9UWVBFKFdlYktpdFByaW50T3BlcmF0 aW9uLCB3ZWJraXRfcHJpbnRfb3BlcmF0aW9uLCBHX1RZUEVfT0JKRUNUKQogCi1zdGF0aWMgdm9p ZCB3ZWJWaWV3RGVzdHJveWVkKEd0a1dpZGdldCogd2ViVmlldywgR09iamVjdCogcHJpbnRPcGVy YXRpb24pCi17Ci0gICAgZ19vYmplY3RfdW5yZWYocHJpbnRPcGVyYXRpb24pOwotfQotCiBzdGF0 aWMgdm9pZCB3ZWJraXRQcmludE9wZXJhdGlvbkNvbnN0cnVjdGVkKEdPYmplY3QqIG9iamVjdCkK IHsKICAgICBXZWJLaXRQcmludE9wZXJhdGlvbiogcHJpbnRPcGVyYXRpb24gPSBXRUJLSVRfUFJJ TlRfT1BFUkFUSU9OKG9iamVjdCk7CkBAIC05NSw3ICs4NCw3IEBAIHN0YXRpYyB2b2lkIHdlYmtp dFByaW50T3BlcmF0aW9uQ29uc3RydWN0ZWQoR09iamVjdCogb2JqZWN0KQogICAgIGlmIChHX09C SkVDVF9DTEFTUyh3ZWJraXRfcHJpbnRfb3BlcmF0aW9uX3BhcmVudF9jbGFzcyktPmNvbnN0cnVj dGVkKQogICAgICAgICBHX09CSkVDVF9DTEFTUyh3ZWJraXRfcHJpbnRfb3BlcmF0aW9uX3BhcmVu dF9jbGFzcyktPmNvbnN0cnVjdGVkKG9iamVjdCk7CiAKLSAgICBwcml2LT53ZWJWaWV3RGVzdHJv eWVkSWQgPSBnX3NpZ25hbF9jb25uZWN0KHByaXYtPndlYlZpZXcsICJkZXN0cm95IiwgR19DQUxM QkFDSyh3ZWJWaWV3RGVzdHJveWVkKSwgcHJpbnRPcGVyYXRpb24pOworICAgIGdfb2JqZWN0X2Fk ZF93ZWFrX3BvaW50ZXIoR19PQkpFQ1QocHJpdi0+d2ViVmlldyksIChncG9pbnRlciopJnByaXYt PndlYlZpZXcpOwogfQogCiBzdGF0aWMgdm9pZCB3ZWJraXRQcmludE9wZXJhdGlvbkdldFByb3Bl cnR5KEdPYmplY3QqIG9iamVjdCwgZ3VpbnQgcHJvcElkLCBHVmFsdWUqIHZhbHVlLCBHUGFyYW1T cGVjKiBwYXJhbVNwZWMpCkBAIC0yNjMsMTEgKzI1MiwxMiBAQCBzdGF0aWMgV2ViS2l0UHJpbnRP cGVyYXRpb25SZXNwb25zZSB3ZWJraXRQcmludE9wZXJhdGlvblJ1bkRpYWxvZyhXZWJLaXRQcmlu dE9wZQogc3RhdGljIHZvaWQgZHJhd1BhZ2VzRm9yUHJpbnRpbmdDb21wbGV0ZWQoV0tFcnJvclJl ZiB3a1ByaW50RXJyb3IsIFdLRXJyb3JSZWYsIHZvaWQqIGNvbnRleHQpCiB7CiAgICAgR1JlZlB0 cjxXZWJLaXRQcmludE9wZXJhdGlvbj4gcHJpbnRPcGVyYXRpb24gPSBhZG9wdEdSZWYoV0VCS0lU X1BSSU5UX09QRVJBVElPTihjb250ZXh0KSk7Ci0gICAgV2ViUGFnZVByb3h5KiBwYWdlID0gd2Vi a2l0V2ViVmlld0Jhc2VHZXRQYWdlKFdFQktJVF9XRUJfVklFV19CQVNFKHByaW50T3BlcmF0aW9u LT5wcml2LT53ZWJWaWV3KSk7CiAKICAgICAvLyBXaGVuIHJ1bm5pbmcgc3luY2hyb25vdXNseSBX ZWJQYWdlUHJveHk6OnByaW50RnJhbWUoKSBjYWxscyBlbmRQcmludGluZygpLgotICAgIGlmIChw cmludE9wZXJhdGlvbi0+cHJpdi0+cHJpbnRNb2RlID09IFByaW50SW5mbzo6UHJpbnRNb2RlQXN5 bmMpCisgICAgaWYgKHByaW50T3BlcmF0aW9uLT5wcml2LT5wcmludE1vZGUgPT0gUHJpbnRJbmZv OjpQcmludE1vZGVBc3luYyAmJiBwcmludE9wZXJhdGlvbi0+cHJpdi0+d2ViVmlldykgeworICAg ICAgICBXZWJQYWdlUHJveHkqIHBhZ2UgPSB3ZWJraXRXZWJWaWV3QmFzZUdldFBhZ2UoV0VCS0lU X1dFQl9WSUVXX0JBU0UocHJpbnRPcGVyYXRpb24tPnByaXYtPndlYlZpZXcpKTsKICAgICAgICAg cGFnZS0+ZW5kUHJpbnRpbmcoKTsKKyAgICB9CiAKICAgICBjb25zdCBXZWJDb3JlOjpSZXNvdXJj ZUVycm9yJiByZXNvdXJjZUVycm9yID0gd2tQcmludEVycm9yID8gdG9JbXBsKHdrUHJpbnRFcnJv ciktPnBsYXRmb3JtRXJyb3IoKSA6IFdlYkNvcmU6OlJlc291cmNlRXJyb3IoKTsKICAgICBpZiAo IXJlc291cmNlRXJyb3IuaXNOdWxsKCkpIHsK