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

Status: VERIFIED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect, P1)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160323030400
STR:
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

Comment 1

[Victor Porof [:vporof][:vp]]

Can't work on this right now, busy with Tofino.

Comment 2

[Victor Porof [:vporof][:vp]]

Can't work on this right now, busy with Tofino.

Comment 3

[Greg Tatum [:gregtatum]]

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

Comment 4

[arni2033]

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.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Joe, status for 48?

Comment 6

[Joe Walker [:jwalker]]

Greg - could you mark a priority for this - is it P1?
Thanks

Comment 7

[Greg Tatum [:gregtatum]]

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.

Comment 8

[Joe Walker [:jwalker]]

Assigning to you Greg, since you're looking at it. Please shout if you need help.
Thanks.

Comment 9

[Greg Tatum [:gregtatum]]

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.

Comment 10

[Greg Tatum [:gregtatum]]

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

[arni2033]

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.

Note:
 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.

Comment 12

[Alexandre Poirot [:ochameau]]

Thanks arni for finding such bugs, your STRs are very valuable!

Comment 13

[Greg Tatum [:gregtatum]]

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.

Comment 14

[Greg Tatum [:gregtatum]]

Attachment: 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.

Comment 15

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e8296bd121

Comment 16

[Jordan Santell [:jsantell] [@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

Comment 17

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d200c4629f9

Comment 18

[Greg Tatum [:gregtatum]]

Attachment: 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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/3cfd5be71eed
Fix shortcut bug in performance tools r=jsantell

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3cfd5be71eed

Comment 21

[Gerry Chang [:gchang]]

Hi Greg,
Since this patch also affects 48/49, are you also considering to uplift this patch to 48/49?

Comment 22

[Greg Tatum [:gregtatum]]

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.

Comment 23

[Md. Rahimul Islam]

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

[bugday-20160713]

Comment 24

[Sylvestre Ledru [:Sylvestre]]

This is probably too late for 48 but yes, Greg, you should fill an uplift request to aurora at least.
More here:
https://wiki.mozilla.org/Release_Management/Uplift_rules

Comment 25

[Greg Tatum [:gregtatum]]

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:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/performance/test/browser_perf-details-render-02-js-flamegraph.js

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

Comment 26

[Gerry Chang [:gchang]]

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.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/20fca29df103