From 782e1c95ba1a7cf68df0902b69d3086f52ae597d Mon Sep 17 00:00:00 2001 From: Alexandre Poirot Date: Thu, 5 Jan 2017 10:26:44 -0800 Subject: [PATCH] Bug 1328004 - Fix race when closing the devtools while a previous instance is still loading. r=jryans MozReview-Commit-ID: 2a58l4DjCtv --HG-- extra : rebase_source : b7eb2f249ff9710f422e699b07d4d56779358aec --- devtools/client/framework/devtools-browser.js | 4 +- devtools/client/framework/devtools.js | 16 ++-- devtools/client/framework/test/browser.ini | 1 + .../framework/test/browser_toolbox_races.js | 81 +++++++++++++++++++ 4 files changed, 94 insertions(+), 8 deletions(-) create mode 100644 devtools/client/framework/test/browser_toolbox_races.js diff --git a/devtools/client/framework/devtools-browser.js b/devtools/client/framework/devtools-browser.js index 0fdc6f33c125..4ba94a7fefdc 100644 --- a/devtools/client/framework/devtools-browser.js +++ b/devtools/client/framework/devtools-browser.js @@ -72,7 +72,7 @@ var gDevToolsBrowser = exports.gDevToolsBrowser = { // - 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); }, /** @@ -193,7 +193,7 @@ var gDevToolsBrowser = exports.gDevToolsBrowser = { if (toolDefinition.preventClosingOnKey || toolbox.hostType == Toolbox.HostType.WINDOW) { toolbox.raise(); } else { - toolbox.destroy(); + gDevTools.closeToolbox(target); } gDevTools.emit("select-tool-command", toolId); } else { diff --git a/devtools/client/framework/devtools.js b/devtools/client/framework/devtools.js index 3ec83cbdee87..c5985a172426 100644 --- a/devtools/client/framework/devtools.js +++ b/devtools/client/framework/devtools.js @@ -465,13 +465,17 @@ DevTools.prototype = { * 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 diff --git a/devtools/client/framework/test/browser.ini b/devtools/client/framework/test/browser.ini index 3321231a29ee..9a1b6c33d73c 100644 --- a/devtools/client/framework/test/browser.ini +++ b/devtools/client/framework/test/browser.ini @@ -60,6 +60,7 @@ skip-if = true # Bug 1177463 - Temporarily hide the minimize button [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 diff --git a/devtools/client/framework/test/browser_toolbox_races.js b/devtools/client/framework/test/browser_toolbox_races.js new file mode 100644 index 000000000000..fedbc4402c97 --- /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", {}); +}