Changeset 97788 in vbox for trunk/src/VBox/HostServices/DragAndDrop

Timestamp:

Dec 12, 2022 6:36:50 PM (2 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

154859

Message:

DnD: Simplified / untangled reset handling:

• Don't implicitly reset the DnD state with the GuestDnDSendCtx / GuestDnDRecvCtx classes; that can cause side effects.
• Don't reset GuestDnDTargets state before sending data to the guest via GuestDnDTarget::sendData().
• Don't reset the DnD state within GuestDnDState::setProgress() anymore. This needs to be done explicitly by the caller waiting for the progress.
• Make sure to also clear the DnD state's callback map within GuestDnDState::reset().
• Implemented a default guest callback, which gets invoked when no other (registered) guest callbacks are in place (anymore).
• Return VERR_CANCELLED to VBoxTray / VBoxClient clients when there was a reset or the current operation was aborted.
• Release (2) log what has been reset.
• Implement reference counting for internal DnD messages within the host service; this should help wrt races between peeking for new client messages and actual retrieving those messages.

Location:

trunk/src/VBox/HostServices/DragAndDrop

Files:

• VBoxDragAndDropSvc.cpp (modified) (11 diffs)
• dndmanager.cpp (modified) (8 diffs)
• dndmanager.h (modified) (3 diffs)

trunk/src/VBox/HostServices/DragAndDrop/VBoxDragAndDropSvc.cpp

@@ -285 +285 @@
 
     /*
-     * Reset the message queue as soon as a new clients connect
+     * Reset the message queue as soon as a new client connects
      * to ensure that every client has the same state.
      */
     if (m_pManager)
-        m_pManager->Reset();
+        m_pManager->Reset(true /* fForce */);
 
     LogFlowFunc(("Client %RU32 connected (VINF_SUCCESS)\n", idClient));
@@ -574 +574 @@
                 if (cParms == 3)
                 {
-                    rc = m_pManager->GetNextMsgInfo(&paParms[0].u.uint32 /* uMsg */, &paParms[1].u.uint32 /* cParms */);
+                    /* Make sure to increase the reference count so that the next message doesn't get removed between
+                     * the guest's GUEST_DND_FN_GET_NEXT_HOST_MSG call and the actual message retrieval call. */
+                    rc = m_pManager->GetNextMsgInfo(true /* fAddRef */,
+                                                    &paParms[0].u.uint32 /* uMsg */, &paParms[1].u.uint32 /* cParms */);
                     if (RT_FAILURE(rc)) /* No queued messages available? */
                     {
@@ -589 +592 @@
                                 /* Note: paParms[2] was set by the guest as blocking flag. */
                             }
+
+                            LogFlowFunc(("Host callback returned %Rrc\n", rc));
                         }
                         else /* No host callback in place, so drag and drop is not supported by the host. */
@@ -1001 +1006 @@
                         data.paParms = paParms;
 
-                        rc = m_SvcCtx.pfnHostCallback(m_SvcCtx.pvHostData, u32Function,
-                                                      &data, sizeof(data));
+                        rc = m_SvcCtx.pfnHostCallback(m_SvcCtx.pvHostData, u32Function, &data, sizeof(data));
                         if (RT_SUCCESS(rc))
                         {
@@ -1010 +1014 @@
                         else
                         {
-                            /*
-                             * In case the guest is too fast asking for the next message
-                             * and the host did not supply it yet, just defer the client's
-                             * return until a response from the host available.
-                             */
-                            LogFlowFunc(("No new messages from the host (yet), deferring request: %Rrc\n", rc));
-                            rc = VINF_HGCM_ASYNC_EXECUTE;
+                            if (rc == VERR_CANCELLED)
+                            {
+                                /* Host indicated that the current operation was cancelled. Tell the guest. */
+                                LogFunc(("Host indicated that operation was cancelled\n", rc));
+                            }
+                            else
+                            {
+                                /*
+                                 * In case the guest is too fast asking for the next message
+                                 * and the host did not supply it yet, just defer the client's
+                                 * return until a response from the host available.
+                                 */
+                                LogFunc(("No new messages from the host (%Rrc), deferring request\n", rc));
+                                rc = VINF_HGCM_ASYNC_EXECUTE;
+                            }
                         }
                     }
@@ -1036 +1048 @@
     if (rc == VINF_HGCM_ASYNC_EXECUTE)
     {
-        LogFlowFunc(("Deferring client %RU32\n", idClient));
+        LogFunc(("Deferring client %RU32\n", idClient));
 
         try
@@ -1061 +1073 @@
     }
 
-    LogFlowFunc(("Returning rc=%Rrc\n", rc));
+    LogFunc(("Returning %Rrc to guest\n", rc));
 }
 
