[DevTools][Memory] Dagre-D3 graph is not updated when switching Label-by

VERIFIED FIXED in Firefox 48

Status

DevTools
Memory
P1
normal
VERIFIED FIXED
2 years ago
a month ago

People

(Reporter: magicp, Assigned: fitzgen)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 48

Firefox Tracking Flags

(firefox47 affected, firefox48 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160304030206

Steps to reproduce:

1. Start Nightly
2. Open DevTools > Memory
3. Switch view to "Dominators"
4. Select any rows -> Dagre-D3 graph is displayed
5. Switch Label-by to "Call Stack" -> Previous Dagre-D3 graph is still displayed


Actual results:

Dagre-D3 graph is not updated when switching Label-by


Expected results:

Is this behavior correct? If this is not, Dagre-D3 graph is updated (clear graph).
(Reporter)

Updated

2 years ago
Blocks: 1221506
Has STR: --- → yes
status-firefox47: --- → affected
Component: Untriaged → Developer Tools: Memory
OS: Unspecified → All
Hardware: Unspecified → All
(Reporter)

Comment 1

2 years ago
> 1. Start Nightly
> 2. Open DevTools > Memory
> 3. Switch view to "Dominators"
+ Take snapshot
> 4. Select any rows -> Dagre-D3 graph is displayed
> 5. Switch Label-by to "Call Stack" -> Previous Dagre-D3 graph is still
> displayed
You are correct, it should change the labels for nodes in the graph as we change the "Label By:" option.
Assignee: nobody → nfitzgerald
Priority: -- → P2
Priority: P2 → P1
Created attachment 8737996 [details] [diff] [review]
Ensure that we maintain the focused node state when changing labels in the dominators view
Attachment #8737996 - Flags: review?(jsantell)
Attachment #8737996 - Flags: review?(jsantell) → review+
Keywords: checkin-needed

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/75bef7c5fd91
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment on attachment 8737996 [details] [diff] [review]
Ensure that we maintain the focused node state when changing labels in the dominators view

Approval Request Comment
[Feature/regressing bug #]:
Bug 1229229

[User impact if declined]:
The selected node's retaining paths will not be updated when changing label types in memory tool. Also focused node will not persist across changing label types in memory tool.

[Describe test coverage new/current, TreeHerder]:
Extensive test coverage for memory tool / feature already in m-c. This patch also added a regression test for this particular bug.

[Risks and why]:
Very little. Devtools only feature.

[String/UUID change made/needed]:
None.
Attachment #8737996 - Flags: approval-mozilla-aurora?
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #7)
> Comment on attachment 8737996 [details] [diff] [review]
> Ensure that we maintain the focused node state when changing labels in the
> dominators view
> 
> Approval Request Comment
> [Feature/regressing bug #]:
> Bug 1229229

Er actually bug 1149385.

Comment 9

2 years ago
Hello magic.cp, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(magicp.jp)
Comment on attachment 8737996 [details] [diff] [review]
Ensure that we maintain the focused node state when changing labels in the dominators view

Regression with new automated test, Aurora47+
Attachment #8737996 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 11

2 years ago
(In reply to Ritu Kothari (:ritu) from comment #9)
> Hello magic.cp, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!

My expected result is that graph is cleared when changing labels in the dominators view. This meaning is that node will be lost focus. New behavior is that keeping node focus and updating graph. It's good to me, but this specification does not work if you selected a node in the load more.
Flags: needinfo?(magicp.jp)
Needs rebasing.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(nfitzgerald)
Hi Nick, can you please address the concern in comment 11? Thanks!
Flags: needinfo?(nfitzgerald)
Based on the verification in comment 11.
Status: RESOLVED → VERIFIED
(In reply to magicp from comment #11)
> (In reply to Ritu Kothari (:ritu) from comment #9)
> > Hello magic.cp, could you please verify this issue is fixed as expected on a
> > latest Nightly build? Thanks!
> 
> My expected result is that graph is cleared when changing labels in the
> dominators view. This meaning is that node will be lost focus. New behavior
> is that keeping node focus and updating graph. It's good to me, but this
> specification does not work if you selected a node in the load more.

Filed bug 1263727 for this.
Flags: needinfo?(nfitzgerald)
https://hg.mozilla.org/releases/mozilla-aurora/rev/71cedf188b34
status-firefox47: affected → fixed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2335247&repo=mozilla-aurora
status-firefox47: fixed → affected

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/7af65df7e09e
status-firefox47: affected → fixed
Backed out for xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2350162&repo=mozilla-aurora

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa74410e52f0
status-firefox47: fixed → affected
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(nfitzgerald)

Comment 21

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/12bbaa4e9c1d
status-firefox47: affected → fixed
*sigh* Total brainfart on that one. Would be nice if approval flags could be cleared on patches that got backed out.
https://hg.mozilla.org/releases/mozilla-aurora/rev/98da47b4d83c
status-firefox47: fixed → affected
Flags: needinfo?(rkothari)
Comment on attachment 8737996 [details] [diff] [review]
Ensure that we maintain the focused node state when changing labels in the dominators view

Seems like this never got uplifted to Fx47, clearing out the flags to reflect that.
Flags: needinfo?(rkothari)
Attachment #8737996 - Flags: approval-mozilla-aurora+

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.