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

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P1)

Description

[Logan Smyth [:loganfsmyth]]

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.

Comment 1

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1473996 - Expose getPropertyValue in devtools server to fully evaluate an object property.

Review commit: https://reviewboard.mozilla.org/r/255512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255512/

Comment 2

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1473996 - Expose fn.apply in the devtools server.

Review commit: https://reviewboard.mozilla.org/r/255514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255514/

Comment 3

[Jason Laster [:jlast]]

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.

Comment 4

[Logan Smyth [:loganfsmyth]]

Comment on attachment 8990435 [details]
Bug 1473996 - Expose getPropertyValue in devtools server to fully evaluate an object property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255512/diff/1-2/

Comment 5

[Logan Smyth [:loganfsmyth]]

Comment on attachment 8990436 [details]
Bug 1473996 - Expose fn.apply in the devtools server.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255514/diff/1-2/

Comment 6

[Nicolas Chevobbe [: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

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 7

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 8

[Logan Smyth [:loganfsmyth]]

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 9

[Logan Smyth [:loganfsmyth]]

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 10

[Logan Smyth [:loganfsmyth]]

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.

Comment 11

[Logan Smyth [:loganfsmyth]]

Comment on attachment 8990435 [details]
Bug 1473996 - Expose getPropertyValue in devtools server to fully evaluate an object property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255512/diff/2-3/

Comment 12

[Logan Smyth [:loganfsmyth]]

Comment on attachment 8990436 [details]
Bug 1473996 - Expose fn.apply in the devtools server.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255514/diff/2-3/

Comment 13

[Logan Smyth [:loganfsmyth]]

@nchevobbe Would you have time next week to make another pass over this?

Comment 14

[Nicolas Chevobbe [: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/#review263984

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8990436 [details]
Bug 1473996 - Expose fn.apply in the devtools server.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (bug 1473996) for failures in devtools/server/tests/unit/test_objectgrips-fn-apply.js on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=dc601e6050f67f30980aabbf13e179790f4a4485&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=188694980

Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=188694980&repo=autoland&lineNumber=2093

Backout: https://hg.mozilla.org/integration/autoland/rev/be97edb6e793a5746b3c18fda6bc062255482e36

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Logan, what do you need to be unblocked here ?

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

The failure that led to the backout were caused by Bug 1468536, where DebuggerObject.global was removed (which was used in this patch).

Comment 20

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1473996 - Expose getPropertyValue in devtools server to fully evaluate an object property. r?nchevobbe

Comment 21

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1473996 - Expose fn.apply in the devtools server. r?nchevobbe

Depends on D6722

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

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.

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9011630 [details]
Bug 1473996 - Expose fn.apply in the devtools server. r?nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/df8f12fd43e7
https://hg.mozilla.org/mozilla-central/rev/8c9d2be6d47e