Changeset 15928 in vbox for trunk/src/VBox/HostServices/SharedClipboard/x11.cpp

Timestamp:

Jan 13, 2009 9:09:28 PM (16 years ago)

Author:

vboxsync

Message:

HostServices/SharedClipboard: fix a deadlock and reintroduce timeouts for data coming from VBox

File:

• trunk/src/VBox/HostServices/SharedClipboard/x11.cpp (modified) (30 diffs)

trunk/src/VBox/HostServices/SharedClipboard/x11.cpp

@@ -79 +79 @@
 } VBOXCLIPBOARDFORMAT;
 
-/** Does the host or the guest currently own the clipboard? */
-enum g_eClipboardOwner { NONE = 0, HOST, GUEST };
+/** Does X11 or VBox currently own the clipboard? */
+enum g_eOwner { NONE = 0, X11, VB };
 
 typedef struct {
@@ -127 +127 @@
 
     /** Does the host or the guest currently own the clipboard? */
-    volatile enum g_eClipboardOwner eOwner;
+    volatile enum g_eOwner eOwner;
 
     /** What is the best text format the host has to offer?  INVALID for none. */
@@ -148 +148 @@
         semaphore, which is triggered when the data arrives. */
     RTSEMEVENT waitForData;
-    /** And because it would not do for the guest to be waiting for the host while the host
-        is waiting for the guest, we set a flag and assert horribly if we spot a deadlock. */
-    uint32_t waiter;
-    /** This mutex is held while an asynchronous operation completes (i.e. the host clipboard is
-        being queried) to make sure that the clipboard is not disconnected during that time.  It
-        is also grabbed when the clipboard client disconnects.  When an asynchronous operation
-        starts completing, it checks that the same client is still connected immediately after
-        grabbing the mutex. */
-    RTSEMMUTEX asyncMutex;
+    /** Who (if anyone) is currently waiting for data?  Used for sanity checks
+     *  when data arrives. */
+    volatile uint32_t waiter;
+    /** This mutex is grabbed during any critical operations on the clipboard
+     * which might clash with others. */
+    RTSEMMUTEX clipboardMutex;
 
     /** Format which we are reading from the host clipboard (valid during a request for the
@@ -182 +179 @@
 
 /**
+ * Reset the contents of the buffer used to pass clipboard data from VBox to X11.
+ * This must be done after every clipboard transfer.
+ */
+static void vboxClipboardEmptyGuestBuffer(void)
+{
+    if (g_ctx.pClient->data.pv != 0)
+        RTMemFree(g_ctx.pClient->data.pv);
+    g_ctx.pClient->data.pv = 0;
+    g_ctx.pClient->data.cb = 0;
+    g_ctx.pClient->data.u32Format = 0;
+}
+
+/**
  * Send a request to VBox to transfer the contents of its clipboard to X11.
  *
@@ -191 +201 @@
 static int vboxClipboardReadDataFromVBox (VBOXCLIPBOARDCONTEXT *pCtx, uint32_t u32Format)
 {
-    VBOXCLIPBOARDCLIENTDATA *pClient = pCtx->pClient;
+    volatile VBOXCLIPBOARDCLIENTDATA *pClient = pCtx->pClient;
 
     LogFlowFunc(("u32Format=%02X\n", u32Format));
+    if (pClient == NULL)
+    {
+        /* This can legitimately happen if we disconnect during a request for
+         * data from X11. */
+        LogFunc(("host requested guest clipboard data after guest had disconnected.\n"));
+        pCtx->guestFormats = 0;
+        pCtx->waiter = NONE;
+        return VERR_TIMEOUT;
+    }
     /* Assert that no other transfer is in process (requests are serialised)
      * and that the last transfer cleaned up properly. */
