Closed Bug 1473996 Opened Last year Closed 11 months ago

Devtools server should expose an API for evaluating object properties and executing functions

Categories

(DevTools :: General, enhancement, P1)

enhancement

Tracking

(firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: loganfsmyth, Assigned: loganfsmyth)

References

Details

Attachments

(4 files)

The SpiderMonkey Debugger.Object API has `.getProperty` which returns a descriptor, but does not have a way to evaluate getters, and doesn't expose a general `.call`/`.apply`.

The concern in the general case is the possibility of unexpectedly triggering user code, but really it should be up to the developers of the devtools debugger to make that clear in the UI, rather than limiting the API itself.
Our current use case in the debugger is to evaluate webpack and other bundler module dependencies so that we can show users what they variables they imported. Currently they are all getters because of how modules are specified.
Assignee: nobody → loganfsmyth
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Comment on attachment 8990435 [details]
Bug 1473996 - Expose getPropertyValue in devtools server to fully evaluate an object property.

https://reviewboard.mozilla.org/r/255512/#review263086

I have some questions but this seems legit to me.
Also, we may add a comment in the spec/actor to indicate that this is risky as it can trigger content code to run, and thus create unwanted side effects.

::: devtools/server/actors/object.js:31
(Diff revision 2)
> +const PROPERTY_VALUE_GETTERS = new WeakMap();
> +function getPropertyGetter(global) {
> +  let getter = PROPERTY_VALUE_GETTERS.get(global);
> +  if (!getter) {
> +    const debugeeGetter = global.executeInGlobal("((o, k) => o[k]);").return;
> +    getter = (obj, key) => {
> +      // eslint-disable-next-line no-useless-call
> +      return debugeeGetter.call(undefined, obj, key);
> +    };
> +    PROPERTY_VALUE_GETTERS.set(global, getter);
> +  }
> +
> +  return getter;
> +}

why isn't this a method on the proto ?

::: devtools/server/actors/object.js:31
(Diff revision 2)
>    isArray,
>    isStorage,
>    isTypedArray,
>  } = require("devtools/server/actors/object/utils");
>  
> +const PROPERTY_VALUE_GETTERS = new WeakMap();

nit: I don't think we are using all-caps for other constants than strings.
Here, if you read this from the code, it's not clear what the constant holds.
Maybe we could use something like propertyValueGettersMap ?

I don't feel too string about this, so feel free to keep it as is.

