GNOME Bugzilla – Bug 767214
Main window closes after 'sudo poweroff' inside live machine
Last modified: 2016-11-21 09:48:09 UTC
This happens if you open a live machine in a separate window. Steps to reproduce: Create a new live libvirt machine, press back button after it starts running, open it in a new window and press 'sudo poweroff'. The new window will close and, after displaying the message 'Live machine will be deleted automatically', Boxes closes completely.
Solving this bug will probably help with https://bugzilla.gnome.org/show_bug.cgi?id=758840
Created attachment 330365 [details] [review] app: Check if window exists before trying to remove it Sometimes a window will receive more than one removal notification, which may result in Boxes thinking that the last window has been removed, thus closing prematurely. Let's check if the removed window still exists in Boxes' window list before actually trying to remove it.
Review of attachment 330365 [details] [review]: This also fixes https://bugzilla.gnome.org/show_bug.cgi?id=768033
Review of attachment 330365 [details] [review]: Good effort but NACK, this is a workaround. Please investigate why the window is receiving the request twice and fix the root cause.
Created attachment 330854 [details] [review] app-window: Disconnect from machine callbacks at removal The problem is that after removing a window, its deleted machine is still able to send signals to it and consequentially to try to remove it again during the actual machine deletion process, which leads to undefined behaviour. Let's simply disconnect the machine's callbacks in its window when the machine is signaled as deleted.
Created attachment 330855 [details] [review] app-window: Disconnect from previous machine's property The problem is that currently a window will listen to notifications from a machine long after it has been replaced with another machine. This leads to unpredictive behaviour, like windows being removed because one of its previous machines has been deleted.
Review of attachment 330854 [details] [review]: ::: src/app-window.vala @@ +439,3 @@ + + machine.disconnect (machine_state_notify_id); + machine_state_notify_id = 0; On the original bug's reviews you told me that this would be equivalent to current_item = null, but it's actually not the case because current_item = null schedules the machine for garbage removal while an asynchronous deletion task is still being performed on it. It's actually that asynchronous deletion task that sets the machine's deleted property to true, resulting in a callback signal that was causing us all the problems.
Review of attachment 330854 [details] [review]: ::: src/app-window.vala @@ +439,3 @@ + + machine.disconnect (machine_state_notify_id); + machine_state_notify_id = 0; Firstly, there is no garbage collector in Vala, it's all reference counting. i-e if you lose the last reference to an object, it is destroyed *immediately*. In retrospect, I don't know why I suggested 'current_item = null' here but this shouldn't be the last reference to the item and we should have at least one reference left in App.collection. Also Machine.delete() is synchronous, and on_machine_deleted_notify() ensures that only the correct window acts on machine deletion: if (this != App.app.main_window && (current_item as Machine).deleted) So i'm a bit confused as to what is going on.
Review of attachment 330854 [details] [review]: ::: src/app-window.vala @@ +439,3 @@ + + machine.disconnect (machine_state_notify_id); + machine_state_notify_id = 0; My mistake, the machine.delete() is synchonous, but the issue still is the same: After the window is removed, during the machine deletion process, Machine.delete () sets Machine.deleted to true, which results in on_machine_deleted_notify() being called. This method, indeed, ensures that we don't try to remove App.app.main_window. But, while trying to remove our non-main window, we end up at this chunk of code, in App.remove_window(): if (windows.length () == 1) return quit_app (); Because we are trying to remove a window that was already removed and because the main window is the only one left, we end up quitting Boxes :(
Review of attachment 330854 [details] [review]: ::: src/app-window.vala @@ +439,3 @@ + + machine.disconnect (machine_state_notify_id); + machine_state_notify_id = 0; Ah ok. Now that explains things. :) Now this fix looks really good. :)
Can we apply this fix to 3.22 branch? I've just hit this in our test suite. thanks, Vladimir
(In reply to vladimir benes from comment #11) > Can we apply this fix to 3.22 branch? I've just hit this in our test suite. I intend to cherry-pick fixes before releasing 3.22.2 today.
Attachment 330854 [details] pushed as b4e7dbc - app-window: Disconnect from machine callbacks at removal Attachment 330855 [details] pushed as 547ce0a - app-window: Disconnect Machine.deleted notify (shortlog improved)