| Summary: | [GTK] UI process crashes when closing the window right after printing with javascript | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
| Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, gustavo, ltilve, mrobinson | ||||||
| Priority: | P2 | Keywords: | Gtk | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Carlos Garcia Campos
2014-01-14 05:47:49 PST
Created attachment 221152 [details]
Patch
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 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. (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. Committed r162057: <http://trac.webkit.org/changeset/162057> |