Changeset 59996 in vbox for trunk/src

Timestamp:

Mar 11, 2016 3:27:55 PM (9 years ago)

Author:

vboxsync

Message:

Main/VirtualBox: postpone the error reporting from VirtualBox object creation to the method calls of the object. COM loses the error (replaces it by REGDB_E_CLASSNOTREG), making troubleshooting very difficult. XPCOM wouldn't need this, but it is applied everywhere for maximum consistency. Many changes elsewhere to propagate the information correctly, and also fixes many outdated comments.

Location:

trunk/src/VBox/Main

Files:

• include/AutoCaller.h (modified) (3 diffs)
• include/ObjectState.h (modified) (4 diffs)
• src-all/AutoCaller.cpp (modified) (11 diffs)
• src-client/ConsoleImpl.cpp (modified) (3 diffs)
• src-client/SessionImpl.cpp (modified) (1 diff)
• src-client/VirtualBoxClientImpl.cpp (modified) (2 diffs)
• src-server/MachineImpl.cpp (modified) (7 diffs)
• src-server/VirtualBoxImpl.cpp (modified) (19 diffs)

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

@@ -6 +6 @@
 
 /*
- * Copyright (C) 2006-2014 Oracle Corporation
+ * Copyright (C) 2006-2016 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -334 +334 @@
      * to the NotReady state using AutoUninitSpan.
      */
-    void setFailed() { mResult = Failed; }
+    void setFailed(HRESULT rc = E_ACCESSDENIED)
+    {
+        mResult = Failed;
+        mFailedRC = rc;
+        mpFailedEI = new ErrorInfo();
+    }
 
     /** Returns the current initialization result. */
@@ -347 +352 @@
     Result mResult : 3; // must be at least total number of bits + 1 (sign)
     bool mOk : 1;
+    HRESULT mFailedRC;
+    ErrorInfo *mpFailedEI;
 };
 

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

@@ -6 +6 @@
 
 /*
- * Copyright (C) 2006-2014 Oracle Corporation
+ * Copyright (C) 2006-2016 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -22 +22 @@
 #include "VBox/com/defs.h"
 #include "VBox/com/AutoLock.h"
+#include "VBox/com/ErrorInfo.h"
 
 // Forward declaration needed, but nothing more.
@@ -97 +98 @@
 
     bool autoInitSpanConstructor(State aExpectedState);
-    void autoInitSpanDestructor(State aNewState);
+    void autoInitSpanDestructor(State aNewState, HRESULT aFailedRC, com::ErrorInfo *aFailedEI);
     State autoUninitSpanConstructor();
     void autoUninitSpanDestructor();
@@ -114 +115 @@
     /** Thread that caused the last state change */
     RTTHREAD mStateChangeThread;
+    /** Result code for failed object initialization */
+    HRESULT mFailedRC;
+    /** Error information for failed object initialization */
+    com::ErrorInfo *mpFailedEI;
     /** Total number of active calls to this object */
     unsigned mCallers;

trunk/src/VBox/Main/src-all/AutoCaller.cpp

@@ -7 +7 @@
 
 /*
- * Copyright (C) 2006-2014 Oracle Corporation
+ * Copyright (C) 2006-2016 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -44 +44 @@
     mStateChangeThread = NIL_RTTHREAD;
     mCallers = 0;
+    mFailedRC = S_OK;
+    mpFailedEI = NULL;
     mZeroCallersSem = NIL_RTSEMEVENT;
     mInitUninitSem = NIL_RTSEMEVENTMULTI;
@@ -58 +60 @@
     mStateChangeThread = NIL_RTTHREAD;
     mState = NotReady;
+    mFailedRC = S_OK;
+    if (mpFailedEI)
+    {
+        delete mpFailedEI;
+        mpFailedEI = NULL;
+    }
     mObj = NULL;
 }
@@ -195 +203 @@
         if (mState == Limited)
             rc = mObj->setError(rc, "The object functionality is limited");
+        else if (FAILED(mFailedRC) && mFailedRC != E_ACCESSDENIED)
+        {
+            /* replay recorded error information */
+            if (mpFailedEI)
+                ErrorInfoKeeper eik(*mpFailedEI);
+            rc = mFailedRC;
+        }
         else
             rc = mObj->setError(rc, "The object is not ready");
@@ -251 +266 @@
     AutoWriteLock stateLock(mStateLock COMMA_LOCKVAL_SRC_POS);
 
+    mFailedRC = S_OK;
+    if (mpFailedEI)
+    {
+        delete mpFailedEI;
+        mpFailedEI = NULL;
+    }
+
     if (mState == aExpectedState)
     {
@@ -260 +282 @@
 }
 
-void ObjectState::autoInitSpanDestructor(State aNewState)
+void ObjectState::autoInitSpanDestructor(State aNewState, HRESULT aFailedRC, com::ErrorInfo *apFailedEI)
 {
     AutoWriteLock stateLock(mStateLock COMMA_LOCKVAL_SRC_POS);
@@ -271 +293 @@
          * during InInit), signal that InInit is finished and they may go on. */
         RTSemEventMultiSignal(mInitUninitSem);
+    }
+
+    if (aNewState == InitFailed)
+    {
+        mFailedRC = aFailedRC;
+        /* apFailedEI may be NULL, when there is no explicit setFailed() call,
+         * which also implies that aFailedRC is S_OK. This case is used by
+         * objects (the majority) which don't want delayed error signalling. */
+        mpFailedEI = apFailedEI;
+    }
+    else
+    {
+        Assert(SUCCEEDED(aFailedRC));
+        Assert(apFailedEI == NULL);
+        Assert(mpFailedEI == NULL);
     }
 
@@ -376 +413 @@
     : mObj(aObj),
       mResult(aResult),
-      mOk(false)
+      mOk(false),
+      mFailedRC(S_OK),
+      mpFailedEI(NULL)
 {
     Assert(mObj);
@@ -394 +433 @@
     /* if the state was other than NotReady, do nothing */
     if (!mOk)
+    {
+        Assert(SUCCEEDED(mFailedRC));
+        Assert(mpFailedEI == NULL);
         return;
+    }
 
     ObjectState::State newState;
@@ -403 +446 @@
     else
         newState = ObjectState::InitFailed;
-    mObj->getObjectState().autoInitSpanDestructor(newState);
+    mObj->getObjectState().autoInitSpanDestructor(newState, mFailedRC, mpFailedEI);
+    mFailedRC = S_OK;
+    mpFailedEI = NULL; /* now owned by ObjectState instance */
     if (newState == ObjectState::InitFailed)
     {
         /* call uninit() to let the object uninit itself after failed init() */
         mObj->uninit();
-        /* Note: the object may no longer exist here (for example, it can call
-         * the destructor in uninit()) */
     }
 }
@@ -453 +496 @@
     else
         newState = ObjectState::Limited;
-    mObj->getObjectState().autoInitSpanDestructor(newState);
-    /** @todo r=klaus: this is like the initial init() failure, but in this
-     * place uninit() is NOT called. Makes only limited sense. */
+    mObj->getObjectState().autoInitSpanDestructor(newState, S_OK, NULL);
+    /* If later AutoReinitSpan can truly fail (today there is no way) then
+     * in this place there needs to be an mObj->uninit() call just like in
+     * the AutoInitSpan destructor. In that case it might make sense to
+     * let AutoReinitSpan inherit from AutoInitSpan, as the code can be
+     * made (almost) identical. */
 }
 

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

@@ -140 +140 @@
  * 1. The user must check for #rc() before using the created structure
  *    (e.g. passing it as a thread function argument). If #rc() returns a
