What is the difference between checking that clipboard is open (by calling GetOpenClipboardWindow()), then opening it with OpenClipboard() AND just calling OpenClipboard(), then looking at the error code to see if it failed because the clipboard is already opened?

Aside from that, it does not seem like a good idea to block (even for up to 25ms) when opening the clipboard. I'm not sure that the users of this API expect such blocking.

Note that this is a fundamental problem with trying to use an exclusive shared resource (the clipboard). So far no one's been able to really solve it. See bug 667792, for example.

I indeed saw some similar change in Scintilla: https://groups.google.com/d/topic/scintilla-interest/HIuFrgTL8RM/discussion

Regarding the patch itself:

* there is a race between GetOpenClipboardWindow() and OpenClipboard(), so the check might be irrelevant.  Moreover, the MSDN page (https://msdn.microsoft.com/en-us/library/windows/desktop/ms649044%28v=vs.85%29.aspx) states that GetOpenClipboardWindow() might return NULL even if the clipboard is open, if it wasn't associated with a window.  I guess a better implementation would check the return value of OpenClipboard() instead, and retry if it failed.

* passing in the timeout and retry count is probably not a great idea if it's generally gonna be the same: it probably should rather be either an implementation detail, or a boolean parameter if some call site can't afford the extra delay.

(duplicate of https://bugzilla.gnome.org/show_bug.cgi?id=706610 though)

Thanks for taking the time to report this.
This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find.

*** This bug has been marked as a duplicate of bug 706610 ***

> * there is a race between GetOpenClipboardWindow() and OpenClipboard(), so
> the check might be irrelevant.  Moreover, the MSDN page
> (https://msdn.microsoft.com/en-us/library/windows/desktop/ms649044%28v=vs.
> 85%29.aspx) states that GetOpenClipboardWindow() might return NULL even if
> the clipboard is open, if it wasn't associated with a window.  I guess a
> better implementation would check the return value of OpenClipboard()
> instead, and retry if it failed.

This is true, but probably it always better to check GetOpenClipboardWindow() before to avoid to have warning like the following on GDK when possible.

WARN  Gdk - ..\..\..\gdk\win32\gdkselection-win32.c:75: OpenClipboard failed: Access is denied.

(In reply to Roberto Tronci from comment #5)
> This is true, but probably it always better to check
> GetOpenClipboardWindow() before to avoid to have warning like the following
> on GDK when possible.
> 
> WARN  Gdk - ..\..\..\gdk\win32\gdkselection-win32.c:75: OpenClipboard
> failed: Access is denied.

This comes from the API_CALL() wrapper, if you don't use it it won't warn on failure.  So just do something like that:

while (--retry_times > 0)
  {
    /* no API_CALL() wrapper as we check the result */
    if (OpenClipboard (open_clipboard_parms))
      return 1;
    Sleep(retry_delay);
  }

/* wrap with API_CALL() as it's the last resort option */
return (API_CALL (OpenClipboard, (open_clipboard_parms)));

(In reply to Colomban Wendling from comment #6)
> This comes from the API_CALL() wrapper, if you don't use it it won't warn on
> failure.  So just do something like that:

Thanks for the hint!

Created attachment 336271 [details] [review]
Clipboard: patch to fix simultaneous access to data

This patch focuses on the issue of retrieving the data from the clipboard in the case of simultaneous access to it. In fact, multiple applications may watch and open the clipboard simultaneously, but only one can open the clipboard while the others will receive an error. This happens also in the case we want only to read the data that it is present in the clipboard, and not modify it. Moreover, as written in MSDN "OpenClipboard fails if another window has the clipboard open", but the Windows API doesn't provide any event to notify that the clipboard is free again. Thus, the only solution, is to try several times to open it until the clipboard can be opened.

The error related to the Windows API function OpenClipboard can happen also in the case that the application wants to open the clipboard to only retrieve the types (mime-types) of the data that are present in the clipboard, or wants to write data into it. This patch is focused only in the case we want to read data from the clipboard (ie the mime-types or the data). For this reason the new function that create a loop to retry the OpenClipboard command replaces it only in some parts of the code and not globally.
The new gtk_win32_open_clipboard function will attempt to access to the clipboard for a specified number of times. Between each try the loop will sleep for a specified amount of microseconds. In this version of the patch the maximum number of tries are 10 and the sleep time between each try is 5 milliseconds. This new function will return false if it consumes all the number of tries without successfully opening the clipboard. This issue has been also tracked into Bugzilla with id 771775. Other bugs try to replace all the OpenClipboard calls, like bug 706610, but in that case other problems can arise in the case of copy events (ie writing data into the clipboard).

Review of attachment 336271 [details] [review]:

The style needs some fixing,

::: gdk/win32/gdkselection-win32.c
@@ +69,3 @@
+                           guint microseconds_retry_delay)
+{
+    while (--retry_times > 0) {

this would be:

  while (--retry_times > 0)
    {
      if (OpenClipboard (hwnd_new_owner)
        return 1;

      g_usleep (microseconds_retry_delay);
    }

  return (API_CALL (OpenClipboard, (hwnd_new_owner)));

I still don't think that exposing the retry count and delay as parameters is very useful.

Created attachment 336273 [details] [review]
Clipboard: patch to fix simultaneous access to data

I've fixed the style.

Review of attachment 336273 [details] [review]:

Not there yet.

::: gdk/win32/gdkselection-win32.c
@@ +69,3 @@
+                           guint microseconds_retry_delay)
+{
+    while (--retry_times > 0) 

2 spaces for indentation

@@ +72,3 @@
+      {
+        if (OpenClipboard (hwnd_new_owner))
+            return 1;

use 2 spaces for indentation

@@ +74,3 @@
+            return 1;
+
+        g_usleep(microseconds_retry_delay);

still missing space before (

(In reply to Colomban Wendling from comment #10)
> I still don't think that exposing the retry count and delay as parameters is
> very useful.

I'm exposing them because, in the case of reading the data from the clipboard I can retry more times than the cases when I want to write the data or open it for other uses. This was the idea behind exposing those parameters, even if for now I'm using this new function only when I have to read the data, in all the other cases I've left all the previous API_CALL of OpenClipboard.

Created attachment 336278 [details] [review]
Clipboard: patch to fix simultaneous access to data

Review of attachment 336278 [details] [review]:

See the comment

::: gdk/win32/gdkselection-win32.c
@@ +69,3 @@
+                           guint microseconds_retry_delay)
+{
+  while (--retry_times > 0) 

before giving the OK, I would definitely like to see here a big fat comment explaining why we need this