Closed Bug 855244 Opened 7 years ago Closed 7 years ago

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

Categories

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

21 Branch
defect
Not set

Tracking

(firefox21 affected, firefox22 affected, firefox23 fixed)

RESOLVED FIXED
Firefox 23
Tracking Status
firefox21 --- affected
firefox22 --- affected
firefox23 --- fixed

People

(Reporter: mihaelav, Assigned: anton)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → anton
Status: NEW → ASSIGNED
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.
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.
Attachment #733181 - Flags: review?(rcampbell)
Attachment #733181 - Flags: review?(past)
Attachment #733181 - Flags: feedback?(bgirard)
Try push seems to be green.
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.
Attachment #733181 - Flags: review?(rcampbell) → review+
passes try.
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.
Attachment #733181 - Flags: review?(rcampbell)
Attachment #733181 - Flags: review?(past)
Attachment #733181 - Flags: review+
Comment on attachment 733181 [details] [diff] [review]
Add support for the Profielr running in multiple tabs

Thanks bugzilla for resetting Rob's review request.
Attachment #733181 - Flags: review?(rcampbell) → review+
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.
Attachment #733181 - Flags: feedback?(bgirard) → feedback+
Fixed nits by past.
Attachment #733181 - Attachment is obsolete: true
Attachment #735332 - Flags: review+
(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.
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6d5c67a4e45e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Depends on: 864681
Depends on: 864687
Depends on: 866472
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.