Created attachment 252792 [details] [review]
win32: retry loop when calling OpenClipboard()

Sorry, IMO the keyword here is horrible. Error handling is one thing, but a thread blocking call of 2s does not belong on the toolkit level, also of course IMO.

If only there was a better solution... currently gtk+ silentely fails, so really hard to workaround this in the app..

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

We really need a solution here, isn't there any event we can listen for when the clipboard is free again?

This was already discussed on the IRC. The problem here is that clipboard actions are expected to have immediate effect. If we delay, for example, putting text into Windows Clipboard by buffering it inside GDK until Windows Clipboard is accessible (clipboard action appears to have "succeeded", even though it is not complete yet), there are two possible bad outcomes:

1) Some (buggy) app will hog the clipboard and we'll be stuck buffering stuff until the clipboard is released (possibly never). No errors from us, but nothing appears in the clipboard.

2) Some (non-buggy) app will get the clipboard, put stuff into it, then release it, at which point we'll get it, and put our stuff into it, erasing the stuff that the other app put there. The user expects the latest Ctrl+C to be the one that has effect, but in this case it will be our delayed Ctrl+C that puts stuff into the clipboard.

The alternative of just blocking on clipboard being ready is not a good idea either.

The cleanest solution is to somehow convey the error to the user, suggesting to try again later. Not sure what the appropriate way is. Most likely GDK will just have to send some kind of event, GTK will have to have a stock way of handling it (a widget or a notification), and the application will have to handle the event and make use of that stock way (or override it).

Well, maybe the 2s limit in Marc-Andre original patch is too much, but it seems to be that a small retry loop would still be a good trade off: if we fail and retry and succeed 100ms later, this will be perfectly acceptable for a user and we fix at least the unlucky case where we tried to access the clipboard exactly at the same time as someone else.


Even if do the "clean solution" and bubble the error up, what can an app do? Surely it should not pop up a dialog saying "Sorry, the clipboard was busy and MS api sucks". My guess would be that 90% apps would just miss to handle this and the remaining 10% would loop and retry, so we may as well do it in gtk

I did not check the code but if we go for the loop, does this mean we would be blocking the main loop?

