1602800 - Can't profile Fenix via `about:debugging`

Status: VERIFIED FIXED

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

Description

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

When connecting to a remote Fenix and hitting the "Profile performance" button, I get a blank panel. The following error also occurs in the browser console:

uncaught exception: Protocol error (unknownError): preference is not of the right type: devtools.performance.recording.entries

Comment 1

[Greg Tatum [:gregtatum]]

:snorp Can you provide the Firefox version for the desktop Firefox, and the remote target? This sounds like a remote debugging protocol backwards compatibility error.

Comment 2

[Greg Tatum [:gregtatum]]

Or perhaps it's because the prefs are now defined in the Firefox preferences, and not in the DevTools anymore.

https://searchfox.org/mozilla-central/source/browser/app/profile/firefox.js#2097

Comment 3

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

I'm using NIghtly for both.

Comment 4

[Julian Descottes [:jdescottes]]

Bug 1599745 definitely broke backward compatibility, remote debugging FF71 from FF72 or 73 leads to a white screen. And I guess browser/app/profile/firefox.js is not used for Fenix, so it probably also broke mobile remote debugging in general.

If this is correct moving the prefs to init/all.js would fix mobile remote debugging, but it won't fix backward compatibility issues.
The client code can't assume that the preferences exist / have the right type on the debuggee.

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1602800 - Move profiler recording preferences from firefox.js to all.js r?gregtatum

We had to do it because firefox android doesn't use that file. There is a
mobile.js file for that purpose. We had to move all recording preferences to
all.js and add the android only preference to mobile.js to be able to handle
that better.

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1602800 - Move the function that gets BrowsingContext ID to a new file inside shared folder r?gregtatum

Depends on D56719

Comment 7

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1602800 - Backout some of the preferences handling to make sure we have defaults for older firefox versions r?gregtatum

Depends on D56720

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16642f81cdc4
Move profiler recording preferences from firefox.js to all.js r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/0694837833fe
Move the function that gets BrowsingContext ID to a new file inside shared folder r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/3263f5baebf0
Backout some of the preferences handling to make sure we have defaults for older firefox versions r=gregtatum

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/16642f81cdc4
https://hg.mozilla.org/mozilla-central/rev/0694837833fe
https://hg.mozilla.org/mozilla-central/rev/3263f5baebf0

Comment 10

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Hey James, the fix is on mozilla-central now. It solved the issue on my end but could you also test and let me know if it works now?
We will also most likely need to uplift those patches to beta since it's also broken.

Comment 11

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

Yup, works with the example app now in latest m-c.

Comment 12

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 9115158 [details]
Bug 1602800 - Move profiler recording preferences from firefox.js to all.js r?gregtatum

Beta/Release Uplift Approval Request

• User impact if declined: Users won't be able to profile Fenix/Firefox Android or older Firefox versions
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: STR:

• Enable remote debugging in your android device(it can be an emulator too, it should already be checked in that case).
• Download and open geckoview_example app(or Firefox Preview Nightly) in that device.
• Check the "Remote Debugging" checkbox in the app settings.
• Connect the Android device with a USB to your desktop.
• Open Firefox in your desktop and navigate to "about:debugging".
• Enable USB devices. Your android device should pop up on the left sidebar.
• Click to "Connect" button right side of your device and go inside that device.
• Click to "Profile Performance" button and see that the opened popup has content(previously it did not).
• Click to "Start Recording" button, then click to "Capture recording".

ER:
It should capture a profile and navigate to "profiler.firefox.com" with that captured profile data.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's not risky, because first patch includes preference changes only, second patch moves a function to a new file and third patch mostly reverts the old change. Also, it's not risky because remote profiling is something gecko developers and power users do. There is no way to break end user workflow with those changes. But without that uplift, we won't be able to profile older Firefox versions.
• String changes made/needed:

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 9115158 [details]
Bug 1602800 - Move profiler recording preferences from firefox.js to all.js r?gregtatum

profiler regression fix, approved for 72.0b8

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e4dc052eb3f3
https://hg.mozilla.org/releases/mozilla-beta/rev/e8d6256cf1f7
https://hg.mozilla.org/releases/mozilla-beta/rev/63f6ab33a4e9

Comment 15

[Stefan Deiac]

Hello, I can confirm that this issue is fixed using the latest version of Firefox Preview Nightly 12/27 #13610605.

Devices:

• LG G7 fit (Android 8.1);
• OnePlus 5T (Android 9);
• Google Pixel (Android Q).