Index: /trunk/src/VBox/Main/VirtualBoxBase.cpp =================================================================== --- /trunk/src/VBox/Main/VirtualBoxBase.cpp (revision 13728) +++ /trunk/src/VBox/Main/VirtualBoxBase.cpp (revision 13729) @@ -1068,73 +1068,67 @@ /** - * Uninitializes all dependent children registered with #addDependentChild(). - * - * Typically called from the uninit() method. Note that this method will call - * uninit() methods of child objects. If these methods need to call the parent - * object during initialization, uninitDependentChildren() must be called before - * the relevant part of the parent is uninitialized, usually at the begnning of - * the parent uninitialization sequence. + * Uninitializes all dependent children registered on this object with + * #addDependentChild(). + * + * Must be called from within the VirtualBoxBaseProto::AutoUninitSpan (i.e. + * typically from this object's uninit() method) to uninitialize children + * before this object goes out of service and becomes unusable. + * + * Note that this method will call uninit() methods of child objects. If + * these methods need to call the parent object during uninitialization, + * #uninitDependentChildren() must be called before the relevant part of the + * parent is uninitialized: usually at the begnning of the parent + * uninitialization sequence. + * + * @note May lock something through the called children. */ void VirtualBoxBaseWithChildrenNEXT::uninitDependentChildren() { - LogFlowThisFuncEnter(); - - AutoWriteLock mapLock (mMapLock); - - LogFlowThisFunc (("count=%u...\n", mDependentChildren.size())); + AutoCaller autoCaller (this); + + /* We don't want to hold the childrenLock() write lock here (necessary + * to protect mDependentChildren) when uninitializing children because + * 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. + * + * The only safe place to not lock and keep accessing our data members + * is the InUninit state (no active call to our object may exist on + * another thread when we are in InUinint, provided that all such calls + * use the AutoCaller class of course). InUinint is also used as a flag + * by removeDependentChild() that prevents touching mDependentChildren + * from outside. Therefore, we assert. + */ + AssertReturnVoid (autoCaller.state() == InUninit); if (mDependentChildren.size()) { - /* We keep the lock until we have enumerated all children. - * Those ones that will try to call removeDependentChild() from a - * different thread will have to wait */ - - Assert (mUninitDoneSem == NIL_RTSEMEVENT); - int vrc = RTSemEventCreate (&mUninitDoneSem); - AssertRC (vrc); - - Assert (mChildrenLeft == 0); - mChildrenLeft = mDependentChildren.size(); - for (DependentChildren::iterator it = mDependentChildren.begin(); - it != mDependentChildren.end(); ++ it) + it != mDependentChildren.end(); ++ it) { VirtualBoxBase *child = (*it).second; Assert (child); + + /* Note that if child->uninit() happens to be called on another + * thread right before us and is not yet finished, the second + * uninit() call will wait until the first one has done so + * (thanks to AutoUninitSpan). */ if (child) child->uninit(); } + /* release all weak references we hold */ mDependentChildren.clear(); } - - /* Wait until all children that called uninit() on their own on other - * threads but stuck waiting for the map lock in removeDependentChild() have - * finished uninitialization. */ - - if (mUninitDoneSem != NIL_RTSEMEVENT) - { - /* let stuck children run */ - mapLock.leave(); - - LogFlowThisFunc (("Waiting for uninitialization of all children...\n")); - - RTSemEventWait (mUninitDoneSem, RT_INDEFINITE_WAIT); - - mapLock.enter(); - - RTSemEventDestroy (mUninitDoneSem); - mUninitDoneSem = NIL_RTSEMEVENT; - Assert (mChildrenLeft == 0); - } - - LogFlowThisFuncLeave(); -} - -/** - * Returns a pointer to the dependent child corresponding to the given - * interface pointer (used as a key in the map of dependent children) or NULL - * if the interface pointer doesn't correspond to any child registered using - * #addDependentChild(). +} + +/** + * Returns a pointer to the dependent child (registered using + * #addDependentChild()) corresponding to the given interface pointer or NULL if + * the given pointer is unrelated. + * + * The relation is checked by using the given interface pointer as a key in the + * map of dependent children. * * Note that ComPtr is used as an argument instead of IUnknown * in @@ -1143,67 +1137,64 @@ * * @param aUnk Pointer to map to the dependent child object. - * @return Pointer to the dependent child object. + * @return Pointer to the dependent VirtualBoxBase child object. + * + * @note Locks #childrenLock() for reading. */ VirtualBoxBaseNEXT * VirtualBoxBaseWithChildrenNEXT::getDependentChild (const ComPtr &aUnk) { - AssertReturn (!!aUnk, NULL); - - AutoWriteLock alock (mMapLock); + AssertReturn (!aUnk.isNull(), NULL); + + AutoCaller autoCaller (this); /* return NULL if uninitDependentChildren() is in action */ - if (mUninitDoneSem != NIL_RTSEMEVENT) + if (autoCaller.state() == InUninit) return NULL; + + AutoReadLock alock (childrenLock()); DependentChildren::const_iterator it = mDependentChildren.find (aUnk); if (it == mDependentChildren.end()) return NULL; + return (*it).second; } +/** Helper for addDependentChild(). */ void VirtualBoxBaseWithChildrenNEXT::doAddDependentChild ( IUnknown *aUnk, VirtualBoxBaseNEXT *aChild) { - AssertReturnVoid (aUnk && aChild); - - AutoWriteLock alock (mMapLock); - - if (mUninitDoneSem != NIL_RTSEMEVENT) - { - /* uninitDependentChildren() is being run. For this very unlikely case, - * we have to increase the number of children left, for symmetry with - * a later #removeDependentChild() call. */ - ++ mChildrenLeft; - return; - } + AssertReturnVoid (aUnk != NULL); + AssertReturnVoid (aChild != NULL); + + AutoCaller autoCaller (this); + + /* sanity */ + AssertReturnVoid (autoCaller.state() == InInit || + autoCaller.state() == Ready || + autoCaller.state() == Limited); + + AutoWriteLock alock (childrenLock()); std::pair result = mDependentChildren.insert (DependentChildren::value_type (aUnk, aChild)); - AssertMsg (result.second, ("Failed to insert a child to the map\n")); -} - + AssertMsg (result.second, ("Failed to insert child %p to the map\n", aUnk)); +} + +/** Helper for removeDependentChild(). */ void VirtualBoxBaseWithChildrenNEXT::doRemoveDependentChild (IUnknown *aUnk) { AssertReturnVoid (aUnk); - AutoWriteLock alock (mMapLock); - - if (mUninitDoneSem != NIL_RTSEMEVENT) - { - /* uninitDependentChildren() is being run. Just decrease the number of - * children left and signal a semaphore if it reaches zero. */ - Assert (mChildrenLeft != 0); - -- mChildrenLeft; - if (mChildrenLeft == 0) - { - int vrc = RTSemEventSignal (mUninitDoneSem); - AssertRC (vrc); - } + AutoCaller autoCaller (this); + + /* return shortly; uninitDependentChildren() will do the job */ + if (autoCaller.state() == InUninit) return; - } + + AutoWriteLock alock (childrenLock()); DependentChildren::size_type result = mDependentChildren.erase (aUnk); - AssertMsg (result == 1, ("Failed to remove the child %p from the map\n", - aUnk)); + AssertMsg (result == 1, ("Failed to remove child %p from the map\n", aUnk)); NOREF (result); } Index: /trunk/src/VBox/Main/include/VirtualBoxBase.h =================================================================== --- /trunk/src/VBox/Main/include/VirtualBoxBase.h (revision 13728) +++ /trunk/src/VBox/Main/include/VirtualBoxBase.h (revision 13729) @@ -1927,10 +1927,10 @@ *
  1. * Given an IUnknown instance, it's possible to quickly determine - * whether this instance represents a child object created by the given - * component, and if so, get a valid VirtualBoxBase pointer to the child - * object. The returned pointer can be then safely casted to the + * whether this instance represents a child object that belongs to the + * given component, and if so, get a valid VirtualBoxBase pointer to the + * child object. The returned pointer can be then safely casted to the * actual class of the child object (to get access to its "internal" * non-interface methods) provided that no other child components implement - * the same initial interface IUnknown is queried from. + * the same orignial COM interface IUnknown is queried from. *
  2. * When the parent object uninitializes itself, it can easily unintialize @@ -1945,21 +1945,12 @@ * its parent to register itself within the list of dependent children. *
  3. - * When a child object it is uninitialized, it calls - * #removeDependentChild() to unregister itself. Since the child's - * uninitialization may originate both from this method and from the child - * itself calling its uninit() on another thread at the same time, please - * make sure that #removeDependentChild() is called: - *
    • - * after the child has successfully entered AutoUninitSpan -- to make - * sure this method is called only once for the given child object - * transitioning from Ready to NotReady. A failure to do so will at - * least likely cause an assertion ("Failed to remove the child from - * the map"). - *
    • - * outside the child object's lock -- to avoid guaranteed deadlocks - * caused by different lock order: (child_lock, map_lock) in uninit() - * and (map_lock, child_lock) in this method. - *
    + * When the child object it is uninitialized, it calls + * #removeDependentChild() to unregister itself. *
