Bug 1201597 - Part 0: Make saveHeapSnapshot return the file path rather than take it as a parameter; r=bholley
authorNick Fitzgerald <fitzgen@gmail.com>
Tue, 15 Sep 2015 11:26:46 +0530
changeset 295091 94138758d2c6e1721dfd1ef9454be6052db5b464
parent 295090 e6f7943f32fca2cd6b332d3c717d54488d973e32
child 295092 844a70e85eba2ec43b8ba399c9f39cababd28b5b
push id5245
push userraliiev@mozilla.com
push dateThu, 29 Oct 2015 11:30:51 +0000
treeherdermozilla-beta@dac831dc1bd0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1201597
milestone43.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 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
dom/webidl/ThreadSafeChromeUtils.webidl
toolkit/devtools/heapsnapshot/HeapSnapshot.cpp
toolkit/devtools/heapsnapshot/tests/mochitest/test_SaveHeapSnapshot.html
toolkit/devtools/heapsnapshot/tests/unit/head_heapsnapshot.js
toolkit/devtools/heapsnapshot/tests/unit/heap-snapshot-worker.js
toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot.js
toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot_with_allocations.js
toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot_worker.js
toolkit/devtools/heapsnapshot/tests/unit/test_SaveHeapSnapshot.js
toolkit/devtools/shared/memory.js
--- a/dom/base/ChromeUtils.h
+++ b/dom/base/ChromeUtils.h
@@ -18,24 +18,24 @@ namespace devtools {
 class HeapSnapshot;
 } // namespace devtools
 
 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,
                                                                    ErrorResult& rv);
 };
 
 class ChromeUtils : public ThreadSafeChromeUtils
 {
--- a/dom/webidl/ThreadSafeChromeUtils.webidl
+++ b/dom/webidl/ThreadSafeChromeUtils.webidl
@@ -9,23 +9,25 @@
  * to Chrome. This interface is exposed in workers, while ChromeUtils is not.
  */
 [ChromeOnly, Exposed=(Window,System,Worker)]
 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.
    *
-   * @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.
    *
    * @param filePath          The file path to read the heap snapshot from.
    */
   [Throws, NewObject]
   static HeapSnapshot readHeapSnapshot(DOMString filePath);
--- a/toolkit/devtools/heapsnapshot/HeapSnapshot.cpp
+++ b/toolkit/devtools/heapsnapshot/HeapSnapshot.cpp
@@ -23,20 +23,22 @@
 #include "mozilla/RangedPtr.h"
 #include "mozilla/Telemetry.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/UniquePtr.h"
 
 #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"
 
 namespace mozilla {
 namespace devtools {
 
 using namespace JS;
@@ -810,32 +812,65 @@ WriteHeapGraph(JSContext* cx,
 
 } // namespace devtools
 
 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();
 
   bool wantNames = true;
   ZoneSet zones;
   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;
 
   nsCOMPtr<nsIOutputStream> outputStream;
   rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream), file,
                                    PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE,
                                    -1, 0);
   if (NS_WARN_IF(rv.Failed()))
--- a/toolkit/devtools/heapsnapshot/tests/mochitest/test_SaveHeapSnapshot.html
+++ b/toolkit/devtools/heapsnapshot/tests/mochitest/test_SaveHeapSnapshot.html
@@ -10,22 +10,16 @@ Bug 1024774 - Sanity test that we can ta
   <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
 </head>
 <body>
 <pre id="test">
 <script>
 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();
 };
 </script>
 </pre>
 </body>
 </html>
