Closed Bug 1241815 Opened 4 years ago Closed 4 years ago

Show message when difference is empty or filtering yields no matches

Categories

(DevTools :: Memory, defect, P1)

defect

Tracking

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

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

People

(Reporter: magicp.jp, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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

Steps to reproduce:

1. Start Nightly (or Aurora)
2. Open Memory tool
3. Repeat "Take Snapshot..." twice
4. Click "Compare Snapshot" button
5. Select baseline and comparison
6. Check on "Invert tree"
7. Switch the group by from "Coarse Type" to "Allocation Stack"


Actual results:

Difference calculation does not end.


Expected results:

Difference calculation is completed.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Memory
OS: Unspecified → All
Hardware: Unspecified → All
From what I tested, the issue only occurs if the two snapshots are identical. 
If there is any difference displayed, the calculation ends properly.
Is this what you mean? Not that there is a spinner forever?

If so, this is what happens when the snapshots are identical, as Julian mentions. I think we could have a better UI by displaying a message saying something like "no difference between the snapshots" or something along those lines.
Flags: needinfo?(magicp.jp)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #2)
> Created attachment 8711058 [details]
> Screen Shot 2016-01-22 at 8.49.02 AM.png
> 
> Is this what you mean? Not that there is a spinner forever?

Yes. And there is no error message in the Browser Console.

06:00:24.717 @@redux/middleware/task#error threw an exception: TypeError: census.report.children is undefined
Stack: _renderCensus@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/memory/components/heap.js:258:1
render@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/memory/components/heap.js:184:14
[38]</ReactCompositeComponentMixin._renderValidatedComponentWithoutOwnerOrContext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6337:29
[38]</ReactCompositeComponentMixin._renderValidatedComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6357:27
[38]</ReactCompositeComponentMixin._updateRenderedComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6310:31
[38]</ReactCompositeComponentMixin._performComponentUpdate@resource://gre/modules/commonjs/toolkit/loader.js -> re DevToolsUtils.js:69:0
06:00:24.721 Handler function threw an exception: TypeError: census.report.children is undefined
Stack: _renderCensus@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/memory/components/heap.js:258:1
render@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/memory/components/heap.js:184:14
[38]</ReactCompositeComponentMixin._renderValidatedComponentWithoutOwnerOrContext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6337:29
[38]</ReactCompositeComponentMixin._renderValidatedComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6357:27
[38]</ReactCompositeComponentMixin._updateRenderedComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6310:31
[38]</ReactCompositeComponentMixin._performComponentUpdate@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6294:5
[38]</ReactCompositeComponentMixin.updateComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6223:7
[38]</ReactCompositeComponentMixin.receiveComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6155:5
[84]</ReactReconciler.receiveComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:13661:5
[31]</ReactChildReconciler.updateChildren@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:4492:9
[73]</ReactMultiChild.Mixin._reconcilerUpdateChildren@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:12195:14
[73]</ReactMultiChild.Mixin._updateChildren@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:12326:26
[73]</ReactMultiChild.Mixin.updateChildren@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:12301:9
[42]</ReactDOMComponent.Mixin._updateDOMChildren@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:7491:7
[42]</ReactDOMComponent.Mixin.updateComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:7320:5
[42]</ReactDOMComponent.Mixin.receiveComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:7265:5
[84]</ReactReconciler.receiveComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:13661:5
[31]</ReactChildReconciler.updateChildren@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:4492:9
[73]</ReactMultiChild.Mixin._reconcilerUpdateChildren@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:12195:14
[73]</ReactMultiChild.Mixin._updateChildren@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:12326:26
[73]</ReactMultiChild.Mixin.updateChildren@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:12301:9
[42]</ReactDOMComponent.Mixin._updateDOMChildren@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:7491:7
[42]</ReactDOMComponent.Mixin.updateComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:7320:5
[42]</ReactDOMComponent.Mixin.receiveComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:7265:5
[84]</ReactReconciler.receiveComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:13661:5
[38]</ReactCompositeComponentMixin._updateRenderedComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6312:7
[38]</ReactCompositeComponentMixin._performComponentUpdate@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6294:5
[38]</ReactCompositeComponentMixin.updateComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6223:7
[38]</ReactCompositeComponentMixin.receiveComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6155:5
[84]</ReactReconciler.receiveComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:13661:5
[38]</ReactCompositeComponentMixin._updateRenderedComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6312:7
[38]</ReactCompositeComponentMixin._performComponentUpdate@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6294:5
[38]</ReactCompositeComponentMixin.updateComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6223:7
[38]</ReactCompositeComponentMixin.performUpdateIfNecessary@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:6171:7
[84]</ReactReconciler.performUpdateIfNecessary@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:13676:5
runBatchedUpdates@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:15368:5
[113]</Mixin.perform@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:17257:13
[113]</Mixin.perform@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:17257:13
[96]</<.perform@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:15325:12
[96]</flushBatchedUpdates@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:15386:7
[113]</Mixin.closeAll@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:17323:11
[113]</Mixin.perform@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:17270:11
[53]</ReactDefaultBatchingStrategy.batchedUpdates@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:8849:7
enqueueUpdate@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:15415:5
enqueueUpdate@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:15005:3
[95]</ReactUpdateQueue.enqueueSetState@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:15171:5
[34]</ReactComponent.prototype.setState@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react.js:5551:3
handleChange@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-redux.js:346:12
createStore/dispatch/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/redux.js:212:15
dispatch@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/redux.js:211:6
waitUntilService/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/wait-service.js:60:20
promiseMiddleware/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/promise.js:16:14
thunk/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/thunk.js:16:9
task/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/task.js:31:12
dispatch@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/redux.js:384:19
handleError/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/redux/middleware/task.js:38:5
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:93:14
Line: 258, column: 1 DevToolsUtils.js:69:0

> If so, this is what happens when the snapshots are identical, as Julian
> mentions. I think we could have a better UI by displaying a message saying
> something like "no difference between the snapshots" or something along
> those lines.

It sounds good.
Flags: needinfo?(magicp.jp)
Priority: -- → P2
Priority: P2 → P1
We have also since fixed the forever spinner issue caused by accessing census.report.children without null-checking, but I still think it would be good to have a placeholder as mentioned above.
We already have tests that we don't throw when receiving empty censuses, didn't feel an overwhelming need to test that we show the empty message too. If you feel otherwise, I can write up a test.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8b776114ebd
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Comment on attachment 8733624 [details] [diff] [review]
Show a message when filtering yields no matches

I'll not be lazy and write a test. Its the right thing to do.
Flags: needinfo?(nfitzgerald)
Attachment #8733624 - Flags: review?(jsantell)
Flags: needinfo?(nfitzgerald)
Summary: [DevTools][Memory] Difference calculation does not end → Show message when difference is empty or filtering yields no matches
Flags: needinfo?(nfitzgerald)
Now with test!
Attachment #8733953 - Flags: review?(jsantell)
Attachment #8733624 - Attachment is obsolete: true
Attachment #8733953 - Flags: review?(jsantell) → review+
https://hg.mozilla.org/mozilla-central/rev/16aaf4cd418c
Status: ASSIGNED → RESOLVED
Closed: 4 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.