Bug 855244 - Add support for the Profiler running in multiple tabs. r=past, r=robcee

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;
   }
 };

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)
 

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,<p>JavaScript Profiler test</p>";
+
+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;
+}

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();

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;
   },
 

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 () {});
       });
     });
   });