855244 - Profiler hangs after pressing "Stop" if it was started in multiple profiles in different tabs

Status: RESOLVED FIXED

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

Description

[Mihaela Velimiroviciu (:mihaelav)]

This is a follow up of bug 830664. In that bug, starting profiling in multiple profiles for *the same tab* was fixed, but you can still start a new (second) profiling process in a new tab while there is one already running in other tab and that causes one of the processes to hang (not able to stop it).

Steps:
1. Open a page and start profiling
2. Open a page in a new tab and start profiling
3. Stop profiling for the first page
4. Go to the other page and stop profiling

Expected result:
In the second page, the profiler should stop and display the processed data

Actual result:
At step 4, the profiler hangs and you cannot interact with it anymore. Processed data is not displayed and the "Stop" button is grayed out. You cannot start profiling for that tab until you reopen the Profiler.

Comment 1

[Anton Kovalyov (:anton)]

So to fix this bug I'll need to detach controller from the devtools panel UI and make it a separate, independent entity. This way any panel (and console API!) will be able to use it to start and stop profiles without any conflicts between them. Unfortunately, there's not enough time to get this fix safely coded, tested and reviewed before the next merge.

Comment 2

[Anton Kovalyov (:anton)]

Attachment: Add support for the Profielr running in multiple tabs

This patch refactors ProfilerController and accomplishes two things: 1) brings me closer to implementing the Console API from bug 817836 and 2) fixes the bug reported here.

First of all, I got rid of ProfilerConnection and moved all the request/response code into the ProfilerController. Seems much cleaner this way. Then I introduced a shared data structure to hold stuff that needs to be shared between multiple ProfilerController instances[1]. Then I rewrote ProfilerController to use sharedData and, at last, I modified our profiler actor to stop the profiler only when the last client disconnects. Before it would stop the profiler on every disconnect event.

Oh and I also removed code that stops the profiler on ProfilerController.stop. This is because checking if there are any active profiles across all tabs is rather painful (I can't use WeakMap as a data structure, for example) and we stop the profiler when everyone disconnects[2] anyway.

BenWa, asking for feedback to make sure I didn't introduce any changes in the actor that are incompatible with the addon.

[1] - For example, time when profiler was started. Since any tab can start a profiler we need to share this data with everyone else.
[2] - For example, when you close the devtools panel in all tabs.

Comment 3

[Anton Kovalyov (:anton)]

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

Comment 4

[Anton Kovalyov (:anton)]

Try push seems to be green.

Comment 5

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

Comment on attachment 733181 [details] [diff] [review]
Add support for the Profielr running in multiple tabs

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

alright, looks good. passes try?

::: browser/devtools/profiler/ProfilerController.jsm
@@ +169,5 @@
>      if (this.isProfileRecording(profile)) {
>        return void cb();
>      }
>  
> +    this.isActive((err, isActive) => {

science fiction.

::: toolkit/devtools/debugger/server/dbg-profiler-actors.js
@@ +29,5 @@
> +    // We stop the profiler only after the last client is
> +    // disconnected. Otherwise there's a problem where
> +    // we stop the profiler as soon as you close the devtools
> +    // panel in one tab even though there might be other
> +    // profiler instances running in other tabs.

good good.

Comment 6

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

passes try.

Comment 7

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

Comment on attachment 733181 [details] [diff] [review]
Add support for the Profielr running in multiple tabs

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

(In reply to Anton Kovalyov (:anton) from comment #2)
> Oh and I also removed code that stops the profiler on
> ProfilerController.stop. This is because checking if there are any active
> profiles across all tabs is rather painful (I can't use WeakMap as a data
> structure, for example) and we stop the profiler when everyone
> disconnects[2] anyway.

Are you sure this will have no performance impact in cases where the profiler may be stopped, but kept around in a detached toolbox, for instance? I don't know if it will, but detached toolboxes tend to be rather long-lived in my experience.

::: browser/devtools/profiler/ProfilerController.jsm
@@ +118,5 @@
> +  },
> +
> +  /**
> +   * Adds actor and type information to data and request over
> +   * the remote debugging protocol.

Nit: "and sends the request"

::: browser/devtools/profiler/test/browser_profiler_bug_855244_multiple_tabs.js
@@ +9,5 @@
> +registerCleanupFunction(function () {
> +  gTab1 = gTab2 = gPanel1 = gPanel2 = null;
> +});
> +
> +function test() {

Nit: it's a good idea to add a short comment in each test file that explains the purpose of the test.

Comment 8

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

Comment on attachment 733181 [details] [diff] [review]
Add support for the Profielr running in multiple tabs

Thanks bugzilla for resetting Rob's review request.

Comment 9

[Benoit Girard (:BenWa)]

Comment on attachment 733181 [details] [diff] [review]
Add support for the Profielr running in multiple tabs

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

::: browser/devtools/profiler/ProfilerController.jsm
@@ +188,4 @@
>          }
>  
> +        sharedData.startTime = (new Date()).getTime();
> +        profile.timeStarted = getCurrentTime();

I intended to do something similar but using profiler marker. This way looks fine too.

Comment 10

[Anton Kovalyov (:anton)]

Attachment: Add support for the Profiler running in multiple tabs

Fixed nits by past.

Comment 11

[Anton Kovalyov (:anton)]

(In reply to Panos Astithas [:past] from comment #7)
> Comment on attachment 733181 [details] [diff] [review]
> Add support for the Profielr running in multiple tabs
> 
> Review of attachment 733181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Anton Kovalyov (:anton) from comment #2)
> > Oh and I also removed code that stops the profiler on
> > ProfilerController.stop. This is because checking if there are any active
> > profiles across all tabs is rather painful (I can't use WeakMap as a data
> > structure, for example) and we stop the profiler when everyone
> > disconnects[2] anyway.
> 
> Are you sure this will have no performance impact in cases where the
> profiler may be stopped, but kept around in a detached toolbox, for
> instance? I don't know if it will, but detached toolboxes tend to be rather
> long-lived in my experience.
> 

No, but the other solution is to keep a data structure that holds all profiles and their statuses from all tabs and iterate over it whenever you stop the profiler. And since we can't use WeakMap there its a leaks nightmare (I tried). Plus, there are two very explicit steps that a developer has to make: 1. Have a detached developer toolbox and 2. Start a profiler there. So it affects (if it does) only very specific users.

Comment 12

[Anton Kovalyov (:anton)]

https://hg.mozilla.org/integration/fx-team/rev/6d5c67a4e45e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d5c67a4e45e