Closed Bug 1255804 Opened 8 years ago Closed 8 years ago

heap-view does not display when importing a snapshot in Dominators view

Categories

(DevTools :: Memory, defect, P1)

defect

Tracking

(firefox45 unaffected, firefox46 affected, firefox47 affected, firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox45 --- unaffected
firefox46 --- affected
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: magicp.jp, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160310030242

Steps to reproduce:

1. Start Firefox 46 or later version
2. Open DevTools > Memory
3. Take snapshot
4. Save snapshot
5. Delete all snapshots
6. Switch to "Dominators" view
7. Import snapshot file


Actual results:

heap-view does not display until selecting snapshot-list-item or switching view/label. (throbber is displaying forever)


Expected results:

heap-view is displayed without extra operations.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Memory
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Priority: -- → P1
Confirmed, also the spinner in the heap-view keeps spinning, but does not display any status information like "Saving census..." etc.
Assignee: nobody → jdescottes
Extracted new snapshot action to read snapshot, take census, and compute the
dominator tree if the current view is the dominator view. This was previously
handled in the takeSnapshotAndCensus action only.

This allows to reuse the read snapshot logic 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).

Review commit: https://reviewboard.mozilla.org/r/41447/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41447/
Attachment #8732903 - Flags: review?(nfitzgerald)
Attachment #8732903 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8732903 [details]
MozReview Request: Bug 1255804 - fix dominator view infinite loading when importing snapshot;r=fitzgen

https://reviewboard.mozilla.org/r/41447/#review37897

r=me with comments below addressed -- thanks a bunch for grabbing this, Julian!

::: devtools/client/memory/actions/snapshot.js:46
(Diff revision 1)
> + * will also be computed.
> + *
> + * @param {HeapAnalysesClient} heapWorker
> + * @param {snapshotId} id
> + */
> +const readSnapshotAndTakeCensus = exports.readSnapshotAndTakeCensus =

I don't really like the name for this function (and I realize this is an existing issue, as takeSnapshotAndCensus has the same problem). Do you have any ideas for a better name?

readSnapshotAndRunAnalyses?

readSnapshotAndGetData?

readSnapshotAndTakeCensusAndMaybeComputeDominatorTreeAsWell? (lol jk)

::: devtools/client/memory/test/unit/test_action-import-snapshot-and-census.js:64
(Diff revision 1)
>  
>    // 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,

Yikes! Good catch, thank you!

::: devtools/client/memory/test/unit/test_action-import-snapshot-dominator-tree.js:39
(Diff revision 1)
> +    waitUntilAction(store, actions.EXPORT_SNAPSHOT_START),
> +    waitUntilAction(store, actions.EXPORT_SNAPSHOT_END)
> +  ]);
> +  dispatch(exportSnapshot(getState().snapshots[0], destPath));
> +  yield exportEvents;
> +  ok(true, "First snapshot exported to temporary file");

You don't need to export a snapshot just to get a snapshot to import. Instead, you can use the webidl APIs to save a snapshot directly to a temp file, and then import that snapshot:

    const snapshotPath = ChromeUtils.saveHeapSnapshot({ runtime: true });

This has less (unrelated) moving parts, and so should make the test a tiny bit faster (less likelihood of intermittent timeouts) and also simpler.

::: devtools/client/memory/test/unit/test_action-import-snapshot-dominator-tree.js:70
(Diff revision 1)
> +  };
> +
> +  let unsubscribe = subscribe(expectStates);
> +  dispatch(importSnapshotAndCensus(heapWorker, destPath));
> +
> +  yield waitUntilState(store, () => i === 8);

Nit: `i === expected.length` for future proofing.
Comment on attachment 8732903 [details]
MozReview Request: Bug 1255804 - fix dominator view infinite loading when importing snapshot;r=fitzgen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41447/diff/1-2/
https://reviewboard.mozilla.org/r/41447/#review37897

Np :)

> I don't really like the name for this function (and I realize this is an existing issue, as takeSnapshotAndCensus has the same problem). Do you have any ideas for a better name?
> 
> readSnapshotAndRunAnalyses?
> 
> readSnapshotAndGetData?
> 
> readSnapshotAndTakeCensusAndMaybeComputeDominatorTreeAsWell? (lol jk)

Obv. readSnapshotAndTakeCensusAndMaybeComputeDominatorTreeAsWell.

So I ended up removing the "reading" part from the method to simply have a "computeSnapshotData" action. I think it's slightly better, let me know how you feel about it!

> You don't need to export a snapshot just to get a snapshot to import. Instead, you can use the webidl APIs to save a snapshot directly to a temp file, and then import that snapshot:
> 
>     const snapshotPath = ChromeUtils.saveHeapSnapshot({ runtime: true });
> 
> This has less (unrelated) moving parts, and so should make the test a tiny bit faster (less likelihood of intermittent timeouts) and also simpler.

Thanks, test is much simpler now. New try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=1975a88f00be

> Nit: `i === expected.length` for future proofing.

Thanks!
Nick: let me know if you are ok with :

> yield dispatch(readSnapshot(heapWorker, id)); 
> yield dispatch(computeSnapshotData(heapWorker, id));

vs 

> yield dispatch(readSnapshotAndTakeCensus(heapWorker, id));
Flags: needinfo?(nfitzgerald)
(In reply to Julian Descottes [:jdescottes] from comment #7)

LGTM!
Flags: needinfo?(nfitzgerald)
https://hg.mozilla.org/mozilla-central/rev/b60392ce5fb4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.