1450944 - Convert ObjectActor to protocol.js

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[Yulia Startsev [:yulia]]

Splitting this off from 1289193:

ObjectActor needs to be updated to protocol.js, along with a number of actors that are instantiated from ObjectActor:

* ObjectActor and related
  - ObjectActor -> From most methods of PromisesActor and WebConsoleActor
      PausedScopedObjectActor -> From most methods of EnvironmentActor, FrameActor, SourceActor and ThreadActor
  - SymbolActor -> via createValueGrip, so similar to ObjectActor and PauseScopedObjectActor
  - LongStringActor -> via createValueGrip, so similar to ObjectActor and PauseScopedObjectActor
                    -> StyleSheetActor.getText, NodeActor.getNodeValue, Device.getWallpaper/screenshotToDataURL
                    -> From many methods of WebConsoleActor, StorageActors
                
  - PropertyIteratorActor -> ObjectActor.enumProperties/enumEntries
  - SymbolIteratorActor -> ObjectActor.enumSymbols

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1450944 - Convert ObjectActor to protocol.js; .

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1450944 - Throw in actor methods instead of returning an error packet; .

Since protocol.js catches exceptions and returns an appropriate
packet as a result, we can use this instead of returning manually
an error packet.

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

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8976532 [details]
Bug 1450944 - Throw in actor methods instead of returning an error packet; .

https://reviewboard.mozilla.org/r/244638/#review250978

Thanks, that's an useful helper!
We may look into old actors already converted to protocol.js to see if we can switch to this.

::: devtools/shared/protocol.js:1034
(Diff revision 1)
>      let pending = this._pendingResponse || Promise.resolve(null);
>      let response = create(pending);
>      this._pendingResponse = response;
> +  },
> +
> +  throwError: function(error, message) {

A comment about the usefulness of this method would help.

Do you think this helper is only meant for old actors or is this something we should promote for any new actor? (I feel the "error" attribute it something only old actors cares about, the new one like inspector just throw with a message)
If you think that's useful for new actors, we should drop a note in docs.

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8976531 [details]
Bug 1450944 - Convert ObjectActor to protocol.js; .

https://reviewboard.mozilla.org/r/244636/#review250968

Thanks for looking into this!
I think you missed a point of protocol.js: "marshalling". Most of my comments are about that.

Also, could you push to DAMP? This patch may have very bad impact on performance :/
You might be blocked on bug 1462561, or may have to do some weird tricks like I did in bug 1449162's 3rd patch to accommodate protocol.js slowness. Also push the current version, as sometimes, the marshalling is a source of slowness.

::: devtools/server/actors/object.js:262
(Diff revision 1)
>     */
> -  onEnumProperties: function(request) {
> -    let actor = new PropertyIteratorActor(this, request.options);
> +  enumProperties: function(options) {
> +    let actor = new PropertyIteratorActor(this, options);
>      this.registeredPool.addActor(actor);
>      this.iterators.add(actor);
>      return { iterator: actor.form() };

Here you return PropertyIterator, not a propertyiterator.data object.

But may be that's because you are still calling `form()` method here. You should not. One of the most useful feature of protocol.js is tha automatic marshalling of actors. Thanks to types mentioned in specs, protocol.js will automagically "manage" (call currentActor.manage(returnedActor), here objectActor.manage(propertyIteratorActor)) and "marshall" (return propertyIterator.form() instead of trying to pass the actor instance that can't be serialized)

So:
1) the type in spec should be
```
  enumProperties: {
      request: {
        options: Arg(0, "nullable:object.enumProperties.Options"),
      },
      response: {
        iterator: RetVal("propertyIterator")
      }
  }
```
2) you should not call form() and do:
```
 return actor.form();
```
3) there is no more need to put these actors in registeredPool/iterators set. protocol.js will automatically manage them.

If you don't want to do that in this patch, you can let the code as is, but use this in the spec:
```
      response: RetVal("json");
```
Using "json" will just let protocol.js pass the method's returned object as-is without any marshalling/managing. It is often handy when converting to protocol.js with steps.

The same comment applies to enumEntries/enumSymbols.

::: devtools/server/actors/pause-scoped.js:14
(Diff revision 1)
>  const { ObjectActor } = require("devtools/server/actors/object");
> +const { extend } = require("devtools/shared/extend");
> +const { ActorClassWithSpec } = require("devtools/shared/protocol");
> +const { objectSpec } = require("devtools/shared/specs/object");
> +
> +const proto = extend({}, ObjectActor.prototype);

See bug 1450956 comment 4. Yulia used the same pattern and it is weird. We end up trying to inherit twice from protocoljs.Actor by doing that.
Yulia did not replied yet to this comment, but feel free to coordinate with her to come to a conclusion about this.

