Bug 1261869 - Fix leaks in devtools; r=ejpbruel

There are two leaks addressed in this commit:

1. The thread actor's `_debuggerSourcesSeen` set was never cleared. This set
exists only as a performance optimization to speed up `_addSource` in cases
where we've already added the source. Unfortunately, this set wasn't getting
cleared when we cleared debuggees out and it ended up keeping the
`Debugger.Source`, its referent, and transitively its referent's global alive. I
figured it was simpler to make it a `WeakSet` than to add it as a special case
in `ThreadActor.prototype._clearDebuggees` and manage the lifetimes by hand. I
think this fits well with its intended use as an ephemeral performance
optimization.

2. Due to a logic error, we were not clearing debuggees in the memory actor's
`Debugger` instance on navigations. This isn't really a "proper" leak, in that
if you forced a GC, the old debuggees would go away as `Debugger` holds them
weakly, however if there was no GC between navigations, then you could still see
the old windows (and everything they "retained") as roots in the snapshot. This
issue is straightforward to fix once identified: ensure that `_clearDebuggees`
is actually called on navigation.

Finally, this commit adds a test that we don't leak Window objects when devtools
are open and we keep refreshing a tab. When it fails, it prints out the leaking
window's retaining paths.

devtools/client/framework/test/shared-head.js

@@ -133,6 +133,19 @@ var removeTab = Task.async(function* (tab) {
   info("Tab removed and finished closing");
 });
 