+ * + * Note that if the parent object does not call #uninitDependentChildren() when + * 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. * * Note that children added by #addDependentChild() are weakly referenced @@ -1969,8 +1960,9 @@ * described here. * - * @note Once again: because of weak referencing, deadlocks and assertions are - * very likely if #addDependentChild() or #removeDependentChild() are used - * incorrectly (called at inappropriate times). Check the above rules once - * more. + * Access to the child list is serialized using the #childrenLock() lock handle + * (which defaults to the general object lock handle (see + * VirtualBoxBase::lockHandle()). This lock is used by all add/remove methods of + * this class so be aware of the need to preserve the {parent, child} lock order + * when calling these methods. * * @todo This is a VirtualBoxBaseWithChildren equivalent that uses the @@ -1984,5 +1976,4 @@ VirtualBoxBaseWithChildrenNEXT() - : mUninitDoneSem (NIL_RTSEMEVENT), mChildrenLeft (0) {} @@ -1991,18 +1982,38 @@ /** - * Adds the given child to the map of dependent children. - * - * Typically called from the child's init() method, from within the - * AutoInitSpan scope. Otherwise, VirtualBoxBase::AutoCaller must be - * used on @a aChild to make sure it is not uninitialized during this - * method's call. + * Lock handle to use when adding/removing child objects from the list of + * children. It is guaranteed that no any other lock is requested in methods + * of this class while holding this lock. + * + * @warning By default, this simply returns the general object's lock handle + * (see VirtualBoxBase::lockHandle()) which is sufficient for most + * cases. + */ + virtual RWLockHandle *childrenLock() { return lockHandle(); } + + /** + * Adds the given child to the list of dependent children. + * + * Usually gets called from the child's init() method. + * + * @note @a aChild (unless it is in InInit state) must be protected by + * VirtualBoxBase::AutoCaller to make sure it is not uninitialized on + * another thread during this method's call. + * + * @note When #childrenLock() is not overloaded (returns the general object + * lock) and this method is called from under the child's read or + * write lock, make sure the {parent, child} locking order is + * preserved by locking the callee (this object) for writing before + * the child's lock. * * @param aChild Child object to add (must inherit VirtualBoxBase AND * implement some interface). + * + * @note Locks #childrenLock() for writing. */ template void addDependentChild (C *aChild) { - AssertReturnVoid (aChild); + AssertReturnVoid (aChild != NULL); doAddDependentChild (ComPtr (aChild), aChild); } @@ -2020,21 +2031,30 @@ /** - * Removes the given child from the map of dependent children. - * - * Typically called from from the child's uninit() method, from within the - * AutoUninitSpan scope. Make sure te child is not locked for reading or - * writing in this case. - * - * If called not from within the AutoUninitSpan scope, - * VirtualBoxBase::AutoCaller must be used on @a aChild to make sure it is - * not uninitialized during this method's call. - * - * @param aChild Child object to remove (must inherit VirtualBoxBase AND - * implement some interface). + * Removes the given child from the list of dependent children. + * + * Usually gets called from the child's uninit() method. + * + * @note Locks #childrenLock() for writing. + * + * @note @a aChild (unless it is in InUninit state) must be protected by + * VirtualBoxBase::AutoCaller to make sure it is not uninitialized on + * another thread during this method's call. + * + * @note When #childrenLock() is not overloaded (returns the general object + * lock) and this method is called from under the child's read or + * write lock, make sure the {parent, child} locking order is + * preserved by locking the callee (this object) for writing before + * the child's lock. This is irrelevant when the method is called from + * under this object's VirtualBoxBaseProto::AutoUninitSpan (i.e. in + * InUninit state) since in this case no locking is done. + * + * @param aChild Child object to remove. + * + * @note Locks #childrenLock() for writing. */ template void removeDependentChild (C *aChild) { - AssertReturnVoid (aChild); + AssertReturnVoid (aChild != NULL); doRemoveDependentChild (ComPtr (aChild)); } @@ -2072,10 +2092,4 @@ typedef std::map DependentChildren; DependentChildren mDependentChildren; - - RTSEMEVENT mUninitDoneSem; - size_t mChildrenLeft; - - /* Protects all the fields above */ - RWLockHandle mMapLock; }; @@ -2092,7 +2106,8 @@ * #addDependentChild() are strongly referenced, so that they cannot * be externally destructed until #removeDependentChild() is called. - * For this reason, strict rules of calling #removeDependentChild() don't - * apply to instances of this class -- it can be called anywhere in the - * child's uninit() implementation. + * + * Also, this class doesn't have the + * VirtualBoxBaseWithChildrenNEXT::getDependentChild() method because it would + * be not fast for long lists. * * @param C type of child objects (must inherit VirtualBoxBase AND @@ -2238,7 +2253,7 @@ * Base class to track component's chlidren of the particular type. * - * This class is similar to VirtualBoxBaseWithChildren, with the exception that - * all children must be of the same type. For this reason, it's not necessary to - * use a map to store children -- a list is used instead. + * This class is similar to VirtualBoxBaseWithChildrenNEXT with the exception + * that all children must be of the same type. For this reason, it's not + * necessary to use a map to store children -- a list is used instead. * * Also, as opposed to VirtualBoxBaseWithChildren, children added by @@ -2248,6 +2263,6 @@ * See individual method descriptions for more information. * - * @param C Type of child objects (must inherit VirtualBoxBase AND implementsome - * interface). + * @param C Type of child objects (must inherit VirtualBoxBase AND implement + * some interface). * * @todo This is a VirtualBoxBaseWithChildren equivalent that uses the @@ -2268,9 +2283,18 @@ /** + * Lock handle to use when adding/removing child objects from the list of + * children. It is guaranteed that no any other lock is requested in methods + * of this class while holding this lock. + * + * @warning By default, this simply returns the general object's lock handle + * (see VirtualBoxBase::lockHandle()) which is sufficient for most + * cases. + */ + virtual RWLockHandle *childrenLock() { return lockHandle(); } + + /** * Adds the given child to the list of dependent children. * * Usually gets called from the child's init() method. - * - * @note Locks this object for writing. * * @note @a aChild (unless it is in InInit state) must be protected by @@ -2278,13 +2302,17 @@ * another thread during this method's call. * - * @note If this method is called from under the child's read or write lock, - * make sure the {parent, child} locking order is preserved by locking - * the callee (this object) for writing before the child's lock. + * @note When #childrenLock() is not overloaded (returns the general object + * lock) and this method is called from under the child's read or + * write lock, make sure the {parent, child} locking order is + * preserved by locking the callee (this object) for writing before + * the child's lock. * * @param aChild Child object to add. + * + * @note Locks #childrenLock() for writing. */ void addDependentChild (C *aChild) { - AssertReturnVoid (aChild); + AssertReturnVoid (aChild != NULL); AutoCaller autoCaller (this); @@ -2295,5 +2323,5 @@ autoCaller.state() == Limited); - AutoWriteLock alock (this); + AutoWriteLock alock (childrenLock()); mDependentChildren.push_back (aChild); } @@ -2304,9 +2332,7 @@ * Usually gets called from the child's uninit() method. * - * Note that once this method returns, the callee (this object) is not - * guaranteed to be valid any more, so the caller must not call its - * other methods. - * - * @note Locks this object for writing. + * 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. * * @note @a aChild (unless it is in InUninit state) must be protected by @@ -2314,9 +2340,15 @@ * another thread during this method's call. * - * @note If this method is called from under the child's read or write lock, - * make sure the {parent, child} locking order is preserved by locking - * the callee (this object) for writing before the child's lock. + * @note When #childrenLock() is not overloaded (returns the general object + * lock) and this method is called from under the child's read or + * write lock, make sure the {parent, child} locking order is + * preserved by locking the callee (this object) for writing before + * the child's lock. This is irrelevant when the method is called from + * under this object's AutoUninitSpan (i.e. in InUninit state) since + * in this case no locking is done. * * @param aChild Child object to remove. + * + * @note Locks #childrenLock() for writing. */ void removeDependentChild (C *aChild) @@ -2330,5 +2362,5 @@ return; - AutoWriteLock alock (this); + AutoWriteLock alock (childrenLock()); mDependentChildren.remove (aChild); } @@ -2339,6 +2371,6 @@ * Returns the read-only list of all dependent children. * - * @note Access the returned list (iterate, get size etc.) only after - * locking this object for reading or for writing! + * @note Access the returned list (iterate, get size etc.) only after making + * sure #childrenLock() is locked for reading or for writing! */ const DependentChildren &dependentChildren() const { return mDependentChildren; } @@ -2355,8 +2387,8 @@ * these methods need to call the parent object during uninitialization, * #uninitDependentChildren() must be called before the relevant part of the - * parent is uninitialized, usually at the begnning of the parent + * parent is uninitialized: usually at the begnning of the parent * uninitialization sequence. * - * @note May lock this object through the called children. + * @note May lock something through the called children. */ void uninitDependentChildren() @@ -2364,9 +2396,9 @@ AutoCaller autoCaller (this); - /* We cannot hold the write lock (necessary here to protect - * mDependentChildren) when uninitializing children because 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 + /* We don't want to hold the childrenLock() write lock here (necessary + * to protect mDependentChildren) when uninitializing children because + * 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. * @@ -2389,5 +2421,5 @@ /* Note that if child->uninit() happens to be called on another - * thread right before us and is not yet finished; the second + * thread right before us and is not yet finished, the second * uninit() call will wait until the first one has done so * (thanks to AutoUninitSpan). */ @@ -2409,9 +2441,9 @@ * another thread during this method's call. * - * @note Locks this object for writing. + * @note Locks #childrenLock() for writing. */ void removeDependentChildren() { - AutoWriteLock alock (this); + AutoWriteLock alock (childrenLock()); mDependentChildren.clear(); }