Bug 1255804 - fix dominator view infinite loading when importing snapshot;r=fitzgen draft
authorJulian Descottes <jdescottes@mozilla.com>
Mon, 21 Mar 2016 17:14:16 +0100
changeset 343072 f43d922fd6860b496770f754f6e8077c540b2980
parent 342840 8279c04272ee46eac5fc60ba52065ed7aefc5b28
child 516683 4a8c3ab2289a7e4b812e87e03e22fcf92b3e8af2
push id13520
push userjdescottes@mozilla.com
push dateMon, 21 Mar 2016 21:44:43 +0000
reviewersfitzgen
bugs1255804
milestone48.0a1
Bug 1255804 - fix dominator view infinite loading when importing snapshot;r=fitzgen Extracted new snapshot action to compute snapshot data: take census + compute dominator-tree if the current view is the dominator-view (logic extracted from takeSnapshotAndCensus action). This action is reused when importing a snpashot. Added a new unit test to test the import from the dominator tree view. Modified exising import unit test to check snapshot.census.display instead of snapshot.display (which was undefined). MozReview-Commit-ID: 8JN1arfaI1H
devtools/client/memory/actions/io.js
devtools/client/memory/actions/snapshot.js
devtools/client/memory/test/unit/test_action-import-snapshot-and-census.js
devtools/client/memory/test/unit/test_action-import-snapshot-dominator-tree.js
devtools/client/memory/test/unit/xpcshell.ini
--- a/devtools/client/memory/actions/io.js
+++ b/devtools/client/memory/actions/io.js
@@ -2,17 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
 const { immutableUpdate, reportException, assert } = require("devtools/shared/DevToolsUtils");
 const { snapshotState: states, actions } = require("../constants");
 const { L10N, openFilePicker, createSnapshot } = require("../utils");
 const telemetry = require("../telemetry");
-const { readSnapshot, takeCensus, selectSnapshot } = require("./snapshot");
+const { selectSnapshot, computeSnapshotData, readSnapshot } = require("./snapshot");
 const { OS } = require("resource://gre/modules/osfile.jsm");
 const VALID_EXPORT_STATES = [states.SAVED, states.READ, states.SAVING_CENSUS, states.SAVED_CENSUS];
 
 exports.pickFileAndExportSnapshot = function (snapshot) {
   return function* (dispatch, getState) {
     let outputFile = yield openFilePicker({
       title: L10N.getFormatStr("snapshot.io.save.window"),
       defaultName: OS.Path.basename(snapshot.path),
@@ -75,17 +75,17 @@ const importSnapshotAndCensus = exports.
     });
     const id = snapshot.id;
 
     dispatch({ type: actions.IMPORT_SNAPSHOT_START, snapshot });
     dispatch(selectSnapshot(snapshot.id));
 
     try {
       yield dispatch(readSnapshot(heapWorker, id));
-      yield dispatch(takeCensus(heapWorker, id));
+      yield dispatch(computeSnapshotData(heapWorker, id));
     } catch (error) {
       reportException("importSnapshot", error);
       dispatch({ type: actions.IMPORT_SNAPSHOT_ERROR, error, id });
     }
 
     dispatch({ type: actions.IMPORT_SNAPSHOT_END, id });
   };
 };
--- a/devtools/client/memory/actions/snapshot.js
+++ b/devtools/client/memory/actions/snapshot.js
@@ -27,22 +27,37 @@ const diffing = require("./diffing");
 const takeSnapshotAndCensus = exports.takeSnapshotAndCensus = function (front, heapWorker) {
   return function *(dispatch, getState) {
     const id = yield dispatch(takeSnapshot(front));
     if (id === null) {
       return;
     }
 
     yield dispatch(readSnapshot(heapWorker, id));
-    if (getSnapshot(getState(), id).state === states.READ) {
-      yield dispatch(takeCensus(heapWorker, id));
+    yield dispatch(computeSnapshotData(heapWorker, id));
+  };
+};
 
-      if (getState().view === viewState.DOMINATOR_TREE) {
-        yield dispatch(computeAndFetchDominatorTree(heapWorker, id));
-      }
+/**
+ * Create the census for the snapshot with the provided snapshot id. If the
+ * current view is the DOMINATOR_TREE view, create the dominator tree for this
+ * snapshot as well.
+ *
+ * @param {HeapAnalysesClient} heapWorker
+ * @param {snapshotId} id
+ */
+const computeSnapshotData = exports.computeSnapshotData = function(heapWorker, id) {
+  return function* (dispatch, getState) {
+    if (getSnapshot(getState(), id).state !== states.READ) {
+      return;
+    }
+
+    yield dispatch(takeCensus(heapWorker, id));
+    if (getState().view === viewState.DOMINATOR_TREE) {
+      yield dispatch(computeAndFetchDominatorTree(heapWorker, id));
     }
   };
 };
 
 /**
  * Selects a snapshot and if the snapshot's census is using a different
  * display, take a new census.
  *
--- a/devtools/client/memory/test/unit/test_action-import-snapshot-and-census.js
+++ b/devtools/client/memory/test/unit/test_action-import-snapshot-and-census.js
@@ -46,27 +46,27 @@ add_task(function *() {
       ok(true, `Found expected state ${expected[i]}`);
       i++;
     }
   };
 
   let unsubscribe = subscribe(expectStates);
   dispatch(importSnapshotAndCensus(heapWorker, destPath));
 
-  yield waitUntilState(store, () => i === 5);
+  yield waitUntilState(store, () => i === expected.length);
   unsubscribe();
-  equal(i, 5, "importSnapshotAndCensus() produces the correct sequence of states in a snapshot");
+  equal(i, expected.length, "importSnapshotAndCensus() produces the correct sequence of states in a snapshot");
   equal(getState().snapshots[1].state, states.SAVED_CENSUS, "imported snapshot is in SAVED_CENSUS state");
   ok(getState().snapshots[1].selected, "imported snapshot is selected");
 
   // Check snapshot data
   let snapshot1 = getState().snapshots[0];
   let snapshot2 = getState().snapshots[1];
 
-  equal(snapshot1.display, snapshot2.display,
+  equal(snapshot1.census.display, snapshot2.census.display,
         "imported snapshot has correct display");
 
   // Clone the census data so we can destructively remove the ID/parents to compare
   // equal census data
   let census1 = stripUnique(JSON.parse(JSON.stringify(snapshot1.census.report)));
   let census2 = stripUnique(JSON.parse(JSON.stringify(snapshot2.census.report)));
 
   equal(JSON.stringify(census1), JSON.stringify(census2), "Imported snapshot has correct census");
new file mode 100644
--- /dev/null
+++ b/devtools/client/memory/test/unit/test_action-import-snapshot-dominator-tree.js
@@ -0,0 +1,78 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+"use strict";
+
+/**
+ * Tests `importSnapshotAndCensus()` when importing snapshots from the dominator
+ * tree view. The snapshot is expected to be loaded and its dominator tree
+ * should be computed.
+ */
+
+let { snapshotState, dominatorTreeState, viewState, } =
+  require("devtools/client/memory/constants");
+let { importSnapshotAndCensus } = require("devtools/client/memory/actions/io");
+let { changeViewAndRefresh } = require("devtools/client/memory/actions/view");
+
+function run_test() {
+  run_next_test();
+}
+
+add_task(function* () {
+  let front = new StubbedMemoryFront();
+  let heapWorker = new HeapAnalysesClient();
+  yield front.attach();
+  let store = Store();
+  let { subscribe, dispatch, getState } = store;
+
+  dispatch(changeViewAndRefresh(viewState.DOMINATOR_TREE, heapWorker));
+  equal(getState().view, viewState.DOMINATOR_TREE,
+        "We should now be in the DOMINATOR_TREE view");
+
+  let i = 0;
+  let expected = [
+    "IMPORTING",
+    "READING",
+    "READ",
+    "SAVING_CENSUS",
+    "SAVED_CENSUS",
+    "dominatorTree:COMPUTING",
+    "dominatorTree:FETCHING",
+    "dominatorTree:LOADED",
+  ];
+  let expectStates = () => {
+    let snapshot = getState().snapshots[0];
+    if (snapshot && hasExpectedState(snapshot, expected[i])) {
+      ok(true, `Found expected state ${expected[i]}`);
+      i++;
+    }
+  };
+
+  let unsubscribe = subscribe(expectStates);
+  const snapshotPath = yield front.saveHeapSnapshot();
+  dispatch(importSnapshotAndCensus(heapWorker, snapshotPath));
+
+  yield waitUntilState(store, () => i === expected.length);
+  unsubscribe();
+  equal(i, expected.length, "importSnapshotAndCensus() produces the correct " +
+    "sequence of states in a snapshot");
+  equal(getState().snapshots[0].dominatorTree.state, dominatorTreeState.LOADED,
+    "imported snapshot's dominator tree is in LOADED state");
+  ok(getState().snapshots[0].selected, "imported snapshot is selected");
+});
+
+/**
+ * Check that the provided snapshot is in the expected state. The expected state
+ * is a snapshotState by default. If the expected state is prefixed by
+ * dominatorTree, a dominatorTree is expected on the provided snapshot, in the
+ * corresponding state from dominatorTreeState.
+ */
+function hasExpectedState(snapshot, expectedState) {
+  let isDominatorState = expectedState.indexOf("dominatorTree:") === 0;
+  if (isDominatorState) {
+    let state = dominatorTreeState[expectedState.replace("dominatorTree:", "")];
+    return snapshot.dominatorTree && snapshot.dominatorTree.state === state;
+  }
+
+  let state = snapshotState[expectedState];
+  return snapshot.state === state;
+}
--- a/devtools/client/memory/test/unit/xpcshell.ini
+++ b/devtools/client/memory/test/unit/xpcshell.ini
@@ -16,16 +16,17 @@ skip-if = toolkit == 'android' || toolki
 [test_action-clear-snapshots_04.js]
 [test_action-clear-snapshots_05.js]
 [test_action-clear-snapshots_06.js]
 [test_action-export-snapshot.js]
 [test_action-filter-01.js]
 [test_action-filter-02.js]
 [test_action-filter-03.js]
 [test_action-import-snapshot-and-census.js]
+[test_action-import-snapshot-dominator-tree.js]
 [test_action-select-snapshot.js]
 [test_action-set-display.js]
 [test_action-set-display-and-refresh-01.js]
 [test_action-set-display-and-refresh-02.js]
 [test_action-take-census.js]
 [test_action-take-snapshot.js]
 [test_action-take-snapshot-and-census.js]
 [test_action-toggle-inverted.js]