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)
DevTools
Memory
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.
Blocks: memory-frontend
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
Updated•8 years ago
|
Priority: -- → P1
Comment 1•8 years ago
|
||
Confirmed, also the spinner in the heap-view keeps spinning, but does not display any status information like "Saving census..." etc.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jdescottes
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
Try push at : https://treeherder.mozilla.org/#/jobs?repo=try&revision=6059cc2e22e9
Updated•8 years ago
|
Attachment #8732903 -
Flags: review?(nfitzgerald) → review+
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
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/
Assignee | ||
Comment 6•8 years ago
|
||
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!
Assignee | ||
Comment 7•8 years ago
|
||
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)
Comment 8•8 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #7) LGTM!
Flags: needinfo?(nfitzgerald)
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b60392ce5fb4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•