Changeset 13887 in vbox for trunk/src/VBox/Main/include

Timestamp:

Nov 5, 2008 6:43:42 PM (16 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

38887

Message:

Main: Fixed: 1) VirtualBoxBaseWithChildrenNEXT::uninitDependentChildren() chould try to uninitialize already deleted children because they were not properly removed from the map (r38669 regression). Also a rare deadlock was possible in this method if a weak child happened to be uninitializing itself on another thread at the same time (a really old regression).

File:

• trunk/src/VBox/Main/include/VirtualBoxBase.h (modified) (10 diffs)

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

@@ -1952 +1952 @@
  * it gets uninitialized, it must call uninit() methods of individual children
  * manually to disconnect them; a failure to do so will cause crashes in these
- * methods when chidren get destroyed.
+ * methods when chidren get destroyed. The same applies to children not calling
+ * #removeDependentChild() when getting destrooyed.
  *
  * Note that children added by #addDependentChild() are <b>weakly</b> referenced
@@ -1965 +1966 @@
  * this class so be aware of the need to preserve the {parent, child} lock order
  * when calling these methods.
+ *
+ * Read individual method descriptions to get further information.
  *
  * @todo This is a VirtualBoxBaseWithChildren equivalent that uses the
@@ -2034 +2037 @@
      *
      * Usually gets called from the child's uninit() method.
+     *
+     * Keep in mind that the called (parent) object may be no longer available
+     * (i.e. may be deleted deleted) after this method returns, so you must not
+     * call any other parent's methods after that!
      *
      * @note Locks #childrenLock() for writing.
@@ -2259 +2266 @@
  * Also, as opposed to VirtualBoxBaseWithChildren, children added by
  * #addDependentChild() are <b>strongly</b> referenced, so that they cannot be
- * deleted until #removeDependentChild() is called on them.
- *
- * See individual method descriptions for more information.
+ * deleted (even by a third party) until #removeDependentChild() is called on
+ * them. This also means that a failure to call #removeDependentChild() and
+ * #uninitDependentChildren() at appropriate times as described in
+ * VirtualBoxBaseWithChildrenNEXT may cause stuck references that won't be able
+ * uninitialize themselves.
+ *
+ * See individual method descriptions for further information.
  *
  * @param C Type of child objects (must inherit VirtualBoxBase AND implement
@@ -2323 +2334 @@
                           autoCaller.state() == Limited);
 
-        AutoWriteLock alock (childrenLock());
+        AutoWriteLock chLock (childrenLock());
         mDependentChildren.push_back (aChild);
     }
@@ -2332 +2343 @@
      * Usually gets called from the child's uninit() method.
      *
-     * Note that once this method returns, the argument (@a aChild) is not
-     * guaranteed to be valid any more, so the caller of this method must not
-     * call its other methods.
+     * Keep in mind that the called (parent) object may be no longer available
+     * (i.e. may be deleted deleted) after this method returns, so you must not
+     * call any other parent's methods after that!
      *
      * @note @a aChild (unless it is in InUninit state) must be protected by
@@ -2358 +2369 @@
         AutoCaller autoCaller (this);
 
-        /* return shortly; uninitDependentChildren() will do the job */
+        /* sanity */
+        AssertReturnVoid (autoCaller.state() == InUninit ||
+                          autoCaller.state() == InInit ||
+                          autoCaller.state() == Ready ||
+                          autoCaller.state() == Limited);
+
+        /* return shortly; we are strongly referenced so the object won't get
+         * deleted if it calls init() before uninitDependentChildren() does
+         * and therefore the list will still contain a valid reference that will
+         * be correctly processed by uninitDependentChildren() anyway */
         if (autoCaller.state() == InUninit)
             return;
 
-        AutoWriteLock alock (childrenLock());
+        AutoWriteLock chLock (childrenLock());
         mDependentChildren.remove (aChild);
     }
@@ -2400 +2420 @@
          * we want to avoid a possible deadlock where we could get stuck in
          * child->uninit() blocked by AutoUninitSpan waiting for the number of
-         * child's callers to drop to zero, while some caller is stuck in our
-         * removeDependentChild() method waiting for the write lock.
+         * child's callers to drop to zero (or for another AutoUninitSpan to
+         * finish), while some other thread is stuck in our
+         * removeDependentChild() method called for that child and waiting for
+         * the childrenLock()'s write lock.
          *
          * The only safe place to not lock and keep accessing our data members
@@ -2408 +2430 @@
          * use the AutoCaller class of course). InUinint is also used as a flag
          * by removeDependentChild() that prevents touching mDependentChildren
-         * from outside. Therefore, we assert.
+         * from outside. Therefore, we assert. Note that InInit is also fine
+         * since no any object may access us by that time.
          */
-        AssertReturnVoid (autoCaller.state() == InUninit);
+        AssertReturnVoid (autoCaller.state() == InUninit ||
+                          autoCaller.state() == InInit);
 
         if (mDependentChildren.size())
@@ -2445 +2469 @@
     void removeDependentChildren()
     {
-        AutoWriteLock alock (childrenLock());
+        AutoWriteLock chLock (childrenLock());
         mDependentChildren.clear();
     }