1046394 - Profiler doesn't work without certified app pref being turned on

Status: VERIFIED FIXED

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

Description

[:Harald Kirschner :digitarald]

Testing against
Gaia      5c43d012a9351e8aaeacda0b87b0886b7f67d33d
Gecko     e181a36ebafaa24e5390db9f597313406edfc794
BuildID   20140627162151
Version   28.0
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014

Tried Firefox stable, Aurora and Nightly and not one Profiler tab was able to start profiling. The button does nothing. We are sending out Flames as reference device and partners are blocked in performance investigations.

Comment 1

[:Harald Kirschner :digitarald]

RDP dump as requested by fitzgen (starts when clicking "Debug" on the app): http://pastie.org/9432803

Comment 2

[:Harald Kirschner :digitarald]

Same (nothing happens when clicking the "Start" button in Profiler) happens on a Flame with 2.0:
Gaia      8cb1a949f2e9650bb2c5598e78a6f24a58bbaf97
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/4bd4b0ae7bbe
BuildID   20140721000201
Version   32.0a2
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014

Tested on Firefox 31.0 on OSX. Other devtools tabs work.

Comment 3

[:Harald Kirschner :digitarald]

It works on a Keon 1.3 using the same Firefox 31.0:

Gaia      228966331097bbaa735feb45bb9b87967ede18c8
Gecko     323d3b82bdc54de26c2ac516a7c0e6947d1cedbf
BuildID   20140107005540
Version   28.0a2
ro.build.version.incremental=34
ro.build.date=mar ene  7 00:45:19 CET 2014

This makes it a Flame specific issue, tested against 1.3 and 2.0 with latest v123 base image.

Naoki hopefully knows who to look into Flame specific bugs.

Comment 4

[Paul Rouget [:paul]]

Works for me on 2.1.

Do you see anything in the browser console? (To open the Browser Console, select "Browser Console" from the Web Developer submenu in the Firefox Menu (or Tools menu if you display the menu bar or are on Mac OS X)).

Does "adb logcat|grep Gecko" tell you anything?

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

The RDP dump shows that there is no profiler actor returned in the listTabs response. That's why the profiler doesn't work. 

I don't have a git checkout of m-c here, but I would be interested to know which hg cset corresponds to e181a36ebafaa24e5390db9f597313406edfc794. I'd like to inspect b2g/chrome/content/shell.js and toolkit/devtools/server/main.js in that revision.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Also, the unrecognized "reconfigure" request means that it doesn't have the patch from bug 864098, which is to be expected as that landed in 29, while 1.3 has 28, right?

I don't believe that would affect the profiler though.

Comment 7

[Paul Rouget [:paul]]

Harald mentioned that it's also broken with 2.0.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

One thing special about the profiler actor is that it's only available if Ci.nsIProfiler is present:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#409

I think that means that only builds created with MOZ_ENABLE_PROFILER_SPS have it. Not sure if the Flame builds are like that or not.

Comment 9

[Paul Rouget [:paul]]

Maybe engineering builds are, not user builds.

Comment 10

[Paul Rouget [:paul]]

We should at least hide the profiler if the actor is not present.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Not sure if it's an issue with all user builds, I've been using PRODUCTION=1 forever, which I believe results in a user build, but I've always had a working profiler.

Comment 12

[Paul Rouget [:paul]]

I'll look at the user builds tomorrow.

Comment 13

[:Harald Kirschner :digitarald]

All builds I tested on were User builds.

Comment 14

[Paul Rouget [:paul]]

> "nsIProfiler" in Ci
> true
> console.log(Ci.nsIProfiler)
> nsJSIID {  }


Does that mean it's been built wit MOZ_ENABLE_PROFILER_SPS?

Comment 15

[Paul Rouget [:paul]]

I see this exception client side:

Exception calling callback: Error: 'registerEventNotifications' request packet has no destination. (resource://gre/modules/devtools/dbg-client.jsm:662) JS Stack trace: [e-mail]:662:1 < [e-mail]:286:5 < ProfilerController.prototype.connect/register/<@controller.js:245:9 < [e-mail]:168:5 < [e-mail]:1943:9 < [e-mail]:263:7 < [e-mail]:1938:13 < [e-mail]:1043:5 < [e-mail]:194:12 < [e-mail]:1933:1 < [e-mail]:269:7 < [e-mail]:1938:13 < [e-mail]:120:7 < [e-mail]:194:12 < [e-mail]:1933:1 < [e-mail]:269:7 < [e-mail]:1938:13 < [e-mail]:298:7 < [e-mail]:194:12 < [e-mail]:1933:1 < [e-mail]:269:7 < [e-mail]:1938:13 < [e-mail]:363:7 < [e-mail]:194:12 < [e-mail]:1933:1 < [e-mail]:269:7 < [e-mail]:1938:13 < [e-mail]:3675:7 < makeSafe/<@XPIProviderUtils.js:146:17 < [e-mail]:127:5 < this.XPIDatabase.getAddon/<@XPIProviderUtils.js:1091:9 < [e-mail]:866:23 < [e-mail]:745:7

Comment 16

[Paul Rouget [:paul]]

So I managed to make the profiler work. We just need to switch the certified apps debugging pref. Still investigating.

Comment 17

[Paul Rouget [:paul]]

I don't know this code well enough. Delegating to Alex.

Comment 18

[Paul Rouget [:paul]]

I added some dumps in profiler.js. File appears to be loaded, but ProfilerActor is never executed.  

> this.addActors("resource://gre/modules/devtools/server/actors/profiler.js")

This is executed too. No exception.

Could it be because we only do:

> DebuggerServer.addGlobalActor(ProfilerActor, "profilerActor");

?

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

I believe that is one part of the problem. The profiler actor is the only actor in the tree that is only global-scoped. Content processes however use the ContentActor as the main (tab-like) actor, which loads every tab-scoped actor that is present. That list would include all actors but the profiler. So adding the following call to the profiler actor would make sure it is present in the protocol:

DebuggerServer.addTabActor(ProfilerActor, "profilerActor");

The profiler frontend however, is explicitly looking for either a global-scoped profiler actor or a tab-scoped profiler actor that it retrieves manually using a listTabs request. This obviously won't work for content processes, so the profiler controller must be modified to be initialized more like the debugger controller for example.

ISTR there was an additional complication by the fact that the profiler has to connect when the toolbox opens on any tool (or perhaps when the console opens), to make sure any console.profile() calls work properly. This is something to keep in mind while fixing this bug.

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: Fix profiler in content process

https://tbpl.mozilla.org/?tree=Try&rev=a7e94666238e

Conservative patch to support profiler in content processes,
even if the certified pref is turned off.

I may have factored this code a bit more (fetching 'profilerActor')
as there is one specific case for each scenario (regular, chrome, content process).
But it may introduce some regression against older gecko.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Alex, is there anyone else who could review this aside from Panos?  It would be nice to have a fix here before he returns if we can.

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8467792 [details] [diff] [review]
Fix profiler in content process

Review of attachment 8467792 [details] [diff] [review]:
-----------------------------------------------------------------

I think Dave is also a potential reviewer for the profiler.

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: Fix profiler in content process

Same patch, with correct reviewer in commit message.

Comment 24

[Paul Rouget [:paul]]

So this will be fixed only in FxOS 2.1, right? Is there a way to disable the profiler for 1.3-2.0 builds?

Comment 25

[:Harald Kirschner :digitarald]

[Blocking Requested - why for this release]: Profiling apps on the phone will not work without this patch, which means anybody wanting to do performance optimizations for an app targeting 2.0 will be blocked and has to switch to another version with another performance behavior.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

This patch doesn't apply. Please rebase.

Comment 27

[Paul Rouget [:paul]]

Victor, can we ask you to rebase this (very tiny) patch? The whole profiler code has changed. I guess we just need to update browser/devtools/profiler/utils/shared.js, but I'm not sure to understand if _registerEventNotifications should called or not (it is not called for the chrome actor).

Comment 28

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

Ok.

Comment 29

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

Not to self: rebase this.

Comment 30

[Alexandre Poirot [:ochameau]]

I already rebased it, but I'm seeing some breakages in the app-manager

Comment 31

[Alexandre Poirot [:ochameau]]

Attachment: Fix profiler in content process.

Reflagging for review as it wasn't a naive rebase.

So it does fix the profiler when used from WebIDE,
but there is something else broken within the app-manager.
It correctly pulls profile data as I can save a valid JSON file,
but the UI is stuck on "Loading...".
No error message on device, nor in Firefox.

Comment 32

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=7605992b4cc1

Comment 33

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

Comment on attachment 8470902 [details] [diff] [review]
Fix profiler in content process.

Review of attachment 8470902 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/profiler/utils/shared.js
@@ +93,5 @@
>      else if (this._target.root) {
>        this._profiler = this._target.root.profilerActor;
>        yield this._registerEventNotifications();
>      }
> +    // Or if we already have the tab specific one, use it immediately

When is this the case? Can it be tested? If so, please add a test.

@@ +101,2 @@
>      // Otherwise, call `listTabs`.
> +    } else {

To respect the formatting in this file, this should be

// Comment
else if () {
}
// Comment
else {
}

Comment 34

[Alexandre Poirot [:ochameau]]

Attachment: Fix profiler in (b2g and e10s) content processes

There is no way for the moment to test this codepath on device,
as it requires an automation running firefox AND a device.
But I thought it would be possible to test it with e10s,
but it appears that we were getting in a earlier condition, the root actor case,
which is bad, as we are debugging the parent process...
That explains why we aren't seeing any JS profiling when opening the profiler on e10s!
So I moved the if (form.profilerActor) up in order to get in this codepath
for both e10s and devices cases.

Victor, can you confirm I don't break anything by doing so?
It seems to fix profiler support on e10s, I now see some JS data.

It means that we should be able to cover this codepath if we have any test
playing with the profiler with e10s turned on. I imagine we may already have one?
But I would hold on landing this patch for tests and ship this fix asap on devices.

Comment 35

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=a68305c25c9a

Comment 36

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

Comment on attachment 8472303 [details] [diff] [review]
Fix profiler in (b2g and e10s) content processes

Review of attachment 8472303 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, ship it.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/410969b1d389

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/410969b1d389

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.0 uplift.

Comment 40

[Alexandre Poirot [:ochameau]]

Attachment: patch for 2.0 branch

Verified with 2.0 firefox and 2.0 device.
Profiler works fine.

Comment 41

[Alexandre Poirot [:ochameau]]

patch ready for uplift.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/90857b1a7058

Comment 43

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I know you just released simulator 2.0 builds for bug 1053730, but it seems to miss this fix (I don't get a Performance tab anymore).  Can you release a build that includes this fix for 2.0?

Comment 44

[Alexandre Poirot [:ochameau]]

I tried to release new builds, but discovered bug 1061653 which breaks the profiler even more.
So I think we can delay 2.0 update until we figure out this new issue with the profiler.

Comment 45

[Liz Henry (:lizzard) (relman/hg->git project)]

Tony, is this something you or maybe Johan could verify before 34 goes to Beta? I just noticed this as a blocker marked as needing verification. Thanks!

Comment 46

[Tony Chung [:tchung]]

(In reply to Liz Henry :lizzard from comment #45)
> Tony, is this something you or maybe Johan could verify before 34 goes to
> Beta? I just noticed this as a blocker marked as needing verification.
> 

Actually, fxosQA doesnt focus on developer tools on firefox.  perhaps harald and team can double check this again and close when verified.

Comment 47

[Johan Lorenzo [:jlorenzo]]

As per comment 46, feel free to ask me again if you can't check.

Comment 48

[:Harald Kirschner :digitarald]

This works for me now.

Comment 49

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Marking as VERIFIED based on comment 48.