Fix file conflict dialog crashes #1639
Conversation
Instead of manually keeping tabs on the signals so we can disconnect them before the data parameter gets destroyed, let GObject automatically track lifetime of the data, which it can do as that data is a GObject itself. This does not change behavior in the normal case, but makes sure the callback simply cannot get called with invalid/freed parameters, even if we did screw anything up (which we used to). This actually would have solved #1630 as well with using the target widgets as data parameters as the signal would have been disconnected as soon as the widget got destroyed, no matter whether we got finalized ourselves or not. The signal IDs were also use as guards to whether the monitor was set up for the related files, but we can just as well use the state of the file list ready handle which should only be NULL when we actually have monitors set up. Even if it wasn't the case, worse case scenario would be removing a non-existent monitor, which is perfectly OK anyway.
|
For anybody who'd merge this, please do not squash the commits together, it'd make more sense to have the split as is (and they all leave the code in an acceptable state, although c81cc65 alone is not a fix. |
There was a problem hiding this comment.
missing g_signal_handler_disconnect in the dispose method:
if (details->src_handler_id)
{
g_signal_handler_disconnect (details->source, details->src_handler_id);
details->src_handler_id = 0;
}
if (details->dest_handler_id)
{
g_signal_handler_disconnect (details->destination, details->dest_handler_id);
details->dest_handler_id = 0;
}|
Github just did something ugly: merged only the LAST commit on a "rebase and merge" will emergency revert and do a manual merge, so as to avoid force-pushing to master |
|
Revert PR showed all three commits, looks like git worked and github just didn't show it all. Running git log or looking at the history on a local pull of master shows all three commits separately. Thus we are good to go here |
|
@lukefromdc thanks, and yep master looks fine |
|
Cherrypicked to 1.26 after testing on that branch |
Fix #1630.
This contains 3 separate but fairly closely related changes (see each commit for details):
dispose()as we ought to instead of waiting forfinalize(). This is the real fix, and is the right thing to do in the GObject world: no object is supposed to hold a reference to any other object afterdispose().g_signal_connect_object()to improve safety of the handler by making sure all parameters are valid at signal dispatch time.1 alone is enough of a fix, but 2 is a nice improvement of the code, and 3 makes things safer. Actually, 2+3 alone are also a fix for #1630, although not as thorough.