GNOME Bugzilla – Bug 746800
SpiceDisplay leak
Last modified: 2016-03-31 13:22:07 UTC
I'm testing with a f22 live cd, but this happened with an installed EL7 as well Click on VM thumbnail, VM maximizes Click on top left arrow to go back to collection view Click again on VM -> this triggers a warning (gnome-boxes:1660): GSpice-CRITICAL **: spice_main_update_display: assertion 'channel != NULL' failed Repeat #1-#3 once more, this triggers 2 warnings If you add diff --git a/src/spice-display.vala b/src/spice-display.vala index 309122c..a99b0b5 100644 --- a/src/spice-display.vala +++ b/src/spice-display.vala @@ -24,6 +24,7 @@ private void ui_state_changed () { if (machine.ui_state == UIState.DISPLAY) { // disable resize guest when minimizing guest widget var display = get_display (0) as Spice.Display; + warning("%p - ui_state_changed display: %p", this, display); display.resize_guest = true; } } @@ -228,6 +229,7 @@ public override void connect_it (Display.OpenFDFunc? open_fd = null) { access_start (); var display = get_display (id) as Spice.Display; display.notify["ready"].connect (() => { + warning("%p - display: %p", this, display); if (display.ready) show (display.channel_id); else (both show up in a backtrace when the warning occurs), you can see that there are 2 different SpiceDisplay instances involved, which is probably what is causing this bug (the old instance no longer is connected and should no longer be used).
Mentioning this here since I don't know if the root cause of the bug below is the one described above, or if it's different. If it still happens after this bug is fixed, then this will need more investigation and could be a spice-gtk bug. Click on VM thumbnail, VM maximizes Click on top left arrow to go back to collection view Click again on VM Click on VM thumbnail, VM maximizes Click on top left arrow to go back to collection view Use selection mode to go to this VM properties, and force shutdown it Press "Left Alt" while the mouse is over the collection view -> Boxes crashes This has been reported downstream in Fedora as https://bugzilla.redhat.com/show_bug.cgi?id=1186335
Created attachment 303031 [details] [review] wip: make sure SpiceDisplay~ is eventually called and destroyed
I tracked down the extra references holding SpiceDisplay, and ended up with this patch, which is mostly a rewrite of lambda to avoid taking the reference of the display in the closure. It also removes some unnecessary signal handler tracking. Of course, this is wip, and it needs to be cleaned up now.
*** Bug 748646 has been marked as a duplicate of this bug. ***
Created attachment 303272 [details] [review] display: connect_it() takes owership of OpenFDFunc In the following patches, we'll need to keep the delegate around to use it from outside connect_it() but that won't be possible without this change since copying of delegates isn't supported by Vala.
Created attachment 303273 [details] [review] wip patch to fix channels & display leak These patches do ensure that SpiceDisplay and SpiceChannel instances are not leaked but it doesn't solve the issue of the warning, nor it being cumulative.
Pushed a bunch of patches that finally fixed this issue. commit: be85eb31aaf1c86a8ffd8025861c6070b3d7ce58 spice-display: Introducing SpiceChannelHandler This patch puts all the channel handling into a separate helper class. Not only it makes channel handling code more readable, it also ensures that we don't end up leaking channel or display references by avoiding the use of closures as signal handlers. commit: 5c653bbdf9158d1096030099150adcce52444c11 display: connect_it() takes owership of OpenFDFunc In the following patches, we'll need to keep the delegate around to use it from outside connect_it() but that won't be possible without this change since copying of delegates isn't supported by Vala. commit: fe8191f6ea20cf351bb84a2a95cea097654d1196 spice-display: Disconnect channel signal handlers Let's ensure channel signal handlers are disconnected and their IDs cleared on disconnection. commit: b0f5c08d4d59fe1583fab52db5f0f5fc94bc9dd4 spice-display: Clean-up main channel on disconnection commit: a17fcbfb585234b7a451396a4c58e4786d746f7c display-page: Drop ref on display upon its removal Once DisplayPage is told to remove the display, it should be giving up its reference to the display being removed. commit: 078c98cd2c8540b19de479811dddbf8dddb4990c box-config: Don't use object from closure Using the object in lambda from closure will add a ref on the object in question and object will leak unless id of signal is saved and signal handler is explicitly disconnected. Since signal handlers are passed the source object, we can simply use that and avoid redundant ref.