Bug 810718 (part 1) - Enforce pldhash RECURSION_LEVEL checks in all builds and abort on failure. r=bsmedberg.

--HG--
extra : rebase_source : 4233dc104d8db99ec19b5b4885a9ddfaa7287427

parser/htmlparser/src/nsHTMLEntities.cpp

@@ -127,10 +127,8 @@ nsHTMLEntities::AddRefTable(void)
       if (!entry->node)
         entry->node = node;
     }
-#ifdef DEBUG
     PL_DHashMarkTableImmutable(&gUnicodeToEntity);
     PL_DHashMarkTableImmutable(&gEntityToUnicode);
-#endif
   }
   ++gTableRefCnt;
   return NS_OK;

toolkit/components/telemetry/Telemetry.cpp

@@ -956,10 +956,8 @@ mFailedLockCount(0)
   for (size_t i = 0; i < ArrayLength(trackedDBs); i++)
     mTrackedDBs.PutEntry(nsDependentCString(trackedDBs[i]));
 
-#ifdef DEBUG
   // Mark immutable to prevent asserts on simultaneous access from multiple threads
   mTrackedDBs.MarkImmutable();
-#endif
   mReporter = new TelemetryReporter();
   NS_RegisterMemoryReporter(mReporter);
 }

xpcom/ds/nsStaticNameTable.cpp

@@ -177,9 +177,7 @@ nsStaticCaseInsensitiveNameTable::Init(const char* const aNames[], int32_t Count
         entry->mString = strPtr;      // not owned!
         entry->mIndex = index;
     }
-#ifdef DEBUG
     PL_DHashMarkTableImmutable(&mNameTable);
-#endif
     return true;
 }
 

xpcom/glue/nsTHashtable.h

@@ -275,7 +275,6 @@ public:
     return PL_DHashTableSizeOfExcludingThis(&mTable, nullptr, mallocSizeOf);
   }
 
-#ifdef DEBUG
   /**
    * Mark the table as constant after initialization.
    *
@@ -288,7 +287,6 @@ public:
 
     PL_DHashMarkTableImmutable(&mTable);
   }
-#endif
 
 protected:
   PLDHashTable mTable;

xpcom/glue/pldhash.cpp

@@ -24,21 +24,28 @@
 #endif
 
 /*
- * The following DEBUG-only code is used to assert that calls to one of
- * table->ops or to an enumerator do not cause re-entry into a call that
- * can mutate the table.
+ * The following code is used to ensure that calls to one of table->ops or to
+ * an enumerator do not cause re-entry into a call that can mutate the table.
+ * Any check that fails will cause an immediate abort, even in non-debug
+ * builds.
  */
-#ifdef DEBUG
 
 /*
- * Most callers that assert about the recursion level don't care about
- * this magical value because they are asserting that mutation is
- * allowed (and therefore the level is 0 or 1, depending on whether they
- * incremented it).
+ * Most callers that check the recursion level don't care about this magical
+ * value because they are checking that mutation is allowed (and therefore the
+ * level is 0 or 1, depending on whether they incremented it).
  *
  * Only PL_DHashTableFinish needs to allow this special value.
  */
-#define IMMUTABLE_RECURSION_LEVEL ((uint16_t)-1)
+#define IMMUTABLE_RECURSION_LEVEL ((uint32_t)-1)
+
+/* This aborts in all builds. */
+#define ABORT_UNLESS(condition)                                               \
+    do {                                                                      \
+        if (!(condition)) {                                                   \
+          NS_RUNTIMEABORT("recursion level error in pldhash");                \
+        }                                                                     \
+    } while (0)
 
 #define RECURSION_LEVEL_SAFE_TO_FINISH(table_)                                \
     (table_->recursionLevel == 0 ||                                           \
@@ -48,21 +55,14 @@
     do {                                                                      \
         if (table_->recursionLevel != IMMUTABLE_RECURSION_LEVEL)              \
             ++table_->recursionLevel;                                         \
-    } while(0)
+    } while (0)
 #define DECREMENT_RECURSION_LEVEL(table_)                                     \
     do {                                                                      \
         if (table->recursionLevel != IMMUTABLE_RECURSION_LEVEL) {             \
-            MOZ_ASSERT(table->recursionLevel > 0);                            \
+            ABORT_UNLESS(table->recursionLevel > 0);                          \
             --table->recursionLevel;                                          \
         }                                                                     \
-    } while(0)
-
-#else
-
-#define INCREMENT_RECURSION_LEVEL(table_)   do { } while(0)
-#define DECREMENT_RECURSION_LEVEL(table_)   do { } while(0)
-
-#endif /* defined(DEBUG) */
+    } while (0)
 
 using namespace mozilla;
 
@@ -225,9 +225,7 @@ PL_DHashTableInit(PLDHashTable *table, const PLDHashTableOps *ops, void *data,
     memset(table->entryStore, 0, nbytes);
     METER(memset(&table->stats, 0, sizeof table->stats));
 
-#ifdef DEBUG
     table->recursionLevel = 0;
-#endif
 
     return true;
 }
@@ -305,7 +303,7 @@ PL_DHashTableFinish(PLDHashTable *table)
     }
 
     DECREMENT_RECURSION_LEVEL(table);
-    MOZ_ASSERT(RECURSION_LEVEL_SAFE_TO_FINISH(table));
+    ABORT_UNLESS(RECURSION_LEVEL_SAFE_TO_FINISH(table));
 
     /* Free entry storage last. */
     table->ops->freeTable(table, table->entryStore);
