Bug 1049051 - Part 2: Remove DeadlockDetectorEntry

xpcom/glue/BlockingResourceBase.cpp

@@ -38,9 +38,7 @@ unsigned BlockingResourceBase::sResourceAcqnChainFrontTPI = (unsigned)-1;
 BlockingResourceBase::DDT* BlockingResourceBase::sDeadlockDetector;
 
 bool
-BlockingResourceBase::DeadlockDetectorEntry::Print(
-    const DeadlockDetectorEntry* aFirstSeen,
-    nsACString& aOut) const
+BlockingResourceBase::Print(nsACString& aOut) const
 {
   fprintf(stderr, "--- %s : %s",
           kResourceTypeName[mType], mName);
@@ -63,16 +61,19 @@ BlockingResourceBase::DeadlockDetectorEntry::Print(
 BlockingResourceBase::BlockingResourceBase(
     const char* aName,
     BlockingResourceBase::BlockingResourceType aType)
+  : mName(aName)
+  , mType(aType)
+  , mAcquired(false)
 {
+  NS_ABORT_IF_FALSE(mName, "Name must be nonnull");
   // PR_CallOnce guaranatees that InitStatics is called in a
   // thread-safe way
   if (PR_SUCCESS != PR_CallOnce(&sCallOnce, InitStatics)) {
     NS_RUNTIMEABORT("can't initialize blocking resource static members");
   }
 
-  mDDEntry = new BlockingResourceBase::DeadlockDetectorEntry(aName, aType);
   mChainPrev = 0;
-  sDeadlockDetector->Add(mDDEntry);
+  sDeadlockDetector->Add(this);
 }
 
 
@@ -83,15 +84,14 @@ BlockingResourceBase::~BlockingResourceBase()
   // base class, or its underlying primitive, will check for such
   // stupid mistakes.
   mChainPrev = 0;             // racy only for stupidly buggy client code
-  sDeadlockDetector->Remove(mDDEntry);
-  mDDEntry = 0;               // owned by deadlock detector
+  sDeadlockDetector->Remove(this);
 }
 
 
 void
 BlockingResourceBase::CheckAcquire()
 {
-  if (mDDEntry->mType == eCondVar) {
+  if (mType == eCondVar) {
     NS_NOTYETIMPLEMENTED(
       "FIXME bug 456272: annots. to allow CheckAcquire()ing condvars");
     return;
@@ -100,7 +100,7 @@ BlockingResourceBase::CheckAcquire()
   BlockingResourceBase* chainFront = ResourceChainFront();
   nsAutoPtr<DDT::ResourceAcquisitionArray> cycle(
     sDeadlockDetector->CheckAcquisition(
-      chainFront ? chainFront->mDDEntry : 0, mDDEntry));
+      chainFront ? chainFront : 0, this));
   if (!cycle) {
     return;
   }
@@ -129,30 +129,30 @@ BlockingResourceBase::CheckAcquire()
 void
 BlockingResourceBase::Acquire()
 {
-  if (mDDEntry->mType == eCondVar) {
+  if (mType == eCondVar) {
     NS_NOTYETIMPLEMENTED(
       "FIXME bug 456272: annots. to allow Acquire()ing condvars");
     return;
   }
-  NS_ASSERTION(!mDDEntry->mAcquired,
+  NS_ASSERTION(!mAcquired,
                "reacquiring already acquired resource");
 
   ResourceChainAppend(ResourceChainFront());
-  mDDEntry->mAcquired = true;
+  mAcquired = true;
 }
 
 
 void
 BlockingResourceBase::Release()
 {
-  if (mDDEntry->mType == eCondVar) {
+  if (mType == eCondVar) {
     NS_NOTYETIMPLEMENTED(
       "FIXME bug 456272: annots. to allow Release()ing condvars");
     return;
   }
 
   BlockingResourceBase* chainFront = ResourceChainFront();
-  NS_ASSERTION(chainFront && mDDEntry->mAcquired,
+  NS_ASSERTION(chainFront && mAcquired,
                "Release()ing something that hasn't been Acquire()ed");
 
   if (chainFront == this) {
@@ -178,7 +178,7 @@ BlockingResourceBase::Release()
     }
   }
 
-  mDDEntry->mAcquired = false;
+  mAcquired = false;
 }
 
 
@@ -193,8 +193,8 @@ BlockingResourceBase::PrintCycle(const DDT::ResourceAcquisitionArray* aCycle,
   fputs("=== Cyclical dependency starts at\n", stderr);
   aOut += "Cyclical dependency starts at\n";
 
-  const DeadlockDetectorEntry* res = aCycle->ElementAt(0);
-  maybeImminent &= res->Print(res, aOut);
+  const DDT::ResourceAcquisitionArray::elem_type res = aCycle->ElementAt(0);
+  maybeImminent &= res->Print(aOut);
 
   DDT::ResourceAcquisitionArray::index_type i;
   DDT::ResourceAcquisitionArray::size_type len = aCycle->Length();
@@ -203,12 +203,12 @@ BlockingResourceBase::PrintCycle(const DDT::ResourceAcquisitionArray* aCycle,
     fputs("\n--- Next dependency:\n", stderr);
     aOut += "\nNext dependency:\n";
 
-    maybeImminent &= (*it)->Print(*it, aOut);
+    maybeImminent &= (*it)->Print(aOut);
   }
 
   fputs("\n=== Cycle completed at\n", stderr);
   aOut += "Cycle completed at\n";
-  (*it)->Print(*it, aOut);
+  (*it)->Print(aOut);
 
   return maybeImminent;
 }

xpcom/glue/BlockingResourceBase.h

@@ -59,81 +59,38 @@ public:
         sDeadlockDetector->SizeOfIncludingThis(aMallocSizeOf) : 0;
   }
 
-private:
-  // forward declaration for the following typedef
-  struct DeadlockDetectorEntry;
-
-  // ``DDT'' = ``Deadlock Detector Type''
-  typedef DeadlockDetector<DeadlockDetectorEntry> DDT;
-
   /**
-   * DeadlockDetectorEntry
-   * We free BlockingResources, but we never free entries in the
-   * deadlock detector.  This struct outlives its BlockingResource
-   * and preserves all the state needed to print subsequent
-   * error messages.
+   * Print
+   * Write a description of this blocking resource to |aOut|.  If
+   * the resource appears to be currently acquired, the current
+   * acquisition context is printed and true is returned.
+   * Otherwise, we print the context from |aFirstSeen|, the
+   * first acquisition from which the code calling |Print()|
+   * became interested in us, and return false.
    *
-   * These objects are owned by the deadlock detector.
+   * *NOT* thread safe.  Reads |mAcquisitionContext| without
+   * synchronization, but this will not cause correctness
+   * problems.
+   *
+   * FIXME bug 456272: hack alert: because we can't write call
+   * contexts into strings, all info is written to stderr, but
+   * only some info is written into |aOut|
    */
-  struct DeadlockDetectorEntry
+  bool Print(nsACString& aOut) const;
+
+  size_t
+  SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
   {
-    DeadlockDetectorEntry(const char* aName,
-                          BlockingResourceType aType)
-      : mName(aName)
-      , mType(aType)
-      , mAcquired(false)
-    {
-      NS_ABORT_IF_FALSE(mName, "Name must be nonnull");
-    }
+    // NB: |mName| is not reported as it's expected to be a static string.
+    //     If we switch to a nsString it should be added to the tally.
+    //     |mChainPrev| is not reported because its memory is not owned.
+    size_t n = aMallocSizeOf(this);
+    return n;
+  }
 
-    size_t
-    SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
-    {
-      // NB: |mName| is not reported as it's expected to be a static string.
-      //     If we switch to a nsString it should be added to the tally.
-      //     |mAcquisitionContext| has no measurable heap allocations in it.
-      size_t n = aMallocSizeOf(this);
-      return n;
-    }
-
-    /**
-     * Print
-     * Write a description of this blocking resource to |aOut|.  If
-     * the resource appears to be currently acquired, the current
-     * acquisition context is printed and true is returned.
-     * Otherwise, we print the context from |aFirstSeen|, the
-     * first acquisition from which the code calling |Print()|
-     * became interested in us, and return false.
-     *
-     * *NOT* thread safe.  Reads |mAcquisitionContext| without
-     * synchronization, but this will not cause correctness
-     * problems.
-     *
-     * FIXME bug 456272: hack alert: because we can't write call
-     * contexts into strings, all info is written to stderr, but
-     * only some info is written into |aOut|
-     */
-    bool Print(const DeadlockDetectorEntry* aFirstSeen,
-               nsACString& aOut) const;
-
-    /**
-     * mName
-     * A descriptive name for this resource.  Used in error
-     * messages etc.
-     */
-    const char* mName;
-    /**
-     * mType
-     * The more specific type of this resource.  Used to implement
-     * special semantics (e.g., reentrancy of monitors).
-     **/
-    BlockingResourceType mType;
-    /**
-     * mAcquired
-     * Indicates if this resource is currently acquired.
-     */
-    bool mAcquired;
-  };
+private:
+  // ``DDT'' = ``Deadlock Detector Type''
+  typedef DeadlockDetector<BlockingResourceBase> DDT;
 
 protected:
   /**
@@ -251,7 +208,7 @@ protected:
    */
   bool GetAcquisitionState()
   {
-    return mDDEntry->mAcquired;
+    return mAcquired;
   }
 
   /**
@@ -262,7 +219,7 @@ protected:
    */
   void SetAcquisitionState(bool aAcquisitionState)
   {
-    mDDEntry->mAcquired = aAcquisitionState;
+    mAcquired = aAcquisitionState;
   }
 
   /**
@@ -275,10 +232,24 @@ protected:
 
 private:
   /**
-   * mDDEntry
-   * The key for this BlockingResourceBase in the deadlock detector.
+   * mName
+   * A descriptive name for this resource.  Used in error
+   * messages etc.
    */
-  DeadlockDetectorEntry* mDDEntry;
+  const char* mName;
+
+  /**
+   * mType
+   * The more specific type of this resource.  Used to implement
+   * special semantics (e.g., reentrancy of monitors).
+   **/
+  BlockingResourceType mType;
+
+  /**
+   * mAcquired
+   * Indicates if this resource is currently acquired.
+   */
+  bool mAcquired;
 
   /**
    * sCallOnce

xpcom/glue/DeadlockDetector.h

@@ -99,7 +99,6 @@ private:
       size_t n = aMallocSizeOf(this);
       n += mOrderedLT.SizeOfExcludingThis(aMallocSizeOf);
       n += mExternalRefs.SizeOfExcludingThis(aMallocSizeOf);
-      n += mResource->SizeOfIncludingThis(aMallocSizeOf);
       return n;
     }
 
@@ -203,7 +202,6 @@ public:
 
     // Now the entry can be safely removed.
     mOrdering.Remove(aResource);
-    delete aResource;
   }
 
   /**