Created attachment 326889 [details] [review]
gtkplacesview: fix uncancellable connections

server_mount_ready_cb() would return early if the connection was
cancelled, without reversing the busy state, leaving the user unable to
connect to any other server.

This commit removes the offending code.

Review of attachment 326889 [details] [review]:

Thanks for digging into this Ernestas!

First, this code in nautilus is generated from gtk+ using the gtk-code-generator.sh in the src/gtk/ folder. So the fix should go in gtk+ and then run this script on nautilus to update it.

About the code itself, so there is a comment there that makes an assumption that if it is cancelled it's because it's in finalize. So that was wrong. But removing that code would cause a crash if the operation finish after the widget is finalized. So the patch is not correct neither.

I wonder how we could detect that we are in finalize in order to avoid accessing invalid data. I can only think about putting a private safe guard boolean "in_destruction" and check it. But I will let this to gtk+.

Also as an unrelated issue in the same code, realized that the code shows a error dialog if the error is G_IO_ERROR_FAILED_HANDLED. The documentation for the constant says "Do not display any error dialog."

Most GVfs backends return G_IO_ERROR_FAILED_HANDLED in case the user cancels the dialog (at least sftp, ftp, afp, smb currently do), so this case needs to be handled correctly.

There also seems to be a use-after-free because after triggering this bug, e.g. in a filechooser, and then closing the filechooser, there is a continuous stream of errors like this: Gtk-CRITICAL **: gtk_entry_progress_pulse: assertion 'GTK_IS_ENTRY (entry)' failed

Created attachment 334797 [details] [review]
placesview: hold onto ref during (un)mount ops

During mount and unmount opertions we keep a reference to the
GtkPlacesView around, so we have a valid view for the callback
code, even in the case that othe external references have been
dropped (i.e. the containing window gets destroyed).

Created attachment 334798 [details] [review]
placesview: properly recover from cancellation

The current code wrongly assumes that cancellation can only happen
as a result widget finalization, and consequentially does not
properly recover from it. Therefore if the operation is cancelled
as a result of user interaction, the entry is will stay disabled
and the spinner will keep spinning. This is fixed by removal of
the early bail out in case of cancellation.

Created attachment 334799 [details] [review]
placesview: override destory to cancel ongoing ops

Since we hold on to a reference during (un)mount operations, we
don't trigger the cancellation of operations in finalize anymore.
Instead we now override the GtkWidget's destroy() and cancel any
ongoing operations there.

Created attachment 334800 [details] [review]
placesview: keep reference during network fetching

Analogous to (un)mount operation, we now keep a reference around
during the ongoing operation and make use of the destroyed flag
to check if we are still alive or if we have been cancelled as
a result of the widget being destroyed.

Review of attachment 334797 [details] [review]:

This works as long as the parents are using g_object_unref, but not when the widget gets a destroy() directly, since that runs dispose(). The correct way to do this is to keep a weak reference with a callback and cancel everything in case that happens. You can take a look how we do it in nautilus-files-view.c for the new_folder dialog.

In any case, this is just my 2 cents, I'm not gtk+ dev :)

Review of attachment 334799 [details] [review]:

IMHO keeping a widget alive until a disk operation is finished is not the best way to deal with this situation. In nautilus we had some issues with this, basically the widget was still in the window for one minute which is not a good UX.
My comment on this patch would be to do similar to what I said before.

Thanks for taking a look at the patches!

> This works as long as the parents are using g_object_unref, but not when the
> widget gets a destroy() directly, since that runs dispose().

I don't quite understand what you mean. In both cases gtk_places_view_destroy() should get called (as intended): a) gtk_widget_destroy() -> g_object_run_dispose() -> g_object_dispose -> gtk_widget_dispose() -> emit destroy signal -> gtk_places_view_destroy ()
b) g_object_unref on the last reference -> g_object_dispose -> see above 

> The correct way
> to do this is to keep a weak reference with a callback and cancel everything
> in case that happens. You can take a look how we do it in
> nautilus-files-view.c for the new_folder dialog.

I don't see how that is any different to what the current code (before the patches) is doing, where we cancel the operation directly in the finalize method.

The main problem with the current code is that cancellation via the user leaves the UI broken (entry disabled, etc), which is due to the early exit in the error handling code. If we want to fix that and restore the UI we have to tell cancellation via the user apart from cancellation due to widget destruction. For this reason the patch keeps the reference to the view around so it can check for the destroyed flag. I am not sure who cancelling the operation via a callback from a weak ref would address this problem. 
I guess we could use a GWeakRef as an argument to all async functions (mount, unmount, network enumeration) and bail out if we can't acquire the strong reference. I don't believe the current patches would make "the widget be in the window for one minute" after destruction though.

(In reply to Christian Kellner from comment #12)
> Thanks for taking a look at the patches!
> 
> > This works as long as the parents are using g_object_unref, but not when the
> > widget gets a destroy() directly, since that runs dispose().
> 
> I don't quite understand what you mean. In both cases
> gtk_places_view_destroy() should get called (as intended): a)
> gtk_widget_destroy() -> g_object_run_dispose() -> g_object_dispose ->
> gtk_widget_dispose() -> emit destroy signal -> gtk_places_view_destroy ()
> b) g_object_unref on the last reference -> g_object_dispose -> see above 
> 

Ah yeah indeed, this was a review of the first patch that didn't contain the upcoming patches.

> > The correct way
> > to do this is to keep a weak reference with a callback and cancel everything
> > in case that happens. You can take a look how we do it in
> > nautilus-files-view.c for the new_folder dialog.
> 
> I don't see how that is any different to what the current code (before the
> patches) is doing, where we cancel the operation directly in the finalize
> method.
> 
> The main problem with the current code is that cancellation via the user
> leaves the UI broken (entry disabled, etc), which is due to the early exit
> in the error handling code. If we want to fix that and restore the UI we
> have to tell cancellation via the user apart from cancellation due to widget
> destruction. For this reason the patch keeps the reference to the view
> around so it can check for the destroyed flag. I am not sure who cancelling
> the operation via a callback from a weak ref would address this problem. 
> I guess we could use a GWeakRef as an argument to all async functions
> (mount, unmount, network enumeration) and bail out if we can't acquire the
> strong reference. I don't believe the current patches would make "the widget
> be in the window for one minute" after destruction though.

Oh yeah exactly, I just think it's cleaner using a gweakref and some struct data or so instead than using the view priv. But yeah, you got it right.

"and bail out if we can't acquire the strong reference"
I'm not sure I understood this. In any case, the weakref allows us to know whether the widget was destroyed to differentiate between destruction and user cancellation.

As said just my 2 cents, probably at this point it's just a matter of opinion/taste.

I think those patches are fine as-is.

Attachment 334797 [details] pushed as 3ed2594 - placesview: hold onto ref during (un)mount ops
Attachment 334798 [details] pushed as 0e5b0b7 - placesview: properly recover from cancellation
Attachment 334799 [details] pushed as 7cdc04c - placesview: override destory to cancel ongoing ops
Attachment 334800 [details] pushed as d4bf215 - placesview: keep reference during network fetching

*** Bug 771160 has been marked as a duplicate of this bug. ***

*** Bug 758243 has been marked as a duplicate of this bug. ***