Changeset 39720 in vbox

Timestamp:

Jan 7, 2012 2:56:25 AM (13 years ago)

Author:

vboxsync

Message:

ConsoleImpl.cpp: Try avoid holding the console lock (read or write) while firing off events.

Location:

trunk/src/VBox/Main

Files:

• include/ConsoleImpl.h (modified) (1 diff)
• src-client/ConsoleImpl.cpp (modified) (33 diffs)

trunk/src/VBox/Main/include/ConsoleImpl.h

@@ -694 +694 @@
     const ComObjPtr<MachineDebugger> mDebugger;
     const ComObjPtr<VRDEServerInfo> mVRDEServerInfo;
+    /** This can safely be used without holding any locks.
+     * An AutoCaller suffices to prevent it being destroy while in use and
+     * internally there is a lock providing the necessary serialization. */
     const ComObjPtr<EventSource> mEventSource;
 #ifdef VBOX_WITH_EXTPACK

trunk/src/VBox/Main/src-client/ConsoleImpl.cpp

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2011 Oracle Corporation
+ * Copyright (C) 2006-2012 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -1853 +1853 @@
 
     AutoCaller autoCaller(this);
-    if (FAILED(autoCaller.rc())) return autoCaller.rc();
-
-    // no need to lock - lifetime constant
-    mEventSource.queryInterfaceTo(aEventSource);
-
-    return S_OK;
+    HRESULT hrc = autoCaller.rc();
+    if (SUCCEEDED(hrc))
+    {
+        // no need to lock - lifetime constant
+        mEventSource.queryInterfaceTo(aEventSource);
+    }
+
+    return hrc;
 }
 
@@ -3100 +3102 @@
     m_mapSharedFolders.insert(std::make_pair(aName, pSharedFolder));
 
-    /* notify console callbacks after the folder is added to the list */
+    /* Notify console callbacks after the folder is added to the list. */
+    alock.release();
     fireSharedFolderChangedEvent(mEventSource, Scope_Session);
 
@@ -3165 +3168 @@
     m_mapSharedFolders.erase(strName);
 
-    /* notify console callbacks after the folder is removed to the list */
+    /* Notify console callbacks after the folder is removed from the list. */
+    alock.release();
     fireSharedFolderChangedEvent(mEventSource, Scope_Session);
 
@@ -4352 +4356 @@
     /* notify console callbacks on success */
     if (SUCCEEDED(rc))
+    {
+        alock.release(); /** @todo 101% safe? */
         fireNetworkAdapterChangedEvent(mEventSource, aNetworkAdapter);
+    }
 
     LogFlowThisFunc(("Leaving rc=%#x\n", rc));
@@ -4639 +4646 @@
 /**
  * Called by IInternalSessionControl::OnSerialPortChange().
- *
- * @note Locks this object for writing.
  */
 HRESULT Console::onSerialPortChange(ISerialPort *aSerialPort)
@@ -4649 +4654 @@
     AssertComRCReturnRC(autoCaller.rc());
 
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    HRESULT rc = S_OK;
-
-    /* don't trigger serial port change if the VM isn't running */
-    SafeVMPtrQuiet ptrVM(this);
-    if (ptrVM.isOk())
-    {
-        /* nothing to do so far */
-        ptrVM.release();
-    }
-
-    /* notify console callbacks on success */
-    if (SUCCEEDED(rc))
-        fireSerialPortChangedEvent(mEventSource, aSerialPort);
-
-    LogFlowThisFunc(("Leaving rc=%#x\n", rc));
-    return rc;
+    fireSerialPortChangedEvent(mEventSource, aSerialPort);
+
+    LogFlowThisFunc(("Leaving rc=%#x\n", S_OK));
+    return S_OK;
 }
 
 /**
  * Called by IInternalSessionControl::OnParallelPortChange().
- *
- * @note Locks this object for writing.
  */
 HRESULT Console::onParallelPortChange(IParallelPort *aParallelPort)
@@ -4681 +4670 @@
     AssertComRCReturnRC(autoCaller.rc());
 
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    HRESULT rc = S_OK;
-
-    /* don't trigger parallel port change if the VM isn't running */
-    SafeVMPtrQuiet ptrVM(this);
-    if (ptrVM.isOk())
-    {
-        /* nothing to do so far */
-        ptrVM.release();
-    }
-
-    /* notify console callbacks on success */
-    if (SUCCEEDED(rc))
-        fireParallelPortChangedEvent(mEventSource, aParallelPort);
-
-    LogFlowThisFunc(("Leaving rc=%#x\n", rc));
-    return rc;
+    fireParallelPortChangedEvent(mEventSource, aParallelPort);
+
+    LogFlowThisFunc(("Leaving rc=%#x\n", S_OK));
+    return S_OK;
 }
 
 /**
  * Called by IInternalSessionControl::OnStorageControllerChange().
- *
- * @note Locks this object for writing.
  */
 HRESULT Console::onStorageControllerChange()
@@ -4713 +4686 @@
     AssertComRCReturnRC(autoCaller.rc());
 
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    HRESULT rc = S_OK;
-
-    /* don't trigger storage controller change if the VM isn't running */
-    SafeVMPtrQuiet ptrVM(this);
-    if (ptrVM.isOk())
-    {
-        /* nothing to do so far */
-        ptrVM.release();
-    }
-
-    /* notify console callbacks on success */
-    if (SUCCEEDED(rc))
-        fireStorageControllerChangedEvent(mEventSource);
-
-    LogFlowThisFunc(("Leaving rc=%#x\n", rc));
-    return rc;
+    fireStorageControllerChangedEvent(mEventSource);
+
+    LogFlowThisFunc(("Leaving rc=%#x\n", S_OK));
+    return S_OK;
 }
 
@@ -4759 +4718 @@
     /* notify console callbacks on success */
     if (SUCCEEDED(rc))
+    {
+        alock.release(); /** @todo 101% safe? */
         fireMediumChangedEvent(mEventSource, aMediumAttachment);
+    }
 
     LogFlowThisFunc(("Leaving rc=%#x\n", rc));
@@ -4833 +4795 @@
     /* notify console callbacks on success */
     if (SUCCEEDED(rc))
+    {
+        alock.release();
         fireCPUExecutionCapChangedEvent(mEventSource, aExecutionCap);
+    }
 
     LogFlowThisFunc(("Leaving rc=%#x\n", rc));
@@ -4893 +4858 @@
     /* notify console callbacks on success */
     if (SUCCEEDED(rc))
+    {
+        alock.release();
         fireVRDEServerChangedEvent(mEventSource);
+    }
 
     return rc;
 }
 
-/**
- * @note Locks this object for reading.
- */
 void Console::onVRDEServerInfoChange()
 {
@@ -4906 +4871 @@
     AssertComRCReturnVoid(autoCaller.rc());
 
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-
     fireVRDEServerInfoChangedEvent(mEventSource);
 }
@@ -4914 +4877 @@
 /**
  * Called by IInternalSessionControl::OnUSBControllerChange().
- *
- * @note Locks this object for writing.
  */
 HRESULT Console::onUSBControllerChange()
@@ -4924 +4885 @@
     AssertComRCReturnRC(autoCaller.rc());
 
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    HRESULT rc = S_OK;
-
-    /* don't trigger USB controller change if the VM isn't running */
-    SafeVMPtrQuiet ptrVM(this);
-    if (ptrVM.isOk())
-    {
-        /// @todo implement one day.
-        // Anyway, if we want to query the machine's USB Controller we need
-        // to cache it to mUSBController in #init() (similar to mDVDDrive).
-        //
-        // bird: While the VM supports hot-plugging, I doubt any guest can
-        // handle it at this time... :-)
-
-        /* nothing to do so far */
-        ptrVM.release();
-    }
-
-    /* notify console callbacks on success */
-    if (SUCCEEDED(rc))
-        fireUSBControllerChangedEvent(mEventSource);
-
-    return rc;
+    fireUSBControllerChangedEvent(mEventSource);
+
+    return S_OK;
 }
 
@@ -4969 +4909 @@
     if (SUCCEEDED(rc))
     {
+        alock.release();
         fireSharedFolderChangedEvent(mEventSource, aGlobal ? (Scope_T)Scope_Global : (Scope_T)Scope_Machine);
     }
@@ -5179 +5120 @@
     /* notify console callbacks on success */
     if (SUCCEEDED(rc))
+    {
+        alock.release();
         fireBandwidthGroupChangedEvent(mEventSource, aBandwidthGroup);
+    }
 
     LogFlowThisFunc(("Leaving rc=%#x\n", rc));
@@ -5214 +5158 @@
     /* notify console callbacks on success */
     if (SUCCEEDED(rc))
+    {
+        alock.release(); /** @todo 101% safe? */
         fireStorageDeviceChangedEvent(mEventSource, aMediumAttachment, aRemove);
+    }
 
     LogFlowThisFunc(("Leaving rc=%#x\n", rc));
@@ -5685 +5632 @@
 }
 
+#ifdef CONSOLE_WITH_EVENT_CACHE
 /**
  * @note Locks this object for writing.
  */
+#endif
 void Console::onMousePointerShapeChange(bool fVisible, bool fAlpha,
                                         uint32_t xHot, uint32_t yHot,
@@ -5702 +5651 @@
     AssertComRCReturnVoid(autoCaller.rc());
 
-#ifndef CONSOLE_WITH_EVENT_CACHE
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-#else
-    /* We need a write lock because we alter the cached callback data */
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    /* Save the callback arguments */
-    mCallbackData.mpsc.visible = fVisible;
-    mCallbackData.mpsc.alpha = fAlpha;
-    mCallbackData.mpsc.xHot = xHot;
-    mCallbackData.mpsc.yHot = yHot;
-    mCallbackData.mpsc.width = width;
-    mCallbackData.mpsc.height = height;
-
-    /* start with not valid */
-    bool wasValid = mCallbackData.mpsc.valid;
-    mCallbackData.mpsc.valid = false;
-
-    com::SafeArray<BYTE> aShape(ComSafeArrayInArg(pShape));
-    if (aShape.size() != 0)
-        mCallbackData.mpsc.shape.initFrom(aShape);
-    else
-        mCallbackData.mpsc.shape.resize(0);
-    mCallbackData.mpsc.valid = true;
+#ifdef CONSOLE_WITH_EVENT_CACHE
+    {
+        /* We need a write lock because we alter the cached callback data */
+        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+        /* Save the callback arguments */
+        mCallbackData.mpsc.visible = fVisible;
+        mCallbackData.mpsc.alpha = fAlpha;
+        mCallbackData.mpsc.xHot = xHot;
+        mCallbackData.mpsc.yHot = yHot;
+        mCallbackData.mpsc.width = width;
+        mCallbackData.mpsc.height = height;
+
+        /* start with not valid */
+        bool wasValid = mCallbackData.mpsc.valid;
+        mCallbackData.mpsc.valid = false;
+
+        com::SafeArray<BYTE> aShape(ComSafeArrayInArg(pShape));
+        if (aShape.size() != 0)
+            mCallbackData.mpsc.shape.initFrom(aShape);
+        else
+            mCallbackData.mpsc.shape.resize(0);
+        mCallbackData.mpsc.valid = true;
+    }
 #endif
 
@@ -5735 +5684 @@
 }
 
+#ifdef CONSOLE_WITH_EVENT_CACHE
 /**
  * @note Locks this object for writing.
  */
+#endif
 void Console::onMouseCapabilityChange(BOOL supportsAbsolute, BOOL supportsRelative, BOOL needsHostCursor)
 {
@@ -5746 +5697 @@
     AssertComRCReturnVoid(autoCaller.rc());
 
-#ifndef CONSOLE_WITH_EVENT_CACHE
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-#else
-    /* We need a write lock because we alter the cached callback data */
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    /* save the callback arguments */
-    mCallbackData.mcc.supportsAbsolute = supportsAbsolute;
-    mCallbackData.mcc.supportsRelative = supportsRelative;
-    mCallbackData.mcc.needsHostCursor = needsHostCursor;
-    mCallbackData.mcc.valid = true;
+#ifdef CONSOLE_WITH_EVENT_CACHE
+    {
+        /* We need a write lock because we alter the cached callback data */
+        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+        /* save the callback arguments */
+        mCallbackData.mcc.supportsAbsolute = supportsAbsolute;
+        mCallbackData.mcc.supportsRelative = supportsRelative;
+        mCallbackData.mcc.needsHostCursor = needsHostCursor;
+        mCallbackData.mcc.valid = true;
+    }
 #endif
 
@@ -5762 +5713 @@
 }
 
-/**
- * @note Locks this object for reading.
- */
 void Console::onStateChange(MachineState_T machineState)
 {
@@ -5770 +5718 @@
     AssertComRCReturnVoid(autoCaller.rc());
 
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
     fireStateChangedEvent(mEventSource, machineState);
 }
 
