Bug 1132502 (part 2) - Don't call AddClassInfo() for BaseShapes. r=jandem.
authorNicholas Nethercote <nnethercote@mozilla.com>
Tue, 19 Apr 2016 10:21:00 +1000
changeset 331703 ffb1d08f2bd00a9490f76c89c58325e32c199066
parent 331702 a4c2e710a68f5c1f148fb2753b29bf4be8bb908f
child 331704 ff00656a1bda864d16795701f976612100bc9baf
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1132502
milestone48.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1132502 (part 2) - Don't call AddClassInfo() for BaseShapes. r=jandem. It appears to cause crashes, and the effects of not calling on memory reporting accuracy are minor. The code should be able to be re-enabled once heap-allocated js::Class instances no longer occur.
js/src/vm/MemoryMetrics.cpp
js/xpconnect/src/XPCJSRuntime.cpp
js/xpconnect/src/XPCMaps.cpp
--- a/js/src/vm/MemoryMetrics.cpp
+++ b/js/src/vm/MemoryMetrics.cpp
@@ -510,19 +510,50 @@ StatsCellCallback(JSRuntime* rt, void* d
         CompartmentStats& cStats = base->compartment()->compartmentStats();
 
         JS::ClassInfo info;        // This zeroes all the sizes.
         info.shapesGCHeapBase += thingSize;
         // No malloc-heap measurements.
 
         cStats.classInfo.add(info);
 
-        const Class* clasp = base->clasp();
-        const char* className = clasp->name;
-        AddClassInfo(granularity, cStats, className, info);
+        // XXX: This code is currently disabled because it occasionally causes
+        // crashes (bug 1132502 and bug 1243529). The best theory as to why is
+        // as follows.
+        //
+        // - XPCNativeScriptableShared have heap-allocated js::Class instances.
+        //
+        // - Once an XPCNativeScriptableShared is destroyed, its js::Class is
+        //   freed, but we can still have a BaseShape with a clasp_ pointer
+        //   that points to the freed js::Class.
+        //
+        // - This dangling pointer isn't used in normal execution, because the
+        //   BaseShape is unreachable.
+        //
+        // - However, memory reporting inspects all GC cells, reachable or not,
+        //   so we trace the dangling pointer and crash.
+        //
+        // One solution would be to mark BaseShapes whose js::Class is
+        // heap-allocated, and skip this code just for them. However, that's a
+        // non-trivial change, and heap-allocated js::Class instances are
+        // likely to go away soon.
+        //
+        // So for now we just skip this code for all BaseShapes. The
+        // consequence is that all BaseShapes will show up in about:memory
+        // under "class(<non-notable classes>)" sub-trees, instead of the more
+        // appropriate, class-specific "class(Foo)" sub-tree. But BaseShapes
+        // typically don't take up that much memory so this isn't a big deal.
+        //
+        // XXX: once bug 1265271 is done this code should be re-enabled.
+        //
+        if (0) {
+            const Class* clasp = base->clasp();
+            const char* className = clasp->name;
+            AddClassInfo(granularity, cStats, className, info);
+        }
         break;
       }
 
       case JS::TraceKind::JitCode: {
         zStats->jitCodesGCHeap += thingSize;
         // The code for a script is counted in ExecutableAllocator::sizeOfCode().
         break;
       }
