Bug 1328004 - Fix race when closing the devtools while a previous instance is still loading. r=jryans a=jcristau
authorAlexandre Poirot <poirot.alex@gmail.com>
Thu, 05 Jan 2017 10:26:44 -0800
changeset 369126 9a1b79873db7526a0c9f33511e224a617af7ca6b
parent 369125 462a50df3d934aaff8483f9aa0f6a231d91f6cae
child 369127 a9ed733f547b0438f8ca6ef4bc7be8dd8fdc520c
push id1369
push userjlorenzo@mozilla.com
push dateMon, 27 Feb 2017 14:59:41 +0000
treeherdermozilla-release@d75a1dba431f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjryans, jcristau
bugs1328004
milestone52.0
Bug 1328004 - Fix race when closing the devtools while a previous instance is still loading. r=jryans a=jcristau 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) {
@@ -178,17 +178,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
@@ -470,23 +470,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;
+  }),
 
   /**
    * Called to tear down a tools provider.
    */
   _teardown: function DT_teardown() {
     for (let [target, toolbox] of this._toolboxes) {
       toolbox.destroy();
     }
--- 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", {});
+}