Indeed, sorry I did not realize this would be blocking the main loop :(

/*2) Some (non-buggy) app will get the clipboard, put stuff into it, then release it, at which point we'll get it, and put our stuff into it, erasing the stuff that the other app put there. The user expects the latest Ctrl+C to be the one that has effect, but in this case it will be our delayed Ctrl+C that puts stuff into the clipboard.*/
This case can be easily overcomed by checking GetClipboardSequenceNumber() that returns different number each time clipboard modified and doesn't require clipboard to be opened.

(In reply to elfmz from comment #10)
>> /*2) Some (non-buggy) app will get the clipboard, put stuff into it, then
>> release it, at which point we'll get it, and put our stuff into it, erasing
>> the stuff that the other app put there. The user expects the latest Ctrl+C
>> to be the one that has effect, but in this case it will be our delayed
>> Ctrl+C that puts stuff into the clipboard.*/
> This case can be easily overcomed by checking GetClipboardSequenceNumber()
> that returns different number each time clipboard modified and doesn't
> require clipboard to be opened.

That could work, i guess. What about other uses of OpenClipboard(), such as gtk_selection_owner_set_for_display()? GTK expects it to succeed immediately.

(In reply to LRN from comment #6)
> This was already discussed on the IRC. The problem here is that clipboard
> actions are expected to have immediate effect. If we delay, for example,
> putting text into Windows Clipboard by buffering it inside GDK until Windows
> Clipboard is accessible (clipboard action appears to have "succeeded", even
> though it is not complete yet), there are two possible bad outcomes:

Well until you have succeeded you don't have to notified that you have "succeeded": in fact, all the GDK functions for retrieving the data are all async.
About the retrying thing: this is actually this is what Microsoft does
https://msdn.microsoft.com/en-us/library/ms158293.aspx

(In reply to Ignacio Casal Quinteiro (nacho) from comment #8)
> I did not check the code but if we go for the loop, does this mean we would
> be blocking the main loop?

Maybe not. 
The loop that I've added in my patch (attachment 336012 [details]) in bug 771775 it is a blocking loop by its nature, but it is inside of some functions that doesn't block the main loop. In fact, the data retrieving functions from the clipboard are blocking by nature (the retrieval of the data could take some time in some cases), thus the GDK functions for retrieving the data are all async and they don't block the main loop. In this scenario I think that my patch doesn't block the main loop either.

(In reply to Roberto Tronci from comment #13)
> (In reply to Ignacio Casal Quinteiro (nacho) from comment #8)
> > I did not check the code but if we go for the loop, does this mean we would
> > be blocking the main loop?
> 
> Maybe not. 
> The loop that I've added in my patch (attachment 336012 [details]) in bug
> 771775 it is a blocking loop by its nature, but it is inside of some
> functions that doesn't block the main loop. In fact, the data retrieving
> functions from the clipboard are blocking by nature (the retrieval of the
> data could take some time in some cases), thus the GDK functions for
> retrieving the data are all async and they don't block the main loop. In
> this scenario I think that my patch doesn't block the main loop either.

Sorry for the extra reply, but probably my previous reply is a little bit unclear. 
What I was saying is that: all the GDK data retrieval async functions (the "wait_for_" ones) are based on the gtk_clipboard_wait_for_contents that creates a new mainloop in the default context (see the code snippet):

---
results.loop = g_main_loop_new (NULL, TRUE);

  gtk_clipboard_request_contents (clipboard, target, 
				  clipboard_received_func,
				  &results);

  if (g_main_loop_is_running (results.loop))
    {
      gdk_threads_leave ();
      g_main_loop_run (results.loop);
      gdk_threads_enter ();
    }

  g_main_loop_unref (results.loop);
---

Thus, if I'm not wrong, the addition of a retry loop if the clipboard is busy when retrieving the data (in a function called in the gtk_clipboard_request_contents hierarchy) will block only this new mainloop that waits for the data and not the main mainloop. Am I wrong?

No idea. I have not studied the inner workings of gtk/gdk clipboard handling in any detail.

> > This case can be easily overcomed by checking GetClipboardSequenceNumber()
> > that returns different number each time clipboard modified and doesn't
> > require clipboard to be opened.
> That could work, i guess. What about other uses of OpenClipboard(), such as
> gtk_selection_owner_set_for_display()? GTK expects it to succeed immediately.
Of course its not a silver bullet, just make work 'copy' operation always succeed fast and never lost data (unless 'overwritten' by another subsequent 'copy' operation that is obviously not a problem).
Failing 'paste' or other serving operations is less significant issue cuz it doesn't directly cause data loss.

> What I was saying is that: all the GDK data retrieval async functions (the
> "wait_for_" ones) are based on the gtk_clipboard_wait_for_contents that
> creates a new mainloop in the default context (see the code snippet):
However, regardless of $subj, such subloops tend to cause deadlock, and seems I've seen such couple of times. Consider what will happen if some code dispatched some event in such subloop will again try to use clipboard.

The massive patch for bug 786509 will also fix this bug.

(In reply to LRN from comment #17)
> The massive patch for bug 786509 will also fix this bug.

Why would DnD related code would change anything with the clipboard, can you explain a bit before I dive deep on the patch?

DnD and clipboard use the same underlying GDK primitives and deal with the same data formats. So substantial fixes to DnD inevitably led to fixes for GDK selection and clipboard. After gaining better understanding of how selections work (GDK is modeled after X and uses its (sometimes confusing) terminology) i was able  to figure out the places where GDK selection API is asynchronous, and figure out how to make use of that to asynchronously deal with OpenClipboard(), as part of the general W32 GDK selection changes.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/443.