Changeset 15898 in vbox for trunk/src/VBox

Timestamp:

Jan 12, 2009 2:42:51 PM (16 years ago)

Author:

vboxsync

Message:

HostServices/SharedClipboard: did some cleaning up of the X11 host clipboard code and removed the race-prone timeouts

Location:

trunk/src/VBox/HostServices/SharedClipboard

Files:

• VBoxClipboard.h (modified) (1 diff)
• x11.cpp (modified) (14 diffs)

trunk/src/VBox/HostServices/SharedClipboard/VBoxClipboard.h

@@ -25 +25 @@
 #define LOG_GROUP LOG_GROUP_HGCM
 #include <VBox/log.h>
-
-enum {
-    /** The number of milliseconds before the clipboard times out. */
-    CLIPBOARDTIMEOUT = 2000
-};
 
 struct _VBOXCLIPBOARDCONTEXT;

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

@@ -194 +194 @@
 
     LogFlowFunc(("u32Format=%02X\n", u32Format));
+    /* 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,
+                       VERR_WRONG_ORDER
+                      );
+    /* No one else (X11 or VBox) should currently be waiting.  The first because
+     * 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;
@@ -201 +214 @@
         return VERR_TIMEOUT;
     }
-    if (!(   pCtx->pClient->data.pv == NULL
-          && pCtx->pClient->data.cb == 0
-          && pCtx->pClient->data.u32Format == 0))
-    {
-        LogRel(("vboxClipboardReadDataFromClient: a guest to host clipboard transfer has been requested, but another is in progress, or has not cleaned up properly.\n"));
-        AssertMsgFailed(("A guest to host clipboard transfer has been requested, but another is in progress, or has not cleaned up properly.\n"));
-    }
-
-    /* Only one of the guest and the host should be waiting at any one time */
-    if (!ASMAtomicCmpXchgU32(&pCtx->waiter, 1, 0))
-    {
-        LogRel(("vboxClipboardReadDataFromClient: deadlock situation - the host and the guest are both waiting for data from the other.\n"));
-        return VERR_DEADLOCK;
-    }
     /* Request data from the guest */
     vboxSvcClipboardReportMsg (pCtx->pClient, VBOX_SHARED_CLIPBOARD_HOST_MSG_READ_DATA, u32Format);
     /* Which will signal us when it is ready. */
-    if (RTSemEventWait(pCtx->waitForData, CLIPBOARDTIMEOUT) != VINF_SUCCESS)
-    {
-        LogRel(("vboxClipboardReadDataFromClient: vboxSvcClipboardReportMsg failed to complete within %d milliseconds\n", CLIPBOARDTIMEOUT));
+    int rc = RTSemEventWait(pCtx->waitForData, RT_INDEFINITE_WAIT);
+    AssertLogRelRCSuccess(rc);
+    if (RT_FAILURE(rc))
+    {
+        /* I believe this should not happen.  Wait until the assertions arrive
+         * to prove the contrary. */
         pCtx->guestFormats = 0;
         pCtx->waiter = 0;
-        return VERR_TIMEOUT;
+        return rc;
     }
     pCtx->waiter = 0;
@@ -684 +686 @@
     XtAppMainLoop(g_ctx.appContext);
     g_ctx.formatList.clear();
-    RTSemEventDestroy(g_ctx.waitForData);
-    RTSemMutexDestroy(g_ctx.asyncMutex);
     LogRel(("Shared clipboard: host clipboard thread terminated successfully\n"));
     return VINF_SUCCESS;
@@ -848 +848 @@
     LogRel(("vboxClipboardDestroy: shutting down host clipboard\n"));
 
+    /* Drop the reference to the client, in case it is still there.  This will
+     * cause any outstanding clipboard data requests from X11 to fail
+     * immediately. */
+    g_ctx.pClient = NULL;
     /* Set the termination flag. */
     XtAppSetExitFlag(g_ctx.appContext);
@@ -856 +860 @@
     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
     {
@@ -863 +875 @@
     } while ((VERR_TIMEOUT == rc) && (count < 300));
     if (RT_SUCCESS(rc))
+    {
+        /*
+         * No one should be waiting on this by now.  Justification:
+         *  - Case 1: VBox is waiting for data from X11:
+         *      Not possible, as it would be waiting on this thread.
+         *  - Case 2: X11 is waiting for data from VBox:
+         *      Not possible, as we checked that the X11 event thread exited
+         *      successfully.
+         */
+        RTSemEventDestroy(g_ctx.waitForData);
+        RTSemMutexDestroy(g_ctx.asyncMutex);
         AssertRC(rcThread);
+    }
     else
         LogRel(("vboxClipboardDestroy: rc=%Rrc\n", rc));
@@ -889 +913 @@
 
     /* Only one client is supported for now */
-    if (g_ctx.pClient != 0)
-    {
-        LogRel(("vboxClipboardConnect: attempted to connect, but a client appears to be already running.\n"));
-        AssertReturn(g_ctx.pClient == 0, VERR_NOT_SUPPORTED);
-    }
+    AssertLogRelReturn(g_ctx.pClient == 0, VERR_NOT_SUPPORTED);
 
     pClient->pCtx = &g_ctx;
@@ -1045 +1065 @@
         /* If vboxClipboardReadDataFromVBox fails then pClient may be invalid */
         LogRelFunc (("vboxClipboardReadDataFromVBox returned %Rrc%s\n", rc,
-                     RT_SUCCESS(rc) ? ", g_ctx.pClient->data.cb == 0" :  ""));
+                    RT_SUCCESS(rc) ? ", g_ctx.pClient->data.cb == 0" :  ""));
         vboxClipboardEmptyGuestBuffer();
         return false;
