Bug 947710 - Stop evaling and catch errors in Manifest Editor. r=vporof
authorJ. Ryan Stinnett <jryans@gmail.com>
Mon, 06 Jan 2014 11:14:21 -0600
changeset 179257 64c3bdede7dd7f510f95909b7393ca5c88019ddf
parent 179256 05d387f1dc78b7454e5eb343f01e95eae6287d1b
child 179258 a27c76ef0c1ed3e680682950df38d3a25e08600f
push id462
push userraliiev@mozilla.com
push dateTue, 22 Apr 2014 00:22:30 +0000
treeherdermozilla-release@ac5db8c74ac0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvporof
bugs947710
milestone29.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 947710 - Stop evaling and catch errors in Manifest Editor. r=vporof
browser/devtools/app-manager/content/manifest-editor.js
browser/devtools/app-manager/test/browser_manifest_editor.js
browser/devtools/debugger/debugger-panes.js
browser/devtools/debugger/debugger-view.js
browser/devtools/shared/widgets/VariablesView.jsm
browser/devtools/webconsole/webconsole.js
--- a/browser/devtools/app-manager/content/manifest-editor.js
+++ b/browser/devtools/app-manager/content/manifest-editor.js
@@ -56,52 +56,68 @@ ManifestEditor.prototype = {
       editor.switch = this._onSwitch;
       editor.delete = this._onDelete;
       editor.new = this._onNew;
     }
 
     return this.update();
   },
 
