Bug 1311737 - Prevent toolbox.selectTool from racing when calling multiple times. r=pbro draft
authorAlexandre Poirot <poirot.alex@gmail.com>
Tue, 25 Oct 2016 06:57:28 -0700
changeset 430299 b569763fb84f7b5bbbcdb9b9c506b6f6e47500d6
parent 430042 3f4c3a3cabaf94958834d3a8935adfb4a887942d
child 535181 8253a16a39beb7c63ae100236285b197cbd73c01
push id33799
push userbmo:poirot.alex@gmail.com
push dateThu, 27 Oct 2016 14:00:21 +0000
reviewerspbro
bugs1311737
milestone52.0a1
Bug 1311737 - Prevent toolbox.selectTool from racing when calling multiple times. r=pbro MozReview-Commit-ID: lZZGviAFl3
devtools/client/framework/test/browser_toolbox_select_event.js
devtools/client/framework/toolbox.js
--- a/devtools/client/framework/test/browser_toolbox_select_event.js
+++ b/devtools/client/framework/test/browser_toolbox_select_event.js
@@ -38,39 +38,64 @@ add_task(function* () {
   yield testSelectEvent("inspector");
   yield testSelectEvent("webconsole");
   yield testSelectEvent("styleeditor");
   yield testSelectEvent("inspector");
   yield testSelectEvent("webconsole");
   yield testSelectEvent("styleeditor");
   yield toolbox.destroy();
 
+  yield testSelectToolRace();
+
   /**
    * Assert that selecting the given toolId raises a select event
    * @param {toolId} Id of the tool to test
    */
-  function testSelectEvent(toolId) {
-    return new Promise(resolve => {
-      toolbox.once("select", (event, id) => {
-        is(id, toolId, toolId + " selected");
-        resolve();
-      });
-      toolbox.selectTool(toolId);
-    });
+  function* testSelectEvent(toolId) {
+    let onSelect = toolbox.once("select");
+    toolbox.selectTool(toolId);
+    let id = yield onSelect;
+    is(id, toolId, toolId + " selected");
   }
 
   /**
    * Assert that selecting the given toolId raises its corresponding
    * selected event
    * @param {toolId} Id of the tool to test
    */
-  function testToolSelectEvent(toolId) {
-    return new Promise(resolve => {
-      toolbox.once(toolId + "-selected", () => {
-        let msg = toolId + " tool selected";
-        is(toolbox.currentToolId, toolId, msg);
-        resolve();
-      });
-      toolbox.selectTool(toolId);
-    });
+  function* testToolSelectEvent(toolId) {
+    let onSelected = toolbox.once(toolId + "-selected");
+    toolbox.selectTool(toolId);
+    yield onSelected;
+    is(toolbox.currentToolId, toolId, toolId + " tool selected");
+  }
+
+  /**
+   * Assert that two calls to selectTool won't race
+   */
+  function* testSelectToolRace() {
+    let toolbox = yield openToolboxForTab(tab, "webconsole");
+    let selected = false;
+    let onSelect = (event, id) => {
+      if (selected) {
+        ok(false, "Got more than one 'select' event");
+      } else {
+        selected = true;
+      }
+    };
+    toolbox.once("select", onSelect);
+    let p1 = toolbox.selectTool("inspector")
+    let p2 = toolbox.selectTool("inspector");
+    // Check that both promises don't resolve too early
+    let checkSelectToolResolution = panel => {
+      ok(selected, "selectTool resolves only after 'select' event is fired");
+      let inspector = toolbox.getPanel("inspector");
+      is(panel, inspector, "selecTool resolves to the panel instance");
+    };
+    p1.then(checkSelectToolResolution);
+    p2.then(checkSelectToolResolution);
+    yield p1;
+    yield p2;
+
+    yield toolbox.destroy();
   }
 });
 
--- a/devtools/client/framework/toolbox.js
+++ b/devtools/client/framework/toolbox.js
@@ -1446,21 +1446,29 @@ Toolbox.prototype = {
     let sep = this.doc.getElementById("toolbox-controls-separator");
     if (id === "options") {
       sep.setAttribute("invisible", "true");
     } else {
       sep.removeAttribute("invisible");
     }
 
     if (this.currentToolId == id) {
-      // re-focus tool to get key events again
-      this.focusTool(id);
+      let panel = this._toolPanels.get(id);
+      if (panel) {
+        // We have a panel instance, so the tool is already fully loaded.
+
+        // re-focus tool to get key events again
+        this.focusTool(id);
 
-      // Return the existing panel in order to have a consistent return value.
-      return promise.resolve(this._toolPanels.get(id));
+        // Return the existing panel in order to have a consistent return value.
+        return promise.resolve(panel);
+      }
+      // Otherwise, if there is no panel instance, it is still loading,
+      // so we are racing another call to selectTool with the same id.
+      return this.once("select").then(() => promise.resolve(this._toolPanels.get(id)));
     }
 
     if (!this.isReady) {
       throw new Error("Can't select tool, wait for toolbox 'ready' event");
     }
 
     let tab = this.doc.getElementById("toolbox-tab-" + id);