From 5ea69ec3a928f679f6b202364ca6f7bd283a1c9b Mon Sep 17 00:00:00 2001 From: Anton Kovalyov Date: Tue, 9 Apr 2013 12:00:30 -0700 Subject: [PATCH] Bug 855244 - Add support for the Profiler running in multiple tabs. r=past, r=robcee --- .../devtools/profiler/ProfilerController.jsm | 316 +++++++----------- browser/devtools/profiler/test/Makefile.in | 1 + ...owser_profiler_bug_855244_multiple_tabs.js | 103 ++++++ .../profiler/test/browser_profiler_run.js | 27 +- .../debugger/server/dbg-profiler-actors.js | 14 +- .../tests/unit/test_profiler_actor.js | 24 +- 6 files changed, 264 insertions(+), 221 deletions(-) create mode 100644 browser/devtools/profiler/test/browser_profiler_bug_855244_multiple_tabs.js diff --git a/browser/devtools/profiler/ProfilerController.jsm b/browser/devtools/profiler/ProfilerController.jsm index de01b00b0332..89dfe24ea91f 100644 --- a/browser/devtools/profiler/ProfilerController.jsm +++ b/browser/devtools/profiler/ProfilerController.jsm @@ -18,6 +18,16 @@ XPCOMUtils.defineLazyGetter(this, "DebuggerServer", function () { return DebuggerServer; }); +/** + * Data structure that contains information that has + * to be shared between separate ProfilerController + * instances. + */ +const sharedData = { + startTime: 0, + data: new WeakMap(), +}; + /** * Makes a structure representing an individual profile. */ @@ -29,163 +39,51 @@ function makeProfile(name) { }; } -/** - * Object acting as a mediator between the ProfilerController and - * DebuggerServer. - */ -function ProfilerConnection(client) { - this.client = client; - this.startTime = 0; +// Three functions below all operate with sharedData +// structure defined above. They should be self-explanatory. + +function addTarget(target) { + sharedData.data.set(target, new Map()); } -ProfilerConnection.prototype = { - actor: null, - startTime: null, +function getProfiles(target) { + return sharedData.data.get(target); +} - /** - * Returns how many milliseconds have passed since the connection - * was started (start time is specificed by the startTime property). - * - * @return number - */ - get currentTime() { - return (new Date()).getTime() - this.startTime; - }, +function getCurrentTime() { + return (new Date()).getTime() - sharedData.startTime; +} - /** - * Connects to a debugee and executes a callback when ready. - * - * @param function aCallback - * Function to be called once we're connected to the client. - */ - connect: function PCn_connect(aCallback) { - this.client.listTabs(function (aResponse) { - this.actor = aResponse.profilerActor; - aCallback(); - }.bind(this)); - }, +/** + * Object to control the JavaScript Profiler over the remote + * debugging protocol. + * + * @param Target target + * A target object as defined in Target.jsm + */ +function ProfilerController(target) { + this.target = target; + this.client = target.client; + this.isConnected = false; - /** - * Sends a message to check if the profiler is currently active. - * - * @param function aCallback - * Function to be called once we have a response from - * the client. It will be called with a single argument - * containing a response object. - */ - isActive: function PCn_isActive(aCallback) { - var message = { to: this.actor, type: "isActive" }; - this.client.request(message, aCallback); - }, + addTarget(target); - /** - * Sends a message to start a profiler. - * - * @param function aCallback - * Function to be called once the profiler is running. - * It will be called with a single argument containing - * a response object. - */ - startProfiler: function PCn_startProfiler(aCallback) { - var message = { - to: this.actor, - type: "startProfiler", - entries: 1000000, - interval: 1, - features: ["js"], - }; - - this.client.request(message, function () { - // Record the current time so we could split profiler data - // in chunks later. - this.startTime = (new Date()).getTime(); - aCallback.apply(null, Array.slice(arguments)); - }.bind(this)); - }, - - /** - * Sends a message to stop a profiler. - * - * @param function aCallback - * Function to be called once the profiler is idle. - * It will be called with a single argument containing - * a response object. - */ - stopProfiler: function PCn_stopProfiler(aCallback) { - var message = { to: this.actor, type: "stopProfiler" }; - this.client.request(message, aCallback); - }, - - /** - * Sends a message to get the generated profile data. - * - * @param function aCallback - * Function to be called once we have the data. - * It will be called with a single argument containing - * a response object. - */ - getProfileData: function PCn_getProfileData(aCallback) { - var message = { to: this.actor, type: "getProfile" }; - this.client.request(message, aCallback); - }, - - /** - * Cleanup. - */ - destroy: function PCn_destroy() { - this.client = null; + // Chrome debugging targets have already obtained a reference + // to the profiler actor. + if (target.chrome) { + this.isConnected = true; + this.actor = target.form.profilerActor; } }; -/** - * Object defining the profiler controller components. - */ -function ProfilerController(target) { - this.profiler = new ProfilerConnection(target.client); - this.profiles = new Map(); - - // Chrome debugging targets have already obtained a reference to the - // profiler actor. - this._connected = !!target.chrome; - - if (target.chrome) { - this.profiler.actor = target.form.profilerActor; - } -} - ProfilerController.prototype = { /** - * Connects to the client unless we're already connected. + * Return a map of profile results for the current target. * - * @param function aCallback - * Function to be called once we're connected. If - * the controller is already connected, this function - * will be called immediately (synchronously). + * @return Map */ - connect: function (aCallback) { - if (this._connected) { - return void aCallback(); - } - - this.profiler.connect(function onConnect() { - this._connected = true; - aCallback(); - }.bind(this)); - }, - - /** - * Checks whether the profiler is active. - * - * @param function aCallback - * Function to be called with a response from the - * client. It will be called with two arguments: - * an error object (may be null) and a boolean - * value indicating if the profiler is active or not. - */ - isActive: function PC_isActive(aCallback) { - this.profiler.isActive(function onActive(aResponse) { - aCallback(aResponse.error, aResponse.isActive); - }); + get profiles() { + return getProfiles(this.target); }, /** @@ -199,6 +97,56 @@ ProfilerController.prototype = { return profile.timeStarted !== null && profile.timeEnded === null; }, + /** + * Connects to the client unless we're already connected. + * + * @param function cb + * Function to be called once we're connected. If + * the controller is already connected, this function + * will be called immediately (synchronously). + */ + connect: function (cb) { + if (this.isConnected) { + return void cb(); + } + + this.client.listTabs((resp) => { + this.actor = resp.profilerActor; + this.isConnected = true; + cb(); + }) + }, + + /** + * Adds actor and type information to data and sends the request over + * the remote debugging protocol. + * + * @param string type + * Method to call on the other side + * @param object data + * Data to send with the request + * @param function cb + * A callback function + */ + request: function (type, data, cb) { + data.to = this.actor; + data.type = type; + this.client.request(data, cb); + }, + + /** + * Checks whether the profiler is active. + * + * @param function cb + * Function to be called with a response from the + * client. It will be called with two arguments: + * an error object (may be null) and a boolean + * value indicating if the profiler is active or not. + */ + isActive: function (cb) { + this.request("isActive", {}, (resp) => cb(resp.error, resp.isActive)); + }, + /** * Creates a new profile and starts the profiler, if needed. * @@ -214,35 +162,42 @@ ProfilerController.prototype = { return; } - let profiler = this.profiler; let profile = makeProfile(name); this.profiles.set(name, profile); - // If profile is already running, no need to do anything. if (this.isProfileRecording(profile)) { return void cb(); } - this.isActive(function (err, isActive) { + this.isActive((err, isActive) => { if (isActive) { - profile.timeStarted = profiler.currentTime; + profile.timeStarted = getCurrentTime(); return void cb(); } - profiler.startProfiler(function onStart(aResponse) { - if (aResponse.error) { - return void cb(aResponse.error); + let params = { + entries: 1000000, + interval: 1, + features: ["js"], + }; + + this.request("startProfiler", params, (resp) => { + if (resp.error) { + return void cb(resp.error); } - profile.timeStarted = profiler.currentTime; + sharedData.startTime = (new Date()).getTime(); + profile.timeStarted = getCurrentTime(); cb(); }); }); }, /** - * Stops the profiler. + * Stops the profiler. NOTE, that we don't stop the actual + * SPS Profiler here. It will be stopped as soon as all + * clients disconnect from the profiler actor. * * @param string name * Name of the profile that needs to be stopped. @@ -252,50 +207,36 @@ ProfilerController.prototype = { * argument: an error object (may be null). */ stop: function PC_stop(name, cb) { - let profiler = this.profiler; - let profile = this.profiles.get(name); - - if (!profile || !this.isProfileRecording(profile)) { + if (!this.profiles.has(name)) { return; } - let isRecording = function () { - for (let [ name, profile ] of this.profiles) { - if (this.isProfileRecording(profile)) { - return true; - } + let profile = this.profiles.get(name); + if (!this.isProfileRecording(profile)) { + return; + } + + this.request("getProfile", {}, (resp) => { + if (resp.error) { + Cu.reportError("Failed to fetch profile data."); + return void cb(resp.error, null); } - return false; - }.bind(this); + let data = resp.profile; + profile.timeEnded = getCurrentTime(); - let onStop = function (data) { - if (isRecording()) { - return void cb(null, data); - } + // Filter out all samples that fall out of current + // profile's range. - profiler.stopProfiler(function onStopProfiler(response) { - cb(response.error, data); - }); - }.bind(this); - - profiler.getProfileData(function onData(aResponse) { - if (aResponse.error) { - Cu.reportError("Failed to fetch profile data before stopping the profiler."); - return void cb(aResponse.error, null); - } - - let data = aResponse.profile; - profile.timeEnded = profiler.currentTime; - - data.threads = data.threads.map(function (thread) { - let samples = thread.samples.filter(function (sample) { + data.threads = data.threads.map((thread) => { + let samples = thread.samples.filter((sample) => { return sample.time >= profile.timeStarted; }); + return { samples: samples }; }); - onStop(data); + cb(null, data); }); }, @@ -303,7 +244,8 @@ ProfilerController.prototype = { * Cleanup. */ destroy: function PC_destroy() { - this.profiler.destroy(); - this.profiler = null; + this.client = null; + this.target = null; + this.actor = null; } }; diff --git a/browser/devtools/profiler/test/Makefile.in b/browser/devtools/profiler/test/Makefile.in index 7ecbc65e1b8b..1a81656d4ba8 100644 --- a/browser/devtools/profiler/test/Makefile.in +++ b/browser/devtools/profiler/test/Makefile.in @@ -18,6 +18,7 @@ MOCHITEST_BROWSER_TESTS = \ browser_profiler_run.js \ browser_profiler_controller.js \ browser_profiler_bug_830664_multiple_profiles.js \ + browser_profiler_bug_855244_multiple_tabs.js \ head.js \ $(NULL) diff --git a/browser/devtools/profiler/test/browser_profiler_bug_855244_multiple_tabs.js b/browser/devtools/profiler/test/browser_profiler_bug_855244_multiple_tabs.js new file mode 100644 index 000000000000..74a831d0ee27 --- /dev/null +++ b/browser/devtools/profiler/test/browser_profiler_bug_855244_multiple_tabs.js @@ -0,0 +1,103 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +const URL = "data:text/html;charset=utf8,

JavaScript Profiler test

"; + +let gTab1, gPanel1; +let gTab2, gPanel2; + +// Tests that you can run the profiler in multiple tabs at the same +// time and that closing the debugger panel in one tab doesn't lock +// profilers in other tabs. + +registerCleanupFunction(function () { + gTab1 = gTab2 = gPanel1 = gPanel2 = null; +}); + +function test() { + waitForExplicitFinish(); + + openTwoTabs() + .then(startTwoProfiles) + .then(stopFirstProfile) + .then(stopSecondProfile) + .then(closeTabs) + .then(openTwoTabs) + .then(startTwoProfiles) + .then(closeFirstPanel) + .then(stopSecondProfile) + .then(closeTabs) + .then(finish); +} + +function openTwoTabs() { + let deferred = Promise.defer(); + + setUp(URL, (tab, browser, panel) => { + gTab1 = tab; + gPanel1 = panel; + + loadTab(URL, (tab, browser) => { + gTab2 = tab; + openProfiler(tab, () => { + let target = TargetFactory.forTab(tab); + gPanel2 = gDevTools.getToolbox(target).getPanel("jsprofiler"); + deferred.resolve(); + }); + }); + }); + + return deferred.promise; +} + +function startTwoProfiles() { + let deferred = Promise.defer(); + gPanel1.controller.start("Profile 1", (err) => { + ok(!err, "Profile in tab 1 started without errors"); + gPanel2.controller.start("Profile 1", (err) => { + ok(!err, "Profile in tab 2 started without errors"); + gPanel1.controller.isActive((err, isActive) => { + ok(isActive, "Profiler is active"); + deferred.resolve(); + }); + }); + }); + + return deferred.promise; +} + +function stopFirstProfile() { + let deferred = Promise.defer(); + + gPanel1.controller.stop("Profile 1", (err, data) => { + ok(!err, "Profile in tab 1 stopped without errors"); + ok(data, "Profile in tab 1 returned some data"); + deferred.resolve(); + }); + + return deferred.promise; +} + +function stopSecondProfile() { + let deferred = Promise.defer(); + + gPanel2.controller.stop("Profile 1", (err, data) => { + ok(!err, "Profile in tab 2 stopped without errors"); + ok(data, "Profile in tab 2 returned some data"); + deferred.resolve(); + }); + + return deferred.promise; +} + +function closeTabs() { + while (gBrowser.tabs.length > 1) { + gBrowser.removeCurrentTab(); + } +} + +function closeFirstPanel() { + let target = TargetFactory.forTab(gTab1); + let toolbox = gDevTools.getToolbox(target); + return toolbox.destroy; +} diff --git a/browser/devtools/profiler/test/browser_profiler_run.js b/browser/devtools/profiler/test/browser_profiler_run.js index 87aa2d672c0f..80e162ffbdaf 100644 --- a/browser/devtools/profiler/test/browser_profiler_run.js +++ b/browser/devtools/profiler/test/browser_profiler_run.js @@ -13,26 +13,12 @@ function test() { gPanel = panel; panel.once("started", onStart); - panel.once("stopped", onStop); panel.once("parsed", onParsed); testUI(); }); } -function attemptTearDown() { - gAttempts += 1; - - if (gAttempts < 2) { - return; - } - - tearDown(gTab, function onTearDown() { - gPanel = null; - gTab = null; - }); -} - function testUI() { ok(gPanel, "Profiler panel exists"); ok(gPanel.activeProfile, "Active profile exists"); @@ -58,13 +44,6 @@ function onStart() { }); } -function onStop() { - gPanel.controller.isActive(function (err, isActive) { - ok(!isActive, "Profiler is idle"); - attemptTearDown(); - }); -} - function onParsed() { function assertSample() { let [win,doc] = getProfileInternals(); @@ -76,7 +55,11 @@ function onParsed() { ok(sample.length > 0, "We have some items displayed"); is(sample[0].innerHTML, "100.0%", "First percentage is 100%"); - attemptTearDown(); + + tearDown(gTab, function onTearDown() { + gPanel = null; + gTab = null; + }); } assertSample(); diff --git a/toolkit/devtools/debugger/server/dbg-profiler-actors.js b/toolkit/devtools/debugger/server/dbg-profiler-actors.js index bf52ed2a0fb0..f61cc30c3d2a 100644 --- a/toolkit/devtools/debugger/server/dbg-profiler-actors.js +++ b/toolkit/devtools/debugger/server/dbg-profiler-actors.js @@ -4,6 +4,8 @@ "use strict"; +var connCount = 0; + /** * Creates a ProfilerActor. ProfilerActor provides remote access to the * built-in profiler module. @@ -13,6 +15,7 @@ function ProfilerActor(aConnection) this._profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler); this._started = false; this._observedEvents = []; + connCount += 1; } ProfilerActor.prototype = { @@ -22,9 +25,18 @@ ProfilerActor.prototype = { for (var event of this._observedEvents) { Services.obs.removeObserver(this, event); } - if (this._profiler && this._started) { + + // 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. + + connCount -= 1; + if (connCount <= 0 && this._profiler && this._started) { this._profiler.StopProfiler(); } + this._profiler = null; }, diff --git a/toolkit/devtools/debugger/tests/unit/test_profiler_actor.js b/toolkit/devtools/debugger/tests/unit/test_profiler_actor.js index ec67a0c76ae0..ef38552ccbe4 100644 --- a/toolkit/devtools/debugger/tests/unit/test_profiler_actor.js +++ b/toolkit/devtools/debugger/tests/unit/test_profiler_actor.js @@ -147,21 +147,23 @@ function test_profile(aClient, aProfiler) function test_profiler_status() { var connectionClosed = DebuggerServer._connectionClosed; - DebuggerServer._connectionClosed = function (conn) { - connectionClosed.call(this, conn); - // Check that closing the connection stops the profiler - do_check_false(Profiler.IsActive()); - do_test_finished(); - }; - var client = new DebuggerClient(DebuggerServer.connectPipe()); - client.connect(function () { - client.listTabs(function(aResponse) { + + client.connect(() => { + client.listTabs((aResponse) => { + DebuggerServer._connectionClosed = function (conn) { + connectionClosed.call(this, conn); + + // Check that closing the last (only?) connection stops the profiler. + do_check_false(Profiler.IsActive()); + do_test_finished(); + } + var profiler = aResponse.profilerActor; do_check_false(Profiler.IsActive()); - client.request({ to: profiler, type: "startProfiler", features: [] }, function (aResponse) { + client.request({ to: profiler, type: "startProfiler", features: [] }, (aResponse) => { do_check_true(Profiler.IsActive()); - client.close(function() { }); + client.close(function () {}); }); }); });