Bug 1328004 - Fix race when closing the devtools while a previous instance is still loading. r=jryans draft
authorAlexandre Poirot <poirot.alex@gmail.com>
Thu, 05 Jan 2017 10:26:44 -0800
changeset 459646 e6f729f4e258db0d54c7b8086bc4d9dacd439376
parent 457899 8f3b24109e3412b36f97277e31ad66856dc609d6
child 541949 d96947d503eecc9b67ef23c7a485470537c08e2b
push id41280
push userbmo:poirot.alex@gmail.com
push dateThu, 12 Jan 2017 10:42:33 +0000
reviewersjryans
bugs1328004
milestone53.0a1
Bug 1328004 - Fix race when closing the devtools while a previous instance is still loading. r=jryans MozReview-Commit-ID: 2a58l4DjCtv
devtools/client/framework/devtools-browser.js
devtools/client/framework/devtools.js
devtools/client/framework/test/browser.ini
devtools/client/framework/test/browser_toolbox_races.js
--- a/devtools/client/framework/devtools-browser.js
+++ b/devtools/client/framework/devtools-browser.js
@@ -67,17 +67,17 @@ var gDevToolsBrowser = exports.gDevTools
   toggleToolboxCommand: function (gBrowser) {
     let target = TargetFactory.forTab(gBrowser.selectedTab);
     let toolbox = gDevTools.getToolbox(target);
 
     // If a toolbox exists, using toggle from the Main window :
     // - should close a docked toolbox
     // - should focus a windowed toolbox
     let isDocked = toolbox && toolbox.hostType != Toolbox.HostType.WINDOW;
-    isDocked ? toolbox.destroy() : gDevTools.showToolbox(target);
+    isDocked ? gDevTools.closeToolbox(target) : gDevTools.showToolbox(target);
   },
 
   /**
    * This function ensures the right commands are enabled in a window,
    * depending on their relevant prefs. It gets run when a window is registered,
    * or when any of the devtools prefs change.
    */
   updateCommandAvailability: function (win) {
@@ -188,17 +188,17 @@ var gDevToolsBrowser = exports.gDevTools
         (toolbox.currentToolId == toolId ||
           (toolId == "webconsole" && toolbox.splitConsole)))
     {
       toolbox.fireCustomKey(toolId);
 
       if (toolDefinition.preventClosingOnKey || toolbox.hostType == Toolbox.HostType.WINDOW) {
         toolbox.raise();
       } else {
-        toolbox.destroy();
+        gDevTools.closeToolbox(target);
       }
       gDevTools.emit("select-tool-command", toolId);
     } else {
       gDevTools.showToolbox(target, toolId).then(() => {
         let target = TargetFactory.forTab(gBrowser.selectedTab);
         let toolbox = gDevTools.getToolbox(target);
 
         toolbox.fireCustomKey(toolId);
--- a/devtools/client/framework/devtools.js
+++ b/devtools/client/framework/devtools.js
@@ -460,23 +460,27 @@ DevTools.prototype = {
   /**
    * Close the toolbox for a given target
    *
    * @return promise
    *         This promise will resolve to false if no toolbox was found
    *         associated to the target. true, if the toolbox was successfully
    *         closed.
    */
-  closeToolbox: function DT_closeToolbox(target) {
-    let toolbox = this._toolboxes.get(target);
-    if (toolbox == null) {
-      return promise.resolve(false);
+  closeToolbox: Task.async(function* (target) {
+    let toolbox = yield this._creatingToolboxes.get(target);
+    if (!toolbox) {
+      toolbox = this._toolboxes.get(target);
     }
-    return toolbox.destroy().then(() => true);
-  },
+    if (!toolbox) {
+      return false;
+    }
+    yield toolbox.destroy();
+    return true;
+  }),
 
   /**
    * Either the SDK Loader has been destroyed by the add-on contribution
    * workflow, or firefox is shutting down.
 
    * @param {boolean} shuttingDown
    *        True if firefox is currently shutting down. We may prevent doing
    *        some cleanups to speed it up. Otherwise everything need to be
--- a/devtools/client/framework/test/browser.ini
+++ b/devtools/client/framework/test/browser.ini
@@ -55,16 +55,17 @@ skip-if = true # Bug 1177463 - Temporari
 [browser_toolbox_options.js]
 [browser_toolbox_options_disable_buttons.js]
 [browser_toolbox_options_disable_cache-01.js]
 [browser_toolbox_options_disable_cache-02.js]
 [browser_toolbox_options_disable_js.js]
 [browser_toolbox_options_enable_serviceworkers_testing.js]
 # [browser_toolbox_raise.js] # Bug 962258
 # skip-if = os == "win"
+[browser_toolbox_races.js]
 [browser_toolbox_ready.js]
 [browser_toolbox_remoteness_change.js]
 run-if = e10s
 [browser_toolbox_select_event.js]
 skip-if = e10s # Bug 1069044 - destroyInspector may hang during shutdown
 [browser_toolbox_selected_tool_unavailable.js]
 [browser_toolbox_sidebar.js]
 [browser_toolbox_sidebar_events.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/framework/test/browser_toolbox_races.js
@@ -0,0 +1,81 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test toggling the toolbox quickly and see if there is any race breaking it.
+
+const URL = "data:text/html;charset=utf-8,Toggling devtools quickly";
+
+add_task(function* () {
+  // Make sure this test starts with the selectedTool pref cleared. Previous
+  // tests select various tools, and that sets this pref.
+  Services.prefs.clearUserPref("devtools.toolbox.selectedTool");
+
+  let tab = yield addTab(URL);
+
+  let created = 0, ready = 0, destroy = 0, destroyed = 0;
+  let onCreated = () => {
+    created++;
+  };
+  let onReady = () => {
+    ready++;
+  };
+  let onDestroy = () => {
+    destroy++;
+  };
+  let onDestroyed = () => {
+    destroyed++;
+  };
+  gDevTools.on("toolbox-created", onCreated);
+  gDevTools.on("toolbox-ready", onReady);
+  gDevTools.on("toolbox-destroy", onDestroy);
+  gDevTools.on("toolbox-destroyed", onDestroyed);
+
+  // The current implementation won't toggle the toolbox many times,
+  // instead it will ignore toggles that happens while the toolbox is still
+  // creating or still destroying.
+
+  // Toggle the toolbox at least 3 times.
+  info("Trying to toggle the toolbox 3 times");
+  while (created < 3) {
+    // Sent multiple event to try to race the code during toolbox creation and destruction
+    toggle();
+    toggle();
+    toggle();
+
+    // Release the event loop to let a chance to actually create or destroy the toolbox!
+    yield wait(50);
+  }
+  info("Toggled the toolbox 3 times");
+
+  // Now wait for the 3rd toolbox to be fully ready before closing it.
+  // We close the last toolbox manually, out of the first while() loop to
+  // avoid races and be sure we end up we no toolbox and waited for all the
+  // requests to be done.
+  while (ready != 3) {
+    yield wait(100);
+  }
+  toggle();
+  while (destroyed != 3) {
+    yield wait(100);
+  }
+
+  is(created, 3, "right number of created events");
+  is(ready, 3, "right number of ready events");
+  is(destroy, 3, "right number of destroy events");
+  is(destroyed, 3, "right number of destroyed events");
+
+  gDevTools.off("toolbox-created", onCreated);
+  gDevTools.off("toolbox-ready", onReady);
+  gDevTools.off("toolbox-destroy", onDestroy);
+  gDevTools.off("toolbox-destroyed", onDestroyed);
+
+  gBrowser.removeCurrentTab();
+});
+
+function toggle() {
+  EventUtils.synthesizeKey("VK_F12", {});
+}