Bug 947710 - Stop evaling and catch errors in Manifest Editor. r=vporof, a=lsblakk
authorJ. Ryan Stinnett <jryans@gmail.com>
Mon, 06 Jan 2014 11:14:21 -0600
changeset 175609 f0607d5d0e071fa8efd6d8059d781c741c7486f1
parent 175608 89680c49db3b77ff338a1fccd93b8f1e6a067394
child 175610 95f4cb9e1f6cb70fbb1e88bafccbae252d73dd9f
push id445
push userffxbld
push dateMon, 10 Mar 2014 22:05:19 +0000
treeherdermozilla-release@dc38b741b04e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvporof, lsblakk
bugs947710
milestone28.0a2
Bug 947710 - Stop evaling and catch errors in Manifest Editor. r=vporof, a=lsblakk
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
@@ -1383,18 +1383,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 => gThreadClient.pauseGrip(aObject)
     });
--- a/browser/devtools/shared/widgets/VariablesView.jsm
+++ b/browser/devtools/shared/widgets/VariablesView.jsm
@@ -1143,17 +1143,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
@@ -2675,17 +2705,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() {
@@ -2766,16 +2796,17 @@ Variable.prototype = Heritage.extend(Sco
           this._disable();
         }
         this.ownerView.new(this, aKey, aValue);
       }
     }, e);
   },
 
   _symbolicName: "",
+  _symbolicPath: null,
   _absoluteName: "",
   _initialDescriptor: null,
   _separatorLabel: null,
   _spacer: null,
   _valueLabel: null,
   _editNode: null,
   _deleteNode: null,
   _addPropertyNode: null,
--- a/browser/devtools/webconsole/webconsole.js
+++ b/browser/devtools/webconsole/webconsole.js
@@ -3516,30 +3516,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