--- a/toolkit/devtools/heapsnapshot/tests/unit/head_heapsnapshot.js
+++ b/toolkit/devtools/heapsnapshot/tests/unit/head_heapsnapshot.js
@@ -101,23 +101,20 @@ function getFilePath(aName, aAllowMissin
 
   if (aUsePlatformPathSeparator && path.match(/^\w:/)) {
     path = path.replace(/\//g, "\\");
   }
 
   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;
 }
 
 /**
  * Save a heap snapshot to the file with the given name in the current
  * directory, read it back as a HeapSnapshot instance, and then take a census of
  * the heap snapshot's serialized heap graph with the provided census options.
  *
@@ -130,26 +127,20 @@ function saveNewHeapSnapshot(fileName=`c
  *        the Debugger's debuggees. If null, serialize the whole heap graph.
  *
  * @param {String} fileName
  *        The file name to save the heap snapshot's core dump file to, within
  *        the current directory.
  *
  * @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);
+function saveHeapSnapshotAndTakeCensus(dbg=null, censusOptions=undefined) {
+  const snapshotOptions = dbg ? { debugger: dbg } : { runtime: true };
+  const filePath = ChromeUtils.saveHeapSnapshot(snapshotOptions);
   ok(filePath, "Should get a file path to save the core dump to.");
-
-  const snapshotOptions = dbg ? { debugger: dbg } : { runtime: true };
-  ChromeUtils.saveHeapSnapshot(filePath, snapshotOptions);
   ok(true, "Should have saved a heap snapshot to " + filePath);
 
   const snapshot = ChromeUtils.readHeapSnapshot(filePath);
   ok(snapshot, "Should have read a heap snapshot back from " + filePath);
   ok(snapshot instanceof HeapSnapshot, "snapshot should be an instance of HeapSnapshot");
 
   equal(typeof snapshot.takeCensus, "function", "snapshot should have a takeCensus method");
   return snapshot.takeCensus(censusOptions);
--- a/toolkit/devtools/heapsnapshot/tests/unit/heap-snapshot-worker.js
+++ b/toolkit/devtools/heapsnapshot/tests/unit/heap-snapshot-worker.js
@@ -1,26 +1,24 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 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,
        "Should have access to ThreadSafeChromeUtils in a worker.");
     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);
     ok(snapshot, "Should be able to read a heap snapshot");
     ok(snapshot instanceof HeapSnapshot, "Should be an instanceof HeapSnapshot");
   } catch (e) {
     ok(false, "Unexpected error inside worker:\n" + e.toString() + "\n" + e.stack);
   } finally {
--- a/toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot.js
+++ b/toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot.js
@@ -4,20 +4,17 @@
 // Test that we can read core dumps into HeapSnapshot instances.
 
 if (typeof Debugger != "function") {
   const { addDebuggerToGlobal } = Cu.import("resource://gre/modules/jsdebugger.jsm", {});
   addDebuggerToGlobal(this);
 }
 
 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);
   ok(snapshot, "Should be able to read a heap snapshot");
   ok(snapshot instanceof HeapSnapshot, "Should be an instanceof HeapSnapshot");
 
   do_test_finished();
 }
--- a/toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot_with_allocations.js
+++ b/toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot_with_allocations.js
@@ -20,20 +20,17 @@ function run_test() {
   debuggee.eval("this.objects = []");
   for (let i = 0; i < 100; i++) {
     debuggee.eval("this.objects.push({})");
   }
 
   // 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);
   ok(snapshot, "Should be able to read a heap snapshot");
   ok(snapshot instanceof HeapSnapshot, "Should be an instanceof HeapSnapshot");
 
   do_test_finished();
 }
--- a/toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot_worker.js
+++ b/toolkit/devtools/heapsnapshot/tests/unit/test_ReadHeapSnapshot_worker.js
@@ -1,19 +1,16 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 // 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 => {
     if (e.data.type !== "assertion") {
       return;
     }
 
     ok(e.data.passed, e.data.msg + "\n" + e.data.stack);
--- a/toolkit/devtools/heapsnapshot/tests/unit/test_SaveHeapSnapshot.js
+++ b/toolkit/devtools/heapsnapshot/tests/unit/test_SaveHeapSnapshot.js
@@ -6,94 +6,77 @@
 if (typeof Debugger != "function") {
   const { addDebuggerToGlobal } = Cu.import("resource://gre/modules/jsdebugger.jsm", {});
   addDebuggerToGlobal(this);
 }
 
 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.");
 }
--- a/toolkit/devtools/shared/memory.js
+++ b/toolkit/devtools/shared/memory.js
@@ -134,18 +134,17 @@ let Memory = exports.Memory = Class({
 
   /**
    * Save a heap snapshot scoped to the current debuggees' portion of the heap
    * graph.
    *
    * @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"),
 
   /**
    * Take a census of the heap. See js/src/doc/Debugger/Debugger.Memory.md for
    * more information.
    */
   takeCensus: expectState("attached", function() {