VirtualBox

Changeset 97788 in vbox


Ignore:
Timestamp:
Dec 12, 2022 6:36:50 PM (2 years ago)
Author:
vboxsync
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
Files:
7 edited

Legend:

Unmodified
Added
Removed

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

    r97763 r97788  
    285285
    286286    /*
    287      * Reset the message queue as soon as a new clients connect
     287     * Reset the message queue as soon as a new client connects
    288288     * to ensure that every client has the same state.
    289289     */
    290290    if (m_pManager)
    291         m_pManager->Reset();
     291        m_pManager->Reset(true /* fForce */);
    292292
    293293    LogFlowFunc(("Client %RU32 connected (VINF_SUCCESS)\n", idClient));
    … …  
    574574                if (cParms == 3)
    575575                {
    576                     rc = m_pManager->GetNextMsgInfo(&paParms[0].u.uint32 /* uMsg */, &paParms[1].u.uint32 /* cParms */);
     576                    /* Make sure to increase the reference count so that the next message doesn't get removed between
     577                     * the guest's GUEST_DND_FN_GET_NEXT_HOST_MSG call and the actual message retrieval call. */
     578                    rc = m_pManager->GetNextMsgInfo(true /* fAddRef */,
     579                                                    &paParms[0].u.uint32 /* uMsg */, &paParms[1].u.uint32 /* cParms */);
    577580                    if (RT_FAILURE(rc)) /* No queued messages available? */
    578581                    {
    … …  
    589592                                /* Note: paParms[2] was set by the guest as blocking flag. */
    590593                            }
     594
     595                            LogFlowFunc(("Host callback returned %Rrc\n", rc));
    591596                        }
    592597                        else /* No host callback in place, so drag and drop is not supported by the host. */
    … …  
    10011006                        data.paParms = paParms;
    10021007
    1003                         rc = m_SvcCtx.pfnHostCallback(m_SvcCtx.pvHostData, u32Function,
    1004                                                       &data, sizeof(data));
     1008                        rc = m_SvcCtx.pfnHostCallback(m_SvcCtx.pvHostData, u32Function, &data, sizeof(data));
    10051009                        if (RT_SUCCESS(rc))
    10061010                        {
    … …  
    10101014                        else
    10111015                        {
    1012                             /*
    1013                              * In case the guest is too fast asking for the next message
    1014                              * and the host did not supply it yet, just defer the client's
    1015                              * return until a response from the host available.
    1016                              */
    1017                             LogFlowFunc(("No new messages from the host (yet), deferring request: %Rrc\n", rc));
    1018                             rc = VINF_HGCM_ASYNC_EXECUTE;
     1016                            if (rc == VERR_CANCELLED)
     1017                            {
     1018                                /* Host indicated that the current operation was cancelled. Tell the guest. */
     1019                                LogFunc(("Host indicated that operation was cancelled\n", rc));
     1020                            }
     1021                            else
     1022                            {
     1023                                /*
     1024                                 * In case the guest is too fast asking for the next message
     1025                                 * and the host did not supply it yet, just defer the client's
     1026                                 * return until a response from the host available.
     1027                                 */
     1028                                LogFunc(("No new messages from the host (%Rrc), deferring request\n", rc));
     1029                                rc = VINF_HGCM_ASYNC_EXECUTE;
     1030                            }
    10191031                        }
    10201032                    }
    … …  
    10361048    if (rc == VINF_HGCM_ASYNC_EXECUTE)
    10371049    {
    1038         LogFlowFunc(("Deferring client %RU32\n", idClient));
     1050        LogFunc(("Deferring client %RU32\n", idClient));
    10391051
    10401052        try
    … …  
    10611073    }
    10621074
    1063     LogFlowFunc(("Returning rc=%Rrc\n", rc));
     1075    LogFunc(("Returning %Rrc to guest\n", rc));
    10641076}
    10651077
    … …  
    11701182            LogFlowFunc(("Cancelling all waiting clients ...\n"));
    11711183
    1172             /* Reset the message queue as the host cancelled the whole operation. */
    1173             m_pManager->Reset();
    1174 
    1175             rc = m_pManager->AddMsg(u32Function, cParms, paParms, true /* fAppend */);
    1176             if (RT_FAILURE(rc))
    1177             {
    1178                 AssertMsgFailed(("Adding new message of type=%RU32 failed with rc=%Rrc\n", u32Function, rc));
    1179                 break;
    1180             }
     1184            /* Forcefully reset the message queue, as the host has cancelled the current operation. */
     1185            m_pManager->Reset(true /* fForce */);
    11811186
    11821187            /*
    … …  
    11961201                                                      /* Protocol v3+ also contains the context ID. */
    11971202                                                      pClient->uProtocolVerDeprecated >= 3 ? 1 : 0);
    1198                 pClient->CompleteDeferred(rc2);
     1203                AssertRC(rc2);
     1204
     1205                /* Return VERR_CANCELLED when waking up the guest side. */
     1206                pClient->CompleteDeferred(VERR_CANCELLED);
    11991207
    12001208                m_clientQueue.erase(itQueue);
    … …  
    12121220        {
    12131221            /* Reset the message queue as a new DnD operation just began. */
    1214             m_pManager->Reset();
     1222            m_pManager->Reset(false /* fForce */);
    12151223
    12161224            fSendToGuest = true;
    … …  
    12721280            uint32_t uMsgNext   = 0;
    12731281            uint32_t cParmsNext = 0;
    1274             int rcNext = m_pManager->GetNextMsgInfo(&uMsgNext, &cParmsNext);
     1282            /* Note: We only want to peek for the next message, hence fAddRef is false. */
     1283            int rcNext = m_pManager->GetNextMsgInfo(false /* fAddRef */, &uMsgNext, &cParmsNext);
    12751284
    12761285            LogFlowFunc(("uMsgClient=%s (%#x), uMsgNext=%s (%#x), cParmsNext=%RU32, rcNext=%Rrc\n",

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

    r97747 r97788  
    112112        if (i > 0)
    113113            Log((" - "));
    114         uint32_t const uType = m_queueMsg[i]->GetType();
    115         Log(("%s (%d / %#x)", DnDHostMsgToStr(uType), uType, uType));
     114        DnDMessage const *pMsg = m_queueMsg[i];
     115        uint32_t const uType = pMsg->GetType();
     116        Log(("%s (%d / %#x) cRefS=%RU32", DnDHostMsgToStr(uType), uType, uType, pMsg->RefCount()));
    116117    }
    117118    Log(("\n"));
    … …  
    123124 *
    124125 * @returns IPRT status code. VERR_NO_DATA if no next message is available.
     126 * @param   fAddRef             Set to \c true to increase the message's reference count, or \c false if not.
    125127 * @param   puType              Where to store the message type.
    126128 * @param   pcParms             Where to store the message parameter count.
    127129 */
    128 int DnDManager::GetNextMsgInfo(uint32_t *puType, uint32_t *pcParms)
     130int DnDManager::GetNextMsgInfo(bool fAddRef, uint32_t *puType, uint32_t *pcParms)
    129131{
    130132    AssertPtrReturn(puType, VERR_INVALID_POINTER);
    … …  
    133135    int rc;
    134136
    135 #ifdef DEBUG
    136     DumpQueue();
    137 #endif
    138 
    139137    if (m_queueMsg.isEmpty())
    140138    {
    … …  
    149147        *pcParms = pMsg->GetParamCount();
    150148
     149        if (fAddRef)
     150            pMsg->AddRef();
     151
    151152        rc = VINF_SUCCESS;
    152153    }
    153154
    154     LogFlowFunc(("Returning uMsg=%s (%#x), cParms=%RU32, rc=%Rrc\n", DnDHostMsgToStr(*puType), *puType, *pcParms, rc));
     155#ifdef DEBUG
     156    DumpQueue();
     157#endif
     158
     159    LogFlowFunc(("Returning uMsg=%s (%#x), cParms=%RU32, fAddRef=%RTbool, rc=%Rrc\n",
     160                 DnDHostMsgToStr(*puType), *puType, *pcParms, fAddRef, rc));
    155161    return rc;
    156162}
    … …  
    158164/**
    159165 * Retrieves the next queued up message and removes it from the queue on success.
    160  * Will return VERR_NO_DATA if no next message is available.
    161  *
    162  * @returns IPRT status code.
     166 *
     167 * @returns VBox status code.
     168 * @retval  VERR_NO_DATA if no next message is available.
    163169 * @param   uMsg                Message type to retrieve.
    164170 * @param   cParms              Number of parameters the \@a paParms array can store.
    … …  
    173179        return VERR_NO_DATA;
    174180
     181#ifdef DEBUG
     182    DumpQueue();
     183#endif
     184
    175185    /* Get the current message. */
    176186    DnDMessage *pMsg = m_queueMsg.first();
    177187    AssertPtr(pMsg);
    178188
    179     m_queueMsg.removeFirst(); /* Remove the current message from the queue. */
     189    if (pMsg->Release() == 0)     /* Not referenced by any client anymore? */
     190        m_queueMsg.removeFirst(); /* Remove the current message from the queue. */
    180191
    181192    /* Fetch the current message info. */
    … …  
    184195    /*
    185196     * If there was an error handling the current message or the user has canceled
    186      * the operation, we need to cleanup all pending events and inform the progress
    187      * callback about our exit.
     197     * the operation, we need to cleanup all pending events.
    188198     */
    189199    if (RT_FAILURE(rc))
    190200    {
    191201        /* Clear any pending messages. */
    192         Reset();
    193 
    194         /* Create a new cancel message to inform the guest + call
    195          * the host whether the current transfer was canceled or aborted
    196          * due to an error. */
    197         try
    198         {
    199             if (rc == VERR_CANCELLED)
    200                 LogFlowFunc(("Operation was cancelled\n"));
    201 
    202             DnDHGCancelMessage *pMsgCancel = new DnDHGCancelMessage();
    203 
    204             int rc2 = AddMsg(pMsgCancel, false /* Prepend */);
    205             AssertRC(rc2);
    206 
    207             if (m_pfnProgressCallback)
    208             {
    209                 LogFlowFunc(("Notifying host about aborting operation (%Rrc) ...\n", rc));
    210                 m_pfnProgressCallback(  rc == VERR_CANCELLED
    211                                       ? DragAndDropSvc::DND_PROGRESS_CANCELLED
    212                                       : DragAndDropSvc::DND_PROGRESS_ERROR,
    213                                       100 /* Percent */, rc,
    214                                       m_pvProgressUser);
    215             }
    216         }
    217         catch(std::bad_alloc &)
    218         {
    219             rc = VERR_NO_MEMORY;
    220         }
     202        Reset(true /* fForce */);
    221203    }
    222204
    … …  
    227209/**
    228210 * Resets the manager by clearing the message queue and internal state.
    229  */
    230 void DnDManager::Reset(void)
     211 *
     212 * @param   fForce              Set to \c true to forcefully also remove still referenced messages, or \c false to only
     213 *                              remove non-referenced messages.
     214 */
     215void DnDManager::Reset(bool fForce)
    231216{
    232217    LogFlowFuncEnter();
    233218
    234     while (!m_queueMsg.isEmpty())
    235     {
    236         delete m_queueMsg.last();
    237         m_queueMsg.removeLast();
    238     }
    239 }
    240 
     219#ifdef DEBUG
     220    DumpQueue();
     221#endif
     222
     223    for (size_t i = 0; i < m_queueMsg.size(); i++)
     224    {
     225        if (   fForce
     226            || m_queueMsg[i]->RefCount() == 0)
     227        {
     228            m_queueMsg.removeAt(i);
     229            i = i > 0 ? i - 1 : 0;
     230        }
     231    }
     232}
     233

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

    r97731 r97788  
    5050
    5151    DnDMessage(void)
    52     {
    53     }
     52        : m_cRefs(0) { }
    5453
    5554    DnDMessage(uint32_t uMsg, uint32_t cParms, VBOXHGCMSVCPARM aParms[])
    56         : Message(uMsg, cParms, aParms) { }
     55        : Message(uMsg, cParms, aParms)
     56        , m_cRefs(0) { }
    5757
    5858    virtual ~DnDMessage(void) { }
     59
     60    uint32_t AddRef(void) { Assert(m_cRefs < 32); return ++m_cRefs; }
     61    uint32_t Release(void) { if (m_cRefs) return --m_cRefs; return m_cRefs; }
     62    uint32_t RefCount(void) const { return m_cRefs; }
     63
     64protected:
     65
     66    /** The message's current reference count. */
     67    uint32_t m_cRefs;
    5968};
    6069
    … …  
    100109    virtual ~DnDManager(void)
    101110    {
    102         Reset();
     111        Reset(true /* fForce */);
    103112    }
    104113
    … …  
    110119#endif
    111120
    112     int GetNextMsgInfo(uint32_t *puType, uint32_t *pcParms);
     121    int GetNextMsgInfo(bool fAddRef, uint32_t *puType, uint32_t *pcParms);
    113122    int GetNextMsg(uint32_t uMsg, uint32_t cParms, VBOXHGCMSVCPARM paParms[]);
    114123
    115     void Reset(void);
     124    void Reset(bool fForce);
    116125
    117126protected:

  • trunk/src/VBox/Main/include/GuestDnDPrivate.h

    r97786 r97788  
    844844    void reset(void);
    845845
     846    /** @name Callback handling.
     847     * @{ */
     848    static DECLCALLBACK(int) i_defaultCallback(uint32_t uMsg, void *pvParms, size_t cbParms, void *pvUser);
     849    int setCallback(uint32_t uMsg, PFNGUESTDNDCALLBACK pfnCallback, void *pvUser = NULL);
     850    /** @} */
     851
     852    /** @name Progress handling.
     853     * @{ */
    846854    bool isProgressCanceled(void) const;
    847     int setCallback(uint32_t uMsg, PFNGUESTDNDCALLBACK pfnCallback, void *pvUser = NULL);
    848855    int setProgress(unsigned uPercentage, uint32_t uState, int rcOp = VINF_SUCCESS, const Utf8Str &strMsg = "");
    849856    HRESULT resetProgress(const ComObjPtr<Guest>& pParent);
    850857    HRESULT queryProgressTo(IProgress **ppProgress);
     858    /** @} */
    851859
    852860public:

  • trunk/src/VBox/Main/src-client/GuestDnDPrivate.cpp

    r97784 r97788  
    204204void GuestDnDSendCtx::reset(void)
    205205{
    206     if (pState)
    207         pState->reset();
    208 
    209206    uScreenID  = 0;
    210207
    … …  
    233230void GuestDnDRecvCtx::reset(void)
    234231{
    235     if (pState)
    236         pState->reset();
    237 
    238232    lstFmtOffered.clear();
    239233    strFmtReq  = "";
    … …  
    354348
    355349    m_lstFormats.clear();
     350    m_mapCallbacks.clear();
    356351
    357352    m_rcGuest = VERR_IPE_UNINITIALIZED_STATUS;
     353}
     354
     355/**
     356 * Default callback handler for guest callbacks.
     357 *
     358 * This handler acts as a fallback in case important callback messages are not being handled
     359 * by the specific callers.
     360 *
     361 * @returns VBox status code. Will get sent back to the host service.
     362 * @retval  VERR_CANCELLED for indicating that the current operation was cancelled.
     363 * @param   uMsg                HGCM message ID (function number).
     364 * @param   pvParms             Pointer to additional message data. Optional and can be NULL.
     365 * @param   cbParms             Size (in bytes) additional message data. Optional and can be 0.
     366 * @param   pvUser              User-supplied pointer on callback registration.
     367 */
     368/* static */
     369DECLCALLBACK(int) GuestDnDState::i_defaultCallback(uint32_t uMsg, void *pvParms, size_t cbParms, void *pvUser)
     370{
     371    GuestDnDState *pThis = (GuestDnDState *)pvUser;
     372    AssertPtrReturn(pThis, VERR_INVALID_POINTER);
     373
     374    LogFlowFunc(("uMsg=%RU32 (%#x)\n", uMsg, uMsg));
     375
     376    int vrc = VERR_IPE_UNINITIALIZED_STATUS;
     377
     378    switch (uMsg)
     379    {
     380        case GUEST_DND_FN_EVT_ERROR:
     381        {
     382            PVBOXDNDCBEVTERRORDATA pCBData = reinterpret_cast<PVBOXDNDCBEVTERRORDATA>(pvParms);
     383            AssertPtr(pCBData);
     384            AssertReturn(sizeof(VBOXDNDCBEVTERRORDATA) == cbParms, VERR_INVALID_PARAMETER);
     385            AssertReturn(CB_MAGIC_DND_EVT_ERROR == pCBData->hdr.uMagic, VERR_INVALID_PARAMETER);
     386
     387            if (RT_SUCCESS(pCBData->rc))
     388            {
     389                AssertMsgFailed(("Guest has sent an error event but did not specify an actual error code\n"));
     390                pCBData->rc = VERR_GENERAL_FAILURE; /* Make sure some error is set. */
     391            }
     392
     393            vrc = pThis->setProgress(100, DND_PROGRESS_ERROR, pCBData->rc,
     394                                     Utf8StrFmt("Received error from guest: %Rrc", pCBData->rc));
     395            AssertRCBreak(vrc);
     396            vrc = pThis->notifyAboutGuestResponse(pCBData->rc);
     397            AssertRCBreak(vrc);
     398            break;
     399        }
     400
     401        default:
     402            AssertMsgBreakStmt(pThis->isProgressCanceled(), ("Transfer not cancelled (yet)!\n"), vrc = VERR_INVALID_STATE);
     403            vrc = VERR_CANCELLED;
     404            break;
     405    }
     406
     407    LogFlowFunc(("Returning rc=%Rrc\n", vrc));
     408    return vrc;
    358409}
    359410
    … …  
    399450
    400451/**
    401  * Sets a callback for a specific HGCM message.
     452 * Sets (registers or unregisters) a callback for a specific HGCM message.
    402453 *
    403454 * @returns VBox status code.
    404455 * @param   uMsg                HGCM message ID to set callback for.
    405  * @param   pfnCallback         Callback function pointer to use.
     456 * @param   pfnCallback         Callback function pointer to use. Pass NULL to unregister.
    406457 * @param   pvUser              User-provided arguments for the callback function. Optional and can be NULL.
    407458 */
    … …  
    410461    GuestDnDCallbackMap::iterator it = m_mapCallbacks.find(uMsg);
    411462
    412     /* Add. */
     463    /* Register. */
    413464    if (pfnCallback)
    414465    {
    415         if (it == m_mapCallbacks.end())
     466        try
    416467        {
    417468            m_mapCallbacks[uMsg] = GuestDnDCallback(pfnCallback, uMsg, pvUser);
    418             return VINF_SUCCESS;
    419         }
    420 
    421         AssertMsgFailed(("Callback for message %RU32 already registered\n", uMsg));
    422         return VERR_ALREADY_EXISTS;
    423     }
    424 
    425     /* Remove. */
     469        }
     470        catch (std::bad_alloc &)
     471        {
     472            return VERR_NO_MEMORY;
     473        }
     474        return VINF_SUCCESS;
     475    }
     476
     477    /* Unregister. */
    426478    if (it != m_mapCallbacks.end())
    427479        m_mapCallbacks.erase(it);
    … …  
    472524                                                   COM_IIDOF(IGuest),
    473525                                                   m_pParent->getComponentName(), strMsg.c_str());
    474             reset();
    475526            break;
    476527        }
    … …  
    491542                AssertComRC(hr);
    492543            }
    493 
    494             reset();
    495544            break;
    496545        }
    … …  
    574623            break;
    575624        }
     625
     626        /* Note: GUEST_DND_FN_EVT_ERROR is handled in either the state's default callback or in specific
     627         *       (overriden) callbacks (e.g. GuestDnDSendCtx / GuestDnDRecvCtx). */
    576628
    577629        case DragAndDropSvc::GUEST_DND_FN_DISCONNECT:
    … …  
    694746        else
    695747        {
    696             LogFlowFunc(("No callback for function %RU32 defined\n", u32Function));
    697             rc = VERR_NOT_SUPPORTED; /* Tell the guest. */
     748            /* Invoke the default callback handler in case we don't have any registered callback above. */
     749            rc = i_defaultCallback(u32Function, pvParms, cbParms, this /* pvUser */);
    698750        }
    699751    }

  • trunk/src/VBox/Main/src-client/GuestDnDSourceImpl.cpp

    r97784 r97788  
    617617void GuestDnDSource::i_reset(void)
    618618{
    619     LogFlowThisFunc(("\n"));
     619    LogRel2(("DnD: Source reset\n"));
    620620
    621621    mData.mRecvCtx.reset();

  • trunk/src/VBox/Main/src-client/GuestDnDTargetImpl.cpp

    r97784 r97788  
    107107            return;
    108108
    109         int vrc = pThis->i_sendData(mpCtx, RT_INDEFINITE_WAIT /* msTimeout */);
    110         if (RT_FAILURE(vrc)) /* In case we missed some error handling within i_sendData(). */
    111         {
    112             if (vrc != VERR_CANCELLED)
    113                 LogRel(("DnD: Sending data to guest failed with %Rrc\n", vrc));
    114 
    115             /* Make sure to fire a cancel request to the guest side in case something went wrong. */
    116             pThis->sendCancel();
    117         }
     109        /* ignore rc */ pThis->i_sendData(mpCtx, RT_INDEFINITE_WAIT /* msTimeout */);
    118110    }
    119111
    … …  
    720712        return setError(E_FAIL, tr("Current drop operation to guest still in progress"));
    721713
    722     /* Reset our internal state. */
    723     i_reset();
    724 
    725714    /* At the moment we only support one transfer at a time. */
    726715    if (GuestDnDInst()->getTargetCount())
    … …  
    878867void GuestDnDTarget::i_reset(void)
    879868{
    880     LogFlowThisFunc(("\n"));
     869    LogRel2(("DnD: Target reset\n"));
    881870
    882871    mData.mSendCtx.reset();
Note: See TracChangeset for help on using the changeset viewer.

© 2024 Oracle Support Privacy / Do Not Sell My Info Terms of Use Trademark Policy Automated Access Etiquette