Bug 1746090 - Annotate edges that invoke destructors on ref-counted values that can be proven to not Release r=jonco

Differential Revision: https://phabricator.services.mozilla.com/D133802

js/src/devtools/rootAnalysis/annotations.js

@@ -17,7 +17,7 @@ var ignoreIndirectCalls = {
 };
 
 // Types that when constructed with no arguments, are "safe" values (they do
-// not contain GC pointers).
+// not contain GC pointers, or values with nontrivial destructors.)
 var typesWithSafeConstructors = new Set([
     "mozilla::Maybe",
     "mozilla::dom::Nullable",
@@ -32,8 +32,14 @@ var resetterMethods = {
     'js::UniquePtr': new Set(["reset"]),
     'mozilla::dom::Nullable': new Set(["SetNull"]),
     'mozilla::dom::TypedArray_base': new Set(["Reset"]),
+    'RefPtr': new Set(["forget"]),
+    'nsCOMPtr': new Set(["forget"]),
 };
 
+function isRefcountedDtor(name) {
+    return name.includes("::~RefPtr(") || name.includes("::~nsCOMPtr(");
+}
+
 function indirectCallCannotGC(fullCaller, fullVariable)
 {
     var caller = readable(fullCaller);

js/src/devtools/rootAnalysis/computeCallgraph.js

@@ -128,7 +128,7 @@ function getAnnotations(functionName, body) {
 
 // Scan through a function body, pulling out all annotations and calls and
 // recording them in callgraph.txt.
-function processBody(functionName, body)
+function processBody(functionName, body, functionBodies)
 {
     if (!('PEdge' in body))
         return;
@@ -161,6 +161,13 @@ function processBody(functionName, body)
         var edgeAttrs = body.attrs[edge.Index[0]] | 0;
 
         for (var callee of getCallees(edge)) {
+            // Special-case some calls when we can derive more information about them, eg
+            // that they are a destructor that won't do anything.
+            if (callee.kind === "direct" && edgeIsNonReleasingDtor(body, edge, callee.name, functionBodies)) {
+                const block = blockIdentifier(body);
+                addToKeyedList(gcEdges, block, { Index: edge.Index, attrs: ATTR_GC_SUPPRESSED | ATTR_NONRELEASING });
+            }
+
             // Individual callees may have additional attrs. The only such
             // bit currently is that nsISupports.{AddRef,Release} are assumed
             // to never GC.
@@ -300,7 +307,7 @@ function process(functionName, functionBodies)
     }
 
     for (var body of functionBodies)
-        processBody(functionName, body);
+        processBody(functionName, body, functionBodies);
 
     // Not strictly necessary, but add an edge from the synthetic "(js-code)"
     // to RunScript to allow better stacks than just randomly selecting a

js/src/devtools/rootAnalysis/t/hazards/source.cpp

@@ -367,3 +367,34 @@ class Subcell : public Cell {
     return f;  // this->f
   }
 };
+
+template <typename T>
+struct RefPtr {
+  ~RefPtr() { GC(); }
+  void forget() {}
+};
+
+Cell* refptr_test1() {
+  Cell cell;
+  RefPtr<float> v1;
+  Cell* ref_unsafe1 = &cell;
+  return ref_unsafe1;
+}
+
+Cell* refptr_test2() {
+  Cell cell;
+  RefPtr<float> v2;
+  Cell* ref_safe2 = &cell;
+  v2.forget();
+  return ref_safe2;
+}
+
+Cell* refptr_test3() {
+  Cell cell;
+  RefPtr<float> v3;
+  Cell* ref_unsafe3 = &cell;
+  if (x) {
+    v3.forget();
+  }
+  return ref_unsafe3;
+}

js/src/devtools/rootAnalysis/t/hazards/test.py

@@ -26,10 +26,10 @@ assert "cell6" not in hazmap
 assert "<returnvalue>" in hazmap
 assert "this" in hazmap
 
-# All hazards should be in f(), loopy(), safevals(), and method()
+# All hazards should be in f(), loopy(), safevals(), method(), and refptr_test{1,3}()
 assert hazmap["cell2"].function == "Cell* f()"
 print(len(set(haz.function for haz in hazards)))
-assert len(set(haz.function for haz in hazards)) == 4
+assert len(set(haz.function for haz in hazards)) == 6
 
 # Check that the correct GC call is reported for each hazard. (cell3 has a
 # hazard from two different GC calls; it doesn't really matter which is
@@ -39,8 +39,11 @@ assert hazmap["cell3"].GCFunction in (
     "void halfSuppressedFunction()",
     "void unsuppressedFunction()",
 )
-assert hazmap["<returnvalue>"].GCFunction.startswith(
+returnval_hazards = set(
-    "void GCInDestructor::~GCInDestructor()"
+    haz.function for haz in hazards if haz.variable == "<returnvalue>"
+)
+assert returnval_hazards == set(
+    ["Cell* f()", "Cell* refptr_test1()", "Cell* refptr_test3()"]
 )
 
 assert "container1" in hazmap

js/src/devtools/rootAnalysis/utility.js

@@ -14,6 +14,7 @@ loadRelativeToScript('dumpCFG.js');
 var ATTR_GC_SUPPRESSED     = 1;
 var ATTR_CANSCRIPT_BOUNDED = 2; // Unimplemented
 var ATTR_DOM_ITERATING     = 4; // Unimplemented
+var ATTR_NONRELEASING      = 8; // ~RefPtr of value whose refcount will not go to zero
 
 var ATTRS_NONE             = 0;
 var ATTRS_ALL              = 7; // All possible bits set