Index: /trunk/src/VBox/HostServices/SharedClipboard/VBoxClipboard.h =================================================================== --- /trunk/src/VBox/HostServices/SharedClipboard/VBoxClipboard.h (revision 15927) +++ /trunk/src/VBox/HostServices/SharedClipboard/VBoxClipboard.h (revision 15928) @@ -25,4 +25,9 @@ #define LOG_GROUP LOG_GROUP_HGCM #include + +enum { + /** The number of milliseconds before the clipboard times out. */ + CLIPBOARDTIMEOUT = 5000 +}; struct _VBOXCLIPBOARDCONTEXT; Index: /trunk/src/VBox/HostServices/SharedClipboard/x11.cpp =================================================================== --- /trunk/src/VBox/HostServices/SharedClipboard/x11.cpp (revision 15927) +++ /trunk/src/VBox/HostServices/SharedClipboard/x11.cpp (revision 15928) @@ -79,6 +79,6 @@ } 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,5 +127,5 @@ /** 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,13 +148,10 @@ 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,4 +179,17 @@ /** + * 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,12 +201,21 @@ 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,18 +223,23 @@ * 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,9 +247,10 @@ /* 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,5 +515,5 @@ /* 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(pClientData) != g_ctx.pClient) { @@ -496,5 +521,5 @@ XtFree(reinterpret_cast(pValue)); LogFlowFunc(("client is no longer connected, returning\n")); - RTSemMutexRelease(g_ctx.asyncMutex); + RTSemMutexRelease(g_ctx.clipboardMutex); return; } @@ -534,9 +559,9 @@ LogFunc (("bad target format\n")); XtFree(reinterpret_cast(pValue)); - RTSemMutexRelease(g_ctx.asyncMutex); + RTSemMutexRelease(g_ctx.clipboardMutex); return; } g_ctx.notifyGuest = true; - RTSemMutexRelease(g_ctx.asyncMutex); + RTSemMutexRelease(g_ctx.clipboardMutex); } @@ -567,10 +592,10 @@ /* 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(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,5 +658,5 @@ } XtFree(reinterpret_cast(pValue)); - RTSemMutexRelease(g_ctx.asyncMutex); + RTSemMutexRelease(g_ctx.clipboardMutex); } @@ -646,5 +671,5 @@ 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,5 +837,5 @@ 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,5 +849,5 @@ { RTSemEventDestroy(g_ctx.waitForData); - RTSemMutexDestroy(g_ctx.asyncMutex); + RTSemMutexDestroy(g_ctx.clipboardMutex); } return rc; @@ -852,5 +877,15 @@ * 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,12 +895,4 @@ 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,5 +912,5 @@ */ RTSemEventDestroy(g_ctx.waitForData); - RTSemMutexDestroy(g_ctx.asyncMutex); + RTSemMutexDestroy(g_ctx.clipboardMutex); AssertRC(rcThread); } @@ -917,5 +944,5 @@ pClient->pCtx = &g_ctx; pClient->pCtx->pClient = pClient; - g_ctx.eOwner = HOST; + g_ctx.eOwner = X11; g_ctx.notifyGuest = true; return VINF_SUCCESS; @@ -961,10 +988,10 @@ 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,17 +1052,4 @@ *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,5 +1354,5 @@ 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,5 +1420,5 @@ { LogFlowFunc (("called, giving VBox clipboard ownership\n")); - g_ctx.eOwner = HOST; + g_ctx.eOwner = X11; g_ctx.notifyGuest = true; } @@ -1434,5 +1448,5 @@ 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,5 +1457,5 @@ } 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,5 +1467,5 @@ 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,5 +1515,5 @@ * 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,8 +1533,8 @@ if (RT_FAILURE(rc)) { - g_ctx.waiter = 0; + g_ctx.waiter = NONE; return rc; } - g_ctx.waiter = 0; + g_ctx.waiter = NONE; } else @@ -1554,5 +1568,8 @@ && 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,4 +1582,5 @@ } } + RTSemMutexRelease(g_ctx.clipboardMutex); RTSemEventSignal(g_ctx.waitForData);