Bug 1201597 - Part 0: Make saveHeapSnapshot return the file path rather than take it as a parameter; r=bholley

This changeset modifies the ThreadSafeChromeUtils::saveHeapSnapshot webidl
method to return the path to the core dump file where the heap snapshot was
serialized rather than taking the file path as a parameter.

By removing the ability for callers to choose a path, we pave the way for
enabling taking heap snapshots in sandboxed child processes (e10s, fxos) that do
not have access to the filesystem directly and must be handed a file descriptor
over IPDL. Additionally, the devtools API consumers were not taking advantage of
the ability to choose a file path, and always saving heap snapshots into the
temp directory anyways.

dom/base/ChromeUtils.h

@@ -23,14 +23,14 @@ namespace dom {
 class ThreadSafeChromeUtils
 {
 public:
-  // Implemented in toolkit/devtools/server/HeapSnapshot.cpp
+  // Implemented in toolkit/devtools/heapsnapshot/HeapSnapshot.cpp
   static void SaveHeapSnapshot(GlobalObject& global,
                                JSContext* cx,
-                               const nsAString& filePath,
                                const HeapSnapshotBoundaries& boundaries,
+                               nsAString& filePath,
                                ErrorResult& rv);
 
-  // Implemented in toolkit/devtools/server/HeapSnapshot.cpp
+  // Implemented in toolkit/devtools/heapsnapshot/HeapSnapshot.cpp
   static already_AddRefed<devtools::HeapSnapshot> ReadHeapSnapshot(GlobalObject& global,
                                                                    JSContext* cx,
                                                                    const nsAString& filePath,

dom/webidl/ThreadSafeChromeUtils.webidl

@@ -14,13 +14,15 @@ interface ThreadSafeChromeUtils {
    * Serialize a snapshot of the heap graph, as seen by |JS::ubi::Node| and
    * restricted by |boundaries|, and write it to the provided file path.
    *
-   * @param filePath          The file path to write the heap snapshot to.
-   *
    * @param boundaries        The portion of the heap graph to write.
+   *
+   * @returns                 The path to the file the heap snapshot was written
+   *                          to. This is guaranteed to be within the temp
+   *                          directory and its file name will match the regexp
+   *                          `\d+(\-\d+)?\.fxsnapshot`.
    */
   [Throws]
-  static void saveHeapSnapshot(DOMString filePath,
-                               optional HeapSnapshotBoundaries boundaries);
+  static DOMString saveHeapSnapshot(optional HeapSnapshotBoundaries boundaries);
 
   /**
    * Deserialize a core dump into a HeapSnapshot.

toolkit/devtools/heapsnapshot/HeapSnapshot.cpp

@@ -28,10 +28,12 @@
 #include "jsapi.h"
 #include "nsCycleCollectionParticipant.h"
 #include "nsCRTGlue.h"
+#include "nsDirectoryServiceDefs.h"
+#include "nsIFile.h"
 #include "nsIOutputStream.h"
 #include "nsISupportsImpl.h"
 #include "nsNetUtil.h"
-#include "nsIFile.h"
+#include "nsPrintfCString.h"
 #include "prerror.h"
 #include "prio.h"
 #include "prtypes.h"
@@ -815,11 +817,45 @@ namespace dom {
 using namespace JS;
 using namespace devtools;
 
+static unsigned long
+msSinceProcessCreation(const TimeStamp& now)
+{
+  bool ignored;
+  auto duration = now - TimeStamp::ProcessCreation(ignored);
+  return (unsigned long) duration.ToMilliseconds();
+}
+
+// Creates the `$TEMP_DIR/XXXXXX-XXX.fxsnapshot` core dump file that heap
+// snapshots are serialized into.
+static already_AddRefed<nsIFile>
+createUniqueCoreDumpFile(ErrorResult& rv, const TimeStamp& now, nsAString& outFilePath)
+{
+  nsCOMPtr<nsIFile> file;
+  rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(file));
+  if (NS_WARN_IF(rv.Failed()))
+    return nullptr;
+
+  auto ms = msSinceProcessCreation(now);
+  rv = file->AppendNative(nsPrintfCString("%lu.fxsnapshot", ms));
+  if (NS_WARN_IF(rv.Failed()))
+    return nullptr;
+
+  rv = file->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0666);
+  if (NS_WARN_IF(rv.Failed()))
+    return nullptr;
+
+  rv = file->GetPath(outFilePath);
+  if (NS_WARN_IF(rv.Failed()))
+    return nullptr;
+
+  return file.forget();
+}
+
 /* static */ void
 ThreadSafeChromeUtils::SaveHeapSnapshot(GlobalObject& global,
                                         JSContext* cx,
-                                        const nsAString& filePath,
                                         const HeapSnapshotBoundaries& boundaries,
+                                        nsAString& outFilePath,
                                         ErrorResult& rv)
 {
   auto start = TimeStamp::Now();
@@ -829,8 +865,7 @@ ThreadSafeChromeUtils::SaveHeapSnapshot(GlobalObject& global,
   uint32_t nodeCount = 0;
   uint32_t edgeCount = 0;
 
-  nsCOMPtr<nsIFile> file;
-  rv = NS_NewLocalFile(filePath, false, getter_AddRefs(file));
+  nsCOMPtr<nsIFile> file = createUniqueCoreDumpFile(rv, start, outFilePath);
   if (NS_WARN_IF(rv.Failed()))
     return;
 

toolkit/devtools/heapsnapshot/tests/mochitest/test_SaveHeapSnapshot.html

@@ -15,13 +15,7 @@ Bug 1024774 - Sanity test that we can take a heap snapshot in a web window.
 SimpleTest.waitForExplicitFinish();
 window.onload = function() {
   ok(ChromeUtils, "The ChromeUtils interface should be exposed in chrome windows.");
-
-  var file = Components.classes["@mozilla.org/file/directory_service;1"]
-    .getService(Components.interfaces.nsIProperties)
-    .get("CurWorkD", Components.interfaces.nsILocalFile);
-  file.append("core-dump.tmp");
-
-  ChromeUtils.saveHeapSnapshot(file.path, { runtime: true });
+  ChromeUtils.saveHeapSnapshot({ runtime: true });
   ok(true, "Should save a heap snapshot and shouldn't throw.");
   SimpleTest.finish();
 };

toolkit/devtools/heapsnapshot/tests/unit/head_heapsnapshot.js

@@ -106,13 +106,10 @@ function getFilePath(aName, aAllowMissing=false, aUsePlatformPathSeparator=false
   return path;
 }
 
-function saveNewHeapSnapshot(fileName=`core-dump-${Math.random()}.tmp`) {
-  const filePath = getFilePath(fileName, true, true);
+function saveNewHeapSnapshot() {
+  const filePath = ChromeUtils.saveHeapSnapshot({ runtime: true });
   ok(filePath, "Should get a file path to save the core dump to.");
-
-  ChromeUtils.saveHeapSnapshot(filePath, { runtime: true });
   ok(true, "Saved a heap snapshot to " + filePath);
-
   return filePath;
 }
 
@@ -135,16 +132,10 @@ function saveNewHeapSnapshot(fileName=`core-dump-${Math.random()}.tmp`) {
  *
  * @returns Census
  */
-function saveHeapSnapshotAndTakeCensus(dbg=null, censusOptions=undefined,
-                                       // Add the Math.random() so that parallel
-                                       // tests are less likely to mess with
-                                       // each other.
-                                       fileName="core-dump-" + (Math.random()) + ".tmp") {
-  const filePath = getFilePath(fileName, true, true);
-  ok(filePath, "Should get a file path to save the core dump to.");
-
+function saveHeapSnapshotAndTakeCensus(dbg=null, censusOptions=undefined) {
   const snapshotOptions = dbg ? { debugger: dbg } : { runtime: true };
-  ChromeUtils.saveHeapSnapshot(filePath, snapshotOptions);
+  const filePath = ChromeUtils.saveHeapSnapshot(snapshotOptions);
+  ok(filePath, "Should get a file path to save the core dump to.");
   ok(true, "Should have saved a heap snapshot to " + filePath);
 
   const snapshot = ChromeUtils.readHeapSnapshot(filePath);

toolkit/devtools/heapsnapshot/tests/unit/heap-snapshot-worker.js

@@ -6,8 +6,6 @@ console.log("Initializing worker.");
 self.onmessage = e => {
   console.log("Starting test.");
   try {
-    const { filePath } = e.data;
-
     ok(typeof ChromeUtils === "undefined",
        "Should not have access to ChromeUtils in a worker.");
     ok(ThreadSafeChromeUtils,
@@ -15,7 +13,7 @@ self.onmessage = e => {
     ok(HeapSnapshot,
        "Should have access to HeapSnapshot in a worker.");
 
-    ThreadSafeChromeUtils.saveHeapSnapshot(filePath, { globals: [this] });
+    const filePath = ThreadSafeChromeUtils.saveHeapSnapshot({ globals: [this] });
     ok(true, "Should be able to save a snapshot.");
 
     const snapshot = ThreadSafeChromeUtils.readHeapSnapshot(filePath);

toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot.js

@@ -9,10 +9,7 @@ if (typeof Debugger != "function") {
 }
 
 function run_test() {
-  const filePath = getFilePath("core-dump-" + Math.random() + ".tmp", true, true);
-  ok(filePath, "Should get a file path");
-
-  ChromeUtils.saveHeapSnapshot(filePath, { globals: [this] });
+  const filePath = ChromeUtils.saveHeapSnapshot({ globals: [this] });
   ok(true, "Should be able to save a snapshot.");
 
   const snapshot = ChromeUtils.readHeapSnapshot(filePath);

toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot_with_allocations.js

@@ -25,10 +25,7 @@ function run_test() {
   // Now save a snapshot that will include the allocation stacks and read it
   // back again.
 
-  const filePath = getFilePath("core-dump-" + Math.random() + ".tmp", true, true);
-  ok(filePath, "Should get a file path");
-
-  ChromeUtils.saveHeapSnapshot(filePath, { runtime: true });
+  const filePath = ChromeUtils.saveHeapSnapshot({ runtime: true });
   ok(true, "Should be able to save a snapshot.");
 
   const snapshot = ChromeUtils.readHeapSnapshot(filePath);

toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot_worker.js

@@ -4,11 +4,8 @@
 // Test that we can read core dumps into HeapSnapshot instances in a worker.
 
 add_task(function* () {
-  const filePath = getFilePath("core-dump-" + Math.random() + ".tmp", true, true);
-  ok(filePath, "Should get a file path");
-
   const worker = new ChromeWorker("resource://test/heap-snapshot-worker.js");
-  worker.postMessage({ filePath });
+  worker.postMessage({});
 
   let assertionCount = 0;
   worker.onmessage = e => {

toolkit/devtools/heapsnapshot/tests/unit/test_SaveHeapSnapshot.js

@@ -11,89 +11,72 @@ if (typeof Debugger != "function") {
 function run_test() {
   ok(ChromeUtils, "Should be able to get the ChromeUtils interface");
 
-  let filePath = getFilePath("core-dump.tmp", true, true);
-  ok(filePath, "Should get a file path");
-
-  testBadParameters(filePath);
-  testGoodParameters(filePath);
+  testBadParameters();
+  testGoodParameters();
 
   do_test_finished();
 }
 
-function testBadParameters(filePath) {
+function testBadParameters() {
   throws(() => ChromeUtils.saveHeapSnapshot(),
          "Should throw if arguments aren't passed in.");
 
-  throws(() => ChromeUtils.saveHeapSnapshot(Object.create(null),
-                                            { runtime: true }),
-         "Should throw if the filePath is not coercible to string.");
-
-  throws(() => ChromeUtils.saveHeapSnapshot(filePath,
-                                            null),
+  throws(() => ChromeUtils.saveHeapSnapshot(null),
          "Should throw if boundaries isn't an object.");
 
-  throws(() => ChromeUtils.saveHeapSnapshot(filePath,
-                                            {}),
+  throws(() => ChromeUtils.saveHeapSnapshot({}),
          "Should throw if the boundaries object doesn't have any properties.");
 
-  throws(() => ChromeUtils.saveHeapSnapshot(filePath,
-                                            { runtime: true,
+  throws(() => ChromeUtils.saveHeapSnapshot({ runtime: true,
                                               globals: [this] }),
          "Should throw if the boundaries object has more than one property.");
 
-  throws(() => ChromeUtils.saveHeapSnapshot(filePath,
-                                            { debugger: {} }),
+  throws(() => ChromeUtils.saveHeapSnapshot({ debugger: {} }),
          "Should throw if the debuggees object is not a Debugger object");
 
-  throws(() => ChromeUtils.saveHeapSnapshot(filePath,
-                                            { globals: [{}] }),
+  throws(() => ChromeUtils.saveHeapSnapshot({ globals: [{}] }),
          "Should throw if the globals array contains non-global objects.");
 
-  throws(() => ChromeUtils.saveHeapSnapshot(filePath,
-                                            { runtime: false }),
+  throws(() => ChromeUtils.saveHeapSnapshot({ runtime: false }),
          "Should throw if runtime is supplied and is not true.");
 
-  throws(() => ChromeUtils.saveHeapSnapshot(filePath,
-                                            { globals: null }),
+  throws(() => ChromeUtils.saveHeapSnapshot({ globals: null }),
          "Should throw if globals is not an object.");
 
-  throws(() => ChromeUtils.saveHeapSnapshot(filePath,
-                                            { globals: {} }),
+  throws(() => ChromeUtils.saveHeapSnapshot({ globals: {} }),
          "Should throw if globals is not an array.");
 
-  throws(() => ChromeUtils.saveHeapSnapshot(filePath,
-                                            { debugger: Debugger.prototype }),
+  throws(() => ChromeUtils.saveHeapSnapshot({ debugger: Debugger.prototype }),
          "Should throw if debugger is the Debugger.prototype object.");
 
-  throws(() => ChromeUtils.saveHeapSnapshot(filePath,
-                                            { get globals() { return [this]; } }),
+  throws(() => ChromeUtils.saveHeapSnapshot({ get globals() { return [this]; } }),
          "Should throw if boundaries property is a getter.");
 }
 
 const makeNewSandbox = () =>
   Cu.Sandbox(CC('@mozilla.org/systemprincipal;1', 'nsIPrincipal')());
 
-function testGoodParameters(filePath) {
+function testGoodParameters() {
   let sandbox = makeNewSandbox();
   let dbg = new Debugger(sandbox);
 
-  ChromeUtils.saveHeapSnapshot(filePath, { debugger: dbg });
+  ChromeUtils.saveHeapSnapshot({ debugger: dbg });
   ok(true, "Should be able to save a snapshot for a debuggee global.");
 
   dbg = new Debugger;
   let sandboxes = Array(10).fill(null).map(makeNewSandbox);
   sandboxes.forEach(sb => dbg.addDebuggee(sb));
 
-  ChromeUtils.saveHeapSnapshot(filePath, { debugger: dbg });
+  ChromeUtils.saveHeapSnapshot({ debugger: dbg });
   ok(true, "Should be able to save a snapshot for many debuggee globals.");
 
   dbg = new Debugger;
-  ChromeUtils.saveHeapSnapshot(filePath, { debugger: dbg });
+  ChromeUtils.saveHeapSnapshot({ debugger: dbg });
   ok(true, "Should be able to save a snapshot with no debuggee globals.");
 
-  ChromeUtils.saveHeapSnapshot(filePath, { globals: [this] });
+  ChromeUtils.saveHeapSnapshot({ globals: [this] });
   ok(true, "Should be able to save a snapshot for a specific global.");
 
-  ChromeUtils.saveHeapSnapshot(filePath, { runtime: true });
+  ChromeUtils.saveHeapSnapshot({ runtime: true });
   ok(true, "Should be able to save a snapshot of the full runtime.");
 }

toolkit/devtools/shared/memory.js

@@ -139,8 +139,7 @@ let Memory = exports.Memory = Class({
    * @returns {String} The snapshot id.
    */
   saveHeapSnapshot: expectState("attached", function () {
-    const path = HeapSnapshotFileUtils.getNewUniqueHeapSnapshotTempFilePath();
-    ThreadSafeChromeUtils.saveHeapSnapshot(path, { debugger: this.dbg });
+    const path = ThreadSafeChromeUtils.saveHeapSnapshot({ debugger: this.dbg });
     return HeapSnapshotFileUtils.getSnapshotIdFromPath(path);
   }, "saveHeapSnapshot"),