Changeset 63182 in vbox for trunk/src/VBox

Timestamp:

Aug 8, 2016 4:16:42 PM (8 years ago)

Author:

vboxsync

Message:

ThreadTask: split createThread up into three methods to avoid having to pass NULL all the time when the type needs specifying. Also, explictly marked the racy variant.

Location:

trunk/src/VBox/Main

Files:

• include/ThreadTask.h (modified) (2 diffs)
• src-all/ExtPackManagerImpl.cpp (modified) (2 diffs)
• src-all/ThreadTask.cpp (modified) (3 diffs)
• src-client/GuestCtrlImpl.cpp (modified) (1 diff)
• src-client/GuestDnDSourceImpl.cpp (modified) (1 diff)
• src-client/GuestDnDTargetImpl.cpp (modified) (1 diff)
• src-client/GuestSessionImpl.cpp (modified) (2 diffs)
• src-client/GuestSessionImplTasks.cpp (modified) (1 diff)
• src-server/VFSExplorerImpl.cpp (modified) (2 diffs)
• src-server/VirtualBoxImpl.cpp (modified) (1 diff)

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

@@ -39 +39 @@
     { };
 
-    HRESULT createThread(PRTTHREAD pThread = NULL, RTTHREADTYPE enmType = RTTHREADTYPE_MAIN_WORKER);
+    HRESULT createThread(void);
+    HRESULT createThreadWithType(RTTHREADTYPE enmType);
+    HRESULT createThreadWithRaceCondition(PRTTHREAD pThread);
 
     virtual void handler() = 0;
@@ -45 +47 @@
 
 protected:
+    HRESULT createThreadInternal(RTTHREADTYPE enmType, PRTTHREAD pThread);
     static DECLCALLBACK(int) taskHandlerThreadProc(RTTHREAD thread, void *pvUser);
 

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

@@ -660 +660 @@
             {
                 ComPtr<Progress> ptrProgress = pTask->ptrProgress;
-                hrc = pTask->createThread(NULL /*phThread*/, RTTHREADTYPE_DEFAULT);
+                hrc = pTask->createThreadWithType(RTTHREADTYPE_DEFAULT);
                 pTask = NULL; /* The _completely_ _undocumented_ createThread method always consumes pTask. */
                 if (SUCCEEDED(hrc))
@@ -2088 +2088 @@
         {
             ComPtr<Progress> ptrProgress = pTask->ptrProgress;
-            hrc = pTask->createThread(NULL, RTTHREADTYPE_DEFAULT);
+            hrc = pTask->createThreadWithType(RTTHREADTYPE_DEFAULT);
             pTask = NULL;               /* always consumed by createThread */
             if (SUCCEEDED(hrc))

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

@@ -20 +20 @@
 #include "ThreadTask.h"
 
+
 /**
  * Starts the task (on separate thread), consuming @a this.
@@ -28 +29 @@
  *
  * Possible way of usage:
+ * @code{.cpp}
+        HRESULT hr;
+        SomeTaskInheritedFromThreadTask *pTask = NULL;
+        try
+        {
+            pTask = new SomeTaskInheritedFromThreadTask(this);
+            if (!pTask->Init()) // some init procedure
+                throw E_FAIL;
+        }
+        catch (...)
+        {
+            if (pTask);
+                delete pTask;
+            return E_FAIL;
+        }
+        return pTask->createThread(); // pTask is always consumed
+   @endcode
  *
- * @code{.cpp}
- * int vrc = VINF_SUCCESS;
- * HRESULT hr = S_OK;
+ * @sa createThreadWithType, createThreadWithRaceCondition
+ */
+HRESULT ThreadTask::createThread(void)
+{
+    return createThreadInternal(RTTHREADTYPE_MAIN_WORKER, NULL /*pThread*/);
+}
+
+
+/**
+ * Same ThreadTask::createThread(), except it takes a thread type parameter.
  *
- * SomeTaskInheritedFromThreadTask* pTask = NULL;
- * try
- * {
- *     pTask = new SomeTaskInheritedFromThreadTask(this);
- *     if (!pTask->Init())//some init procedure
- *     {
- *         delete pTask;
- *         throw E_FAIL;
- *     }
- *     //this function delete pTask in case of exceptions, so
- *     there is no need the call of delete operator
+ * @param   enmType     The thread type.
+ */
+HRESULT ThreadTask::createThreadWithType(RTTHREADTYPE enmType)
+{
+    return createThreadInternal(enmType, NULL /*pThread*/);
+}
+
+
+/**
+ * Same ThreadTask::createThread(), except it returns a thread handle.
  *
- *     hr = pTask->createThread();
- * }
- * catch(...)
- * {
- *     vrc = E_FAIL;
- * }
- * @endcode
+ * If the task thread is incorrectly mananged, the caller may easily race the
+ * completion and termination of the task thread!  Use with care!
+ *
+ * @param   pThread     Handle of the worker thread.
  */
-HRESULT ThreadTask::createThread(PRTTHREAD pThread /*= NULL*/, RTTHREADTYPE enmType /*= RTTHREADTYPE_MAIN_WORKER*/)
+HRESULT ThreadTask::createThreadWithRaceCondition(PRTTHREAD pThread)
 {
-    m_pThread = pThread;
+    return createThreadInternal(RTTHREADTYPE_MAIN_WORKER, pThread);
+}
+
+
+/**
+ * Internal worker for ThreadTask::createThread,
+ * ThreadTask::createThreadWithType, ThreadTask::createThreadwithRaceCondition.
+ *
+ * @note Always consumes @a this!
+ */
+HRESULT ThreadTask::createThreadInternal(RTTHREADTYPE enmType, PRTTHREAD pThread)
+{
+    m_pThread = pThread; /** @todo this is utter non-sense! */
     int vrc = RTThreadCreate(m_pThread,
                              taskHandlerThreadProc,
@@ -69 +102 @@
     return E_FAIL;
 }
+
 
 /**

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

@@ -584 +584 @@
                 }
 
-                hr = pTask->createThread(NULL, RTTHREADTYPE_MAIN_HEAVY_WORKER);
+                hr = pTask->createThreadWithType(RTTHREADTYPE_MAIN_HEAVY_WORKER);
 
                 if (SUCCEEDED(hr))

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

@@ -383 +383 @@
 /** @todo r=bird: The code using hThreadRcv is racing the thread termination. Since the thread isn't
  * created waitable, the handle goes away if we it terminates before our RTThreadUserWait call returns. */
-        hr = pTask->createThread(&hThreadRcv);
+        hr = pTask->createThreadWithRaceCondition(&hThreadRcv);
 
     }

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

@@ -655 +655 @@
 /** @todo r=bird: The code using hThreadSend is racing the thread termination. Since the thread isn't
  * created waitable, the handle goes away if we it terminates before our RTThreadUserWait call returns. */
-        hr = pTask->createThread(&rcThreadSend);
+        hr = pTask->createThreadWithRaceCondition(&rcThreadSend);
 
     }

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

@@ -2512 +2512 @@
         }
 