@@ -540,20 +571,24 @@ StatsCellCallback(JSRuntime* rt, void* d
         JS::ClassInfo info;        // This zeroes all the sizes.
         if (shape->inDictionary())
             info.shapesGCHeapDict += thingSize;
         else
             info.shapesGCHeapTree += thingSize;
         shape->addSizeOfExcludingThis(rtStats->mallocSizeOf_, &info);
         cStats.classInfo.add(info);
 
-        const BaseShape* base = shape->base();
-        const Class* clasp = base->clasp();
-        const char* className = clasp->name;
-        AddClassInfo(granularity, cStats, className, info);
+        // XXX: once bug 1265271 is done, occur, this code should be
+        // re-enabled. (See the big comment on the BaseShape case above.)
+        if (0) {
+            const BaseShape* base = shape->base();
+            const Class* clasp = base->clasp();
+            const char* className = clasp->name;
+            AddClassInfo(granularity, cStats, className, info);
+        }
         break;
       }
 
       case JS::TraceKind::ObjectGroup: {
         ObjectGroup* group = static_cast<ObjectGroup*>(thing);
         zStats->objectGroupsGCHeap += thingSize;
         zStats->objectGroupsMallocHeap += group->sizeOfExcludingThis(rtStats->mallocSizeOf_);
         break;
--- a/js/xpconnect/src/XPCJSRuntime.cpp
+++ b/js/xpconnect/src/XPCJSRuntime.cpp
@@ -2121,18 +2121,18 @@ ReportZoneStats(const JS::ZoneStats& zSt
 
     return NS_OK;
 
 #   undef STRING_LENGTH
 }
 
 static nsresult
 ReportClassStats(const ClassInfo& classInfo, const nsACString& path,
-                 nsIHandleReportCallback* cb, nsISupports* closure,
-                 size_t& gcTotal)
+                 const nsACString& shapesPath, nsIHandleReportCallback* cb,
+                 nsISupports* closure, size_t& gcTotal)
 {
     // We deliberately don't use ZCREPORT_BYTES, so that these per-class values
     // don't go into sundries.
 
     if (classInfo.objectsGCHeap > 0) {
         REPORT_GC_BYTES(path + NS_LITERAL_CSTRING("objects/gc-heap"),
             classInfo.objectsGCHeap,
             "Objects, including fixed slots.");
@@ -2192,47 +2192,47 @@ ReportClassStats(const ClassInfo& classI
 
     if (classInfo.objectsMallocHeapMisc > 0) {
         REPORT_BYTES(path + NS_LITERAL_CSTRING("objects/malloc-heap/misc"),
             KIND_HEAP, classInfo.objectsMallocHeapMisc,
             "Miscellaneous object data.");
     }
 
     if (classInfo.shapesGCHeapTree > 0) {
-        REPORT_GC_BYTES(path + NS_LITERAL_CSTRING("shapes/gc-heap/tree"),
+        REPORT_GC_BYTES(shapesPath + NS_LITERAL_CSTRING("shapes/gc-heap/tree"),
             classInfo.shapesGCHeapTree,
         "Shapes in a property tree.");
     }
 
     if (classInfo.shapesGCHeapDict > 0) {
-        REPORT_GC_BYTES(path + NS_LITERAL_CSTRING("shapes/gc-heap/dict"),
+        REPORT_GC_BYTES(shapesPath + NS_LITERAL_CSTRING("shapes/gc-heap/dict"),
             classInfo.shapesGCHeapDict,
         "Shapes in dictionary mode.");
     }
 
     if (classInfo.shapesGCHeapBase > 0) {
-        REPORT_GC_BYTES(path + NS_LITERAL_CSTRING("shapes/gc-heap/base"),
+        REPORT_GC_BYTES(shapesPath + NS_LITERAL_CSTRING("shapes/gc-heap/base"),
             classInfo.shapesGCHeapBase,
             "Base shapes, which collate data common to many shapes.");
     }
 
     if (classInfo.shapesMallocHeapTreeTables > 0) {
-        REPORT_BYTES(path + NS_LITERAL_CSTRING("shapes/malloc-heap/tree-tables"),
+        REPORT_BYTES(shapesPath + NS_LITERAL_CSTRING("shapes/malloc-heap/tree-tables"),
             KIND_HEAP, classInfo.shapesMallocHeapTreeTables,
             "Property tables of shapes in a property tree.");
     }
 
     if (classInfo.shapesMallocHeapDictTables > 0) {
-        REPORT_BYTES(path + NS_LITERAL_CSTRING("shapes/malloc-heap/dict-tables"),
+        REPORT_BYTES(shapesPath + NS_LITERAL_CSTRING("shapes/malloc-heap/dict-tables"),
             KIND_HEAP, classInfo.shapesMallocHeapDictTables,
             "Property tables of shapes in dictionary mode.");
     }
 
     if (classInfo.shapesMallocHeapTreeKids > 0) {
-        REPORT_BYTES(path + NS_LITERAL_CSTRING("shapes/malloc-heap/tree-kids"),
+        REPORT_BYTES(shapesPath + NS_LITERAL_CSTRING("shapes/malloc-heap/tree-kids"),
             KIND_HEAP, classInfo.shapesMallocHeapTreeKids,
             "Kid hashes of shapes in a property tree.");
     }
 
     return NS_OK;
 }
 
 static nsresult
@@ -2270,28 +2270,33 @@ ReportCompartmentStats(const JS::Compart
         }
     }
 
     nsCString nonNotablePath = cJSPathPrefix;
     nonNotablePath += cStats.isTotals
                     ? NS_LITERAL_CSTRING("classes/")
                     : NS_LITERAL_CSTRING("classes/class(<non-notable classes>)/");
 
-    rv = ReportClassStats(cStats.classInfo, nonNotablePath, cb, closure,
-                          gcTotal);
+    // XXX: shapes need special treatment until bug 1265271 is fixed.
+    nsCString shapesPath = cJSPathPrefix;
+    shapesPath += NS_LITERAL_CSTRING("classes/");
+
+    rv = ReportClassStats(cStats.classInfo, nonNotablePath, shapesPath, cb,
+                          closure, gcTotal);
     NS_ENSURE_SUCCESS(rv, rv);
 
     for (size_t i = 0; i < cStats.notableClasses.length(); i++) {
         MOZ_ASSERT(!cStats.isTotals);
         const JS::NotableClassInfo& classInfo = cStats.notableClasses[i];
 
         nsCString classPath = cJSPathPrefix +
             nsPrintfCString("classes/class(%s)/", classInfo.className_);
 
-        rv = ReportClassStats(classInfo, classPath, cb, closure, gcTotal);
+        rv = ReportClassStats(classInfo, classPath, shapesPath, cb, closure,
+                              gcTotal);
         NS_ENSURE_SUCCESS(rv, rv);
     }
 
     // Note that we use cDOMPathPrefix here.  This is because we measure orphan
     // DOM nodes in the JS reporter, but we want to report them in a "dom"
     // sub-tree rather than a "js" sub-tree.
     ZCREPORT_BYTES(cDOMPathPrefix + NS_LITERAL_CSTRING("orphan-nodes"),
         cStats.objectsPrivate,
--- a/js/xpconnect/src/XPCMaps.cpp
+++ b/js/xpconnect/src/XPCMaps.cpp
@@ -545,16 +545,25 @@ XPCNativeScriptableSharedMap::GetNewOrUs
 
     XPCNativeScriptableShared key(flags, name, /* populate = */ false);
     auto entry = static_cast<Entry*>(mTable->Add(&key, fallible));
     if (!entry)
         return false;
 
     XPCNativeScriptableShared* shared = entry->key;
 
+    // XXX: this XPCNativeScriptableShared is heap-allocated, which means the
+    // js::Class it contains is also heap-allocated. This causes problems for
+    // memory reporting. See the comment above the BaseShape case in
+    // StatsCellCallback() in js/src/vm/MemoryMetrics.cpp.
+    //
+    // When the code below is removed (bug 1265271) and there are no longer any
+    // heap-allocated js::Class instances, the disabled code in
+    // StatsCellCallback() should be reinstated.
+    //
     if (!shared) {
         entry->key = shared =
             new XPCNativeScriptableShared(flags, key.TransferNameOwnership(),
                                           /* populate = */ true);
     }
     si->SetScriptableShared(shared);
     return true;
 }