::: devtools/server/actors/pause-scoped.js:35
(Diff revision 1)
> - * should only be received while in the paused state.
> - *
> - * @param method Function
> - *        The function we are decorating.
> - */
> -PauseScopedActor.withPaused = function(method) {
> +   * chain when it constructs the ActorClass. For this reason we are using `extend` to
> +   * maintain the properties of ObjectActor.prototype
> +   **/
> +  const isPaused = () => this.threadActor
> +    ? this.threadActor.state === "paused"
> +    : true;

I don't quite follow this.
Can't you put `isPaused` on `proto`?
Because as-is, you will re-created the `isPaused` function on each call made to `withPaused`.

::: devtools/server/actors/pause-scoped.js:44
(Diff revision 1)
> -   * Returns the wrongState response packet for this actor.
> -   */
> -  _wrongState: function() {
>      return {
>        error: "wrongState",
>        message: this.constructor.name +

(I'm not sure if, with all these protocol.js hacks around prototypese, `constructor.name` is still going to be what we expect it to be?)

::: devtools/server/actors/pause-scoped.js:91
(Diff revision 1)
>    }),
>  });
>  
> -Object.assign(PauseScopedObjectActor.prototype.requestTypes, {
> -  "threadGrip": PauseScopedObjectActor.prototype.onThreadGrip,
> -});
> +module.exports = {
> +  PauseScopedObjectActor: ActorClassWithSpec(objectSpec, proto)
> +};

Is there any reason to use this unusual export instead  of the more common:
```
exports.PauseScopedObjectActor =  ActorClassWithSpec(objectSpec, proto);
```

::: devtools/server/actors/thread.js:996
(Diff revision 1)
>                    message: "Only thread-lifetime actors can be released." };
>          }
>          continue;
>        }
> +      if (actor.onRelease) {
> -      actor.onRelease();
> +        actor.onRelease();

Do we still get non-protocol.js actors here?
If yes, it would be worth mentioning in a comment which ones.

::: devtools/server/actors/thread.js:1411
(Diff revision 1)
> -        actor.registeredPool !== this.threadLifetimePool,
>        getGlobalDebugObject: () => this.globalDebugObject
>      });
>      pool.addActor(actor);
>      pool.objectActors.set(value, actor);
> -    return actor.grip();
> +    return actor.form();

Followup note: ThreadActor is weird as it isn't typed. Its spec is empty. But if it was, here you could type `objectGrip` with `obj` type and drop the call to `form` as well as the pool if `actor` lifetime is related to `threadActor` lifecycle. (I'm not sure it is the case)

::: devtools/shared/specs/index.js:131
(Diff revision 1)
>      types: ["domnode", "domnodelist"],
>      spec: "devtools/shared/specs/node",
>      front: "devtools/shared/fronts/node",
>    },
>    {
> +    types: ["obj"],

as it is used by some other specs, you should add "object.descriptor" in this `types` array.
`types` list all the exported types of this spec.

::: devtools/shared/specs/object.js:17
(Diff revision 1)
> +} = require("devtools/shared/protocol");
> +
> +// We need to load property and symbol iterator spec so we can use both
> +// symboliterator.data and propertyiterator.data dictTypes here.
> +require("devtools/shared/specs/property-iterator");
> +require("devtools/shared/specs/symbol-iterator");

The main goal of specs/index.js is to prevent that.
You should not require specs like this but instead put "propertyiterator.data" and "symboliterator.data" in specs/index.js's "types" arrays, like this:
```
  types: ["propertyIterator", "propertyiterator.data"],
  ...
  types: ["symbolIterator", "symboliterator.data"],
```

But. I don't see why enumProperties and enumSymbols are returning "*iterator.data". Both are returning actors. See my comment next in these methods.

::: devtools/shared/specs/object.js:33
(Diff revision 1)
> +  // Only set when `value` does not exist and there is a setter for the property.
> +  set: "nullable:json",
> +});
> +
> +types.addDictType("object.definitionSite", {
> +  source: "object.definitionSite.originalActor",

Ahah for once, protocol.js should simplify things here :)
You should be able to do:
  `source: "source",`
here, and drop the manual call to `.form()` in the implementation of definitionSite:
  source: originalLocation.originalSourceActor.form(),
=>
  source: originalLocation.originalSourceActor,

::: devtools/shared/specs/object.js:71
(Diff revision 1)
> +  function: "nullable:json",
> +  bindings: "nullable:object.bindings"
> +});
> +
> +types.addDictType("object.scope", {
> +  scope: "object.scope.data"

Same comment than `definitionSite`.
You should do that here:
`  scope: "env",`
and from scope implementation:
`scope: envActor.form()`
=>
`scope: envActor`

::: devtools/shared/specs/object.js:120
(Diff revision 1)
> +  fulfillmentStack: RetVal("array:json")
> +});
> +
> +types.addDictType("object.rejectionStack", {
> +  rejectionStack: RetVal("array:json")
> +});