- *    failure, the Console object may not be used by the task (see
- *    Console::addCaller() for more details).
+ *    failure, the Console object may not be used by the task.
  * 2. On successful initialization, the structure keeps the Console caller
  *    until destruction (to ensure Console remains in the Ready state and won't
@@ -9462 +9461 @@
     ComObjPtr<Console> pConsole = task->mConsole;
 
-    /* Note: no need to use addCaller() because VMPowerUpTask does that */
+    /* Note: no need to use AutoCaller because VMPowerUpTask does that */
 
     /* The lock is also used as a signal from the task initiator (which
@@ -9987 +9986 @@
         const ComObjPtr<Console> &that = task->mConsole;
 
-        /* Note: no need to use addCaller() to protect Console because VMTask does
+        /* Note: no need to use AutoCaller to protect Console because VMTask does
          * that */
 

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

@@ -1159 +1159 @@
          *
          *  bird: Seems E_ACCESSDENIED is what gets returned these days; see
-         *        VirtualBoxBase::addCaller.
+         *        ObjectState::addCaller.
          */
         if (mType != SessionType_WriteLock && (rc == E_UNEXPECTED || rc == E_ACCESSDENIED))

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

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2010-2014 Oracle Corporation
+ * Copyright (C) 2010-2016 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -78 +78 @@
 
     rc = mData.m_pVirtualBox.createLocalObject(CLSID_VirtualBox);
-    AssertComRCReturnRC(rc);
+    if (FAILED(rc))
+        return rc;
+
+    /* Error return is postponed to method calls, fetch info now. */
+    ULONG rev;
+    rc = mData.m_pVirtualBox->COMGETTER(Revision)(&rev);
+    if (FAILED(rc))
+        return rc;
 
     rc = unconst(mData.m_pEventSource).createObject();

trunk/src/VBox/Main/src-server/MachineImpl.cpp

@@ -831 +831 @@
          * sessions. Since in this case we are definitely called by
          * VirtualBox::uninit(), we may be sure that SessionMachine::uninit()
-         * won't happen on the client watcher thread (because it does
-         * VirtualBox::addCaller() for the duration of the
-         * SessionMachine::checkForDeath() call, so that VirtualBox::uninit()
+         * won't happen on the client watcher thread (because it has a
+         * VirtualBox caller for the duration of the
+         * SessionMachine::i_checkForDeath() call, so that VirtualBox::uninit()
          * cannot happen until the VirtualBox caller is released). This is
          * important, because SessionMachine::uninit() cannot correctly operate
@@ -12474 +12474 @@
 /**
  *  Uninitializes this session object. If the reason is other than
- *  Uninit::Unexpected, then this method MUST be called from #checkForDeath()
+ *  Uninit::Unexpected, then this method MUST be called from #i_checkForDeath()
  *  or the client watcher code.
  *
@@ -12596 +12596 @@
     if (aReason == Uninit::Unexpected)
     {
-        /* Uninitialization didn't come from #checkForDeath(), so tell the
+        /* Uninitialization didn't come from #i_checkForDeath(), so tell the
          * client watcher thread to update the set of machines that have open
          * sessions. */
@@ -12654 +12654 @@
 
     /*
-     *  An expected uninitialization can come only from #checkForDeath().
+     *  An expected uninitialization can come only from #i_checkForDeath().
      *  Otherwise it means that something's gone really wrong (for example,
      *  the Session implementation has released the VirtualBox reference
@@ -13333 +13333 @@
 
         /* go to the closing state (essential for all open*Session() calls and
-         * for #checkForDeath()) */
+         * for #i_checkForDeath()) */
         Assert(mData->mSession.mState == SessionState_Locked);
         mData->mSession.mState = SessionState_Unlocking;
@@ -13352 +13352 @@
         }
 
-        /*  Create the progress object the client will use to wait until
-         * #checkForDeath() is called to uninitialize this session object after
+        /* Create the progress object the client will use to wait until
+         * #i_checkForDeath() is called to uninitialize this session object after
          * it releases the IPC semaphore.
          * Note! Because we're "reusing" mProgress here, this must be a proxy
@@ -14720 +14720 @@
          * process in this case is waiting inside Session::close() for the
          * "end session" process object to complete, while #uninit() called by
-         * #checkForDeath() on the Watcher thread is waiting for the pending
+         * #i_checkForDeath() on the Watcher thread is waiting for the pending
          * operation to complete. For now, we accept this inconsistent behavior
          * and simply do nothing here. */

trunk/src/VBox/Main/src-server/VirtualBoxImpl.cpp

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2015 Oracle Corporation
+ * Copyright (C) 2006-2016 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -373 +373 @@
     LogFlowThisFunc(("Version: %s, Package: %s, API Version: %s\n", sVersion.c_str(), sPackageType.c_str(), sAPIVersion.c_str()));
 