-    AssertLogRelReturn(   pCtx->pClient->data.pv == NULL
-                       && pCtx->pClient->data.cb == 0
-                       && pCtx->pClient->data.u32Format == 0,
+    AssertLogRelReturn(   pClient->data.pv == NULL
+                       && pClient->data.cb == 0
+                       && pClient->data.u32Format == 0,
                        VERR_WRONG_ORDER
                       );
@@ -204 +223 @@
      * requests from X11 are serialised and the second because VBox previously
      * grabbed the clipboard, so it should not be waiting for data from us. */
-    AssertLogRelReturn (ASMAtomicCmpXchgU32(&pCtx->waiter, 1, 0), VERR_DEADLOCK);
-    if (pClient == 0)
-    {
-        /* This can legitimately happen if we disconnect during a request for
-         * data from X11. */
-        LogFunc(("host requested guest clipboard data after guest had disconnected.\n"));
-        pCtx->guestFormats = 0;
-        pCtx->waiter = 0;
-        return VERR_TIMEOUT;
-    }
-    /* Request data from the guest */
-    vboxSvcClipboardReportMsg (pCtx->pClient, VBOX_SHARED_CLIPBOARD_HOST_MSG_READ_DATA, u32Format);
-    /* Which will signal us when it is ready. */
-    int rc = RTSemEventWait(pCtx->waitForData, RT_INDEFINITE_WAIT);
+    AssertLogRelReturn (ASMAtomicCmpXchgU32(&pCtx->waiter, X11, NONE), VERR_DEADLOCK);
+    /* Request data from VBox */
+    vboxSvcClipboardReportMsg(pCtx->pClient,
+                              VBOX_SHARED_CLIPBOARD_HOST_MSG_READ_DATA,
+                              u32Format);
+    /* Which will signal us when it is ready.  We use a timeout here because
+     * we can't be sure that the guest will behave correctly. */
+    int rc = RTSemEventWait(pCtx->waitForData, CLIPBOARDTIMEOUT);
+    if (rc == VERR_TIMEOUT)
+        rc = VINF_SUCCESS;  /* Timeout handling follows. */
+    /* Now we have a potential race between the HGCM thread delivering the data
+     * and our setting waiter to NONE to say that we are no longer waiting for
+     * it.  We solve this as follows: both of these operations are done under
+     * the clipboard mutex.  The HGCM thread will only deliver the data if we
+     * are still waiting after it acquires the mutex.  After we release the
+     * mutex, we finally do our check to see whether the data was delivered. */
+    RTSemMutexRequest(g_ctx.clipboardMutex, RT_INDEFINITE_WAIT);
+    pCtx->waiter = NONE;
+    RTSemMutexRelease(g_ctx.clipboardMutex);
     AssertLogRelRCSuccess(rc);
     if (RT_FAILURE(rc))
@@ -223 +247 @@
         /* I believe this should not happen.  Wait until the assertions arrive
          * to prove the contrary. */
+        vboxClipboardEmptyGuestBuffer();
         pCtx->guestFormats = 0;
-        pCtx->waiter = 0;
         return rc;
     }
-    pCtx->waiter = 0;
+    if (pClient->data.pv == NULL)
+        return VERR_TIMEOUT;
     LogFlowFunc(("wait completed.  Returning.\n"));
     return VINF_SUCCESS;
@@ -490 +515 @@
     /* We grab this mutex whenever an asynchronous clipboard operation completes and while
        disconnecting a client from the clipboard to stop these operations colliding. */
-    RTSemMutexRequest(g_ctx.asyncMutex, RT_INDEFINITE_WAIT);
+    RTSemMutexRequest(g_ctx.clipboardMutex, RT_INDEFINITE_WAIT);
     if (reinterpret_cast<VBOXCLIPBOARDCLIENTDATA *>(pClientData) != g_ctx.pClient)
     {
@@ -496 +521 @@
         XtFree(reinterpret_cast<char *>(pValue));
         LogFlowFunc(("client is no longer connected, returning\n"));
-        RTSemMutexRelease(g_ctx.asyncMutex);
+        RTSemMutexRelease(g_ctx.clipboardMutex);
         return;
     }
@@ -534 +559 @@
         LogFunc (("bad target format\n"));
         XtFree(reinterpret_cast<char *>(pValue));
-        RTSemMutexRelease(g_ctx.asyncMutex);
+        RTSemMutexRelease(g_ctx.clipboardMutex);
         return;
     }
     g_ctx.notifyGuest = true;
-    RTSemMutexRelease(g_ctx.asyncMutex);
+    RTSemMutexRelease(g_ctx.clipboardMutex);
 }
 
