Code review comment for lp://qastaging/~mardy/gnome-control-center-signon/lp1122520

Revision history for this message
David King (amigadave) wrote :

> I'm afraid that this might turn into a philosophical discussion :-)

Sounds interesting. :-)

> First, I think we need to recognize that this is a hack around what IMHO is a
> wrong practice yet relatively common in GLib: continuing keeping an object
> alive (and kicking at callbacks!) after all explicit references to it have
> been dropped.

This is true, but with one caveat: "Object methods should be able to run without program error in-between [dispose and finalize]." https://developer.gnome.org/gobject/stable/gobject-memory.html#gobject-memory-cycles

> I agree with most of what Alex was saying in that thread; I actually even
> considered the idea of running the main loop in the dispose method, but
> quickly discarded it. :-)
>
> However, your criticism seems to be based on a misunderstanding: the
> g_return_if_fail() is used only as a non-essential extra check, just for
> safety.

My mistake, thanks for pointing it out.

> So, what this fix does is to set that pointer to NULL if the object has been
> disposed, so that all callbacks happening after it don't perform any action.

Indeed, and this is the part of the approach that I disagree with. Admittedly, the object method will not access invalid memory, but checking whether the object is valid in a "private" method is a symptom to me that something is wrong. A neater solution might be for the asynchronous operations to own (or hold a reference to) everything required to complete the operation, or one of the other solutions mentioned in the mailing list thread.

« Back to merge proposal