Bug 1499289 - Change how we deal with getter evaluation in JsPropertyProvider; r=bgrins.
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Mon, 07 Jan 2019 09:49:10 +0000
changeset 509973 9b92ad8d977db7070f1f889df64fa166eb53f676
parent 509972 013d4c77819e0d8cbace78b4bdd626bc8476c8e4
child 509974 bc5767b55411524f9e137e954bc7f863ab0680c4
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbgrins
bugs1499289
milestone66.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 1499289 - Change how we deal with getter evaluation in JsPropertyProvider; r=bgrins. While trying to implement the invoke getter from autocomplete popup, it became clear to me that the initial implementation in js-property-provider wasn't good enough, as we need to keep track of all the authorizations the user gave when working on a given expression. In order to handle this, JsPropertyProvider now returns an array of strings representing the path to the unsafe getter when no matching authorizations are provided. If authorizations are provided, we can check for each properties that the user authorized the execution. This way, we can handle deep object access after a getter (e.g. `x.myGetter.foo.bar.baz.entries`) without asking the user if they want to invoke `myGetter` on each step of the completion. This makes handling intermediary getters (e.g. `x.myGetter.foo.myOtherGetter.bar`) way easier as well. In the UI, the user will be prompted to invoke the getter one after the other (if for example they try to complete a pasted expression which contains multiple getters, they will have prompts for `myGetter`, and then for `myOtherGetter`). We wire-up the webconsole client and the webconsole actor for the autocomplete function, to make them ready for frontend use. The existing JsPropertyProvider getters test are updated to match the change of parameter (invokeGetter -> authorizedEvaluations), and some tests are added to make sure everything work as intended. Differential Revision: https://phabricator.services.mozilla.com/D12940
devtools/server/actors/webconsole.js
devtools/shared/webconsole/client.js
devtools/shared/webconsole/js-property-provider.js
devtools/shared/webconsole/test/unit/test_js_property_provider.js
--- a/devtools/server/actors/webconsole.js
+++ b/devtools/server/actors/webconsole.js
@@ -1208,32 +1208,40 @@ WebConsoleActor.prototype =
         dbgObject = this.dbg.addDebuggee(this.evalWindow);
       }
 
       const result = JSPropertyProvider({
         dbgObject,
         environment,
         inputValue: request.text,
         cursor: request.cursor,
-        invokeUnsafeGetter: false,
         webconsoleActor: this,
         selectedNodeActor: request.selectedNodeActor,
+        authorizedEvaluations: request.authorizedEvaluations,
       });
 
       if (!hadDebuggee && dbgObject) {
         this.dbg.removeDebuggee(this.evalWindow);
       }
 
       if (result === null) {
         return {
           from: this.actorID,
           matches: null,
         };
       }
 
+      if (result && result.isUnsafeGetter === true) {
+        return {
+          from: this.actorID,
+          isUnsafeGetter: true,
+          getterPath: result.getterPath,
+        };
+      }
+
       matches = result.matches || new Set();
       matchProp = result.matchProp;
       isElementAccess = result.isElementAccess;
 
       // We consider '$' as alphanumeric because it is used in the names of some
       // helper functions; we also consider whitespace as alphanum since it should not
       // be seen as break in the evaled string.
       const lastNonAlphaIsDot = /[.][a-zA-Z0-9$\s]*$/.test(reqText);