If I'm following all stacks methods correctly, they are all `"array:source"`, but for that to work, you will have to remove the manual call to `.form()` on the sources actors in the implementation.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Alexandre Poirot [:ochameau] from comment #3)
> Comment on attachment 8976532 [details]
> Bug 1450944 - Throw in actor methods instead of returning an error packet; .
> 
> https://reviewboard.mozilla.org/r/244638/#review250978
> 
> Thanks, that's an useful helper!
> We may look into old actors already converted to protocol.js to see if we
> can switch to this.
> 
> ::: devtools/shared/protocol.js:1034
> (Diff revision 1)
> >      let pending = this._pendingResponse || Promise.resolve(null);
> >      let response = create(pending);
> >      this._pendingResponse = response;
> > +  },
> > +
> > +  throwError: function(error, message) {
> 
> A comment about the usefulness of this method would help.
> 
> Do you think this helper is only meant for old actors or is this something
> we should promote for any new actor? (I feel the "error" attribute it
> something only old actors cares about, the new one like inspector just throw
> with a message)
> If you think that's useful for new actors, we should drop a note in docs.

I noticed that some client code do check the error string when we expect a given type of failure to happen. For new actors, it could be useful for the same reasons.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8976531 [details]
Bug 1450944 - Convert ObjectActor to protocol.js; .

https://reviewboard.mozilla.org/r/244636/#review250968

> Is there any reason to use this unusual export instead  of the more common:
> ```
> exports.PauseScopedObjectActor =  ActorClassWithSpec(objectSpec, proto);
> ```

no, I think at some point we were returning multiple objects from here and I found it cleaner. But since we only have one now, let's keep things consistent.

> Do we still get non-protocol.js actors here?
> If yes, it would be worth mentioning in a comment which ones.

Yes, we can have object/long-string actors, which still use onRelease. I added a comment in the patch

> Followup note: ThreadActor is weird as it isn't typed. Its spec is empty. But if it was, here you could type `objectGrip` with `obj` type and drop the call to `form` as well as the pool if `actor` lifetime is related to `threadActor` lifecycle. (I'm not sure it is the case)

I'll file a follow-up for that

> as it is used by some other specs, you should add "object.descriptor" in this `types` array.
> `types` list all the exported types of this spec.

oh, I didn't understood the multi-items array in index, now it's more clear.
It feels much better indeed.

> The main goal of specs/index.js is to prevent that.
> You should not require specs like this but instead put "propertyiterator.data" and "symboliterator.data" in specs/index.js's "types" arrays, like this:
> ```
>   types: ["propertyIterator", "propertyiterator.data"],
>   ...
>   types: ["symbolIterator", "symboliterator.data"],
> ```
> 
> But. I don't see why enumProperties and enumSymbols are returning "*iterator.data". Both are returning actors. See my comment next in these methods.

yes, made the chage to the actor and spec

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Alex, do you mind if I remove the manual handling of actor pools in Bug 1408326 ?
It looks like there is some refactor to do and I'd prefer to not pile-up too much on this one.

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)
> Alex, do you mind if I remove the manual handling of actor pools in Bug
> 1408326 ?
> It looks like there is some refactor to do and I'd prefer to not pile-up too
> much on this one.

It depends on what you do.
If you are using valid actor types, protocol.js marshalling will automatically "manage" the returned actors and by doing so, register them automatically in pools.
Otherwise, if you are using "json", you will still have to call actor.grip() and put actors in custom pools.
Is this only for one actor/callsite or all the actors/callsites involved in this patch?

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8976531 [details]
Bug 1450944 - Convert ObjectActor to protocol.js; .

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

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8976532 [details]
Bug 1450944 - Throw in actor methods instead of returning an error packet; .

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

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Okay, so I tried to address all the comments made.
Although I'm not sure of the solution on PauseScopedObjectActor, at least we don't extend Actor twice now.

TRY was green with those changes https://treeherder.mozilla.org/#/jobs?repo=try&revision=b850bf43ed9bd2df81ff75320f143805cd475985&selectedJob=180897192

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8976531 [details]
Bug 1450944 - Convert ObjectActor to protocol.js; .

https://reviewboard.mozilla.org/r/244636/#review254388