+/**
+ * Refresh the given tab.
+ * @param {Object} tab The tab to be refreshed.
+ * @return Promise<undefined> resolved when the tab is successfully refreshed.
+ */
+var refreshTab = Task.async(function*(tab) {
+  info("Refreshing tab.");
+  const finished = once(gBrowser.selectedBrowser, "load", true);
+  gBrowser.reloadTab(gBrowser.selectedTab);
+  yield finished;
+  info("Tab finished refreshing.");
+});
+
 /**
  * Simulate a key event from a <key> element.
  * @param {DOMNode} key

devtools/client/memory/test/browser/browser.ini

@@ -4,6 +4,7 @@ subsuite = devtools
 support-files =
   head.js
   doc_big_tree.html
+  doc_empty.html
   doc_steady_allocation.html
   !/devtools/client/framework/test/shared-head.js
   !/devtools/client/framework/test/shared-redux-head.js
@@ -23,6 +24,7 @@ support-files =
 [browser_memory_no_auto_expand.js]
     skip-if = debug # bug 1219554
 [browser_memory_percents_01.js]
+[browser_memory_refresh_does_not_leak.js]
 [browser_memory_simple_01.js]
 [browser_memory_transferHeapSnapshot_e10s_01.js]
 [browser_memory_tree_map-01.js]

devtools/client/memory/test/browser/browser_memory_refresh_does_not_leak.js

@@ -0,0 +1,108 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Test that refreshing the page with devtools open does not leak the old
+// windows from previous navigations.
+//
+// IF THIS TEST STARTS FAILING, YOU ARE LEAKING EVERY WINDOW EVER NAVIGATED TO
+// WHILE DEVTOOLS ARE OPEN! THIS IS NOT SPECIFIC TO THE MEMORY TOOL ONLY!
+
+"use strict";
+
+const HeapSnapshotFileUtils = require("devtools/shared/heapsnapshot/HeapSnapshotFileUtils");
+const { getLabelAndShallowSize } = require("devtools/shared/heapsnapshot/DominatorTreeNode");
+
+const TEST_URL = "http://example.com/browser/devtools/client/memory/test/browser/doc_empty.html";
+
+function* getWindowsInSnapshot(front) {
+  dumpn("Taking snapshot.");
+  const path = yield front.saveHeapSnapshot();
+  dumpn("Took snapshot with path = " + path);
+  const snapshot = ChromeUtils.readHeapSnapshot(path);
+  dumpn("Read snapshot into memory, taking census.");
+  const report = snapshot.takeCensus({
+    breakdown: {
+      by: "objectClass",
+      then: { by: "bucket" },
+      other: { by: "count", count: true, bytes: false },
+    }
+  });
+  dumpn("Took census, window count = " + report.Window.count);
+  return report.Window;
+}
+
+const DESCRIPTION = {
+  by: "coarseType",
+  objects: {
+    by: "objectClass",
+    then: { by: "count", count: true, bytes: false },
+    other: { by: "count", count: true, bytes: false },
+  },
+  strings: { by: "count", count: true, bytes: false },
+  scripts: {
+    by: "internalType",
+    then: { by: "count", count: true, bytes: false },
+  },
+  other: {
+    by: "internalType",
+    then: { by: "count", count: true, bytes: false },
+  }
+};
+
+this.test = makeMemoryTest(TEST_URL, function* ({ tab, panel }) {
+  const heapWorker = panel.panelWin.gHeapAnalysesClient;
+  const front = panel.panelWin.gFront;
+  const store = panel.panelWin.gStore;
+  const { getState, dispatch } = store;
+  const doc = panel.panelWin.document;
+
+  const startWindows = yield getWindowsInSnapshot(front);
+  dumpn("Initial windows found = " + startWindows.map(w => "0x" + w.toString(16)).join(", "));
+  is(startWindows.length, 1);
+
+  yield refreshTab(tab);
+
+  const endWindows = yield getWindowsInSnapshot(front);
+  is(endWindows.length, 1);
+
+  if (endWindows.length === 1) {
+    return;
+  }
+
+  dumpn("Test failed, diagnosing leaking windows.");
+  dumpn("(This may fail if a moving GC has relocated the initial Window objects.)");
+
+  dumpn("Taking full runtime snapshot.");
+  const path = yield front.saveHeapSnapshot({ boundaries: { runtime: true } });
+  dumpn("Full runtime's snapshot path = " + path);
+
+  dumpn("Reading full runtime heap snapshot.");
+  const snapshot = ChromeUtils.readHeapSnapshot(path);
+  dumpn("Done reading full runtime heap snapshot.");
+
+  const dominatorTree = snapshot.computeDominatorTree();
+  const paths = snapshot.computeShortestPaths(dominatorTree.root, startWindows, 50);
+
+  for (let i = 0; i < startWindows.length; i++) {
+    dumpn("Shortest retaining paths for leaking Window 0x" + startWindows[i].toString(16) + " =========================");
+    let j = 0;
+    for (let retainingPath of paths.get(startWindows[i])) {
+      if (retainingPath.find(part => part.predecessor === startWindows[i])) {
+        // Skip paths that loop out from the target window and back to it again.
+        continue;
+      }
+
+      dumpn("    Path #" + (++j) + ": --------------------------------------------------------------------");
+      for (let part of retainingPath) {
+        const { label } = getLabelAndShallowSize(part.predecessor, snapshot, DESCRIPTION);
+        dumpn("        0x" + part.predecessor.toString(16) +
+              " (" + label.join(" > ") + ")");
+        dumpn("               |");
+        dumpn("              " + part.edge);
+        dumpn("               |");
+        dumpn("               V");
+      }
+      dumpn("        0x" + startWindows[i].toString(16) + " (objects > Window)");
+    }
+  }
+});

devtools/client/memory/test/browser/doc_empty.html

@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <meta charset="utf-8"/>
+    </head>
+    <body>
+        This is an empty window.
+    </body>
+</html>

devtools/server/actors/memory.js

@@ -47,8 +47,8 @@ exports.MemoryActor = protocol.ActorClassWithSpec(memorySpec, {
 
   getState: actorBridgeWithSpec("getState"),
 
-  saveHeapSnapshot: function () {
-    return this.bridge.saveHeapSnapshot();
+  saveHeapSnapshot: function (boundaries) {
+    return this.bridge.saveHeapSnapshot(boundaries);
   },
 
   takeCensus: actorBridgeWithSpec("takeCensus"),

devtools/server/actors/script.js

@@ -620,7 +620,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
     }
 
     this._state = "attached";
-    this._debuggerSourcesSeen = new Set();
+    this._debuggerSourcesSeen = new WeakSet();
 
     Object.assign(this._options, aRequest.options || {});
     this.sources.setOptions(this._options);

devtools/server/performance/memory.js

@@ -120,8 +120,8 @@ var Memory = exports.Memory = Class({
    */
   _onWindowReady: function ({ isTopLevel }) {
     if (this.state == "attached") {
+      this._clearDebuggees();
       if (isTopLevel && this.isRecordingAllocations()) {
-        this._clearDebuggees();
         this._frameCache.initFrames();
       }
       this.dbg.addDebuggees();
@@ -140,15 +140,19 @@ var Memory = exports.Memory = Class({
    * Save a heap snapshot scoped to the current debuggees' portion of the heap
    * graph.
    *
+   * @param {Object|null} boundaries
+   *
    * @returns {String} The snapshot id.
    */
-  saveHeapSnapshot: expectState("attached", function () {
+  saveHeapSnapshot: expectState("attached", function (boundaries = null) {
     // If we are observing the whole process, then scope the snapshot
     // accordingly. Otherwise, use the debugger's debuggees.
-    const opts = this.parent instanceof ChromeActor || this.parent instanceof ChildProcessActor
-      ? { runtime: true }
-      : { debugger: this.dbg };
-    const path = ThreadSafeChromeUtils.saveHeapSnapshot(opts);
+    if (!boundaries) {
+      boundaries = this.parent instanceof ChromeActor || this.parent instanceof ChildProcessActor
+        ? { runtime: true }
+        : { debugger: this.dbg };
+    }
+    const path = ThreadSafeChromeUtils.saveHeapSnapshot(boundaries);
     return HeapSnapshotFileUtils.getSnapshotIdFromPath(path);
   }, "saveHeapSnapshot"),
 

devtools/shared/fronts/memory.js

@@ -35,10 +35,14 @@ const MemoryFront = protocol.FrontClassWithSpec(memorySpec, {
    *         Always force a bulk data copy of the saved heap snapshot, even when
    *         the server and client share a file system.
    *
+   * @params {Object|undefined} options.boundaries
+   *         The boundaries for the heap snapshot. See
+   *         ThreadSafeChromeUtils.webidl for more details.
+   *
    * @returns Promise<String>
    */
   saveHeapSnapshot: protocol.custom(Task.async(function* (options = {}) {
-    const snapshotId = yield this._saveHeapSnapshotImpl();
+    const snapshotId = yield this._saveHeapSnapshotImpl(options.boundaries);
 
     if (!options.forceCopy &&
         (yield HeapSnapshotFileUtils.haveHeapSnapshotTempFile(snapshotId))) {

devtools/shared/specs/memory.js

@@ -111,6 +111,9 @@ const memorySpec = generateActorSpec({
       response: { value: RetVal("number") }
     },
     saveHeapSnapshot: {
+      request: {
+        boundaries: Arg(0, "nullable:json")
+      },
       response: {
         snapshotId: RetVal("string")
       }