@@ -448,9 +446,7 @@ ChangeTable(PLDHashTable *table, int deltaLog2)
         return false;
 
     /* We can't fail from here on, so update table parameters. */
-#ifdef DEBUG
     uint32_t recursionLevel = table->recursionLevel;
-#endif
     table->hashShift = PL_DHASH_BITS - newLog2;
     table->removedCount = 0;
     table->generation++;
@@ -461,9 +457,7 @@ ChangeTable(PLDHashTable *table, int deltaLog2)
     oldEntryAddr = oldEntryStore = table->entryStore;
     table->entryStore = newEntryStore;
     PLDHashMoveEntry moveEntry = table->ops->moveEntry;
-#ifdef DEBUG
     table->recursionLevel = recursionLevel;
-#endif
 
     /* Copy only live entries, leaving removed ones behind. */
     uint32_t oldCapacity = 1u << oldLog2;
@@ -489,7 +483,7 @@ PL_DHashTableOperate(PLDHashTable *table, const void *key, PLDHashOperator op)
 {
     PLDHashEntryHdr *entry;
 
-    MOZ_ASSERT(op == PL_DHASH_LOOKUP || table->recursionLevel == 0);
+    ABORT_UNLESS(op == PL_DHASH_LOOKUP || table->recursionLevel == 0);
     INCREMENT_RECURSION_LEVEL(table);
 
     PLDHashNumber keyHash = table->ops->hashKey(table, key);
@@ -597,7 +591,7 @@ PL_DHashTableOperate(PLDHashTable *table, const void *key, PLDHashOperator op)
 void
 PL_DHashTableRawRemove(PLDHashTable *table, PLDHashEntryHdr *entry)
 {
-    MOZ_ASSERT(table->recursionLevel != IMMUTABLE_RECURSION_LEVEL);
+    ABORT_UNLESS(table->recursionLevel != IMMUTABLE_RECURSION_LEVEL);
 
     NS_ASSERTION(PL_DHASH_ENTRY_IS_LIVE(entry),
                  "PL_DHASH_ENTRY_IS_LIVE(entry)");
@@ -641,7 +635,7 @@ PL_DHashTableEnumerate(PLDHashTable *table, PLDHashEnumerator etor, void *arg)
         entryAddr += entrySize;
     }
 
-    MOZ_ASSERT(!didRemove || table->recursionLevel == 1);
+    ABORT_UNLESS(!didRemove || table->recursionLevel == 1);
 
     /*
      * Shrink or compress if a quarter or more of all entries are removed, or
@@ -716,13 +710,11 @@ PL_DHashTableSizeOfIncludingThis(const PLDHashTable *table,
                                             mallocSizeOf, arg);
 }
 
-#ifdef DEBUG
 void
 PL_DHashMarkTableImmutable(PLDHashTable *table)
 {
     table->recursionLevel = IMMUTABLE_RECURSION_LEVEL;
 }
-#endif
 
 #ifdef PL_DHASHMETER
 #include <math.h>

xpcom/glue/pldhash.h

@@ -8,6 +8,7 @@
 /*
  * Double hashing, a la Knuth 6.
  */
+#include "mozilla/Atomics.h"
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/Types.h"
 #include "nscore.h"
@@ -182,13 +183,7 @@ struct PLDHashTable {
     const PLDHashTableOps *ops;         /* virtual operations, see below */
     void                *data;          /* ops- and instance-specific data */
     int16_t             hashShift;      /* multiplicative hash shift */
-    /*
-     * |recursionLevel| is only used in debug builds, but is present in opt
-     * builds to avoid binary compatibility problems when mixing DEBUG and
-     * non-DEBUG components.  (Actually, even if it were removed,
-     * sizeof(PLDHashTable) wouldn't change, due to struct padding.)
-     */
-    uint16_t            recursionLevel; /* used to detect unsafe re-entry */
+    mozilla::Atomic<uint32_t> recursionLevel; /* detects unsafe re-entry */
     uint32_t            entrySize;      /* number of bytes in an entry */
     uint32_t            entryCount;     /* number of entries in table */
     uint32_t            removedCount;   /* removed entry sentinels in table */
@@ -549,24 +544,22 @@ PL_DHashTableSizeOfIncludingThis(const PLDHashTable *table,
                                  mozilla::MallocSizeOf mallocSizeOf,
                                  void *arg = nullptr);
 
-#ifdef DEBUG
 /**
  * Mark a table as immutable for the remainder of its lifetime.  This
- * changes the implementation from ASSERTing one set of invariants to
- * ASSERTing a different set.
+ * changes the implementation from checking one set of invariants to
+ * checking a different set.
  *
  * When a table is NOT marked as immutable, the table implementation
- * asserts that the table is not mutated from its own callbacks.  It
+ * checks that the table is not mutated from its own callbacks.  It
  * assumes the caller protects the table from being accessed on multiple
  * threads simultaneously.
  *
- * When the table is marked as immutable, the re-entry assertions will
+ * When the table is marked as immutable, the re-entry checks will
  * no longer trigger erroneously due to multi-threaded access.  Instead,
- * mutations will cause assertions.
+ * mutations will cause checks to fail.
  */
 NS_COM_GLUE void
 PL_DHashMarkTableImmutable(PLDHashTable *table);
-#endif
 
 #ifdef PL_DHASHMETER
 #include <stdio.h>