bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

All shortcuts stop working (until 1st click) if I clear sidebar in Profiler after switching between Waterfall and JS Flame Chart

VERIFIED FIXED in Firefox 49



Developer Tools: Performance Tools (Profiler/Timeline)
2 years ago
a year ago


(Reporter: arni2033, Assigned: gregtatum)



Firefox 50

Firefox Tracking Flags

(firefox47 unaffected, firefox48 wontfix, firefox49 fixed, firefox50 verified)



(1 attachment, 1 obsolete attachment)



2 years ago
>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160323030400
1. Open new tab
2. Open Devtools -> Performance
3. Press button "Start Recording Performance". Press it again to end recording.
4. Make sure that "Waterfall" tab in "Performance" is selected. Click "JS Flame Chart" tab.
5. Click "clear" to clear results of performance recording.
6. Press F12 or Ctrl+T or even Ctrl+Tab/PageDown

AR:  No visible action in Step 6
ER:  Shortcuts should work

This is regression between 2016-03-21 and 2016-03-22 (can't determine the bug). Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=ae45c837f71ff925e848bb7f44353681a581207f&tochange=8bcc1369627e4017fc5adcd3284ecf330909ffa1
Flags: needinfo?(vporof)
Can't work on this right now, busy with Tofino.
Flags: needinfo?(vporof)
Can't work on this right now, busy with Tofino.

Comment 3

2 years ago
I can't reproduce this on a Mac, I'll need to get on a Windows machine.

Comment 4

2 years ago
Without one detail this bug looks more horrifying than it is.
If I add Step 7 - "Click somewhere", then all shortcuts will start working again.
Summary: All shortcuts stop working if I clear sidebar in Profiler after switching between Waterfall and JS Flame Chart → All shortcuts stop working (until 1st click) if I clear sidebar in Profiler after switching between Waterfall and JS Flame Chart
Joe, status for 48?
Flags: needinfo?(jwalker)
Greg - could you mark a priority for this - is it P1?
Flags: needinfo?(jwalker) → needinfo?(gtatum)


2 years ago
Flags: needinfo?(gtatum)
Priority: -- → P1

Comment 7

2 years ago
Hmm... not sure why this wasn't showing up in my to-triage search. Marked it as P1. I'll see if I can reproduce it successfully.
Assigning to you Greg, since you're looking at it. Please shout if you need help.
Assignee: nobody → gtatum
status-firefox49: --- → affected
status-firefox50: --- → affected

Comment 9

2 years ago
To clarify the STR, this happens anytime the "JS Flame Chart" panel is open. The switching action doesn't appear to have any affect on the behavior of the bug.

In addition on the regression range, it appears to happen at Bug 1251291.
My suspicion is that something is greedily capturing keypress events as I'm not noticing any errors being thrown anywhere. I've investigated all of the key events that I can find and haven't turned anything up yet.

I added console.log statements to key events in:
 * `devtools/client/shared/widgets/FlameGraph.js`
 * `devtools/client/shared/widgets/view-helpers.js`
 * `devtools/client/shared/key-shortcuts.js`

And none of them are firing after performing the STR.

All of the "PerformanceController.clearRecordings" handlers and functionality seem to be consistent across the different views, so those don't appear to be the issue. I'm not really sure what to look at next at this point.

Comment 11

2 years ago
To ask somebody who works on XUL/events, obviously. Shortcuts to switch to the next/previous tab couldn't be prevented by JS even in chrome context, so something must be wrong on the XUL side.

 If it turns out to be XUL bug or any other component than "DevTools", please change component
 in this bug report page, or notify so that I could file the same bug with other component.
 I'm asking, because I'm really sick of people stealing my bugs w/o reason. Thanks for understanding.
Thanks arni for finding such bugs, your STRs are very valuable!
ochameau pointed out on IRC that commenting out this.graph.focus() from details-js-flamegraph.js makes the bug go away. I'll start working on that direction.
Created attachment 8766886 [details] [diff] [review]
Fix shortcut bug in performance tools

I ended up stripping out the FlameGraph's focus functionality. All of the event
bindings still work for the graph whether or not the window is focused. The error
happens when the component hides the iframe while it has focus. I'm wondering if
it's not a XUL-related issue, as I couldn't grab the focus back to a different
window manually after the window was hidden. The only way I could get it to work
was to focus a different window pre-emptively, which felt messier than just removing
the unneeded code.

I'm currently waiting on the try server to open back up to push some tests, plus
I need to figure out who can review this patch.


2 years ago
Attachment #8766886 - Flags: review?(jsantell)
Comment on attachment 8766886 [details] [diff] [review]
Fix shortcut bug in performance tools

Review of attachment 8766886 [details] [diff] [review]:

r+ if the key shortcuts still work in the FlameGraph
Attachment #8766886 - Flags: review?(jsantell) → review+
Created attachment 8769205 [details] [diff] [review]
Fix shortcut bug in performance tools

Looks like the last one did kill the shortcuts until first click. I was
testing using the mouse scroll wheel. This should do the trick now.
The shortcuts all work now with this one. It doesn't work only putting
the focus on the main window, the iframes have to be manually blurred.
Attachment #8769205 - Flags: review?(jsantell)


2 years ago
Attachment #8766886 - Attachment is obsolete: true
Attachment #8769205 - Flags: review?(jsantell) → review+
status-firefox47: --- → unaffected


2 years ago
Keywords: checkin-needed

Comment 19

2 years ago
Pushed by cbook@mozilla.com:
Fix shortcut bug in performance tools r=jsantell
Keywords: checkin-needed

Comment 20

2 years ago
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Hi Greg,
Since this patch also affects 48/49, are you also considering to uplift this patch to 48/49?
Flags: needinfo?(gtatum)
Oh yes, that would probably be a good idea to do. Should I rebase my patch for 48 and 49? I've never done that before.
Flags: needinfo?(gtatum)

Comment 23

2 years ago
I have successfully reproduce this bug on firefox nightly 48.0a1 (2016-03-23)
with windows 7 (32 bit)
Mozilla/5.0 (Windows NT 6.1; rv:48.0) Gecko/20100101 Firefox/48.0

I found this fix on latest nightly 50.0a1 (2016-07-17)

Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID : 20160717030211

This is probably too late for 48 but yes, Greg, you should fill an uplift request to aurora at least.
More here:
Flags: needinfo?(gtatum)
status-firefox48: affected → wontfix
Comment on attachment 8769205 [details] [diff] [review]
Fix shortcut bug in performance tools

Approval Request Comment
[Feature/regressing bug #]: 1259228

[User impact if declined]: Shortcuts will be disabled for a user in the performance JS flame graph view devtools until switching to a new tab when this bug is hit.

[Describe test coverage new/current, TreeHerder]: The performance tool has an integration test for verifying the flame graph loads and tears down correctly here:

And full mochitests for its functionality here: https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools%2Fclient%2Fshared%2Ftest+path%3Aflame&redirect=false

[Risks and why]: Low risks, as this patch only sends out some focus and blur events to the UI on state changes.

[String/UUID change made/needed]: none
Flags: needinfo?(gtatum)
Attachment #8769205 - Flags: approval-mozilla-aurora?
Comment on attachment 8769205 [details] [diff] [review]
Fix shortcut bug in performance tools

Review of attachment 8769205 [details] [diff] [review]:

This patch fixes a regression about shortcut disabled in the performance JS flame graph view. Let's take it in aurora.
Attachment #8769205 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 27

2 years ago
status-firefox49: affected → fixed


2 years ago
status-firefox50: fixed → verified


a year ago
See Also: → bug 1327424
You need to log in before you can comment on or make changes to this bug.