-    /* Get the VirtualBox home directory. */
-    {
-        char szHomeDir[RTPATH_MAX];
-        int vrc = com::GetVBoxUserHomeDirectory(szHomeDir, sizeof(szHomeDir));
-        if (RT_FAILURE(vrc))
-            return setError(E_FAIL,
-                            tr("Could not create the VirtualBox home directory '%s' (%Rrc)"),
-                            szHomeDir, vrc);
-
-        unconst(m->strHomeDir) = szHomeDir;
-    }
-
-    LogRel(("Home directory: '%s'\n", m->strHomeDir.c_str()));
-
-    i_reportDriverVersions();
-
-    /* compose the VirtualBox.xml file name */
-    unconst(m->strSettingsFilePath) = Utf8StrFmt("%s%c%s",
-                                                 m->strHomeDir.c_str(),
-                                                 RTPATH_DELIMITER,
-                                                 VBOX_GLOBAL_SETTINGS_FILE);
+    /* Important: DO NOT USE any kind of "early return" (except the single
+     * one above, checking the init span success) in this method. It is vital
+     * for correct error handling that it has only one point of return, which
+     * does all the magic on COM to signal object creation success and
+     * reporting the error later for every API method. COM translates any
+     * unsuccessful object creation to REGDB_E_CLASSNOTREG errors or similar
+     * unhelpful ones which cause us a lot of grief with troubleshooting. */
+
     HRESULT rc = S_OK;
     bool fCreate = false;
     try
     {
+        /* Get the VirtualBox home directory. */
+        {
+            char szHomeDir[RTPATH_MAX];
+            int vrc = com::GetVBoxUserHomeDirectory(szHomeDir, sizeof(szHomeDir));
+            if (RT_FAILURE(vrc))
+                throw setError(E_FAIL,
+                               tr("Could not create the VirtualBox home directory '%s' (%Rrc)"),
+                               szHomeDir, vrc);
+
+            unconst(m->strHomeDir) = szHomeDir;
+        }
+
+        LogRel(("Home directory: '%s'\n", m->strHomeDir.c_str()));
+
+        i_reportDriverVersions();
+
+        /* compose the VirtualBox.xml file name */
+        unconst(m->strSettingsFilePath) = Utf8StrFmt("%s%c%s",
+                                                     m->strHomeDir.c_str(),
+                                                     RTPATH_DELIMITER,
+                                                     VBOX_GLOBAL_SETTINGS_FILE);
         // load and parse VirtualBox.xml; this will throw on XML or logic errors
         try
@@ -511 +519 @@
 
             ComObjPtr<NATNetwork> pNATNetwork;
-            if (SUCCEEDED(rc = pNATNetwork.createObject()))
-            {
-                rc = pNATNetwork->init(this, net);
-                AssertComRCReturnRC(rc);
-            }
-
+            rc = pNATNetwork.createObject();
+            AssertComRCThrowRC(rc);
+            rc = pNATNetwork->init(this, net);
+            AssertComRCThrowRC(rc);
             rc = i_registerNATNetwork(pNATNetwork, false /* aSaveRegistry */);
-            AssertComRCReturnRC(rc);
+            AssertComRCThrowRC(rc);
         }
 
@@ -579 +585 @@
     }
 
-    /* Confirm a successful initialization when it's the case */
-    if (SUCCEEDED(rc))
-        autoInitSpan.setSucceeded();
-
 #ifdef VBOX_WITH_EXTPACK
     /* Let the extension packs have a go at things. */
@@ -592 +594 @@
 #endif
 
-    LogFlowThisFunc(("rc=%08X\n", rc));
+    /* Confirm a successful initialization when it's the case. Must be last,
+     * as on failure it will uninitialize the object. */
+    if (SUCCEEDED(rc))
+        autoInitSpan.setSucceeded();
+    else
+        autoInitSpan.setFailed(rc);
+
+    LogFlowThisFunc(("rc=%hrc\n", rc));
     LogFlowThisFuncLeave();
     LogFlow(("===========================================================\n"));
-    return rc;
+    /* Unconditionally return success, because the error return is delayed to
+     * the attribute/method calls through the Zombie object state. */
+    return S_OK;
 }
 