-        hr = pTask->createThread(NULL, RTTHREADTYPE_MAIN_HEAVY_WORKER);
+        hr = pTask->createThreadWithType(RTTHREADTYPE_MAIN_HEAVY_WORKER);
 
         if (SUCCEEDED(hr))
@@ -2585 +2585 @@
         }
 
-        hr = pTask->createThread(NULL, RTTHREADTYPE_MAIN_HEAVY_WORKER);
+        hr = pTask->createThreadWithType(RTTHREADTYPE_MAIN_HEAVY_WORKER);
 
         if (SUCCEEDED(hr))

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

@@ -1064 +1064 @@
                 }
 
-                hr = pTask->createThread(NULL, RTTHREADTYPE_MAIN_HEAVY_WORKER);
+                hr = pTask->createThreadWithType(RTTHREADTYPE_MAIN_HEAVY_WORKER);
 
                 if (SUCCEEDED(hr))

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

@@ -413 +413 @@
 
         //this function delete task in case of exceptions, so there is no need in the call of delete operator
-        rc = pTask->createThread(NULL, RTTHREADTYPE_MAIN_HEAVY_WORKER);
+        rc = pTask->createThreadWithType(RTTHREADTYPE_MAIN_HEAVY_WORKER);
     }
     catch (HRESULT aRC)
@@ -531 +531 @@
 
         //this function delete task in case of exceptions, so there is no need in the call of delete operator
-        rc = pTask->createThread(NULL, RTTHREADTYPE_MAIN_HEAVY_WORKER);
+        rc = pTask->createThreadWithType(RTTHREADTYPE_MAIN_HEAVY_WORKER);
     }
     catch (HRESULT aRC)

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

@@ -2567 +2567 @@
 
         //this function delete pTask in case of exceptions, so there is no need in the call of delete operator
-        hr = pTask->createThread(NULL, RTTHREADTYPE_MAIN_WORKER);
+        hr = pTask->createThreadWithType(RTTHREADTYPE_MAIN_WORKER);
 
     }