Bug 1328004 - Fix race when closing the devtools while a previous instance is still loading. r=jryans
authorAlexandre Poirot <poirot.alex@gmail.com>
Thu, 05 Jan 2017 10:26:44 -0800
changeset 376395 7dc8c7773abf41145c9d2e9ff141fcb762996729
parent 376394 879c999acb1de04f0107e373948f364dee390192
child 376396 a8851a90b0ea8ba8f4ab868ac5217f80b9603005
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjryans
bugs1328004
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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", {});
+}