Bug 1144649 - Make CCGraph::AddNodeToMap fallible again. r=smaug, a=sledru
authorAndrew McCreight <continuation@gmail.com>
Thu, 09 Apr 2015 16:00:00 -0400
changeset 258451 7717f3aa4cf6
parent 258450 333017ad43a9
child 258452 4359c16b7f44
push id4670
push userryanvm@gmail.com
push date2015-04-13 19:15 +0000
treeherdermozilla-beta@05508ccf3ae8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug, sledru
bugs1144649
milestone38.0
Bug 1144649 - Make CCGraph::AddNodeToMap fallible again. r=smaug, a=sledru Crashing here is apparently fairly common. This restores the old behavior, so we at least don't crash immediately, but instead enter a slow downward spiral of leaking. This improves on the old behavior in that we only try and fail to grow the hash table once, rather than on every add. khuey I think reported that the browser got very slow, because you are going through the very slowest path of the allocator over and over.
xpcom/base/nsCycleCollector.cpp
--- a/xpcom/base/nsCycleCollector.cpp
+++ b/xpcom/base/nsCycleCollector.cpp
@@ -825,19 +825,20 @@ struct CCGraph
 {
   NodePool mNodes;
   EdgePool mEdges;
   nsTArray<WeakMapping> mWeakMaps;
   uint32_t mRootCount;
 
 private:
   PLDHashTable mPtrToNodeMap;
+  bool mOutOfMemory;
 
 public:
-  CCGraph() : mRootCount(0) {}
+  CCGraph() : mRootCount(0), mOutOfMemory(false) {}
 
   ~CCGraph()
   {
     if (mPtrToNodeMap.IsInitialized()) {
       PL_DHashTableFinish(&mPtrToNodeMap);
     }
   }
 
@@ -850,16 +851,17 @@ public:
 
   void Clear()
   {
     mNodes.Clear();
     mEdges.Clear();
     mWeakMaps.Clear();
     mRootCount = 0;
     PL_DHashTableFinish(&mPtrToNodeMap);
+    mOutOfMemory = false;
   }
 
 #ifdef DEBUG
   bool IsEmpty()
   {
     return mNodes.IsEmpty() && mEdges.IsEmpty() &&
            mWeakMaps.IsEmpty() && mRootCount == 0 &&
            !mPtrToNodeMap.IsInitialized();
@@ -895,18 +897,28 @@ CCGraph::FindNode(void* aPtr)
     static_cast<PtrToNodeEntry*>(PL_DHashTableSearch(&mPtrToNodeMap, aPtr));
   return e ? e->mNode : nullptr;
 }
 
 PtrToNodeEntry*
 CCGraph::AddNodeToMap(void* aPtr)
 {
   JS::AutoSuppressGCAnalysis suppress;
-  return static_cast<PtrToNodeEntry*>
-    (PL_DHashTableAdd(&mPtrToNodeMap, aPtr)); // infallible add
+  if (mOutOfMemory) {
+    return nullptr;
+  }
+
+  PtrToNodeEntry* e = static_cast<PtrToNodeEntry*>
+    (PL_DHashTableAdd(&mPtrToNodeMap, aPtr, fallible));
+  if (!e) {
+    mOutOfMemory = true;
+    MOZ_ASSERT(false, "Ran out of memory while building cycle collector graph");
+    return nullptr;
+  }
+  return e;
 }
 
 void
 CCGraph::RemoveNodeFromMap(void* aPtr)
 {
   PL_DHashTableRemove(&mPtrToNodeMap, aPtr);
 }
 
@@ -2174,16 +2186,20 @@ CCGraphBuilder::CCGraphBuilder(CCGraph& 
 CCGraphBuilder::~CCGraphBuilder()
 {
 }
 
 PtrInfo*
 CCGraphBuilder::AddNode(void* aPtr, nsCycleCollectionParticipant* aParticipant)
 {
   PtrToNodeEntry* e = mGraph.AddNodeToMap(aPtr);
+  if (!e) {
+    return nullptr;
+  }
+
   PtrInfo* result;
   if (!e->mNode) {
     // New entry.
     result = mNodeBuilder.Add(aPtr, aParticipant);
     e->mNode = result;
     NS_ASSERTION(result, "mNodeBuilder.Add returned null");
   } else {
     result = e->mNode;