From 5c84bd2eb8993ca48a0d98142822abb9ab8b2a24 Mon Sep 17 00:00:00 2001 From: Joe Walker Date: Fri, 4 Jan 2013 20:31:38 +0000 Subject: [PATCH] Bug 795988 - Closing browser window with Developer Toolbar open leaks an everything; r=paul,ttaubert --- browser/base/content/browser.js | 5 + browser/devtools/commandline/gcli.jsm | 95 ++++++++++++------- .../commandline/test/browser_gcli_exec.js | 19 ++-- browser/devtools/framework/Toolbox.jsm | 13 ++- browser/devtools/shared/DeveloperToolbar.jsm | 7 ++ 5 files changed, 90 insertions(+), 49 deletions(-) diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js index 71be56bda704..cdfd307b230a 100644 --- a/browser/base/content/browser.js +++ b/browser/base/content/browser.js @@ -1516,6 +1516,11 @@ var gBrowserInit = { gDevToolsBrowser.forgetBrowserWindow(window); + let desc = Object.getOwnPropertyDescriptor(window, "DeveloperToolbar"); + if (desc && !desc.get) { + DeveloperToolbar.destroy(); + } + // First clean up services initialized in gBrowserInit.onLoad (or those whose // uninit methods don't depend on the services having been initialized). allTabs.uninit(); diff --git a/browser/devtools/commandline/gcli.jsm b/browser/devtools/commandline/gcli.jsm index 320b1ae4adcd..54719b53e736 100644 --- a/browser/devtools/commandline/gcli.jsm +++ b/browser/devtools/commandline/gcli.jsm @@ -2697,11 +2697,10 @@ canon.onCanonChange = util.createEvent('canon.onCanonChange'); * CommandOutputManager stores the output objects generated by executed * commands. * - * CommandOutputManager is exposed (via canon.commandOutputManager) to the the - * outside world and could (but shouldn't) be used before gcli.startup() has - * been called. This could should be defensive to that where possible, and we - * should certainly document if the use of it or similar will fail if used too - * soon. + * CommandOutputManager is exposed to the the outside world and could (but + * shouldn't) be used before gcli.startup() has been called. + * This could should be defensive to that where possible, and we should + * certainly document if the use of it or similar will fail if used too soon. */ function CommandOutputManager() { this.onOutput = util.createEvent('CommandOutputManager.onOutput'); @@ -2709,12 +2708,6 @@ function CommandOutputManager() { canon.CommandOutputManager = CommandOutputManager; -/** - * We maintain a global command output manager for the majority case where - * there is only one important set of outputs. - */ -canon.commandOutputManager = new CommandOutputManager(); - }); /* @@ -2744,6 +2737,18 @@ define('gcli/util', ['require', 'exports', 'module' ], function(require, exports var eventDebug = false; +/** + * Patch up broken console API from node + */ +if (eventDebug) { + if (console.group == null) { + console.group = function() { console.log(arguments); }; + } + if (console.groupEnd == null) { + console.groupEnd = function() { console.log(arguments); }; + } +} + /** * Useful way to create a name for a handler, used in createEvent() */ @@ -2825,6 +2830,10 @@ exports.createEvent = function(name) { * @param scope Optional 'this' object for the function call */ event.add = function(func, scope) { + if (eventDebug) { + console.log('Adding listener to ' + name); + } + handlers.push({ func: func, scope: scope }); }; @@ -2835,16 +2844,20 @@ exports.createEvent = function(name) { * @param scope Optional 'this' object for the function call */ event.remove = function(func, scope) { + if (eventDebug) { + console.log('Removing listener from ' + name); + } + var found = false; handlers = handlers.filter(function(test) { - var noMatch = (test.func !== func && test.scope !== scope); - if (!noMatch) { + var match = (test.func === func && test.scope === scope); + if (match) { found = true; } - return noMatch; + return !match; }); if (!found) { - console.warn('Failed to remove handler from ' + name); + console.warn('Handler not found. Attached to ' + name); } }; @@ -4065,6 +4078,7 @@ exports.setDocument = function(document) { */ exports.unsetDocument = function() { doc = undefined; + exports._empty = undefined; }; /** @@ -5172,6 +5186,7 @@ var l10n = require('gcli/l10n'); var canon = require('gcli/canon'); var Q = require('gcli/promise'); +var CommandOutputManager = require('gcli/canon').CommandOutputManager; var Status = require('gcli/types').Status; var Conversion = require('gcli/types').Conversion; @@ -5530,9 +5545,11 @@ UnassignedAssignment.prototype.getStatus = function(arg) { * @param doc A DOM Document passed to commands using the Execution Context in * order to allow creation of DOM nodes. If missing Requisition will use the * global 'document'. + * @param commandOutputManager A custom commandOutputManager to which output + * should be sent (optional) * @constructor */ -function Requisition(environment, doc) { +function Requisition(environment, doc, commandOutputManager) { this.environment = environment; this.document = doc; if (this.document == null) { @@ -5543,6 +5560,7 @@ function Requisition(environment, doc) { // Ignore } } + this.commandOutputManager = commandOutputManager || new CommandOutputManager(); // The command that we are about to execute. // @see setCommandConversion() @@ -5573,8 +5591,6 @@ function Requisition(environment, doc) { this.commandAssignment.onAssignmentChange.add(this._commandAssignmentChanged, this); this.commandAssignment.onAssignmentChange.add(this._assignmentChanged, this); - this.commandOutputManager = canon.commandOutputManager; - this.onAssignmentChange = util.createEvent('Requisition.onAssignmentChange'); this.onTextChange = util.createEvent('Requisition.onTextChange'); } @@ -7055,21 +7071,21 @@ exports.shutdown = function() { * @param options Object containing user customization properties, including: * - blurDelay (default=150ms) * - debug (default=false) - * - commandOutputManager (default=canon.commandOutputManager) * @param components Object that links to other UI components. GCLI provided: * - document + * - requisition */ function FocusManager(options, components) { options = options || {}; this._document = components.document || document; + this._requisition = components.requisition; + this._debug = options.debug || false; this._blurDelay = options.blurDelay || 150; this._window = this._document.defaultView; - this._commandOutputManager = options.commandOutputManager || - canon.commandOutputManager; - this._commandOutputManager.onOutput.add(this._outputted, this); + this._requisition.commandOutputManager.onOutput.add(this._outputted, this); this._blurDelayTimeout = null; // Result of setTimeout in delaying a blur this._monitoredElements = []; // See addMonitoredElement() @@ -7098,7 +7114,7 @@ FocusManager.prototype.destroy = function() { eagerHelper.onChange.remove(this._eagerHelperChanged, this); this._document.removeEventListener('focus', this._focused, true); - this._commandOutputManager.onOutput.remove(this._outputted, this); + this._requisition.commandOutputManager.onOutput.remove(this._outputted, this); for (var i = 0; i < this._monitoredElements.length; i++) { var monitor = this._monitoredElements[i]; @@ -7116,7 +7132,7 @@ FocusManager.prototype.destroy = function() { delete this._focused; delete this._document; delete this._window; - delete this._commandOutputManager; + delete this._requisition; }; /** @@ -9075,7 +9091,7 @@ var resource = require('gcli/types/resource'); var host = require('gcli/host'); var intro = require('gcli/ui/intro'); -var commandOutputManager = require('gcli/canon').commandOutputManager; +var CommandOutputManager = require('gcli/canon').CommandOutputManager; /** * Handy utility to inject the content document (i.e. for the viewed page, @@ -9111,6 +9127,7 @@ function setContentDocument(document) { * - environment * - scratchpad (optional) * - chromeWindow + * - commandOutputManager (optional) */ function FFDisplay(options) { if (options.eval) { @@ -9119,13 +9136,19 @@ function FFDisplay(options) { setContentDocument(options.contentDocument); host.chromeWindow = options.chromeWindow; - this.onOutput = commandOutputManager.onOutput; - this.requisition = new Requisition(options.environment, options.outputDocument); + this.commandOutputManager = options.commandOutputManager; + if (this.commandOutputManager == null) { + this.commandOutputManager = new CommandOutputManager(); + } + + this.onOutput = this.commandOutputManager.onOutput; + this.requisition = new Requisition(options.environment, + options.outputDocument, + this.commandOutputManager); - // Create a FocusManager for the various parts to register with this.focusManager = new FocusManager(options, { - // TODO: can we kill chromeDocument here? - document: options.chromeDocument + document: options.chromeDocument, + requisition: this.requisition, }); this.onVisibilityChange = this.focusManager.onVisibilityChange; @@ -9169,7 +9192,7 @@ function FFDisplay(options) { * separate method */ FFDisplay.prototype.maybeShowIntro = function() { - intro.maybeShowIntro(commandOutputManager); + intro.maybeShowIntro(this.commandOutputManager); }; /** @@ -9214,7 +9237,7 @@ FFDisplay.prototype.destroy = function() { // DOM node hunter script from looking in all the nooks and crannies, so it's // better if we can be leak-free without deleting them: // - consoleWrap, resizer, tooltip, completer, inputter, - // - focusManager, onVisibilityChange, requisition + // - focusManager, onVisibilityChange, requisition, commandOutputManager }; /** @@ -10008,7 +10031,8 @@ function Completer(options, components) { this.inputter.onAssignmentChange.add(this.update, this); this.inputter.onChoiceChange.add(this.update, this); - if (components.autoResize) { + this.autoResize = components.autoResize; + if (this.autoResize) { this.inputter.onResize.add(this.resized, this); var dimensions = this.inputter.getDimensions(); @@ -10031,7 +10055,10 @@ Completer.prototype.destroy = function() { this.inputter.onInputChange.remove(this.update, this); this.inputter.onAssignmentChange.remove(this.update, this); this.inputter.onChoiceChange.remove(this.update, this); - this.inputter.onResize.remove(this.resized, this); + + if (this.autoResize) { + this.inputter.onResize.remove(this.resized, this); + } delete this.document; delete this.element; diff --git a/browser/devtools/commandline/test/browser_gcli_exec.js b/browser/devtools/commandline/test/browser_gcli_exec.js index 01d35e5c875c..f6a2350de2aa 100644 --- a/browser/devtools/commandline/test/browser_gcli_exec.js +++ b/browser/devtools/commandline/test/browser_gcli_exec.js @@ -42,7 +42,7 @@ function test() { var Requisition = require('gcli/cli').Requisition; -var canon = require('gcli/canon'); +var CommandOutputManager = require('gcli/canon').CommandOutputManager; // var mockCommands = require('gclitest/mockCommands'); var nodetype = require('gcli/types/node'); @@ -53,16 +53,22 @@ var actualOutput; var hideExec = false; var skip = 'skip'; -exports.setup = function() { +var environment = { value: 'example environment data' }; +var commandOutputManager = new CommandOutputManager(); +var requisition = new Requisition(environment, null, commandOutputManager); + +exports.setup = function(options) { mockCommands.setup(); mockCommands.onCommandExec.add(commandExeced); - canon.commandOutputManager.onOutput.add(commandOutputed); + + commandOutputManager.onOutput.add(commandOutputed); }; -exports.shutdown = function() { +exports.shutdown = function(options) { mockCommands.shutdown(); mockCommands.onCommandExec.remove(commandExeced); - canon.commandOutputManager.onOutput.remove(commandOutputed); + + commandOutputManager.onOutput.remove(commandOutputed); }; function commandExeced(ev) { @@ -74,9 +80,6 @@ function commandOutputed(ev) { } function exec(command, expectedArgs) { - var environment = {}; - - var requisition = new Requisition(environment); var outputObject = requisition.exec({ typed: command, hidden: hideExec }); assert.is(command.indexOf(actualExec.command.name), 0, 'Command name: ' + command); diff --git a/browser/devtools/framework/Toolbox.jsm b/browser/devtools/framework/Toolbox.jsm index 21e216a55e17..3e1fd33a1f38 100644 --- a/browser/devtools/framework/Toolbox.jsm +++ b/browser/devtools/framework/Toolbox.jsm @@ -16,6 +16,7 @@ XPCOMUtils.defineLazyModuleGetter(this, "Hosts", "resource:///modules/devtools/ToolboxHosts.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "CommandUtils", "resource:///modules/devtools/DeveloperToolbar.jsm"); + XPCOMUtils.defineLazyGetter(this, "toolboxStrings", function() { let bundle = Services.strings.createBundle("chrome://browser/locale/devtools/toolbox.properties"); let l10n = function(name) { @@ -28,13 +29,12 @@ XPCOMUtils.defineLazyGetter(this, "toolboxStrings", function() { return l10n; }); -// DO NOT put Require.jsm or gcli.jsm into lazy getters as this breaks the -// requisition import a few lines down. -Cu.import("resource:///modules/devtools/gcli.jsm"); -Cu.import("resource://gre/modules/devtools/Require.jsm"); +XPCOMUtils.defineLazyGetter(this, "Requisition", function() { + Cu.import("resource://gre/modules/devtools/Require.jsm"); + Cu.import("resource:///modules/devtools/gcli.jsm"); -let Requisition = require('gcli/cli').Requisition; -let CommandOutputManager = require('gcli/canon').CommandOutputManager; + return require('gcli/cli').Requisition; +}); this.EXPORTED_SYMBOLS = [ "Toolbox" ]; @@ -334,7 +334,6 @@ Toolbox.prototype = { let toolbarSpec = CommandUtils.getCommandbarSpec("devtools.toolbox.toolbarSpec"); let environment = { chromeDocument: frame.ownerDocument }; let requisition = new Requisition(environment); - requisition.commandOutputManager = new CommandOutputManager(); let buttons = CommandUtils.createButtons(toolbarSpec, this._target, this.doc, requisition); diff --git a/browser/devtools/shared/DeveloperToolbar.jsm b/browser/devtools/shared/DeveloperToolbar.jsm index 311160a0608f..d76d782c4d96 100644 --- a/browser/devtools/shared/DeveloperToolbar.jsm +++ b/browser/devtools/shared/DeveloperToolbar.jsm @@ -456,7 +456,12 @@ DeveloperToolbar.prototype.hide = function DT_hide() */ DeveloperToolbar.prototype.destroy = function DT_destroy() { + if (this._lastState == NOTIFICATIONS.HIDE) { + return; + } + this._chromeWindow.getBrowser().tabContainer.removeEventListener("TabSelect", this, false); + this._chromeWindow.getBrowser().tabContainer.removeEventListener("TabClose", this, false); this._chromeWindow.getBrowser().removeEventListener("load", this, true); this._chromeWindow.getBrowser().removeEventListener("beforeunload", this, true); @@ -483,6 +488,8 @@ DeveloperToolbar.prototype.destroy = function DT_destroy() delete this.outputPanel; delete this.tooltipPanel; */ + + this._lastState = NOTIFICATIONS.HIDE; }; /**