Closed Bug 1259228 Opened 4 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

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

VERIFIED FIXED
Firefox 50
Tracking Status
firefox47 --- unaffected
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- verified

People

(Reporter: arni2033, Assigned: gregtatum)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

>>>   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
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.
I can't reproduce this on a Mac, I'll need to get on a Windows machine.
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?
Thanks
Flags: needinfo?(jwalker) → needinfo?(gtatum)
Flags: needinfo?(gtatum)
Priority: -- → P1
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.
Thanks.
Assignee: nobody → gtatum
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.
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.
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.
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.
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+
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)
Attachment #8766886 - Attachment is obsolete: true
Attachment #8769205 - Flags: review?(jsantell) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/3cfd5be71eed
Fix shortcut bug in performance tools r=jsantell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3cfd5be71eed
Status: NEW → RESOLVED
Closed: 3 years ago
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)
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]
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
Flags: needinfo?(gtatum)
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
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+
Status: RESOLVED → VERIFIED
See Also: → 1327424
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.