@@ -1120 +1140 @@
         /* If vboxClipboardReadDataFromVBox fails then pClient may be invalid */
         LogRelFunc (("vboxClipboardReadDataFromVBox returned %Rrc%s\n", rc,
-                      RT_SUCCESS(rc) ? ", g_ctx.pClient->data.cb == 0" :  ""));
+                     RT_SUCCESS(rc) ? ", g_ctx.pClient->data.cb == 0" :  ""));
         vboxClipboardEmptyGuestBuffer();
         return false;
@@ -1319 +1339 @@
 
     LogFlowFunc(("\n"));
+    /* Drop requests that we receive too late. */
+    if (g_ctx.eOwner != GUEST)
+        return false;
     if (   (*atomSelection != g_ctx.atomClipboard)
         && (*atomSelection != g_ctx.atomPrimary)
@@ -1475 +1498 @@
             return VERR_NO_DATA;  /* The guest thinks we have data and we don't */
         }
-        /* Only one of the host and the guest should ever be waiting. */
-        if (!ASMAtomicCmpXchgU32(&g_ctx.waiter, 1, 0))
-        {
-            LogRel(("vboxClipboardReadData: detected a deadlock situation - the host and the guest are waiting for each other.\n"));
-            return VERR_DEADLOCK;
-        }
+        /* No one else (VBox or X11) should currently be waiting.  The first because
+         * 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);
         g_ctx.requestHostFormat = g_ctx.hostTextFormat;
         g_ctx.requestBuffer = pv;
@@ -1492 +1513 @@
                             vboxClipboardGetDataFromX11, reinterpret_cast<XtPointer>(g_ctx.pClient),
                             CurrentTime);
-        /* When the data arrives, the vboxClipboardGetProc callback will be called.  The
+        /* When the data arrives, the vboxClipboardGetDataFromX11 callback will be called.  The
            callback will signal the event semaphore when it has processed the data for us. */
 
-#ifdef RT_OS_SOLARIS
-        /*
-         * Trusted Xorg requires a bigger timeout.
-         */
-        unsigned long cTimeoutMillies = CLIPBOARDTIMEOUT;
-        if (is_system_labeled())
-            cTimeoutMillies = 1000 * 120;
-
-        if (RTSemEventWait(g_ctx.waitForData, cTimeoutMillies) != VINF_SUCCESS)
-#else
-        if (RTSemEventWait(g_ctx.waitForData, CLIPBOARDTIMEOUT) != VINF_SUCCESS)
-#endif
+        int rc = RTSemEventWait(g_ctx.waitForData, RT_INDEFINITE_WAIT);
+        if (RT_FAILURE(rc))
         {
-            /* No need to polute the release log for this. */
-            // LogRel(("vboxClipboardReadDataFromClient: XtGetSelectionValue failed to complete within %d milliseconds\n", CLIPBOARDTIMEOUT));
-            /* A time out can legitimately occur if a client is temporarily too busy to answer fast */
-            // g_ctx.hostTextFormat = INVALID;
-            // g_ctx.hostBitmapFormat = INVALID;
             g_ctx.waiter = 0;
-            return VERR_TIMEOUT;
+            return rc;
         }
         g_ctx.waiter = 0;
@@ -1542 +1548 @@
     LogFlowFunc (("called\n"));
 
-    /*
-     * The guest returns data that was requested in the WM_RENDERFORMAT handler.
-     */
-    if (!(   pClient->data.pv == NULL
-          && pClient->data.cb == 0
-          && pClient->data.u32Format == 0))
-    {
-        LogRel(("vboxClipboardWriteData: clipboard data has arrived from the guest, but another transfer is in process or has not cleaned up properly.\n"));
-        AssertMsgFailed(("vboxClipboardWriteData: clipboard data has arrived from the guest, but another transfer is in process or has not cleaned up properly.\n"));
-    }
+    /* Assert that no other transfer is in process (requests are serialised)
+     * or has not cleaned up properly. */
+    AssertLogRelReturnVoid (   pClient->data.pv == NULL
+                            && pClient->data.cb == 0
+                            && pClient->data.u32Format == 0);
 
     if (cb > 0)
@@ -1567 +1568 @@
     RTSemEventSignal(g_ctx.waitForData);
 }
+