1650658 - Can't jump to debugger tab from performance tab in call tree view

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P2)

Description

[marvin]

Attachment: Screenshot 2020-07-05 at 20.29.29.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:79.0) Gecko/20100101 Firefox/79.0

Steps to reproduce:

1. Go to https://preactjs.com
2. Open devtools
3. Click on performance tab in devtools
4. Click record
5. Reload page
6. Stop recording
7. Click on Call Tree view and try to click on any js links

Actual results:

Nothing

Expected results:

Jump to the source location in the debugger like the title suggests

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Logan Smyth [:loganfsmyth]]

This was regressed by https://phabricator.services.mozilla.com/D68537 because it removed PerformanceController.toolbox but presently, that variable is used in 4 places (https://searchfox.org/mozilla-central/search?q=PerformanceController.toolbox&path=) to call PerformanceController.toolbox.viewSourceInDebugger(...), so now all of these places throw because toolbox is undefined.

@daisuke I can't quite tell why toolbox was removed, would that be easy to add back in?

Comment 3

[Daisuke Akatsuka (:daisuke)]

Thank you very much for reporting and I'm sorry for the inconvenience, marvin!
I could reproduce the issue.

Hi Logan, thank you very much for finding the cause!
Yes, it seems that it was caused by my patch.
I had thought that the toolbox is never referred from any places, I'm so sorry..
I'll fix this soon.

Comment 4

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1650658: Revert PerformanceController to have toolbox.

Comment 5

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1650658 - Pass line and column position as numbers instead of strings. r=davidwalsh!

Comment 6

[marvin]

Thank you very much for reporting and I'm sorry for the inconvenience, marvin!

(...)

I had thought that the toolbox is never referred from any places, I'm so sorry.. I'll fix this soon.

@daisuke No worries, stuff like that happens to all of us and is a normal part of being a developer. Don't beat yourself up about it :)

Comment 7

[Daisuke Akatsuka (:daisuke)]

(In reply to marvin from comment #6)

@daisuke No worries, stuff like that happens to all of us and is a normal part of being a developer. Don't beat yourself up about it :)
Thanks :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93092fe1e43c
Revert PerformanceController to have toolbox. r=loganfsmyth

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/93092fe1e43c

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0e848b64e25
Pass line and column position as numbers instead of strings. r=davidwalsh

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/f0e848b64e25

Comment 12

[Logan Smyth [:loganfsmyth]]

@daisuke How do you feel about uplifting these patches? Seems like a good idea to me and unlikely to have any issues.

Comment 13

[Daisuke Akatsuka (:daisuke)]

(In reply to Logan Smyth [:loganfsmyth] from comment #12)

@daisuke How do you feel about uplifting these patches? Seems like a good idea to me and unlikely to have any issues.

Hi @loganfsmyth!
Yes, I don't see any problems.
Could you do this since I am not sure the uplifting risk of your patch much?

Comment 14

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9162376 [details]
Bug 1650658: Revert PerformanceController to have toolbox.

Beta/Release Uplift Approval Request

• User impact if declined: Users will be unable to click filenames in the Devtools Performance tool to open them in the debugger.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is not risky because it is reverting a change made in another patch by adding a property to an object that was incorrectly removed. The related patch in D82950 is also not risky because the only change is to parse a string value into a number value.
• String changes made/needed:

Comment 15

[(PTO June 19-July 7) Ryan VanderMeulen]

Comment on attachment 9162376 [details]
Bug 1650658: Revert PerformanceController to have toolbox.

Devtools regression fix. Approved for 79.0b9.

Comment 16

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-beta/rev/a898b11a3713
https://hg.mozilla.org/releases/mozilla-beta/rev/496739475b35

Comment 17

[Anca Soncutean, Desktop QA]

I’ve reproduced this issue with Fx 80.0a1 (2020-07-05) on Windows 10.
The issue is fixed with Fx 80.0a1 (2020-07-19) and Fx 79.0b9 on Windows 10 and Ubuntu 18.04.