Thanks Nicolas, it looks good with the couple of comments addressed.

Can you ensure DAMP do not report significant regression before landing?

::: devtools/server/actors/pause-scoped.js:20
(Diff revision 2)
> - * A base actor for any actors that should only respond receive messages in the
> - * paused state. Subclasses may expose a `threadActor` which is used to help
> - * determine when we are in a paused state. Subclasses should set their own
> - * "constructor" property if they want better error messages. You should never
> - * instantiate a PauseScopedActor directly, only through subclasses.
> - */
> + * Protocol.js expects only the prototype object, and does not maintain the prototype
> + * chain when it constructs the ActorClass. For this reason we are using `extend` to
> + * maintain the properties of ObjectActor.prototype
> + **/
> +const proto = {
> +  ...ObjectActorProto,

The issue with this is that it will break getters.
They will be executed right here once and passed as static property values to `proto`.

Now ObjectActorProto don't have any getter, but it may be safer to use `extend` here:
```
const proto = extend(ObjectActorProto, {
  typeName: "pauseobj",
  ...
```
That's what we do for tab actors:
https://searchfox.org/mozilla-central/search?q=extend(&case=false&regexp=false&path=actors%2F

::: devtools/shared/specs/object.js:37
(Diff revision 2)
> +
> +types.addDictType("object.prototypeproperties", {
> +  prototype: "object.descriptor",
> +  ownProperties: "nullable:json",
> +  ownSymbols: "nullable:json",
> +  safeGetterValues: "nullable:json",

ownSymbols could be typed more precisely. It looks like an array of object.descriptor's.

::: devtools/shared/specs/object.js:45
(Diff revision 2)
> +types.addDictType("object.prototype", {
> +  prototype: "object.descriptor",
> +});
> +
> +types.addDictType("object.property", {
> +  descriptor: RetVal("nullable:object.descriptor"),

I'm surprised this works!
`RetVal` should only be used in `response` object, not in the objects passed to `addDictType`.
Here, it should be:
```
types.addDictType("object.property", {
  descriptor: "nullable:object.descriptor",
});
```
(please fix the other places where you do that)

::: devtools/shared/specs/object.js:82
(Diff revision 2)
> +types.addDictType("object.parameterNames", {
> +  parameterNames: RetVal("nullable:array:string")
> +});
> +
> +types.addDictType("object.dependentPromises", {
> +  promises: RetVal("array:json")

Isn't "array:object.descriptor"?

::: devtools/shared/specs/promises.js:17
(Diff revision 2)
>  
>  // Teach protocol.js how to deal with legacy actor types
>  types.addType("ObjectActor", {
> -  write: actor => actor.grip(),
> +  write: actor => actor.form(),
>    read: grip => grip
>  });

Oh! I didn't knew about this code, this was a nice trick for mixing old actors into protocol.js.

But now, you should replace all "ObjectActor" by "obj" in this spec and get rid of this custom type.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

> Oh! I didn't knew about this code, this was a nice trick for mixing old actors into protocol.js.
> But now, you should replace all "ObjectActor" by "obj" in this spec and get rid of this custom type.

This seems to require more than just that from my quick tries this week, so I created Bug 1468995 to not make this bug linger on too long.

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8976531 [details]
Bug 1450944 - Convert ObjectActor to protocol.js; .

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

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8976532 [details]
Bug 1450944 - Throw in actor methods instead of returning an error packet; .

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

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Damp results looks okay to me: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=07b4ea08ee3a0f96b8e4d3e57a3df00c85ef3a34&newProject=try&newRevision=74afd091929aaa9784edc577dec32b44c90c6be8&framework=1

Comment 17

[Alexandre Poirot [:ochameau]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #16)
> Damp results looks okay to me:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=07b4ea08ee3a0f96b8e4d3e57a3df00c
> 85ef3a34&newProject=try&newRevision=74afd091929aaa9784edc577dec32b44c90c6be8&
> framework=1

Looks good to me.
There seems to be a small impact on a couple of tests still:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=07b4ea08ee3a0f96b8e4d3e57a3df00c85ef3a34&newProject=try&newRevision=74afd091929aaa9784edc577dec32b44c90c6be8&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1
1% on complicated.inspector.open and custom.jsdebugger.stepOut.
Hopefully some general protocol.js optimization will revert that small overhead.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7d0c02ef6e3
Convert ObjectActor to protocol.js; r=ochameau.
https://hg.mozilla.org/integration/autoland/rev/2b6a9750e786
Throw in actor methods instead of returning an error packet; r=ochameau.

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a7d0c02ef6e3
https://hg.mozilla.org/mozilla-central/rev/2b6a9750e786