--- a/devtools/shared/webconsole/client.js
+++ b/devtools/shared/webconsole/client.js
@@ -376,27 +376,38 @@ WebConsoleClient.prototype = {
    *
    * @param {String} string
    *        The code you want to autocomplete.
    * @param {Number} cursor
    *        Cursor location inside the string. Index starts from 0.
    * @param {String} frameActor
    *        The id of the frame actor that made the call.
    * @param {String} selectedNodeActor: Actor id of the selected node in the inspector.
+   * @param {Array} authorizedEvaluations
+   *        Array of the properties access which can be executed by the engine.
+   *        Example: [["x", "myGetter"], ["x", "myGetter", "y", "anotherGetter"]] to
+   *        retrieve properties of `x.myGetter.` and `x.myGetter.y.anotherGetter`.
    * @return request
    *         Request object that implements both Promise and EventEmitter interfaces
    */
-  autocomplete: function(string, cursor, frameActor, selectedNodeActor) {
+  autocomplete: function(
+    string,
+    cursor,
+    frameActor,
+    selectedNodeActor,
+    authorizedEvaluations
+  ) {
     const packet = {
       to: this._actor,
       type: "autocomplete",
       text: string,
       cursor,
       frameActor,
       selectedNodeActor,
+      authorizedEvaluations,
     };
     return this._client.request(packet);
   },
 
   /**
    * Clear the cache of messages (page errors and console API calls).
    *
    * @return request
--- a/devtools/shared/webconsole/js-property-provider.js
+++ b/devtools/shared/webconsole/js-property-provider.js
@@ -210,33 +210,34 @@ function analyzeInputString(str) {
  *        scope for autocompletion.
  *        It is null if the debugger is not paused.
  * - {String} inputValue
  *        Value that should be completed.
  * - {Number} cursor (defaults to inputValue.length).
  *        Optional offset in the input where the cursor is located. If this is
  *        omitted then the cursor is assumed to be at the end of the input
  *        value.
- * - {Boolean} invokeUnsafeGetter (defaults to false).
- *        Optional boolean to indicate if the function should execute unsafe getter
- *        in order to retrieve its result's properties.
+ * - {Array} authorizedEvaluations (defaults to []).
+ *        Optional array containing all the different properties access that the engine
+ *        can execute in order to retrieve its result's properties.
  *        ⚠️ This should be set to true *ONLY* on user action as it may cause side-effects
  *        in the content page ⚠️
  * - {WebconsoleActor} webconsoleActor
  *        A reference to a webconsole actor which we can use to retrieve the last
  *        evaluation result or create a debuggee value.
  * - {String}: selectedNodeActor
  *        The actor id of the selected node in the inspector.
  * @returns null or object
  *          If the inputValue is an unsafe getter and invokeUnsafeGetter is false, the
  *          following form is returned:
  *
  *          {
  *            isUnsafeGetter: true,
- *            getterName: {String} The name of the unsafe getter
+ *            getterPath: {Array<String>} An array of the property chain leading to the
+ *                        getter. Example: ["x", "myGetter"]
  *          }
  *
  *          If no completion valued could be computed, and the input is not an unsafe
  *          getter, null is returned.
  *
  *          Otherwise an object with the following form is returned:
  *            {
  *              matches: Set<string>
@@ -246,17 +247,17 @@ function analyzeInputString(str) {
  *                               access (e.g. `window["addEvent`).
  *            }
  */
 function JSPropertyProvider({
   dbgObject,
   environment,
   inputValue,
   cursor,
-  invokeUnsafeGetter = false,
+  authorizedEvaluations = [],
   webconsoleActor,
   selectedNodeActor,
 }) {
   if (cursor === undefined) {
     cursor = inputValue.length;
   }
 
   inputValue = inputValue.substring(0, cursor);
@@ -443,37 +444,35 @@ function JSPropertyProvider({
     if (typeof prop === "string") {
       prop = prop.trim();
     }
 
     if (prop === undefined || prop === null || prop === "") {
       return null;
     }
 
-    if (!invokeUnsafeGetter && DevToolsUtils.isUnsafeGetter(obj, prop)) {
-      // If the unsafe getter is not the last property access of the input, bail out as
-      // things might get complex.
-      if (index !== properties.length - 1) {
-        return null;
-      }
+    const propPath = [firstProp].concat(properties.slice(0, index + 1));
+    const authorized = authorizedEvaluations.some(
+      x => JSON.stringify(x) === JSON.stringify(propPath));
 
+    if (!authorized && DevToolsUtils.isUnsafeGetter(obj, prop)) {
       // If we try to access an unsafe getter, return its name so we can consume that
       // on the frontend.
       return {
         isUnsafeGetter: true,
-        getterName: prop,
+        getterPath: propPath,
       };
     }
 
     if (hasArrayIndex(prop)) {
       // The property to autocomplete is a member of array. For example
       // list[i][j]..[n]. Traverse the array to get the actual element.
       obj = getArrayMemberProperty(obj, null, prop);
     } else {
-      obj = DevToolsUtils.getProperty(obj, prop, invokeUnsafeGetter);
+      obj = DevToolsUtils.getProperty(obj, prop, authorized);
     }
 
     if (!isObjectUsable(obj)) {
       return null;
     }
   }
 
   const prepareReturnedObject = matches => {
--- a/devtools/shared/webconsole/test/unit/test_js_property_provider.js
+++ b/devtools/shared/webconsole/test/unit/test_js_property_provider.js
@@ -259,57 +259,122 @@ function runChecks(dbgObject, environmen
 
   info("Test with an anonymous generator.");
   const gen2Result = Cu.evalInSandbox("gen2.next().value", sandbox);
   results = propertyProvider("gen2.");
   test_has_result(results, "next");
   const gen2NextResult = Cu.evalInSandbox("gen2.next().value", sandbox);
   Assert.equal(gen2Result + 1, gen2NextResult);
 
-  info("Test that getters are not executed if invokeUnsafeGetter is undefined");
+  info("Test that getters are not executed if authorizedEvaluations is undefined");
   results = propertyProvider("testGetters.x.");
-  Assert.deepEqual(results, {isUnsafeGetter: true, getterName: "x"});
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "x"]});
 
   results = propertyProvider("testGetters.x[");
-  Assert.deepEqual(results, {isUnsafeGetter: true, getterName: "x"});
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "x"]});
 
   results = propertyProvider("testGetters.x.hell");
-  Assert.deepEqual(results, {isUnsafeGetter: true, getterName: "x"});
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "x"]});
 
   results = propertyProvider("testGetters.x['hell");
