GNOME Bugzilla – Bug 733767
GtkPrintOperation does not keep itself alive through callbacks
Last modified: 2014-08-01 04:46:06 UTC
LRN: nacho, there's a bug in gedit printing code nacho: LRN, there are probably several :) LRN: done_cb() calls unrefs the job and the operation at the end, then returns. Up the stack, we return from a call to print_pages() of GtkPrintOperation, and it checks priv for errors. At this point priv points to freed memory, because "priv" is the private part of the operation object that done_cb() just offed nacho: LRN, lemme check nacho: LRN, shouldn't gtk ref the operation? nacho: LRN, so pbor would be one better to ask about this, but it feels to me like done is the right place to unref the operation nacho: as from the docs nacho: and if that is true, gtk should ensure to have a ref till done is emitted
Created attachment 281739 [details] [review] Ensure that print operation is alive until we're done
Review of attachment 281739 [details] [review]: in the same file there are other places where the DONE signal is emitted, do they have the same issue? if yes, it would be clearer to factor out a helper like static void emit_done (op) { /* make sure op stays alive during all signal handlers of DONE */ ref(op); emit(DONE); unref(op); }
signals[DONE] is emitted in: preview_iface_end_preview() print_pages_idle_done() print_pages() preview_iface_end_preview() is called from gtk_print_operation_preview_end_preview() gtk_print_operation_preview_end_preview() is called from preview_print_idle_done() gtk_print_operation_preview_handler() gets a reference to the operation, and preview_print_idle_done() later on drop it after emitting signals[DONE] So this one seems to be OK. print_pages_idle_done() is called from print_pages(), thus it's covered by the changes to print_pages() calls. print_pages() is only called from gtk_print_operation_run (), and my patch already fixes that.
I still think it is clearer to just take the extra ref only around the emission where needed. As far as I can see this is just needed in print pages because for the other cases there is data->op holding a ref
(In reply to comment #4) > I still think it is clearer to just take the extra ref only around the emission > where needed. Taking a ref directly around the emission (i.e. take_ref(); emit_signal(); drop_ref();) is not going to help if callers up the stack still need access to the object that we take the ref for (it will be destroyed once the ref is dropped). If you mean "around" as in "up the stack, to the point where the object being reffed is no longer needed" then that is exactly what my patch does. > As far as I can see this is just needed in print pages because > for the other cases there is data->op holding a ref That is what i said in my previous comment.
uhm, I guess the problem is the access to priv->error this code could do with a refactoring to clean up all this coupling between methods... that said, the patch is ok to go in
Attachment 281739 [details] pushed as e934ddd - Ensure that print operation is alive until we're done