@@ -735 +746 @@
 void VirtualBox::uninit()
 {
-    Assert(!m->uRegistryNeedsSaving);
-    if (m->uRegistryNeedsSaving)
-        i_saveSettings();
+    /* Must be done outside the AutoUninitSpan, as it expects AutoCaller to
+     * be successful. This needs additional checks to protect against double
+     * uninit, as then the pointer is NULL. */
+    if (RT_VALID_PTR(m))
+    {
+        Assert(!m->uRegistryNeedsSaving);
+        if (m->uRegistryNeedsSaving)
+            i_saveSettings();
+    }
 
     /* Enclose the state transition Ready->InUninit->NotReady */
@@ -840 +857 @@
     // clean up our instance data
     delete m;
+    m = NULL;
 
     /* Unload hard disk plugin backends. */
@@ -1259 +1277 @@
                                firmwareDesc[i].fileName);
         int rc = i_calculateFullPath(shortName, fullName);
-        AssertRCReturn(rc, rc);
+        AssertRCReturn(rc, VBOX_E_IPRT_ERROR);
         if (RTFileExists(fullName.c_str()))
         {
@@ -1269 +1287 @@
         char pszVBoxPath[RTPATH_MAX];
         rc = RTPathExecDir(pszVBoxPath, RTPATH_MAX);
-        AssertRCReturn(rc, rc);
+        AssertRCReturn(rc, VBOX_E_IPRT_ERROR);
         fullName = Utf8StrFmt("%s%c%s",
                               pszVBoxPath,
@@ -2812 +2830 @@
 
     AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturn(autoCaller.rc(), FALSE);
 
     BOOL allowChange = TRUE;
@@ -3180 +3198 @@
 
     AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturnRC(autoCaller.rc());
 
     {
@@ -4141 +4159 @@
 {
     AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturnRC(autoCaller.rc());
 
     AssertReturn(isWriteLockOnCurrentThread(), E_FAIL);
     AssertReturn(!m->strSettingsFilePath.isEmpty(), E_FAIL);
+
+    i_unmarkRegistryModified(i_getGlobalRegistryId());
 
     HRESULT rc = S_OK;
@@ -4315 +4335 @@
 
     AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturnRC(autoCaller.rc());
 
     AutoCaller mediumCaller(pMedium);
-    AssertComRCReturn(mediumCaller.rc(), mediumCaller.rc());
+    AssertComRCReturnRC(mediumCaller.rc());
 
     const char *pszDevType = NULL;
@@ -4421 +4441 @@
 
     AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturnRC(autoCaller.rc());
 
     AutoCaller mediumCaller(pMedium);
-    AssertComRCReturn(mediumCaller.rc(), mediumCaller.rc());
+    AssertComRCReturnRC(mediumCaller.rc());
 
     // caller must hold the media tree write lock
@@ -4511 +4531 @@
 
     AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturnRC(autoCaller.rc());
 
     MediaList llMedia2Close;
@@ -5007 +5027 @@
 
     AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturnRC(autoCaller.rc());
 
     // Acquire a lock on the VirtualBox object early to avoid lock order issues
@@ -5018 +5038 @@
 
     AutoCaller dhcpServerCaller(aDHCPServer);
-    AssertComRCReturn(dhcpServerCaller.rc(), dhcpServerCaller.rc());
+    AssertComRCReturnRC(dhcpServerCaller.rc());
 
     Bstr name;
@@ -5069 +5089 @@
 
     AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturnRC(autoCaller.rc());
 
     AutoCaller dhcpServerCaller(aDHCPServer);
-    AssertComRCReturn(dhcpServerCaller.rc(), dhcpServerCaller.rc());
+    AssertComRCReturnRC(dhcpServerCaller.rc());
 
     AutoWriteLock vboxLock(this COMMA_LOCKVAL_SRC_POS);
@@ -5251 +5271 @@
 
     AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturnRC(autoCaller.rc());
 
     AutoCaller natNetworkCaller(aNATNetwork);
-    AssertComRCReturn(natNetworkCaller.rc(), natNetworkCaller.rc());
+    AssertComRCReturnRC(natNetworkCaller.rc());
 
     Bstr name;