-/**
- * @note Locks this object for reading.
- */
 void Console::onAdditionsStateChange()
 {
@@ -5782 +5726 @@
     AssertComRCReturnVoid(autoCaller.rc());
 
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
     fireAdditionsStateChangedEvent(mEventSource);
 }
 
 /**
- * @note Locks this object for reading.
- *       This notification only is for reporting an incompatible
- *       Guest Additions interface, *not* the Guest Additions version!
- *
- *       The user will be notified inside the guest if new Guest
- *       Additions are available (via VBoxTray/VBoxClient).
+ * @remarks This notification only is for reporting an incompatible
+ *          Guest Additions interface, *not* the Guest Additions version!
+ *
+ *          The user will be notified inside the guest if new Guest
+ *          Additions are available (via VBoxTray/VBoxClient).
  */
 void Console::onAdditionsOutdated()
@@ -5799 +5741 @@
     AssertComRCReturnVoid(autoCaller.rc());
 
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-}
-
+    /** @todo implement this */
+}
+
+#ifdef CONSOLE_WITH_EVENT_CACHE
 /**
  * @note Locks this object for writing.
  */
+#endif
 void Console::onKeyboardLedsChange(bool fNumLock, bool fCapsLock, bool fScrollLock)
 {
@@ -5810 +5754 @@
     AssertComRCReturnVoid(autoCaller.rc());
 
-#ifndef CONSOLE_WITH_EVENT_CACHE
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-#else
-    /* We need a write lock because we alter the cached callback data */
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    /* save the callback arguments */
-    mCallbackData.klc.numLock = fNumLock;
-    mCallbackData.klc.capsLock = fCapsLock;
-    mCallbackData.klc.scrollLock = fScrollLock;
-    mCallbackData.klc.valid = true;
+#ifdef CONSOLE_WITH_EVENT_CACHE
+    {
+        /* We need a write lock because we alter the cached callback data */
+        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+        /* save the callback arguments */
+        mCallbackData.klc.numLock = fNumLock;
+        mCallbackData.klc.capsLock = fCapsLock;
+        mCallbackData.klc.scrollLock = fScrollLock;
+        mCallbackData.klc.valid = true;
+    }
 #endif
 
@@ -5826 +5770 @@
 }
 
-/**
- * @note Locks this object for reading.
- */
 void Console::onUSBDeviceStateChange(IUSBDevice *aDevice, bool aAttached,
                                      IVirtualBoxErrorInfo *aError)
@@ -5835 +5776 @@
     AssertComRCReturnVoid(autoCaller.rc());
 
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
     fireUSBDeviceStateChangedEvent(mEventSource, aDevice, aAttached, aError);
 }
 
-/**
- * @note Locks this object for reading.
- */
 void Console::onRuntimeError(BOOL aFatal, IN_BSTR aErrorID, IN_BSTR aMessage)
 {
@@ -5847 +5784 @@
     AssertComRCReturnVoid(autoCaller.rc());
 
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
     fireRuntimeErrorEvent(mEventSource, aFatal, aErrorID, aMessage);
 }
 
-/**
- * @note Locks this object for reading.
- */
 HRESULT Console::onShowWindow(BOOL aCheck, BOOL *aCanShow, LONG64 *aWinId)
 {
@@ -5865 +5798 @@
     AssertComRCReturnRC(autoCaller.rc());
 
-    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
     VBoxEventDesc evDesc;
-
     if (aCheck)
     {
@@ -5887 +5818 @@
             else
             {
-                Assert(FALSE);
+                AssertFailed();
                 *aCanShow = TRUE;
             }
@@ -5904 +5835 @@
             evDesc.getEvent(pEvent.asOutParam());
             ComPtr<IShowWindowEvent> pShowEvent = pEvent;
-            LONG64 aEvWinId = 0;
             if (pShowEvent)
             {
-                pShowEvent->COMGETTER(WinId)(&aEvWinId);
-                if ((aEvWinId != 0) && (*aWinId == 0))
-                    *aWinId = aEvWinId;
+                LONG64 iEvWinId = 0;
+                pShowEvent->COMGETTER(WinId)(&iEvWinId);
+                if (iEvWinId != 0 && *aWinId == 0)
+                    *aWinId = iEvWinId;
             }
             else
-                Assert(FALSE);
+                AssertFailed();
         }
     }