-  Assert.deepEqual(results, {isUnsafeGetter: true, getterName: "x"});
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "x"]});
+
+  info("Test that getters are not executed if authorizedEvaluations does not match");
+  results = propertyProvider("testGetters.x.", {authorizedEvaluations: []});
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "x"]});
+
+  results = propertyProvider("testGetters.x.", {
+    authorizedEvaluations: [["testGetters"]],
+  });
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "x"]});
 
-  info("Test that deep getter property access does not return intermediate getters");
+  results = propertyProvider("testGetters.x.", {
+    authorizedEvaluations: [["testGtrs", "x"]],
+  });
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "x"]});
+
+  results = propertyProvider("testGetters.x.", {
+    authorizedEvaluations: [["x"]],
+  });
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "x"]});
+
+  info("Test that deep getter property access returns intermediate getters");
   results = propertyProvider("testGetters.y.y.");
-  Assert.ok(results === null);
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "y"]});
 
   results = propertyProvider("testGetters['y'].y.");
-  Assert.ok(results === null);
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "y"]});
 
   results = propertyProvider("testGetters['y']['y'].");
-  Assert.ok(results === null);
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "y"]});
 
   results = propertyProvider("testGetters.y['y'].");
-  Assert.ok(results === null);
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "y"]});
+
+  info("Test that deep getter property access invoke intermediate getters");
+  results = propertyProvider("testGetters.y.y.", {
+    authorizedEvaluations: [["testGetters", "y"]],
+  });
+  Assert.deepEqual(results,
+    {isUnsafeGetter: true, getterPath: ["testGetters", "y", "y"]});
+
+  results = propertyProvider("testGetters['y'].y.", {
+    authorizedEvaluations: [["testGetters", "y"]],
+  });
+  Assert.deepEqual(results,
+    {isUnsafeGetter: true, getterPath: ["testGetters", "y", "y"]});
 
-  info("Test that getters are executed if invokeUnsafeGetter is true");
-  results = propertyProvider("testGetters.x.", {invokeUnsafeGetter: true});
+  results = propertyProvider("testGetters['y']['y'].", {
+    authorizedEvaluations: [["testGetters", "y"]],
+  });
+  Assert.deepEqual(results,
+    {isUnsafeGetter: true, getterPath: ["testGetters", "y", "y"]});
+
+  results = propertyProvider("testGetters.y['y'].", {
+    authorizedEvaluations: [["testGetters", "y"]],
+  });
+  Assert.deepEqual(
+    results, {isUnsafeGetter: true, getterPath: ["testGetters", "y", "y"]});
+
+  info("Test that getters are executed if matching an authorizedEvaluation element");
+  results = propertyProvider("testGetters.x.", {
+    authorizedEvaluations: [["testGetters", "x"]],
+  });
   test_has_exact_results(results, ["hello", "world"]);
   Assert.ok(Object.keys(results).includes("isUnsafeGetter") === false);
-  Assert.ok(Object.keys(results).includes("getterName") === false);
+  Assert.ok(Object.keys(results).includes("getterPath") === false);
+
+  results = propertyProvider("testGetters.x.", {
+    authorizedEvaluations: [["testGetters", "x"], ["y"]],
+  });
+  test_has_exact_results(results, ["hello", "world"]);
+  Assert.ok(Object.keys(results).includes("isUnsafeGetter") === false);
+  Assert.ok(Object.keys(results).includes("getterPath") === false);
 
   info("Test that executing getters filters with provided string");
-  results = propertyProvider("testGetters.x.hell", {invokeUnsafeGetter: true});
+  results = propertyProvider("testGetters.x.hell", {
+    authorizedEvaluations: [["testGetters", "x"]],
+  });
   test_has_exact_results(results, ["hello"]);
 
-  results = propertyProvider("testGetters.x['hell", {invokeUnsafeGetter: true});
+  results = propertyProvider("testGetters.x['hell", {
+    authorizedEvaluations: [["testGetters", "x"]],
+  });
   test_has_exact_results(results, ["'hello'"]);
 
-  info("Test that children getters are executed if invokeUnsafeGetter is true");
-  results = propertyProvider("testGetters.y.y.", {invokeUnsafeGetter: true});
+  info("Test children getters are not executed if not included in authorizedEvaluation");
+  results = propertyProvider("testGetters.y.y.", {
+    authorizedEvaluations: [["testGetters", "y", "y"]],
+  });
+  Assert.deepEqual(results, {isUnsafeGetter: true, getterPath: ["testGetters", "y"]});
+
+  info("Test children getters are executed if matching an authorizedEvaluation element");
+  results = propertyProvider("testGetters.y.y.", {
+    authorizedEvaluations: [["testGetters", "y"], ["testGetters", "y", "y"]],
+  });
   test_has_result(results, "trim");
 
   info("Test with number literals");
   results = propertyProvider("1.");
   Assert.ok(results === null, "Does not complete on possible floating number");
 
   results = propertyProvider("(1)..");
   Assert.ok(results === null, "Does not complete on invalid syntax");