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

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Developer Tools: Memory
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: magicp, Assigned: jdescottes)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 48
Points:
---

Firefox Tracking Flags

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

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Blocks: 1221506
Has STR: --- → yes
status-firefox45: --- → unaffected
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
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
Created attachment 8732903 [details]
MozReview Request: Bug 1255804 - fix dominator view infinite loading when importing snapshot;r=fitzgen

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)

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b60392ce5fb4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Duplicate of this bug: 1242015
You need to log in before you can comment on or make changes to this bug.