@@ -1170 +1182 @@
             LogFlowFunc(("Cancelling all waiting clients ...\n"));
 
-            /* Reset the message queue as the host cancelled the whole operation. */
-            m_pManager->Reset();
-
-            rc = m_pManager->AddMsg(u32Function, cParms, paParms, true /* fAppend */);
-            if (RT_FAILURE(rc))
-            {
-                AssertMsgFailed(("Adding new message of type=%RU32 failed with rc=%Rrc\n", u32Function, rc));
-                break;
-            }
+            /* Forcefully reset the message queue, as the host has cancelled the current operation. */
+            m_pManager->Reset(true /* fForce */);
 
             /*
@@ -1196 +1201 @@
                                                       /* Protocol v3+ also contains the context ID. */
                                                       pClient->uProtocolVerDeprecated >= 3 ? 1 : 0);
-                pClient->CompleteDeferred(rc2);
+                AssertRC(rc2);
+
+                /* Return VERR_CANCELLED when waking up the guest side. */
+                pClient->CompleteDeferred(VERR_CANCELLED);
 
                 m_clientQueue.erase(itQueue);
@@ -1212 +1220 @@
         {
             /* Reset the message queue as a new DnD operation just began. */
-            m_pManager->Reset();
+            m_pManager->Reset(false /* fForce */);
 
             fSendToGuest = true;
@@ -1272 +1280 @@
             uint32_t uMsgNext   = 0;
             uint32_t cParmsNext = 0;
-            int rcNext = m_pManager->GetNextMsgInfo(&uMsgNext, &cParmsNext);
+            /* Note: We only want to peek for the next message, hence fAddRef is false. */
+            int rcNext = m_pManager->GetNextMsgInfo(false /* fAddRef */, &uMsgNext, &cParmsNext);
 
             LogFlowFunc(("uMsgClient=%s (%#x), uMsgNext=%s (%#x), cParmsNext=%RU32, rcNext=%Rrc\n",

trunk/src/VBox/HostServices/DragAndDrop/dndmanager.cpp

@@ -112 +112 @@
         if (i > 0)
             Log((" - "));
-        uint32_t const uType = m_queueMsg[i]->GetType();
-        Log(("%s (%d / %#x)", DnDHostMsgToStr(uType), uType, uType));
+        DnDMessage const *pMsg = m_queueMsg[i];
+        uint32_t const uType = pMsg->GetType();
+        Log(("%s (%d / %#x) cRefS=%RU32", DnDHostMsgToStr(uType), uType, uType, pMsg->RefCount()));
     }
     Log(("\n"));
@@ -123 +124 @@
  *
  * @returns IPRT status code. VERR_NO_DATA if no next message is available.
+ * @param   fAddRef             Set to \c true to increase the message's reference count, or \c false if not.
  * @param   puType              Where to store the message type.
  * @param   pcParms             Where to store the message parameter count.
  */
-int DnDManager::GetNextMsgInfo(uint32_t *puType, uint32_t *pcParms)
+int DnDManager::GetNextMsgInfo(bool fAddRef, uint32_t *puType, uint32_t *pcParms)
 {
     AssertPtrReturn(puType, VERR_INVALID_POINTER);
@@ -133 +135 @@
     int rc;
 
-#ifdef DEBUG
-    DumpQueue();
-#endif
-
     if (m_queueMsg.isEmpty())
     {
@@ -149 +147 @@
         *pcParms = pMsg->GetParamCount();
 
+        if (fAddRef)
+            pMsg->AddRef();
+
         rc = VINF_SUCCESS;
     }
 
-    LogFlowFunc(("Returning uMsg=%s (%#x), cParms=%RU32, rc=%Rrc\n", DnDHostMsgToStr(*puType), *puType, *pcParms, rc));
+#ifdef DEBUG
+    DumpQueue();
+#endif
+
+    LogFlowFunc(("Returning uMsg=%s (%#x), cParms=%RU32, fAddRef=%RTbool, rc=%Rrc\n",
+                 DnDHostMsgToStr(*puType), *puType, *pcParms, fAddRef, rc));
     return rc;
 }
@@ -158 +164 @@
 /**
  * Retrieves the next queued up message and removes it from the queue on success.
- * Will return VERR_NO_DATA if no next message is available.
- *
- * @returns IPRT status code.
+ *
+ * @returns VBox status code.
+ * @retval  VERR_NO_DATA if no next message is available.
  * @param   uMsg                Message type to retrieve.
  * @param   cParms              Number of parameters the \@a paParms array can store.
@@ -173 +179 @@
         return VERR_NO_DATA;
 
+#ifdef DEBUG
+    DumpQueue();
+#endif
+
     /* Get the current message. */
     DnDMessage *pMsg = m_queueMsg.first();
     AssertPtr(pMsg);
 
-    m_queueMsg.removeFirst(); /* Remove the current message from the queue. */
+    if (pMsg->Release() == 0)     /* Not referenced by any client anymore? */
+        m_queueMsg.removeFirst(); /* Remove the current message from the queue. */
 
     /* Fetch the current message info. */
@@ -184 +195 @@
     /*
      * If there was an error handling the current message or the user has canceled
-     * the operation, we need to cleanup all pending events and inform the progress
-     * callback about our exit.
+     * the operation, we need to cleanup all pending events.
      */
     if (RT_FAILURE(rc))
     {
         /* Clear any pending messages. */
-        Reset();
-
-        /* Create a new cancel message to inform the guest + call
-         * the host whether the current transfer was canceled or aborted
-         * due to an error. */
-        try
-        {
-            if (rc == VERR_CANCELLED)
-                LogFlowFunc(("Operation was cancelled\n"));
-
-            DnDHGCancelMessage *pMsgCancel = new DnDHGCancelMessage();
-
-            int rc2 = AddMsg(pMsgCancel, false /* Prepend */);
-            AssertRC(rc2);
-
-            if (m_pfnProgressCallback)
-            {
-                LogFlowFunc(("Notifying host about aborting operation (%Rrc) ...\n", rc));
-                m_pfnProgressCallback(  rc == VERR_CANCELLED
-                                      ? DragAndDropSvc::DND_PROGRESS_CANCELLED
-                                      : DragAndDropSvc::DND_PROGRESS_ERROR,
-                                      100 /* Percent */, rc,
-                                      m_pvProgressUser);
-            }
-        }
-        catch(std::bad_alloc &)
-        {
-            rc = VERR_NO_MEMORY;
-        }
+        Reset(true /* fForce */);
     }
 
@@ -227 +209 @@
 /**
  * Resets the manager by clearing the message queue and internal state.
- */
-void DnDManager::Reset(void)
+ *
+ * @param   fForce              Set to \c true to forcefully also remove still referenced messages, or \c false to only
+ *                              remove non-referenced messages.
+ */
+void DnDManager::Reset(bool fForce)
 {
     LogFlowFuncEnter();
 
-    while (!m_queueMsg.isEmpty())
-    {
-        delete m_queueMsg.last();
-        m_queueMsg.removeLast();
-    }
-}
-
+#ifdef DEBUG
+    DumpQueue();
+#endif
+
+    for (size_t i = 0; i < m_queueMsg.size(); i++)
+    {
+        if (   fForce
+            || m_queueMsg[i]->RefCount() == 0)
+        {
+            m_queueMsg.removeAt(i);
+            i = i > 0 ? i - 1 : 0;
+        }
+    }
+}
+

trunk/src/VBox/HostServices/DragAndDrop/dndmanager.h

@@ -50 +50 @@
 
     DnDMessage(void)
-    {
-    }
+        : m_cRefs(0) { }
 
     DnDMessage(uint32_t uMsg, uint32_t cParms, VBOXHGCMSVCPARM aParms[])
-        : Message(uMsg, cParms, aParms) { }
+        : Message(uMsg, cParms, aParms)
+        , m_cRefs(0) { }
 
     virtual ~DnDMessage(void) { }
+
+    uint32_t AddRef(void) { Assert(m_cRefs < 32); return ++m_cRefs; }
+    uint32_t Release(void) { if (m_cRefs) return --m_cRefs; return m_cRefs; }
+    uint32_t RefCount(void) const { return m_cRefs; }
+
+protected:
+
+    /** The message's current reference count. */
+    uint32_t m_cRefs;
 };
 
@@ -100 +109 @@
     virtual ~DnDManager(void)
     {
-        Reset();
+        Reset(true /* fForce */);
     }
 
@@ -110 +119 @@
 #endif
 
-    int GetNextMsgInfo(uint32_t *puType, uint32_t *pcParms);
+    int GetNextMsgInfo(bool fAddRef, uint32_t *puType, uint32_t *pcParms);
     int GetNextMsg(uint32_t uMsg, uint32_t cParms, VBOXHGCMSVCPARM paParms[]);
 
-    void Reset(void);
+    void Reset(bool fForce);
 
 protected: