751034 - Support remote profiling Via Remote debugging protocol

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

The aim is to control the profiler module using the Remote debugging protocol to support profiling Fennec, B2G and possibly desktop remotely.

https://wiki.mozilla.org/Remote_Debugging_Protocol

Comment 1

[Benoit Girard (:BenWa)]

WIP:
Profiling add-on: https://github.com/bgirard/Gecko-Profiler-Addon/commit/9376ed7b0663380cbc819e241d4abba17a93f7b4
Gecko changes: See attachment

I have basic communication with the root actor working. I need to expose a profiler actor that will wrap the functionality in the profiler module.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: WIP

I'm not sure how to hook in my actor in 'dbg-server.js' without being invasive. What do you think of this?

Comment 3

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

(In reply to Benoit Girard (:BenWa) from comment #2)
> I'm not sure how to hook in my actor in 'dbg-server.js' without being
> invasive. What do you think of this?

I think the best way to hook your actors is following the example extension in:

https://github.com/campd/sample-debug-actors

The most relevant bits are here:

https://github.com/campd/sample-debug-actors/blob/master/chrome/content/sample-actors.js

which basically boils down to call DebuggerServer.addTabRequest("actorName", actorHandler). That way you don't need to mess with anything else, besides your own actor. The comments in that file explain various other bits of how an actor is structured for cleanup, etc.

Also, see overlay.js in that repo for how the startup is done, which pretty much matches what you already do.

Comment 4

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

Comment on attachment 620187 [details] [diff] [review]
WIP

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

As discussed in IRC, I agree that this is a fine way to do it, if you want to have the profiler always on, and not as an extension. One minor thing to note, is that you don't have to put the actors in a module, as we generally load them inside the debugger server module.

Comment 5

[Benoit Girard (:BenWa)]

If I change it to profiler-actors.js I'm not sure where to what url to use if it's not a module and how to include it in the Makefile.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: patch

I know loading the JSM is wrong. Not sure how to get a js file to be picked up in tools/profiler and what URL to use there.

Other then that this patch should be all we need. The rest of the changes should be done in the client add-on.

Comment 7

[Jim Blandy :jimb]

Comment on attachment 620442 [details] [diff] [review]
patch

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

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +134,5 @@
> +    try {
> +      // If we disable the profiler on non Tier-1 platforms we wont
> +      // find this script.
> +      this.addActors("resource://gre/modules/profiler-actors.jsm");
> +    } catch(e) {}

Isn't this going to mask all kinds of problems loading the file: syntax errors, etc.? If so... well, let me just say that eating errors is one thing that will get me really upset. :)

Comment 8

[Benoit Girard (:BenWa)]

Well I figured worse case it would disable the profiler actor if there was a syntax error and not impact the dbg-server.

Maybe I can look for a better way to check at runtime if that script is present.

Comment 9

[Dave Camp (:dcamp)]

Comment on attachment 620442 [details] [diff] [review]
patch

I'm going to pass the review over to Panos, but if Jim wants to take it that would be fine too.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

What about having a 'DebuggerServer.addRequest' API that would allow to dynamically extend the 'BrowserRootActor.prototype.requestTypes' array (also from within an extension). This way we don't need to change dbg-browser-actors.js every time a new actor (a child of the root) appears.

DebuggerServer.addRequest = function DS_addRequest(aName, aFunction) {
  DebuggerServer.BrowserRootActor.prototype.requestTypes[aName] = function(aRequest) {
    return aFunction(this, aRequest);
  }
};

This would be similar to what is there for tab actors: 'DebuggerServer.addTabRequest'

The function could live in dbg-browser-actors.js 

Honza

Comment 11

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

Comment on attachment 620442 [details] [diff] [review]
patch

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

(In reply to Benoit Girard (:BenWa) from comment #5)
> If I change it to profiler-actors.js I'm not sure where to what url to use
> if it's not a module and how to include it in the Makefile.

I see the native part gets put in the components folder, maybe you can register the JS part as a component, too?

::: toolkit/devtools/debugger/server/dbg-browser-actors.js
@@ +206,5 @@
>   * The request types this actor can handle.
>   */
>  BrowserRootActor.prototype.requestTypes = {
> +  "listTabs": BrowserRootActor.prototype.onListTabs,
> +  "getProfiler": BrowserRootActor.prototype.onGetProfiler

I don't see onGetProfiler defined in this patch. Perhaps you forgot to hg qref?

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +134,5 @@
> +    try {
> +      // If we disable the profiler on non Tier-1 platforms we wont
> +      // find this script.
> +      this.addActors("resource://gre/modules/profiler-actors.jsm");
> +    } catch(e) {}

I share Jim's concern. One way to check for the existence of the actors is like this:

FileUtils.getFile("CurProcD", "profiler-actors.jsm", true).exists()

@@ +245,5 @@
>  
>      // Create a root actor for the connection and send the hello packet.
>      conn.rootActor = this.createRootActor(conn);
>      conn.addActor(conn.rootActor);
> +    if (this.createProfilerActor()) {

You don't want to invoke the function here, just check for its presence.

@@ +246,5 @@
>      // Create a root actor for the connection and send the hello packet.
>      conn.rootActor = this.createRootActor(conn);
>      conn.addActor(conn.rootActor);
> +    if (this.createProfilerActor()) {
> +      conn.addActor(this.createProfilerActor());

If you are going to need an extension to use the profiler anyway, then I think Honza's proposal in comment 10 is better than hard-wiring the profile actor here. Jim, what do you think?

Unless I got this backwards and you don't need the extension for recording the profile. In that case there would have to be some way to start the server and register the actors. What would the UI be in that case?

::: tools/profiler/profiler-actors.jsm
@@ +53,5 @@
> + */
> +function ProfilerActor()
> +{
> +  this.conn = null; // set by ActorPool.actor
> +  this.profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);

We don't hard-wire these things in other actors, just pass the connection as a parameter to the constructor. And if you need to avoid creating multiple nsIProfiler instances, you can store a reference to the profiler actor in the browser root actor (under _profilerActor or something) and reuse it.

@@ +58,5 @@
> +}
> +
> +ProfilerActor.prototype = {
> +  actorPrefix: "profiler",
> +  actorID: "profiler",

With the above you don't need to hard-wire the actorID, just let the pool assign one and the client can get it from the getProfiler protocol response.

@@ +79,5 @@
> +    return { "profile": profile }    
> +  },
> +  onIsActive: function(aRequest) {
> +    var isActive = this.profiler.IsActive();
> +    return { "isActive": isActice }    

isActice -> isActive

@@ +106,5 @@
> +  "getProfile": ProfilerActor.prototype.onGetProfile,
> +  "isActive": ProfilerActor.prototype.onIsActive,
> +  "getResponsivenessTimes": ProfilerActor.prototype.onGetResponsivenessTimes,
> +  "getFeatures": ProfilerActor.prototype.onGetFeatures,
> +  "getSharedLibraryInformation": ProfilerActor.prototype.onGetSharedLibraryInformation

I can't provide feedback on the profiler protocol bits, as I haven't tested the extension yet. I'll get back to you on this after I've had a chance to play with it first.

Comment 12

[Josh Matthews [:jdm]]

>::: toolkit/devtools/debugger/server/dbg-server.js
>@@ +134,5 @@
>> +    try {
>> +      // If we disable the profiler on non Tier-1 platforms we wont
>> +      // find this script.
>> +      this.addActors("resource://gre/modules/profiler-actors.jsm");
>> +    } catch(e) {}
>
>I share Jim's concern. One way to check for the existence of the actors is like this:
>
>FileUtils.getFile("CurProcD", "profiler-actors.jsm", true).exists()

On the other hand, the exists() check is extra IO on the main thread, and we have been specifically stamping out exists/open pairs in the rest of the codebase.

Comment 13

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

(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #12)
> >::: toolkit/devtools/debugger/server/dbg-server.js
> >@@ +134,5 @@
> >> +    try {
> >> +      // If we disable the profiler on non Tier-1 platforms we wont
> >> +      // find this script.
> >> +      this.addActors("resource://gre/modules/profiler-actors.jsm");
> >> +    } catch(e) {}
> >
> >I share Jim's concern. One way to check for the existence of the actors is like this:
> >
> >FileUtils.getFile("CurProcD", "profiler-actors.jsm", true).exists()
> 
> On the other hand, the exists() check is extra IO on the main thread, and we
> have been specifically stamping out exists/open pairs in the rest of the
> codebase.

That is a good point. In that case the extension API described in comment 10 becomes even more attractive. Alternatively, we can provide a stub actor implementation for non Tier-1 platforms, or a generic base implementation with hooks in there for platform-specific bits.

Comment 14

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

(In reply to Panos Astithas [:past] from comment #11)
> @@ +106,5 @@
> > +  "getProfile": ProfilerActor.prototype.onGetProfile,
> > +  "isActive": ProfilerActor.prototype.onIsActive,
> > +  "getResponsivenessTimes": ProfilerActor.prototype.onGetResponsivenessTimes,
> > +  "getFeatures": ProfilerActor.prototype.onGetFeatures,
> > +  "getSharedLibraryInformation": ProfilerActor.prototype.onGetSharedLibraryInformation
> 
> I can't provide feedback on the profiler protocol bits, as I haven't tested
> the extension yet. I'll get back to you on this after I've had a chance to
> play with it first.

I played with the extension a bit and I have some feedback on the protocol. Great visualization, btw!

I put the packet exchange that I observed in a pastebin for reference:

http://past.pastebin.mozilla.org/1625237

My first observation is that only one method seems to be working at the moment, 'getProfileStr'. I don't know if I had to do something different to trigger 'getResponsivenessTimes', 'getFeatures', etc.

The initial getProfiler actor lookup request is missing, as discussed previously. With that, the client can get the actor ID to sent its followup requests to, without having to hard-code the name 'profiler'.

The data exchanged via getProfileStr is a big opaque string. If you take a look at the remote protocol specification, the common practice is to break down the data in objects and properties. I think that the form of the data you are exchanging maps nicely to that, although I haven't dived deeply into the specifics. What kind of data is being exchanged through 'getProfile'?

'startProfiler'/'stopProfiler' don't seem to be triggered by the respective buttons. The same is true for 'getSharedLibraryInformation' although the client appears to take advantage of the fact that it's running in the same process as the server to find them?

That's all I have so far, I'll gladly take another pass if you can tell me how to trigger the other request handlers.

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Panos Astithas [:past] from comment #13)
> That is a good point. In that case the extension API described in comment 10
> becomes even more attractive. Alternatively, we can provide a stub actor
> implementation for non Tier-1 platforms, or a generic base implementation
> with hooks in there for platform-specific bits.
Should we have another bug reported for DebuggerServer.addRequest or we discuss & commit as part of this bug...?

Honza

Comment 16

[Benoit Girard (:BenWa)]

(In reply to Panos Astithas [:past] from comment #14)
> I played with the extension a bit and I have some feedback on the protocol.
> Great visualization, btw!

Cool, you might of missed this but it's possible to focus on sub ranges while the browser is non responsive which is the primary feature of this profiler, catching short jank.

> 
> I put the packet exchange that I observed in a pastebin for reference:
> 
> http://past.pastebin.mozilla.org/1625237
> 
> My first observation is that only one method seems to be working at the
> moment, 'getProfileStr'. I don't know if I had to do something different to
> trigger 'getResponsivenessTimes', 'getFeatures', etc.

This is current just a test. Down the road all the calls would be used to select profiling features, set sampling rates and get data to graph responsiveness.

> 
> The initial getProfiler actor lookup request is missing, as discussed
> previously. With that, the client can get the actor ID to sent its followup
> requests to, without having to hard-code the name 'profiler'.
> 
> The data exchanged via getProfileStr is a big opaque string. If you take a
> look at the remote protocol specification, the common practice is to break
> down the data in objects and properties. I think that the form of the data
> you are exchanging maps nicely to that, although I haven't dived deeply into
> the specifics. What kind of data is being exchanged through 'getProfile'?

This is exactly the 'getProfile' format. getProfileStr is our old format that I'm keeping around as I switch all the components to use the JSON format.

> 
> 'startProfiler'/'stopProfiler' don't seem to be triggered by the respective
> buttons. The same is true for 'getSharedLibraryInformation' although the
> client appears to take advantage of the fact that it's running in the same
> process as the server to find them?

The addon was only sending a few test messages over the protocol. Once this land I will add a 'remote' mode where you will give it connection information to a device such as Fennec/B2G and data will be exchanged over the protocol.

(In reply to Jan Honza Odvarko from comment #15)
> Should we have another bug reported for DebuggerServer.addRequest or we
> discuss & commit as part of this bug...?

Filed bug 752896

Comment 17

[Jim Blandy :jimb]

(In reply to Benoit Girard (:BenWa) from comment #8)
> Maybe I can look for a better way to check at runtime if that script is
> present.

The catch clause should at least call Cu.reportError.

Comment 18

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #11)
> If you are going to need an extension to use the profiler anyway, then I
> think Honza's proposal in comment 10 is better than hard-wiring the profile
> actor here. Jim, what do you think?

Yeah, I agree. Making people patch the root actor every time they want to add new functionality seems like it would be discouraging.

Comment 19

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #18)
> Yeah, I agree. Making people patch the root actor every time they want to
> add new functionality seems like it would be discouraging.

... and it doesn't work for add-ons.

Comment 20

[Benoit Girard (:BenWa)]

This bug is still depending on some architectural changes in the debug protocol but I can't find the bug, does someone know where it is.

Our current signal scheme is being stretched to its limits and is caused very query users interactions and wont scale to multi-process on b2g so this is blocking several profiling improvements.

Comment 21

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

(In reply to Benoit Girard (:BenWa) from comment #20)
> This bug is still depending on some architectural changes in the debug
> protocol but I can't find the bug, does someone know where it is.
> 
> Our current signal scheme is being stretched to its limits and is caused
> very query users interactions and wont scale to multi-process on b2g so this
> is blocking several profiling improvements.

I'll get back to bug 753401 in the next few days.

Comment 22

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

Attachment: updated patch against the new API

So after the changes in bug 753401, the only required bit is the profiler actor definition. I've moved it next to the other remote debugging protocol actor definitions, since this is where the web console actors are going to land for now. If at some point we decide to reconsider, we can move them somewhere else.

The changes in the profiler actor are minimal, a "disconnect" method, no actorID hard-coding, and minor cleanups. This patch combined with some changes in the add-on client (for which a pull request will be forthcoming), result in the same functionality as in the previous patch. Benoit, let me know what you think, and then we can ask another devtools peer to review it, so we can land this.

Comment 23

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

Attachment: protocol log

A log of the protocol interactions for reference, since my previous pastebin has expired.

Comment 24

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

(In reply to Panos Astithas [:past] from comment #22)
> This patch combined with some
> changes in the add-on client (for which a pull request will be forthcoming),
> result in the same functionality as in the previous patch.

https://github.com/bgirard/Gecko-Profiler-Addon/pull/19

Comment 25

[Benoit Girard (:BenWa)]

Comment on attachment 650112 [details] [diff] [review]
updated patch against the new API

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

Looks great.

::: toolkit/devtools/debugger/server/dbg-profiler-actors.js
@@ +19,5 @@
> +  actorPrefix: "profiler",
> +
> +  disconnect: function() {
> +    if (this.profiler) {
> +      this.profiler.StopProfiler();

I'm not sure if stopping the profiler on disconnect is the right thing to do. Will this get called if you have multiple connections open and one is closed? Also other things can be controlling the profiler (console.profile, c++) and since we only have a global profiling instance I think stopping the profiler on a disconnect may be very confusing if you're controlling profiling via another method and not expecting it to stop if doing an unrelated action (using the debugger).

Comment 26

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

(In reply to Benoit Girard (:BenWa) from comment #25)
> I'm not sure if stopping the profiler on disconnect is the right thing to
> do. Will this get called if you have multiple connections open and one is
> closed? Also other things can be controlling the profiler (console.profile,
> c++) and since we only have a global profiling instance I think stopping the
> profiler on a disconnect may be very confusing if you're controlling
> profiling via another method and not expecting it to stop if doing an
> unrelated action (using the debugger).

I wasn't sure either. disconnect is called when the actor is removed from the pool, so it's the place for any cleanup activities necessary. I'm not clear on the profiler lifecycle, so if there is nothing that needs to be done here, even better.

I assume that you want to start the sampling/data-collection process as soon as the browser starts and maybe stop it when the browser ends. The debugger server however seems to start too soon as it is now in the add-on, and I think it would be best to start it when the user initiates some sort of interaction that should result in results being shown. The lifecycle of the protocol actors doesn't necessarily have to be the same as the lifecycle of the sampling components. I know the client is just an early prototype, but I just wanted to throw that out there.

Comment 27

[Benoit Girard (:BenWa)]

(In reply to Panos Astithas [:past] from comment #26)
> (In reply to Benoit Girard (:BenWa) from comment #25)
> > I'm not sure if stopping the profiler on disconnect is the right thing to
> > do. Will this get called if you have multiple connections open and one is
> > closed? Also other things can be controlling the profiler (console.profile,
> > c++) and since we only have a global profiling instance I think stopping the
> > profiler on a disconnect may be very confusing if you're controlling
> > profiling via another method and not expecting it to stop if doing an
> > unrelated action (using the debugger).
> 
> I wasn't sure either. disconnect is called when the actor is removed from
> the pool, so it's the place for any cleanup activities necessary. I'm not
> clear on the profiler lifecycle, so if there is nothing that needs to be
> done here, even better.

It might make sense to track if the profiler was started using the debug protocol (startProfiler) using a bool and if so then we stop it on disconnect. It's a bit harry if something else is reading but we don't really support that use case anyways.

> 
> I assume that you want to start the sampling/data-collection process as soon
> as the browser starts and maybe stop it when the browser ends. The debugger
> server however seems to start too soon as it is now in the add-on, and I
> think it would be best to start it when the user initiates some sort of
> interaction that should result in results being shown. The lifecycle of the
> protocol actors doesn't necessarily have to be the same as the lifecycle of
> the sampling components. I know the client is just an early prototype, but I
> just wanted to throw that out there.

We have an environment variable MOZ_PROFILER_STARTUP that will start the profiler as early as possible (XRE_main). If users are interested in sampling startup they can start the browser with this variable (or force it using a patch) and then attach with the protocol to dump the profile once the browser is started.

Comment 28

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

Attachment: Patch v3

(In reply to Benoit Girard (:BenWa) from comment #27)
> (In reply to Panos Astithas [:past] from comment #26)
> > (In reply to Benoit Girard (:BenWa) from comment #25)
> > > I'm not sure if stopping the profiler on disconnect is the right thing to
> > > do. Will this get called if you have multiple connections open and one is
> > > closed? Also other things can be controlling the profiler (console.profile,
> > > c++) and since we only have a global profiling instance I think stopping the
> > > profiler on a disconnect may be very confusing if you're controlling
> > > profiling via another method and not expecting it to stop if doing an
> > > unrelated action (using the debugger).
> > 
> > I wasn't sure either. disconnect is called when the actor is removed from
> > the pool, so it's the place for any cleanup activities necessary. I'm not
> > clear on the profiler lifecycle, so if there is nothing that needs to be
> > done here, even better.
> 
> It might make sense to track if the profiler was started using the debug
> protocol (startProfiler) using a bool and if so then we stop it on
> disconnect. It's a bit harry if something else is reading but we don't
> really support that use case anyways.

Did that, and also made the profiler reference private. Requesting review might be a bit premature, since bug 753401 hasn't landed yet, but it shouldn't hurt.

Comment 29

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 650504 [details] [diff] [review]
Patch v3

yeah, this looks fine to me.

Comment 30

[Benoit Girard (:BenWa)]

I'm working on adding a JS Source view to the profiler. I think for this to work properly we will need to fetch the document from the target since some script may be generated conditionally based on the session and UA. We should add something to the profiling protocol to fetch a document and return the source.

Comment 31

[Dave Camp (:dcamp)]

See bug 772119 and bug 755661

Comment 32

[Mike Hommey [:glandium]]

The patch needs to be updated to remove the Ci and Cc declarations:
TypeError: redeclaration of var Ci - @chrome://global/content/devtools/dbg-profiler-actors.js:6

Comment 33

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

Attachment: Patch v4

(In reply to Mike Hommey [:glandium] from comment #32)
> The patch needs to be updated to remove the Ci and Cc declarations:
> TypeError: redeclaration of var Ci -
> @chrome://global/content/devtools/dbg-profiler-actors.js:6

Done.

Comment 34

[Mike Hommey [:glandium]]

Comment on attachment 658871 [details] [diff] [review]
Patch v4

Carrying over r+, since the only change was removing Cc and Ci definitions.

Comment 35

[Mike Hommey [:glandium]]

Attachment: part 2 - Add profiler actor when adding the browser actors

Comment 36

[Mike Hommey [:glandium]]

Attachment: part 3 - Send notifications through remote debugging protocol when the profiler starts/stops

Comment 37

[Mike Hommey [:glandium]]

Attachment: Support remote profiling Via Remote debugging protocol.

The arguments to StartProfiler were in the wrong order.

Comment 38

[Mike Hommey [:glandium]]

Comment on attachment 660769 [details] [diff] [review]
Support remote profiling Via Remote debugging protocol.

Carrying over r+

Comment 39

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

Comment on attachment 660588 [details] [diff] [review]
part 2 - Add profiler actor when adding the browser actors

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

This looks fine as long as you are sure that all products (incl. Fennec and B2G) contain nsIProfiler and won't throw here.

There are also people looking into getting the remote protocol to work for Thunderbird. Is nsIProfiler present there, too?

Comment 40

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

Comment on attachment 660590 [details] [diff] [review]
part 3 - Send notifications through remote debugging protocol when the profiler starts/stops

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

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +166,5 @@
>  const UnsolicitedNotifications = {
>    "newScript": "newScript",
>    "tabDetached": "tabDetached",
> +  "tabNavigated": "tabNavigated",
> +  "profilerStateChanged": "profilerStateChanged"

Nit: we keep this list sorted.

::: toolkit/devtools/debugger/server/dbg-profiler-actors.js
@@ +12,2 @@
>  {
> +  this._conn = aConnection;

If you don't use the connection in the constructor you don't need this, because one is patched to it when it is added to the actor pool. This is why this.conn.send in the observe method works, even though you store a _conn here.

@@ +14,4 @@
>    this._profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
>    this._started = false;
> +
> +  Cu.import("resource://gre/modules/Services.jsm");

This is redundant, since Services is already imported by dbg-server.js.

@@ +15,5 @@
>    this._started = false;
> +
> +  Cu.import("resource://gre/modules/Services.jsm");
> +  Services.obs.addObserver(this, "profiler-started", false);
> +  Services.obs.addObserver(this, "profiler-stopped", false);

Since the protocol provides operations to start and stop the profiler, what is the intended use for these noticications? Isn't the client controlling the profiler lifecycle?

Comment 41

[Mike Hommey [:glandium]]

(In reply to Panos Astithas [:past] from comment #40)
> Since the protocol provides operations to start and stop the profiler, what
> is the intended use for these noticications? Isn't the client controlling
> the profiler lifecycle?

It does, but it's also possible it's not alone doing so.

Comment 42

[Mike Hommey [:glandium]]

Attachment: part 2 - Add profiler actor when adding the browser actors

This avoids registering the actor when nsIProfiler is not available.

Comment 43

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

https://hg.mozilla.org/integration/fx-team/rev/d8921aee4a5d
https://hg.mozilla.org/integration/fx-team/rev/f11a44846f26
https://hg.mozilla.org/integration/fx-team/rev/f0a944193de7

Comment 44

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/d8921aee4a5d
https://hg.mozilla.org/mozilla-central/rev/f11a44846f26
https://hg.mozilla.org/mozilla-central/rev/f0a944193de7