::: devtools/server/actors/object.js:33
(Diff revision 2)
> +  let getter = PROPERTY_VALUE_GETTERS.get(global);
> +  if (!getter) {

I think that we usually try to do an early return because it doesn't require you to jump to the end of an if block to see what happen if the code does not enter the if.

So here something like

```js
if (getter) {
  return getter;
}

// ...
```

::: devtools/server/actors/object.js:35
(Diff revision 2)
>  
> +const PROPERTY_VALUE_GETTERS = new WeakMap();
> +function getPropertyGetter(global) {
> +  let getter = PROPERTY_VALUE_GETTERS.get(global);
> +  if (!getter) {
> +    const debugeeGetter = global.executeInGlobal("((o, k) => o[k]);").return;

so, here I guess `o` stands for object, and `k` for key ? If so, we should directly use those names so it's more obvious

::: devtools/server/actors/object.js:525
(Diff revision 2)
> +    const value = getPropertyGetter(this.obj.global)(this.obj, name);
> +
> +    return { value: this._buildCompletion(value) };
> +  },
> +
> +  _buildCompletion(value) {

could we add some jsdoc for this function as well ?

::: devtools/server/actors/object.js:528
(Diff revision 2)
> +  },
> +
> +  _buildCompletion(value) {
> +    let completionGrip = null;
> +
> +    if (value) {

we could add a comment to explain when `value` might be falsy

::: devtools/server/tests/unit/test_objectgrips-property-value.js:12
(Diff revision 2)
> +function run_test() {
> +  run_test_with_server(DebuggerServer, function() {
> +    run_test_with_server(WorkerDebuggerServer, do_test_finished);
> +  });
> +  do_test_pending();
> +}
> +
> +function run_test_with_server(server, callback) {
> +  gCallback = callback;
> +  initTestDebuggerServer(server);
> +  gDebuggee = addTestGlobal("test-grips", server);
> +  gDebuggee.eval(function stopMe(arg1) {
> +    debugger;
> +  }.toString());
> +
> +  gClient = new DebuggerClient(server.connectPipe());
> +  gClient.connect().then(function() {
> +    attachTestTabAndResume(
> +      gClient,
> +      "test-grips",
> +      function(response, tabClient, threadClient) {
> +        gThreadClient = threadClient;
> +        test_object_grip(() => {
> +          gThreadClient.resume(function() {
> +            gClient.close().then(gCallback);
> +          });
> +        });
> +      },
> +    );
> +  });
> +}

I found those not ideal to read. Could we switch them to async/await, and avoid having global variables, but explicitely pass them to the functions who need them.

(like https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/devtools/server/tests/unit/test_objectgrips-20.js#11-20 )

::: devtools/server/tests/unit/test_objectgrips-property-value.js:44
(Diff revision 2)
> +function assert_properties(objClient, props, done, keys = Object.keys(props)) {
> +  if (keys.length === 0) {
> +    return done();
> +  }
> +
> +  const key = keys.shift();
> +  const prop = props[key];
> +
> +  return objClient.getPropertyValue(key).then(({ value }) => {
> +    if (prop && "return" in prop) {
> +      assert_value(value.return, prop.return);
> +    }
> +    if (prop && "throw" in prop) {
> +      assert_value(value.throw, prop.throw);
> +    }
> +    if (!prop) {
> +      assert_value(value, prop);
> +    }
> +
> +    assert_properties(objClient, props, done, keys);
> +  });
> +}

same here, could we use an async function an iterate over keys instead of manipulating the array being passed an doing recursion ?

::: devtools/server/tests/unit/test_objectgrips-property-value.js:70
(Diff revision 2)
> +  if (typeof expected === "object") {
> +    for (const key of Object.keys(expected)) {
> +      Assert.equal(actual[key], expected[key]);
> +    }
> +  } else {
> +    Assert.equal(actual, expected);
> +  }

I think we can use `deepEqual(actual, expected)` here

::: devtools/server/tests/unit/test_objectgrips-property-value.js:117
(Diff revision 2)
> +          return: {
> +            type: "object",
> +            class: "Function",
> +            name: "method",
> +          },

why do we return this here ?

::: devtools/server/tests/unit/test_objectgrips-property-value.js:131
(Diff revision 2)
> +      get stringNormal(){
> +        return "a value";
> +      },

could we add other test case for getter returning: booleans (true and false), numbers (including 0, -Infinity, …), empty string, undefined, null, the result of a property which is also a getter, the result of a property which is a getter and throws, ...

any case you can think of that might go wrong in some way :)
Comment on attachment 8990436 [details]
Bug 1473996 - Expose fn.apply in the devtools server.

https://reviewboard.mozilla.org/r/255514/#review263092

Thanks Logan for those patches and their tests.
I have some questions here, and it would be great if the test could be modernized for readibility sake.

::: devtools/server/actors/object.js:536
(Diff revision 2)
> +   * @param {Array<any>} args
> +   *        The array of un-decoded actor objects, or primitives.
> +   */
> +  apply: function(context, args) {
> +    const debugeeContext = this._getDebuggeeValue(context);
> +    const debugeeArgs = (args || []).map(arg => this._getDebuggeeValue(arg));

maybe we could default to empty array in the function parameter (`apply: function(context, args = []) {`) so this is a bit simpler to read.

Also, once done, we can do `const debugeeArgs = args.map(this._getDebugeeValue, this)`

::: devtools/server/actors/object.js:538
(Diff revision 2)
> +   */
> +  apply: function(context, args) {
> +    const debugeeContext = this._getDebuggeeValue(context);
> +    const debugeeArgs = (args || []).map(arg => this._getDebuggeeValue(arg));
> +
> +    const value = this.obj.apply(debugeeContext, debugeeArgs);

what if this.obj isn't a function ? Should we make sure to handle this ?

::: devtools/server/actors/object.js:544
(Diff revision 2)
> +
> +    return { value: this._buildCompletion(value) };
> +  },
> +
> +  _getDebuggeeValue(grip) {
> +    if (grip && typeof grip === "object") {

I think we should check if grip.actor does really exist. 
Also, what if getActor(grip.actor) returns null, or throws ?

::: devtools/server/tests/unit/test_objectgrips-fn-apply.js:7
(Diff revision 2)
> +var gDebuggee;
> +var gClient;
> +var gThreadClient;
> +var gCallback;
> +
> +function run_test() {
> +  run_test_with_server(DebuggerServer, function() {
> +    run_test_with_server(WorkerDebuggerServer, do_test_finished);
> +  });
> +  do_test_pending();
> +}

nit: same comment than the previous review, ket's try to switch this test to a more modern code style using async/await and explicitly passing arguments instead of relying on globals

::: devtools/server/tests/unit/test_objectgrips-fn-apply.js:101
(Diff revision 2)
> +      obj1: {},
> +      obj2: {},
> +      context(arg) {
> +        return this === arg ? "correct context" : "wrong context";
> +      },
> +      args(...parts) {

let's call this sum so it's more obvious what we are tryign to do
Attachment #8990436 - Flags: review?(nchevobbe)
Attachment #8990435 - Flags: review?(nchevobbe)
Comment on attachment 8990435 [details]
Bug 1473996 - Expose getPropertyValue in devtools server to fully evaluate an object property.

https://reviewboard.mozilla.org/r/255512/#review263086

> nit: I don't think we are using all-caps for other constants than strings.
> Here, if you read this from the code, it's not clear what the constant holds.
> Maybe we could use something like propertyValueGettersMap ?
> 
> I don't feel too string about this, so feel free to keep it as is.

Sounds good.

> why isn't this a method on the proto ?

Yeah, I can move it. Mostly just style preference I think. I usually use standalone functions for anything that is trivially side-effect free and has minimal dependencies.

> I think that we usually try to do an early return because it doesn't require you to jump to the end of an if block to see what happen if the code does not enter the if.
> 
> So here something like
> 
> ```js
> if (getter) {
>   return getter;
> }
> 
> // ...
> ```

Will do. Just another style difference.

> I found those not ideal to read. Could we switch them to async/await, and avoid having global variables, but explicitely pass them to the functions who need them.
> 
> (like https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/devtools/server/tests/unit/test_objectgrips-20.js#11-20 )

Sounds good. I usually aim to be consistent with existing code and this is similar to the approach used in `test_objectgrips-01.js`. I didn't realize the tests were a mixture of styles already.

> why do we return this here ?

No return, just a property called `return` :)

> could we add other test case for getter returning: booleans (true and false), numbers (including 0, -Infinity, …), empty string, undefined, null, the result of a property which is also a getter, the result of a property which is a getter and throws, ...
> 
> any case you can think of that might go wrong in some way :)

Adding tests for all the primitive types seems counterproductive since then we're just testing the behavior of `createValueGrip()` rather than the logic for evaluating the properties.

> the result of a property which is also a getter

Isn't that what this test is? Not sure I follow what you have in mind here.

> the result of a property which is a getter and throws

I think the `stringAbrupt` test covers that?
Comment on attachment 8990436 [details]
Bug 1473996 - Expose fn.apply in the devtools server.

https://reviewboard.mozilla.org/r/255514/#review263092

> maybe we could default to empty array in the function parameter (`apply: function(context, args = []) {`) so this is a bit simpler to read.
> 
> Also, once done, we can do `const debugeeArgs = args.map(this._getDebugeeValue, this)`

I believe `args` in this case can be `null` so the default param wouldn't work. Also more difficult because the object client itself doesn't have a way to represent optional arguments, so users would have to explicitly pass `undefined` where `null` is probably more what they'd expect.
Comment on attachment 8990435 [details]
Bug 1473996 - Expose getPropertyValue in devtools server to fully evaluate an object property.

https://reviewboard.mozilla.org/r/255512/#review263086

> I think we can use `deepEqual(actual, expected)` here

In this case I was worried because the object has stuff like `actor: "server1.conn0.obj25"` that I wanted to avoid hard-coding that in the test, so I figured I'd skip comparing that.
@nchevobbe Would you have time next week to make another pass over this?
Comment on attachment 8990435 [details]
Bug 1473996 - Expose getPropertyValue in devtools server to fully evaluate an object property.

https://reviewboard.mozilla.org/r/255512/#review263984
Attachment #8990435 - Flags: review?(nchevobbe) → review+
Comment on attachment 8990436 [details]
Bug 1473996 - Expose fn.apply in the devtools server.

https://reviewboard.mozilla.org/r/255514/#review263986
Attachment #8990436 - Flags: review?(nchevobbe) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ba8a6f1857d
Expose getPropertyValue in devtools server to fully evaluate an object property. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/dc601e6050f6
Expose fn.apply in the devtools server. r=nchevobbe
Flags: needinfo?(pbrosset) → needinfo?(nchevobbe)
Logan, what do you need to be unblocked here ?
Flags: needinfo?(nchevobbe) → needinfo?(loganfsmyth)
The failure that led to the backout were caused by Bug 1468536, where DebuggerObject.global was removed (which was used in this patch).
Depends on: 888390
Comment on attachment 9011629 [details]
Bug 1473996 - Expose getPropertyValue in devtools server to fully evaluate an object property. r?nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.
Attachment #9011629 - Flags: review+
Comment on attachment 9011630 [details]
Bug 1473996 - Expose fn.apply in the devtools server. r?nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.
Attachment #9011630 - Flags: review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df8f12fd43e7
Expose getPropertyValue in devtools server to fully evaluate an object property. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/8c9d2be6d47e
Expose fn.apply in the devtools server. r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/df8f12fd43e7
https://hg.mozilla.org/mozilla-central/rev/8c9d2be6d47e
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Flags: needinfo?(lsmyth)
You need to log in before you can comment on or make changes to this bug.