@@ -567 +592 @@
     /* We grab this mutex whenever an asynchronous clipboard operation completes and while
        disconnecting a client from the clipboard to stop these operations colliding. */
-    RTSemMutexRequest(g_ctx.asyncMutex, RT_INDEFINITE_WAIT);
+    RTSemMutexRequest(g_ctx.clipboardMutex, RT_INDEFINITE_WAIT);
     if (reinterpret_cast<VBOXCLIPBOARDCLIENTDATA *>(pClientData) != g_ctx.pClient)
     {
         /* If the client is no longer connected, just return. */
         LogFlowFunc(("client is no longer connected, returning\n"));
-        RTSemMutexRelease(g_ctx.asyncMutex);
+        RTSemMutexRelease(g_ctx.clipboardMutex);
         return;
     }
@@ -633 +658 @@
     }
     XtFree(reinterpret_cast<char *>(pValue));
-    RTSemMutexRelease(g_ctx.asyncMutex);
+    RTSemMutexRelease(g_ctx.clipboardMutex);
 }
 
@@ -646 +671 @@
     Log3 (("%s: called\n", __PRETTY_FUNCTION__));
     /* Get the current clipboard contents */
-    if (g_ctx.eOwner == HOST && g_ctx.pClient != 0)
+    if (g_ctx.eOwner == X11 && g_ctx.pClient != 0)
     {
         Log3 (("%s: requesting the targets that the host clipboard offers\n",
@@ -812 +837 @@
     LogRel(("Initializing host clipboard service\n"));
     RTSemEventCreate(&g_ctx.waitForData);
-    RTSemMutexCreate(&g_ctx.asyncMutex);
+    RTSemMutexCreate(&g_ctx.clipboardMutex);
     rc = vboxClipboardInitX11();
     if (RT_SUCCESS(rc))
@@ -824 +849 @@
     {
         RTSemEventDestroy(g_ctx.waitForData);
-        RTSemMutexDestroy(g_ctx.asyncMutex);
+        RTSemMutexDestroy(g_ctx.clipboardMutex);
     }
     return rc;
@@ -852 +877 @@
      * immediately. */
     g_ctx.pClient = NULL;
-    /* Set the termination flag. */
+    if (g_ctx.eOwner == VB)
+        /* X11 may be waiting for data from VBox.  At this point it is no
+         * longer going to arrive, and we must release it to allow the event
+         * loop to terminate.  In this case the buffer where VBox would have
+         * written the clipboard data will still be empty and we will just
+         * return "no data" to X11.  Any subsequent attempts to get the data
+         * from VBox will fail immediately as the client reference is gone. */
+        RTSemEventSignal(g_ctx.waitForData);
+    /* Set the termination flag.  This has been observed to block if it was set
+     * during a request for clipboard data coming from X11, so only we do it
+     * after releasing any such requests. */
     XtAppSetExitFlag(g_ctx.appContext);
     /* Wake up the event loop */
@@ -860 +895 @@
     XSendEvent(XtDisplay(g_ctx.widget), XtWindow(g_ctx.widget), false, 0, &ev);
     XFlush(XtDisplay(g_ctx.widget));
-    if (g_ctx.eOwner == GUEST)
-        /* X11 may be waiting for data from VBox.  At this point it is no
-         * longer going to arrive, and we must release it to allow the event
-         * loop to terminate.  In this case the buffer where VBox would have
-         * written the clipboard data will still be empty and we will just
-         * return "no data" to X11.  Any subsequent attempts to get the data
-         * from VBox will fail immediately as the client reference is gone. */
-        RTSemEventSignal(g_ctx.waitForData);
     do
     {
@@ -885 +912 @@
          */
         RTSemEventDestroy(g_ctx.waitForData);
-        RTSemMutexDestroy(g_ctx.asyncMutex);
+        RTSemMutexDestroy(g_ctx.clipboardMutex);
         AssertRC(rcThread);
     }
@@ -917 +944 @@
     pClient->pCtx = &g_ctx;
     pClient->pCtx->pClient = pClient;
-    g_ctx.eOwner = HOST;
+    g_ctx.eOwner = X11;
     g_ctx.notifyGuest = true;
     return VINF_SUCCESS;
@@ -961 +988 @@
     LogFlow(("vboxClipboardDisconnect\n"));
 
-    RTSemMutexRequest(g_ctx.asyncMutex, RT_INDEFINITE_WAIT);
+    RTSemMutexRequest(g_ctx.clipboardMutex, RT_INDEFINITE_WAIT);
     g_ctx.pClient = NULL;
     g_ctx.eOwner = NONE;
     g_ctx.hostTextFormat = INVALID;
     g_ctx.hostBitmapFormat = INVALID;
-    RTSemMutexRelease(g_ctx.asyncMutex);
+    RTSemMutexRelease(g_ctx.clipboardMutex);
 }
 
@@ -1025 +1052 @@
     *piFormatReturn = 32;
     return true;
-}
-
-/**
- * Reset the contents of the buffer used to pass clipboard data from VBox to X11.
- * This must be done after every clipboard transfer.
- */
-static void vboxClipboardEmptyGuestBuffer(void)
-{
-    if (g_ctx.pClient->data.pv != 0)
-        RTMemFree(g_ctx.pClient->data.pv);
-    g_ctx.pClient->data.pv = 0;
-    g_ctx.pClient->data.cb = 0;
-    g_ctx.pClient->data.u32Format = 0;
 }
 
@@ -1340 +1354 @@
     LogFlowFunc(("\n"));
     /* Drop requests that we receive too late. */
-    if (g_ctx.eOwner != GUEST)
+    if (g_ctx.eOwner != VB)
         return false;
     if (   (*atomSelection != g_ctx.atomClipboard)
@@ -1406 +1420 @@
 {
     LogFlowFunc (("called, giving VBox clipboard ownership\n"));
-    g_ctx.eOwner = HOST;
+    g_ctx.eOwner = X11;
     g_ctx.notifyGuest = true;
 }
@@ -1434 +1448 @@
         return;
     }
-    if (g_ctx.eOwner == GUEST)
+    if (g_ctx.eOwner == VB)
     {
         /* We already own the clipboard, so no need to grab it, especially as that can lead
@@ -1443 +1457 @@
     }
     Log2 (("%s: giving the guest clipboard ownership\n", __PRETTY_FUNCTION__));
-    g_ctx.eOwner = GUEST;
+    g_ctx.eOwner = VB;
     g_ctx.hostTextFormat = INVALID;
     g_ctx.hostBitmapFormat = INVALID;
@@ -1453 +1467 @@
           guest formats are found which we understand. */
         g_ctx.notifyGuest = true;
-        g_ctx.eOwner = HOST;
+        g_ctx.eOwner = X11;
     }
     XtOwnSelection(g_ctx.widget, g_ctx.atomPrimary, CurrentTime, vboxClipboardConvertForX11,
@@ -1501 +1515 @@
          * requests from VBox are serialised and the second because X11 previously
          * grabbed the clipboard, so it should not be waiting for data from us. */
-        AssertLogRelReturn (ASMAtomicCmpXchgU32(&g_ctx.waiter, 1, 0), VERR_DEADLOCK);
+        AssertLogRelReturn (ASMAtomicCmpXchgU32(&g_ctx.waiter, VB, NONE), VERR_DEADLOCK);
         g_ctx.requestHostFormat = g_ctx.hostTextFormat;
         g_ctx.requestBuffer = pv;
@@ -1519 +1533 @@
         if (RT_FAILURE(rc))
         {
-            g_ctx.waiter = 0;
+            g_ctx.waiter = NONE;
             return rc;
         }
-        g_ctx.waiter = 0;
+        g_ctx.waiter = NONE;
     }
     else
@@ -1554 +1568 @@
                             && pClient->data.u32Format == 0);
 
-    if (cb > 0)
+    /* Grab the mutex and check that X11 is still waiting for the data before
+     * delivering it.  See the explanation in vboxClipboardReadDataFromVBox. */
+    RTSemMutexRequest(g_ctx.clipboardMutex, RT_INDEFINITE_WAIT);
+    if (g_ctx.waiter == X11 && cb > 0)
     {
         pClient->data.pv = RTMemAlloc (cb);
@@ -1565 +1582 @@
         }
     }
+    RTSemMutexRelease(g_ctx.clipboardMutex);
 
     RTSemEventSignal(g_ctx.waitForData);