Changeset 49976 in vbox

Timestamp:

Dec 18, 2013 2:58:43 PM (11 years ago)

Author:

vboxsync

Message:

Main/Machine+ClientWatcher: fix for the generic watcher, it missed crashes of spawning sessions (e.g. launching a GUI VM if the DISPLAY variable isn't set), 4.3 regression

Location:

trunk/src/VBox/Main

Files:

• include/MachineImpl.h (modified) (1 diff)
• src-server/ClientWatcher.cpp (modified) (5 diffs)
• src-server/MachineImpl.cpp (modified) (6 diffs)

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

@@ -791 +791 @@
     { return isSessionOpen(aMachine, aControl, true /* aAllowClosing */); }
 
-#ifndef VBOX_WITH_GENERIC_SESSION_WATCHER
     bool checkForSpawnFailure();
-#endif /* !VBOX_WITH_GENERIC_SESSION_WATCHER */
 
     HRESULT prepareRegister();

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

@@ -692 +692 @@
 #elif defined(VBOX_WITH_GENERIC_SESSION_WATCHER)
 
-    bool update = false;
     bool updateSpawned = false;
 
@@ -733 +732 @@
                 break;
 
-            if (RT_SUCCESS(rc) || update || updateSpawned)
+            /** @todo this quite big effort for catching machines in spawning
+             * state which can't be caught by the token mechanism (as the token
+             * can't be in the other process yet) could be eliminated if the
+             * reaping is made smarter, having cross-reference information
+             * from the pid to the corresponding machine object. Both cases do
+             * more or less the same thing anyway. */
+            if (RT_SUCCESS(rc) || updateSpawned)
             {
                 /* RT_SUCCESS(rc) means an update event is signaled */
 
-#if 0
                 // get reference to the machines list in VirtualBox
                 VirtualBox::MachinesOList &allMachines = that->mVirtualBox->getMachinesList();
@@ -744 +748 @@
                 AutoReadLock thatLock(allMachines.getLockHandle() COMMA_LOCKVAL_SRC_POS);
 
-                if (RT_SUCCESS(rc) || update)
-                {
-                    /* obtain a new set of opened machines */
-                    machines.clear();
+                if (RT_SUCCESS(rc) || updateSpawned)
+                {
+                    /* obtain a new set of spawned machines */
+                    spawnedMachines.clear();
 
                     for (MachinesOList::iterator it = allMachines.begin();
@@ -753 +757 @@
                          ++it)
                     {
-                        ComObjPtr<SessionMachine> sm;
-                        if ((*it)->isSessionOpenOrClosing(sm))
-                            machines.push_back(sm);
-                    }
-
-                    cnt = machines.size();
-                    LogFlowFunc(("UPDATE: direct session count = %d\n", cnt));
-                }
-
-                if (RT_SUCCESS(rc) || updateSpawned)
-                {
-                    /* obtain a new set of spawned machines */
-                    spawnedMachines.clear();
-
-                    for (MachinesOList::iterator it = allMachines.begin();
-                         it != allMachines.end();
-                         ++it)
-                    {
                         if ((*it)->isSessionSpawning())
                             spawnedMachines.push_back(*it);
@@ -779 +765 @@
                 }
 
+                NOREF(cnt);
                 // machines lock unwinds here
-#else
-                NOREF(cnt);
-#endif
-            }
-
-#if 0
-            update = false;
-            for (size_t i = 0; i < cnt; ++i)
-                update |= (machines[i])->checkForDeath();
+            }
 
             updateSpawned = false;
             for (size_t i = 0; i < cntSpawned; ++i)
                 updateSpawned |= (spawnedMachines[i])->checkForSpawnFailure();
-#else
-            NOREF(cntSpawned);
-#endif
 
             /* reap child processes */

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

@@ -8213 +8213 @@
         /* restore the session state */
         mData->mSession.mState = SessionState_Unlocked;
+        alock.release();
+        mParent->addProcessToReap(pid);
         /* The failure may occur w/o any error info (from RPC), so provide one */
         return setError(VBOX_E_VM_ERROR,
@@ -8225 +8227 @@
     mData->mSession.mState = SessionState_Spawning;
     mData->mSession.mType = strFrontend;
+
+    alock.release();
+    mParent->addProcessToReap(pid);
 
     LogFlowThisFuncLeave();
@@ -8296 +8301 @@
 }
 
-#ifndef VBOX_WITH_GENERIC_SESSION_WATCHER
 /**
  * Called from the client watcher thread to check for unexpected client process
@@ -8390 +8394 @@
     return false;
 }
-#endif /* !VBOX_WITH_GENERIC_SESSION_WATCHER */
 
 /**
@@ -12985 +12988 @@
 
     // we need to lock this object in uninit() because the lock is shared
-    // with mPeer (as well as data we modify below). mParent->addProcessToReap()
-    // and others need mParent lock, and USB needs host lock.
+    // with mPeer (as well as data we modify below). mParent lock is needed
+    // by several calls to it, and USB needs host lock.
     AutoMultiWriteLock3 multilock(mParent, mParent->host(), this COMMA_LOCKVAL_SRC_POS);
 
@@ -13045 +13048 @@
         mConsoleTaskData.mSnapshot->uninit();
         releaseSavedStateFile(strStateFile, NULL /* pSnapshotToIgnore */ );
-    }
-
-    if (!mData->mSession.mType.isEmpty())
-    {
-        /* mType is not null when this machine's process has been started by
-         * Machine::LaunchVMProcess(), therefore it is our child.  We
-         * need to queue the PID to reap the process (and avoid zombies on
-         * Linux). */
-        Assert(mData->mSession.mPID != NIL_RTPROCESS);
-        mParent->addProcessToReap(mData->mSession.mPID);
     }