Bug 1539502 - [devtools] Prevent leaking all transient global while recording memory leaks. r=jdescottes

This is two reason for leaking the globals:
* The JIT/CachedIR that are introduced by trackingAllocationSites=true
and can be fixed by calling minimizeMemoryUsage to purge CachedIR objects.
* The Debugger::allocationsLog which relates to Debugger.Memory.drainAllocationLog
and can be fixed by calling drainAllocationLog to clear allocationsLog.
We also have to be careful about disable allocation site recording while doing the GCs.
On start and on stop.

Both were keeping strong references to the globals.

In this patch, I'm also extending the coverage of the AllocationTracker
to better assert what precise leaks are reported.

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

Author: ochameau

devtools/client/framework/test/allocations/head.js

@@ -106,6 +106,9 @@ async function startRecordingAllocations({
         await tracker.startRecordingAllocations(debug_allocations);
       }
     );
+    // Trigger a GC in the parent process as this additional ContentTask
+    // seems to make harder to release objects created before we start recording.
+    await tracker.doGC();
   }
 
   await tracker.startRecordingAllocations(DEBUG_ALLOCATIONS);

devtools/shared/test-helpers/allocation-tracker.js

@@ -146,7 +146,15 @@ exports.allocationTracker = function({
     async startRecordingAllocations(debug_allocations) {
       // Do a first pass of GC, to ensure all to-be-freed objects from the first run
       // are really freed.
+      // We have to temporarily disable allocation-site recording in order to ensure
+      // freeing everything and especially avoid retaining objects in the allocation-log
+      // related to `drainAllocationLog` feature.
+      dbg.memory.allocationSamplingProbability = 0.0;
+      // Also force clearing the allocation log in order to prevent holding alive globals
+      // which have been destroyed before we start recording
+      this.flushAllocations();
       await this.doGC();
+      dbg.memory.allocationSamplingProbability = 1.0;
 
       // Measure the current process memory usage
       const memory = this.getAllocatedMemory();
@@ -172,16 +180,23 @@ exports.allocationTracker = function({
     },
 
     async stopRecordingAllocations(debug_allocations) {
+      // We have to flush the allocation log in order to prevent leaking some objects
+      // being hold in memory solely by their allocation-site (i.e. `SavedFrame` in `Debugger::allocationsLog`)
+      if (debug_allocations != "allocations") {
+        this.flushAllocations();
+      }
+
       // Before computing allocations, re-do some GCs in order to free all what is to-be-freed.
       await this.doGC();
 
       const memory = this.getAllocatedMemory();
       const objects = this.stillAllocatedObjects();
 
+      let leaks;
       if (debug_allocations == "allocations") {
         this.logAllocationLog();
       } else if (debug_allocations == "leaks") {
-        this.logAllocationSitesDiff(this.data.allocations);
+        leaks = this.logAllocationSitesDiff(this.data.allocations);
       }
 
       return {
@@ -190,6 +205,7 @@ exports.allocationTracker = function({
         objectsWithStack:
           objects.objectsWithStack - this.data.objects.objectsWithStack,
         memory: memory - this.data.memory,
+        leaks,
       };
     },
 
@@ -307,6 +323,7 @@ exports.allocationTracker = function({
           JSON.stringify(allocationList, null, 2) +
           "\n"
       );
+      return allocationList;
     },
 
     /**
@@ -436,6 +453,15 @@ exports.allocationTracker = function({
         // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
         await new Promise(resolve => setTimeout(resolve, 1000));
       }
+
+      // Also call minimizeMemoryUsage as that's the only way to purge JIT cache.
+      // CachedIR objects (JIT related objects) are ultimately leading to keep
+      // all transient globals in memory. For some reason, when enabling trackingAllocationSites=true
+      // we compute stack traces (SavedFrame) for each object being allocated.
+      // This either create new CachedIR -or- force holding alive existing CachedIR
+      // and CachedIR itself hold strong references to the transient globals.
+      // See bug 1733480.
+      await new Promise(resolve => MemoryReporter.minimizeMemoryUsage(resolve));
     },
 
     /**

devtools/shared/test-helpers/browser.ini

@@ -5,4 +5,4 @@ support-files =
   allocation-tracker.js
 
 [browser_allocation_tracker.js]
-skip-if = verify # Bug 1730507 - objects without stacks get allocated during the GC of the first test when running multiple times
+skip-if = debug | verify # Bug 1730507 - objects without stacks get allocated during the GC of the first test when running multiple times. Also avoid running in debug as we don't try to track memory from debug builds.

devtools/shared/test-helpers/browser_allocation_tracker.js

@@ -20,6 +20,10 @@ const TrackedObjects = loader.require(
   "devtools/shared/test-helpers/tracked-objects.jsm"
 );
 
+// This test record multiple times complete heap snapshot,
+// so that it can take a little bit to complete.
+requestLongerTimeout(2);
+
 add_task(async function() {
   // Use a sandbox to allocate test javascript object in order to avoid any
   // external noise
@@ -150,3 +154,97 @@ add_task(async function() {
 
   TrackedObjects.clear();
 });
+
+add_task(async function() {
+  info("Test start and stop recording with tracked objects");
+
+  const sandbox = Cu.Sandbox(window);
+  const tracker = allocationTracker({ watchGlobal: sandbox });
+  await tracker.startRecordingAllocations("leaks");
+
+  Cu.evalInSandbox("this.foo = {};", sandbox, null, "sandbox.js", 1);
+
+  const record = await tracker.stopRecordingAllocations("leaks");
+  is(
+    record.objectsWithStack,
+    1,
+    "We get only one leaked objects, the foo object of the sandbox."
+  );
+  ok(
+    record.objectsWithoutStack > 10,
+    "We get an handful of objects without stacks. Most likely created by Memory API itself."
+  );
+
+  is(
+    record.leaks.length,
+    2,
+    "We get the one leak and the objects with missing stacks"
+  );
+  is(
+    record.leaks[0].src,
+    "UNKNOWN",
+    "First item is the objects with missing stacks"
+  );
+  // In theory the two following values should be equal,
+  // but they aren't always because of some dark matter around objects with missing stacks.
+  // `count` is computed out of `takeCensus`, while `objectsWithoutStack` uses `drainAllocationsLog`
+  // While the first go through the current GC graph, the second is a record of allocations over time,
+  // this probably explain why there is some subtle difference
+  ok(
+    record.leaks[0].count <= record.objectsWithoutStack,
+    "For now, the leak report intermittently assume there is less leaked objects than the summary"
+  );
+  is(record.leaks[1].src, "sandbox.js", "Second item if about our 'foo' leak");
+  is(record.leaks[1].count, 1, "We leak one object on this file");
+  is(record.leaks[1].lines.length, 1, "We leak from only one line");
+  is(record.leaks[1].lines[0], "1: 1", "On first line, we leak one object");
+  tracker.stop();
+
+  TrackedObjects.clear();
+});
+
+add_task(async function() {
+  info("Test that transient globals are not leaked");
+
+  const tracker = allocationTracker({ watchAllGlobals: true });
+
+  let sandboxBefore = Cu.Sandbox(window);
+  // We need to allocate at least one object from the global to reproduce the leak
+  Cu.evalInSandbox(
+    "this.foo = {};",
+    sandboxBefore,
+    null,
+    "sandbox-before.js",
+    1
+  );
+  const weakBefore = Cu.getWeakReference(sandboxBefore);
+  sandboxBefore = null;
+
+  await tracker.startRecordingAllocations();
+
+  ok(
+    !weakBefore.get(),
+    "Sandbox created before the record should have been freed by GCs done by startRecordingAllocations"
+  );
+
+  let sandboxDuring = Cu.Sandbox(window);
+  // We need to allocate at least one object from the global to reproduce the leak
+  Cu.evalInSandbox(
+    "this.bar = {};",
+    sandboxDuring,
+    null,
+    "sandbox-during.js",
+    1
+  );
+  const weakDuring = Cu.getWeakReference(sandboxDuring);
+  sandboxDuring = null;
+
+  await tracker.stopRecordingAllocations();
+
+  ok(
+    !weakDuring.get(),
+    "Sandbox should have been freed by GCs done by stopRecordingAllocations"
+  );
+
+  tracker.stop();
+});