-  _onEval: function(evalString) {
-    let manifest = this.manifest;
-    eval("manifest" + evalString);
+  _onEval: function(variable, value) {
+    let parent = this._descend(variable.ownerView.symbolicPath);
+    try {
+      parent[variable.name] = JSON.parse(value);
+    } catch(e) {
+      Cu.reportError(e);
+    }
+
     this.update();
   },
 
   _onSwitch: function(variable, newName) {
-    let manifest = this.manifest;
-    let newSymbolicName = variable.ownerView.symbolicName +
-                          "['" + newName + "']";
-    if (newSymbolicName == variable.symbolicName) {
+    if (variable.name == newName) {
       return;
     }
 
-    let evalString = "manifest" + newSymbolicName + " = " +
-                     "manifest" + variable.symbolicName + ";" +
-                     "delete manifest" + variable.symbolicName;
+    let parent = this._descend(variable.ownerView.symbolicPath);
+    parent[newName] = parent[variable.name];
+    delete parent[variable.name];
 
-    eval(evalString);
     this.update();
   },
 
   _onDelete: function(variable) {
-    let manifest = this.manifest;
-    let evalString = "delete manifest" + variable.symbolicName;
-    eval(evalString);
+    let parent = this._descend(variable.ownerView.symbolicPath);
+    delete parent[variable.name];
   },
 
   _onNew: function(variable, newName, newValue) {
-    let manifest = this.manifest;
-    let symbolicName = variable.symbolicName + "['" + newName + "']";
-    let evalString = "manifest" + symbolicName + " = " + newValue + ";";
-    eval(evalString);
+    let parent = this._descend(variable.symbolicPath);
+    try {
+      parent[newName] = JSON.parse(newValue);
+    } catch(e) {
+      Cu.reportError(e);
+    }
+
     this.update();
   },
 
+  /**
+   * Returns the value located at a given path in the manifest.
+   * @param path array
+   *        A string for each path component: ["developer", "name"]
+   */
+  _descend: function(path) {
+    let parent = this.manifest;
+    while (path.length) {
+      parent = parent[path.shift()];
+    }
+    return parent;
+  },
+
   update: function() {
     this.editor.createHierarchy();
     this.editor.rawObject = this.manifest;
     this.editor.commitHierarchy();
 
     // Wait until the animation from commitHierarchy has completed
     let deferred = promise.defer();
     setTimeout(deferred.resolve, this.editor.lazyEmptyDelay + 1);
--- a/browser/devtools/app-manager/test/browser_manifest_editor.js
+++ b/browser/devtools/app-manager/test/browser_manifest_editor.js
@@ -1,33 +1,34 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 "use strict";
 
 const {Services} = Cu.import("resource://gre/modules/Services.jsm");
 
 const MANIFEST_EDITOR_ENABLED = "devtools.appmanager.manifestEditor.enabled";
 
-
 let gManifestWindow, gManifestEditor;
 
 function test() {
   waitForExplicitFinish();
 
   Task.spawn(function() {
     Services.prefs.setBoolPref(MANIFEST_EDITOR_ENABLED, true);
     let tab = yield openAppManager();
     yield selectProjectsPanel();
     yield addSamplePackagedApp();
     yield showSampleProjectDetails();
 
     gManifestWindow = getManifestWindow();
     gManifestEditor = getProjectsWindow().UI.manifestEditor;
     yield changeManifestValue("name", "the best app");
+    yield changeManifestValueBad("name", "the worst app");
     yield addNewManifestProperty("developer", "foo", "bar");
+    yield addNewManifestPropertyBad("developer", "blob", "bob");
     gManifestWindow = null;
     gManifestEditor = null;
 
     yield removeSamplePackagedApp();
     yield removeTab(tab);
     Services.prefs.setBoolPref(MANIFEST_EDITOR_ENABLED, false);
     finish();
   });
@@ -59,16 +60,43 @@ function changeManifestValue(key, value)
     is(valueElem.value, '"' + value + '"',
        "Value doesn't match expected value");
 
     is(gManifestEditor.manifest[key], value,
        "Manifest doesn't contain expected value");
   });
 }
 
+function changeManifestValueBad(key, value) {
+  return Task.spawn(function() {
+    let propElem = gManifestWindow.document
+                   .querySelector("[id ^= '" + key + "']");
+    is(propElem.querySelector(".name").value, key,
+       "Key doesn't match expected value");
+
+    let valueElem = propElem.querySelector(".value");
+    EventUtils.sendMouseEvent({ type: "mousedown" }, valueElem, gManifestWindow);
+
+    let valueInput = propElem.querySelector(".element-value-input");
+    // Leaving out quotes will result in an error, so no change should be made.
+    valueInput.value = value;
+    EventUtils.sendKey("RETURN", gManifestWindow);
+
+    yield waitForUpdate();
+    // Elements have all been replaced, re-select them
+    propElem = gManifestWindow.document.querySelector("[id ^= '" + key + "']");
+    valueElem = propElem.querySelector(".value");
+    isnot(valueElem.value, '"' + value + '"',
+       "Value was changed, but it should not have been");
+
+    isnot(gManifestEditor.manifest[key], value,
+       "Manifest was changed, but it should not have been");
+  });
+}
+
 function addNewManifestProperty(parent, key, value) {
   return Task.spawn(function() {
     let parentElem = gManifestWindow.document
                      .querySelector("[id ^= '" + parent + "']");
     ok(parentElem,
       "Found parent element");
     let addPropertyElem = parentElem
                           .querySelector(".variables-view-add-property");
@@ -98,8 +126,39 @@ function addNewManifestProperty(parent, 
     let valueElem = newElem.querySelector(".value");
     is(valueElem.value, '"' + value + '"',
        "Value doesn't match expected value");
 
     is(gManifestEditor.manifest[parent][key], value,
        "Manifest doesn't contain expected value");
   });
 }
+
+function addNewManifestPropertyBad(parent, key, value) {
+  return Task.spawn(function() {
+    let parentElem = gManifestWindow.document
+                     .querySelector("[id ^= '" + parent + "']");
+    ok(parentElem,
+      "Found parent element");
+    let addPropertyElem = parentElem
+                          .querySelector(".variables-view-add-property");
+    ok(addPropertyElem,
+      "Found add-property button");
+
+    EventUtils.sendMouseEvent({ type: "mousedown" }, addPropertyElem, gManifestWindow);
+
+    let nameInput = parentElem.querySelector(".element-name-input");
+    nameInput.value = key;
+    EventUtils.sendKey("TAB", gManifestWindow);
+
+    let valueInput = parentElem.querySelector(".element-value-input");
+    // Leaving out quotes will result in an error, so no change should be made.
+    valueInput.value = value;
+    EventUtils.sendKey("RETURN", gManifestWindow);
+
+    yield waitForUpdate();
+
+    let newElem = gManifestWindow.document.querySelector("[id ^= '" + key + "']");
+    ok(!newElem, "Key was added, but it should not have been");
+    ok(!(key in gManifestEditor.manifest[parent]),
+       "Manifest contains key, but it should not");
+  });
+}
--- a/browser/devtools/debugger/debugger-panes.js
+++ b/browser/devtools/debugger/debugger-panes.js
@@ -1851,18 +1851,19 @@ VariableBubbleView.prototype = {
     if (VariablesView.isPrimitive({ value: objectActor })) {
       let className = VariablesView.getClass(objectActor);
       let textContent = VariablesView.getString(objectActor);
       this._tooltip.setTextContent([textContent], className, "plain");
     } else {
       this._tooltip.setVariableContent(objectActor, {
         searchPlaceholder: L10N.getStr("emptyPropertiesFilterText"),
         searchEnabled: Prefs.variablesSearchboxVisible,
-        eval: aString => {
-          DebuggerController.StackFrames.evaluate(aString);
+        eval: (variable, value) => {
+          let string = variable.evaluationMacro(variable, value);
+          DebuggerController.StackFrames.evaluate(string);
           DebuggerView.VariableBubble.hideContents();
         }
       }, {
         getEnvironmentClient: aObject => gThreadClient.environment(aObject),
         getObjectClient: aObject => gThreadClient.pauseGrip(aObject),
         simpleValueEvalMacro: this._getSimpleValueEvalMacro(evalPrefix),
         getterOrSetterEvalMacro: this._getGetterOrSetterEvalMacro(evalPrefix),
         overrideValueEvalMacro: this._getOverrideValueEvalMacro(evalPrefix)
--- a/browser/devtools/debugger/debugger-view.js
+++ b/browser/devtools/debugger/debugger-view.js
@@ -157,17 +157,20 @@ let DebuggerView = {
    * Initializes the VariablesView instance and attaches a controller.
    */
   _initializeVariablesView: function() {
     this.Variables = new VariablesView(document.getElementById("variables"), {
       searchPlaceholder: L10N.getStr("emptyVariablesFilterText"),
       emptyText: L10N.getStr("emptyVariablesText"),
       onlyEnumVisible: Prefs.variablesOnlyEnumVisible,
       searchEnabled: Prefs.variablesSearchboxVisible,
-      eval: DebuggerController.StackFrames.evaluate,
+      eval: (variable, value) => {
+        let string = variable.evaluationMacro(variable, value);
+        DebuggerController.StackFrames.evaluate(string);
+      },
       lazyEmpty: true
     });
 
     // Attach a controller that handles interfacing with the debugger protocol.
     VariablesViewController.attach(this.Variables, {
       getEnvironmentClient: aObject => gThreadClient.environment(aObject),
       getObjectClient: aObject => {
         return aObject instanceof DebuggerController.Tracer.WrappedObject
--- a/browser/devtools/shared/widgets/VariablesView.jsm
+++ b/browser/devtools/shared/widgets/VariablesView.jsm
@@ -1168,17 +1168,17 @@ VariablesView.getterOrSetterEvalMacro = 
  * @param Property aItem
  *        The current getter or setter property.
  */
 VariablesView.getterOrSetterDeleteCallback = function(aItem) {
   aItem._disable();
 
   // Make sure the right getter/setter to value override macro is applied
   // to the target object.
-  aItem.ownerView.eval(aItem.evaluationMacro(aItem, ""));
+  aItem.ownerView.eval(aItem, "");
 
   return true; // Don't hide the element.
 };
 
 
 /**
  * A Scope is an object holding Variable instances.
  * Iterable via "for (let [name, variable] of instance) { }".
@@ -2286,23 +2286,53 @@ Variable.prototype = Heritage.extend(Sco
     let descriptor = Object.create(aDescriptor);
     descriptor.get = VariablesView.getGrip(aDescriptor.get);
     descriptor.set = VariablesView.getGrip(aDescriptor.set);
 
     return this.addItem(aName, descriptor);
   },
 
   /**
-   * Gets this variable's path to the topmost scope.
+   * Gets this variable's path to the topmost scope in the form of a string
+   * meant for use via eval() or a similar approach.
    * For example, a symbolic name may look like "arguments['0']['foo']['bar']".
    * @return string
    */
   get symbolicName() this._symbolicName,
 
   /**
+   * Gets this variable's symbolic path to the topmost scope.
+   * @return array
+   * @see Variable._buildSymbolicPath
+   */
+  get symbolicPath() {
+    if (this._symbolicPath) {
+      return this._symbolicPath;
+    }
+    this._symbolicPath = this._buildSymbolicPath();
+    return this._symbolicPath;
+  },
+
+  /**
+   * Build this variable's path to the topmost scope in form of an array of
+   * strings, one for each segment of the path.
+   * For example, a symbolic path may look like ["0", "foo", "bar"].
+   * @return array
+   */
+  _buildSymbolicPath: function(path = []) {
+    if (this.name) {
+      path.unshift(this.name);
+      if (this.ownerView instanceof Variable) {
+        return this.ownerView._buildSymbolicPath(path);
+      }
+    }
+    return path;
+  },
+
+  /**
    * Returns this variable's value from the descriptor if available.
    * @return any
    */
   get value() this._initialDescriptor.value,
 
   /**
    * Returns this variable's getter from the descriptor if available.
    * @return object
@@ -2710,17 +2740,17 @@ Variable.prototype = Heritage.extend(Sco
    * Makes this variable's value editable.
    */
   _activateValueInput: function(e) {
     EditableValue.create(this, {
       onSave: aString => {
         if (!this._variablesView.preventDisableOnChange) {
           this._disable();
         }
-        this.ownerView.eval(this.evaluationMacro(this, aString));
+        this.ownerView.eval(this, aString);
       }
     }, e);
   },
 
   /**
    * Disables this variable prior to a new name switch or value evaluation.
    */
   _disable: function() {
@@ -2801,16 +2831,17 @@ Variable.prototype = Heritage.extend(Sco
           this._disable();
         }
         this.ownerView.new(this, aKey, aValue);
       }
     }, e);
   },
 
   _symbolicName: "",
+  _symbolicPath: null,
   _absoluteName: "",
   _initialDescriptor: null,
   _separatorLabel: null,
   _valueLabel: null,
   _spacer: null,
   _editNode: null,
   _deleteNode: null,
   _addPropertyNode: null,
--- a/browser/devtools/webconsole/webconsole.js
+++ b/browser/devtools/webconsole/webconsole.js
@@ -3511,30 +3511,34 @@ JSTerm.prototype = {
 
   /**
    * The evaluation function used by the variables view when editing a property
    * value.
    *
    * @private
    * @param object aOptions
    *        The options used for |this._updateVariablesView()|.
-   * @param string aString
-   *        The string that the variables view wants to evaluate.
+   * @param object aVar
+   *        The Variable object instance for the edited property.
+   * @param string aValue
+   *        The value the edited property was changed to.
    */
-  _variablesViewEvaluate: function JST__variablesViewEvaluate(aOptions, aString)
+  _variablesViewEvaluate:
+  function JST__variablesViewEvaluate(aOptions, aVar, aValue)
   {
     let updater = this._updateVariablesView.bind(this, aOptions);
     let onEval = this._silentEvalCallback.bind(this, updater);
+    let string = aVar.evaluationMacro(aVar, aValue);
 
     let evalOptions = {
       frame: this.SELECTED_FRAME,
       bindObjectActor: aOptions.objectActor.actor,
     };
 
-    this.requestEvaluation(aString, evalOptions).then(onEval, onEval);
+    this.requestEvaluation(string, evalOptions).then(onEval, onEval);
   },
 
   /**
    * The property deletion function used by the variables view when a property
    * is deleted.
    *
    * @private
    * @param object aOptions