From 396353cbb46049a5072af7b27c2cfc4e21e938db Mon Sep 17 00:00:00 2001 From: Victor Porof Date: Fri, 3 Jan 2014 23:41:28 +0200 Subject: [PATCH] Bug 952767 - Variables view scopes should always be lazily populated, r=past --- .../devtools/debugger/debugger-controller.js | 10 +- browser/devtools/debugger/debugger-view.js | 3 + .../test/browser_dbg_closure-inspection.js | 128 +++++----------- .../browser_dbg_variables-view-override-01.js | 18 ++- .../browser_dbg_variables-view-override-02.js | 40 +++-- .../devtools/shared/widgets/VariablesView.jsm | 1 - .../widgets/VariablesViewController.jsm | 142 +++++++----------- 7 files changed, 133 insertions(+), 209 deletions(-) diff --git a/browser/devtools/debugger/debugger-controller.js b/browser/devtools/debugger/debugger-controller.js index ccfe22077589..06f781eee1c8 100644 --- a/browser/devtools/debugger/debugger-controller.js +++ b/browser/devtools/debugger/debugger-controller.js @@ -887,11 +887,8 @@ StackFrames.prototype = { } } while ((environment = environment.parent)); - // Signal that scope environments have been shown and commit the current - // variables view hierarchy to briefly flash items that changed between the - // previous and current scope/variables/properties. + // Signal that scope environments have been shown. window.emit(EVENTS.FETCHED_SCOPES); - DebuggerView.Variables.commitHierarchy(); }, /** @@ -1005,11 +1002,8 @@ StackFrames.prototype = { expRef.separatorStr = L10N.getStr("variablesSeparatorLabel"); } - // Signal that watch expressions have been fetched and commit the - // current variables view hierarchy to briefly flash items that changed - // between the previous and current scope/variables/properties. + // Signal that watch expressions have been fetched. window.emit(EVENTS.FETCHED_WATCH_EXPRESSIONS); - DebuggerView.Variables.commitHierarchy(); }); }, diff --git a/browser/devtools/debugger/debugger-view.js b/browser/devtools/debugger/debugger-view.js index 9b4467318e20..233b6b55b112 100644 --- a/browser/devtools/debugger/debugger-view.js +++ b/browser/devtools/debugger/debugger-view.js @@ -181,6 +181,9 @@ let DebuggerView = { // Relay events from the VariablesView. this.Variables.on("fetched", (aEvent, aType) => { switch (aType) { + case "scopes": + window.emit(EVENTS.FETCHED_SCOPES); + break; case "variables": window.emit(EVENTS.FETCHED_VARIABLES); break; diff --git a/browser/devtools/debugger/test/browser_dbg_closure-inspection.js b/browser/devtools/debugger/test/browser_dbg_closure-inspection.js index f04c12837d22..4087f3d17ee5 100644 --- a/browser/devtools/debugger/test/browser_dbg_closure-inspection.js +++ b/browser/devtools/debugger/test/browser_dbg_closure-inspection.js @@ -1,7 +1,5 @@ -/* - * Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ - */ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ const TAB_URL = EXAMPLE_URL + "doc_closures.html"; @@ -15,6 +13,7 @@ function test() { gDebuggee = aDebuggee; gPanel = aPanel; gDebugger = gPanel.panelWin; + gDebuggee.gRecurseLimit = 2; waitForSourceShown(gPanel, ".html") .then(testClosure) @@ -33,45 +32,25 @@ function test() { gDebuggee); }); - gDebuggee.gRecurseLimit = 2; - return waitForDebuggerEvents(gPanel, gDebugger.EVENTS.FETCHED_SCOPES).then(() => { - let deferred = promise.defer(); - - let gVars = gDebugger.DebuggerView.Variables, - localScope = gVars.getScopeAtIndex(0), - globalScope = gVars.getScopeAtIndex(1), - localNodes = localScope.target.querySelector(".variables-view-element-details").childNodes, - globalNodes = globalScope.target.querySelector(".variables-view-element-details").childNodes; + let gVars = gDebugger.DebuggerView.Variables; + let localScope = gVars.getScopeAtIndex(0); + let localNodes = localScope.target.querySelector(".variables-view-element-details").childNodes; is(localNodes[4].querySelector(".name").getAttribute("value"), "person", "Should have the right property name for |person|."); - is(localNodes[4].querySelector(".value").getAttribute("value"), "Object", "Should have the right property value for |person|."); // Expand the 'person' tree node. This causes its properties to be // retrieved and displayed. let personNode = gVars.getItemForNode(localNodes[4]); + let personFetched = waitForDebuggerEvents(gPanel, gDebugger.EVENTS.FETCHED_PROPERTIES); personNode.expand(); - is(personNode.expanded, true, "person should be expanded at this point."); - // Poll every few milliseconds until the properties are retrieved. - // It's important to set the timer in the chrome window, because the - // content window timers are disabled while the debuggee is paused. - let count1 = 0; - let intervalID = window.setInterval(function(){ - info("count1: " + count1); - if (++count1 > 50) { - ok(false, "Timed out while polling for the properties."); - window.clearInterval(intervalID); - deferred.reject("Timed out."); - return; - } - if (!personNode._retrieved) { - return; - } - window.clearInterval(intervalID); + return personFetched.then(() => { + is(personNode.expanded, true, + "|person| should be expanded at this point."); is(personNode.get("getName").target.querySelector(".name") .getAttribute("value"), "getName", @@ -90,27 +69,16 @@ function test() { // retrieved and displayed. let getFooNode = personNode.get("getFoo"); let getNameNode = personNode.get("getName"); + let funcsFetched = waitForDebuggerEvents(gPanel, gDebugger.EVENTS.FETCHED_PROPERTIES, 2); + let funcClosuresFetched = waitForDebuggerEvents(gPanel, gDebugger.EVENTS.FETCHED_SCOPES, 2); getFooNode.expand(); getNameNode.expand(); - is(getFooNode.expanded, true, "person.getFoo should be expanded at this point."); - is(getNameNode.expanded, true, "person.getName should be expanded at this point."); - // Poll every few milliseconds until the properties are retrieved. - // It's important to set the timer in the chrome window, because the - // content window timers are disabled while the debuggee is paused. - let count2 = 0; - let intervalID1 = window.setInterval(function(){ - info("count2: " + count2); - if (++count2 > 50) { - ok(false, "Timed out while polling for the properties."); - window.clearInterval(intervalID1); - deferred.reject("Timed out."); - return; - } - if (!getFooNode._retrieved || !getNameNode._retrieved) { - return; - } - window.clearInterval(intervalID1); + return funcsFetched.then(() => { + is(getFooNode.expanded, true, + "|person.getFoo| should be expanded at this point."); + is(getNameNode.expanded, true, + "|person.getName| should be expanded at this point."); is(getFooNode.get("").target.querySelector(".name") .getAttribute("value"), "", @@ -125,30 +93,18 @@ function test() { .getAttribute("value"), "", "The closure node has no value for getName."); - // Expand the Closure nodes. + // Expand the closure nodes. This causes their environments to be + // retrieved and displayed. let getFooClosure = getFooNode.get(""); let getNameClosure = getNameNode.get(""); getFooClosure.expand(); getNameClosure.expand(); - is(getFooClosure.expanded, true, "person.getFoo closure should be expanded at this point."); - is(getNameClosure.expanded, true, "person.getName closure should be expanded at this point."); - // Poll every few milliseconds until the properties are retrieved. - // It's important to set the timer in the chrome window, because the - // content window timers are disabled while the debuggee is paused. - let count3 = 0; - let intervalID2 = window.setInterval(function(){ - info("count3: " + count3); - if (++count3 > 50) { - ok(false, "Timed out while polling for the properties."); - window.clearInterval(intervalID2); - deferred.reject("Timed out."); - return; - } - if (!getFooClosure._retrieved || !getNameClosure._retrieved) { - return; - } - window.clearInterval(intervalID2); + return funcClosuresFetched.then(() => { + is(getFooClosure.expanded, true, + "|person.getFoo| closure should be expanded at this point."); + is(getNameClosure.expanded, true, + "|person.getName| closure should be expanded at this point."); is(getFooClosure.get("Function scope [_pfactory]").target.querySelector(".name") .getAttribute("value"), "Function scope [_pfactory]", @@ -166,27 +122,15 @@ function test() { // Expand the scope nodes. let getFooInnerScope = getFooClosure.get("Function scope [_pfactory]"); let getNameInnerScope = getNameClosure.get("Function scope [_pfactory]"); + let innerFuncsFetched = waitForDebuggerEvents(gPanel, gDebugger.EVENTS.FETCHED_PROPERTIES, 2); getFooInnerScope.expand(); getNameInnerScope.expand(); - is(getFooInnerScope.expanded, true, "person.getFoo inner scope should be expanded at this point."); - is(getNameInnerScope.expanded, true, "person.getName inner scope should be expanded at this point."); - // Poll every few milliseconds until the properties are retrieved. - // It's important to set the timer in the chrome window, because the - // content window timers are disabled while the debuggee is paused. - let count4 = 0; - let intervalID3 = window.setInterval(function(){ - info("count4: " + count4); - if (++count4 > 50) { - ok(false, "Timed out while polling for the properties."); - window.clearInterval(intervalID3); - deferred.reject("Timed out."); - return; - } - if (!getFooInnerScope._retrieved || !getNameInnerScope._retrieved) { - return; - } - window.clearInterval(intervalID3); + return funcsFetched.then(() => { + is(getFooInnerScope.expanded, true, + "|person.getFoo| inner scope should be expanded at this point."); + is(getNameInnerScope.expanded, true, + "|person.getName| inner scope should be expanded at this point."); // Only test that each function closes over the necessary variable. // We wouldn't want future SpiderMonkey closure space @@ -203,14 +147,10 @@ function test() { is(getNameInnerScope.get("name").target.querySelector(".value") .getAttribute("value"), '"Bob"', "The name node has the expected value."); - - deferred.resolve(); - }, 100); - }, 100); - }, 100); - }, 100); - - return deferred.promise; + }); + }); + }); + }); }); } } diff --git a/browser/devtools/debugger/test/browser_dbg_variables-view-override-01.js b/browser/devtools/debugger/test/browser_dbg_variables-view-override-01.js index 48dcf2ca85f8..5f44741d8efa 100644 --- a/browser/devtools/debugger/test/browser_dbg_variables-view-override-01.js +++ b/browser/devtools/debugger/test/browser_dbg_variables-view-override-01.js @@ -49,12 +49,12 @@ function test() { is(firstScope._store.size, 3, "The first scope should have all the variables available."); - is(secondScope._store.size, 3, - "The second scope shoild have all the variables available."); - is(thirdScope._store.size, 3, - "The third scope shoild have all the variables available."); + is(secondScope._store.size, 0, + "The second scope shoild have no variables available yet."); + is(thirdScope._store.size, 0, + "The third scope shoild have no variables available yet."); is(globalScope._store.size, 0, - "The global scope shoild have no variables available."); + "The global scope shoild have no variables available yet."); // Test getOwnerScopeForVariableOrProperty with simple variables. @@ -95,6 +95,14 @@ function test() { // Test getOwnerScopeForVariableOrProperty with a simple variable // from non-topmost scopes. + // Only need to wait for a single FETCHED_VARIABLES event, just for the + // global scope, because the other local scopes already have the + // arguments and variables available as evironment bindings. + let fetched = waitForDebuggerEvents(panel, events.FETCHED_VARIABLES); + secondScope.expand(); + thirdScope.expand(); + globalScope.expand(); + let someVar2 = secondScope.get("a"); let someOwner2 = variables.getOwnerScopeForVariableOrProperty(someVar2); is(someOwner2, secondScope, diff --git a/browser/devtools/debugger/test/browser_dbg_variables-view-override-02.js b/browser/devtools/debugger/test/browser_dbg_variables-view-override-02.js index f9da58be6877..dadfa3ba3e5c 100644 --- a/browser/devtools/debugger/test/browser_dbg_variables-view-override-02.js +++ b/browser/devtools/debugger/test/browser_dbg_variables-view-override-02.js @@ -15,37 +15,51 @@ function test() { let variables = win.DebuggerView.Variables; // Wait for the hierarchy to be committed by the VariablesViewController. - let committed = promise.defer(); - variables.oncommit = committed.resolve; + let committedLocalScopeHierarchy = promise.defer(); + variables.oncommit = committedLocalScopeHierarchy.resolve; // Allow this generator function to yield first. executeSoon(() => debuggee.test()); yield waitForSourceAndCaretAndScopes(panel, ".html", 23); - yield committed.promise; + yield committedLocalScopeHierarchy.promise; let firstScope = variables.getScopeAtIndex(0); let secondScope = variables.getScopeAtIndex(1); let thirdScope = variables.getScopeAtIndex(2); let someVar1 = firstScope.get("a"); - let someVar2 = secondScope.get("a"); - let someVar3 = thirdScope.get("a"); - let argsVar1 = firstScope.get("arguments"); - let argsVar2 = secondScope.get("arguments"); - let argsVar3 = thirdScope.get("arguments"); is(someVar1.target.hasAttribute("overridden"), false, "The first 'a' variable should not be marked as being overridden."); - is(someVar2.target.hasAttribute("overridden"), true, - "The second 'a' variable should be marked as being overridden."); - is(someVar3.target.hasAttribute("overridden"), true, - "The third 'a' variable should be marked as being overridden."); - is(argsVar1.target.hasAttribute("overridden"), false, "The first 'arguments' variable should not be marked as being overridden."); + + // Wait for the hierarchy to be committed by the VariablesViewController. + let committedSecondScopeHierarchy = promise.defer(); + variables.oncommit = committedSecondScopeHierarchy.resolve; + secondScope.expand(); + yield committedSecondScopeHierarchy.promise; + + let someVar2 = secondScope.get("a"); + let argsVar2 = secondScope.get("arguments"); + + is(someVar2.target.hasAttribute("overridden"), true, + "The second 'a' variable should be marked as being overridden."); is(argsVar2.target.hasAttribute("overridden"), true, "The second 'arguments' variable should be marked as being overridden."); + + // Wait for the hierarchy to be committed by the VariablesViewController. + let committedThirdScopeHierarchy = promise.defer(); + variables.oncommit = committedThirdScopeHierarchy.resolve; + thirdScope.expand(); + yield committedThirdScopeHierarchy.promise; + + let someVar3 = thirdScope.get("a"); + let argsVar3 = thirdScope.get("arguments"); + + is(someVar3.target.hasAttribute("overridden"), true, + "The third 'a' variable should be marked as being overridden."); is(argsVar3.target.hasAttribute("overridden"), true, "The third 'arguments' variable should be marked as being overridden."); diff --git a/browser/devtools/shared/widgets/VariablesView.jsm b/browser/devtools/shared/widgets/VariablesView.jsm index 5c29fbc40fe8..aadc0b26afdc 100644 --- a/browser/devtools/shared/widgets/VariablesView.jsm +++ b/browser/devtools/shared/widgets/VariablesView.jsm @@ -2078,7 +2078,6 @@ Scope.prototype = { _enumItems: null, _nonEnumItems: null, _fetched: false, - _retrieved: false, _committed: false, _isLocked: false, _isExpanded: false, diff --git a/browser/devtools/shared/widgets/VariablesViewController.jsm b/browser/devtools/shared/widgets/VariablesViewController.jsm index 45866f96ed92..14c7d2aa8a52 100644 --- a/browser/devtools/shared/widgets/VariablesViewController.jsm +++ b/browser/devtools/shared/widgets/VariablesViewController.jsm @@ -152,8 +152,6 @@ VariablesViewController.prototype = { aTarget.setGrip(aGrip.initial + aResponse.substring); aTarget.hideArrow(); - // Mark the string as having retrieved. - aTarget._retrieved = true; deferred.resolve(); }); @@ -172,13 +170,6 @@ VariablesViewController.prototype = { _populateFromObject: function(aTarget, aGrip) { let deferred = promise.defer(); - // Mark the specified variable as having retrieved all its properties. - let finish = variable => { - variable._retrieved = true; - this.view.commitHierarchy(); - deferred.resolve(); - }; - let objectClient = this._getObjectClient(aGrip); objectClient.getPrototypeAndProperties(aResponse => { let { ownProperties, prototype } = aResponse; @@ -224,12 +215,12 @@ VariablesViewController.prototype = { // in the current scope chain. Not necessarily an actual error, // it just means that there's no closure for the function. console.warn(aResponse.error + ": " + aResponse.message); - return void finish(aTarget); + return void deferred.resolve(); } - this._addVarScope(aTarget, aResponse.scope).then(() => finish(aTarget)); + this._populateWithClosure(aTarget, aResponse.scope).then(deferred.resolve); }); } else { - finish(aTarget); + deferred.resolve(); } }); @@ -237,48 +228,43 @@ VariablesViewController.prototype = { }, /** - * Adds the scope chain elements (closures) of a function variable to the - * view. + * Adds the scope chain elements (closures) of a function variable. * * @param Variable aTarget * The variable where the properties will be placed into. * @param Scope aScope * The lexical environment form as specified in the protocol. */ - _addVarScope: function(aTarget, aScope) { + _populateWithClosure: function(aTarget, aScope) { let objectScopes = []; let environment = aScope; let funcScope = aTarget.addItem(""); - funcScope._target.setAttribute("scope", ""); - funcScope._fetched = true; + funcScope.target.setAttribute("scope", ""); funcScope.showArrow(); + do { // Create a scope to contain all the inspected variables. let label = StackFrameUtils.getScopeLabel(environment); - // Block scopes have the same label, so make addItem allow duplicates. + + // Block scopes may have the same label, so make addItem allow duplicates. let closure = funcScope.addItem(label, undefined, true); - closure._target.setAttribute("scope", ""); - closure._fetched = environment.class == "Function"; + closure.target.setAttribute("scope", ""); closure.showArrow(); + // Add nodes for every argument and every other variable in scope. if (environment.bindings) { - this._addBindings(closure, environment.bindings); - funcScope._retrieved = true; - closure._retrieved = true; + this._populateWithEnvironmentBindings(closure, environment.bindings); } else { - let deferred = Promise.defer(); + let deferred = promise.defer(); objectScopes.push(deferred.promise); this._getEnvironmentClient(environment).getBindings(response => { - this._addBindings(closure, response.bindings); - funcScope._retrieved = true; - closure._retrieved = true; + this._populateWithEnvironmentBindings(closure, response.bindings); deferred.resolve(); }); } } while ((environment = environment.parent)); - aTarget.expand(); - return Promise.all(objectScopes).then(() => { + return promise.all(objectScopes).then(() => { // Signal that scopes have been fetched. this.view.emit("fetched", "scopes", funcScope); }); @@ -287,32 +273,32 @@ VariablesViewController.prototype = { /** * Adds nodes for every specified binding to the closure node. * - * @param Variable closure - * The node where the bindings will be placed into. - * @param object bindings + * @param Variable aTarget + * The variable where the bindings will be placed into. + * @param object aBindings * The bindings form as specified in the protocol. */ - _addBindings: function(closure, bindings) { - for (let argument of bindings.arguments) { - let name = Object.getOwnPropertyNames(argument)[0]; - let argRef = closure.addItem(name, argument[name]); - let argVal = argument[name].value; - this.addExpander(argRef, argVal); - } + _populateWithEnvironmentBindings: function(aTarget, aBindings) { + // Add nodes for every argument in the scope. + aTarget.addItems(aBindings.arguments.reduce((accumulator, arg) => { + let name = Object.getOwnPropertyNames(arg)[0]; + let descriptor = arg[name]; + accumulator[name] = descriptor; + return accumulator; + }, {}), { + // Arguments aren't sorted. + sorted: false, + // Expansion handlers must be set after the properties are added. + callback: this.addExpander + }); - let aVariables = bindings.variables; - let variableNames = Object.keys(aVariables); - - // Sort all of the variables before adding them, if preferred. - if (VARIABLES_SORTING_ENABLED) { - variableNames.sort(); - } - // Add the variables to the specified scope. - for (let name of variableNames) { - let varRef = closure.addItem(name, aVariables[name]); - let varVal = aVariables[name].value; - this.addExpander(varRef, varVal); - } + // Add nodes for every other variable in the scope. + aTarget.addItems(aBindings.variables, { + // Not all variables need to force sorted properties. + sorted: VARIABLES_SORTING_ENABLED, + // Expansion handlers must be set after the properties are added. + callback: this.addExpander + }); }, /** @@ -328,12 +314,10 @@ VariablesViewController.prototype = { // Attach evaluation macros as necessary. if (aTarget.getter || aTarget.setter) { aTarget.evaluationMacro = this._overrideValueEvalMacro; - let getter = aTarget.get("get"); if (getter) { getter.evaluationMacro = this._getterOrSetterEvalMacro; } - let setter = aTarget.get("set"); if (setter) { setter.evaluationMacro = this._getterOrSetterEvalMacro; @@ -352,19 +336,13 @@ VariablesViewController.prototype = { aTarget.showArrow(); } - if (aSource.type == "block" || aSource.type == "function") { - // Block and function environments already contain scope arguments and - // corresponding variables as bindings. - this.populate(aTarget, aSource); - } else { - // Make sure that properties are always available on expansion. - aTarget.onexpand = () => this.populate(aTarget, aSource); + // Make sure that properties are always available on expansion. + aTarget.onexpand = () => this.populate(aTarget, aSource); - // Some variables are likely to contain a very large number of properties. - // It's a good idea to be prepared in case of an expansion. - if (aTarget.shouldPrefetch) { - aTarget.addEventListener("mouseover", aTarget.onexpand, false); - } + // Some variables are likely to contain a very large number of properties. + // It's a good idea to be prepared in case of an expansion. + if (aTarget.shouldPrefetch) { + aTarget.addEventListener("mouseover", aTarget.onexpand, false); } // Register all the actors that this controller now depends on. @@ -404,9 +382,11 @@ VariablesViewController.prototype = { // If the target is a Variable or Property then we're fetching properties. if (VariablesView.isVariable(aTarget)) { this._populateFromObject(aTarget, aSource).then(() => { - deferred.resolve(); // Signal that properties have been fetched. this.view.emit("fetched", "properties", aTarget); + // Commit the hierarchy because new items were added. + this.view.commitHierarchy(); + deferred.resolve(); }); return deferred.promise; } @@ -414,43 +394,29 @@ VariablesViewController.prototype = { switch (aSource.type) { case "longString": this._populateFromLongString(aTarget, aSource).then(() => { - deferred.resolve(); // Signal that a long string has been fetched. this.view.emit("fetched", "longString", aTarget); + deferred.resolve(); }); break; case "with": case "object": this._populateFromObject(aTarget, aSource.object).then(() => { - deferred.resolve(); // Signal that variables have been fetched. this.view.emit("fetched", "variables", aTarget); + // Commit the hierarchy because new items were added. + this.view.commitHierarchy(); + deferred.resolve(); }); break; case "block": case "function": - // Add nodes for every argument and every other variable in scope. - let args = aSource.bindings.arguments; - if (args) { - for (let arg of args) { - let name = Object.getOwnPropertyNames(arg)[0]; - let ref = aTarget.addItem(name, arg[name]); - let val = arg[name].value; - this.addExpander(ref, val); - } - } - - aTarget.addItems(aSource.bindings.variables, { - // Not all variables need to force sorted properties. - sorted: VARIABLES_SORTING_ENABLED, - // Expansion handlers must be set after the properties are added. - callback: this.addExpander - }); - + this._populateWithEnvironmentBindings(aTarget, aSource.bindings); // No need to signal that variables have been fetched, since // the scope arguments and variables are already attached to the // environment bindings, so pausing the active thread is unnecessary. - + // Commit the hierarchy because new items were added. + this.view.commitHierarchy(); deferred.resolve(); break; default: