1037992 - [meta] Refactor the thread actor to use protocol.js

Status: NEW

(DevTools :: Framework, task, P5)

Description

[Eddy Bruel [:ejpbruel]]

A long term goal for the devtools team is to refactor all debugger actors to use protocol.js. The script actors are the most prominent amongst those, so they seem like a good place to start.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor ThreadActor to use protocol.js

This patch refactors ThreadActor and its specializations, ChromeDebuggerActor and AddonDebuggerActor, to use protocol.js. It's not perfect yet (we could do better than simply returning a json packet as the return value for each request, for instance), but that's something we can address in a followup patch. The immediate goal here is to rewrite the actors in terms of ActorClass.

Protocol.js expects method names to match the type field of their corresponding request packet. Because of this, I had to rename the sources property to threadSources.

The attach and interrupt methods need some special handling because they enter a nested event loop. They need to send their response before the event loop is entered, and consequently, some way to inform protocol.js that a response doesn't need to be sent when the handler returns. For this purpose, I've added a new dummy type, NoResponseNeeded.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

We actually had bug 698841 for this.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8455050 [details] [diff] [review]
Refactor ThreadActor to use protocol.js

Review of attachment 8455050 [details] [diff] [review]:
-----------------------------------------------------------------

This is a surprisingly small patch and it results in unsurprisingly cleaner code. But what about converting dbg-client.jsm to use actor fronts? That could be an even bigger win in terms of simplification and would add a sanity check to these changes.

::: toolkit/devtools/server/actors/script.js
@@ +491,5 @@
>   */
> +types.addDictType("location", {
> +  url: "string",
> +  line: "number",
> +  column: "nullable:number",

Why is addDictType necessary here? Since the location properties are all primitives it seems that "json" would be the type we need here.

Also, the comment above applies to the ThreadActor below.

@@ +783,5 @@
>      }
>  
>      this._state = "attached";
>  
> +    update(this._options, options || {});

The "|| {}" part shouldn't be necessary with the argument's default value, right?

@@ +828,5 @@
> +    request: {
> +      options: {
> +        autoBlackBox: Option(0, "boolean"),
> +        ignoreCaughtExceptions: Option(0, "boolean"),
> +        pauseOnDOMEvents: Option(0, "boolean"),

pauseOnDOMEvents is not a boolean, its value will be an array or a string.

@@ +853,5 @@
>      if (this.state == "exited") {
>        return { error: "wrongState" };
>      }
>  
> +    update(this._options, options || {});

How about we use {} as the default options value instead?

@@ +863,5 @@
> +    request: {
> +      options: {
> +        autoBlackBox: Option(0, "boolean"),
> +        ignoreCaughtExceptions: Option(0, "boolean"),
> +        pauseOnDOMEvents: Option(0, "boolean"),

Same here: pauseOnDOMEvents is not a boolean, its value will be an array or a string.

@@ +1197,5 @@
> +      ignoreCaughtExceptions: Option(0, "boolean"),
> +      pauseOnExceptions: Option(0, "boolean"),
> +      pauseOnDOMEvents: Option(0),
> +      resumeLimit: Option(0),
> +    },

This doesn't look like it's buying us much compared to the simpler: Arg(0, "json").

@@ +1524,5 @@
> +      condition: Arg(1, "nullable:string"),
> +    },
> +    response: RetVal(types.addDictType("setBreakpointResponse", {
> +      actualLocation: "location",
> +    })),

This too doesn't seem much better than RetVal("json").

@@ +4832,5 @@
>   *        An object with preNest and postNest methods for calling when entering
>   *        and exiting a nested event loop.
>   */
> +const ChromeDebuggerActor = Class({
> +  extends: ThreadActor,

Hmm, doesn't extending via Class() result in reusing the typeName of the parent? I can't find this pattern anywhere in our code. Won't that be a problem if/when we try to serialize these actors?

Perhaps this is the kind of problem that will appear when the actor fronts are implemented.

@@ +5663,5 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2; js-indent-level: 2 -*- */
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Er, what is that?

::: toolkit/devtools/server/protocol.js
@@ +950,5 @@
>          let args = spec.request.read(packet, this);
>  
>          let ret = this[spec.name].apply(this, args);
>  
> +        if (spec.oneway || (ret instanceof NoResponseNeeded)) {

Nit: instanceof has a higher precedence than || so the parens are unnecessary.

Comment 5

[Eddy Bruel [:ejpbruel]]

Attachment: Implement ProtocolError.

As far as I can tell, protocol.js does not offer a way to send back custom error packets to the client, something that the script actors do quite often.

It looks like it would be easy to tweak protocol.js to support this, by implementing a custom error class. Here's a patch that does just that.

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Change EnvironmentActor to protocol.js

I'm still a bit unfamiliar with protocol.js, so I have a quick question.

EnvironmentActor supports a bindings request which returns a list of bindings for that environment. The response is essentially a JSON object, but some fields in it are value grips, and therefore might be actors.

Since the structure of the bindings object can't be known ahead of time, the best we can do here is to make the response RetVal("json"), right? That is, we can't really make a special type for this.

Comment 7

[Eddy Bruel [:ejpbruel]]

I have more patches coming up, but it looks like jryans just landed his mass move of the devtools code, so I'm going to have to do some rebasing tomorrow morning before filing the rest.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8664243 [details] [diff] [review]
Change EnvironmentActor to protocol.js

Review of attachment 8664243 [details] [diff] [review]:
-----------------------------------------------------------------

I think a JSON value should work, which you can easily test once the patch is testable. Another approach in similar cases is to define a new type for this particular response, if you need special marshaling. Here is such a case from Valence:

https://github.com/mozilla/valence/blob/master/lib/chromium/value.js#L26

You can see the chromium_grip type being used in the webconsole, for example:

https://github.com/mozilla/valence/blob/master/lib/chromium/webconsole.js#L12

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: Implement ProtocolError.

Fixed a minor bug in this patch.

Comment 10

[Eddy Bruel [:ejpbruel]]

Attachment: Change EnvironmentActor to protocol.js

A couple of notes on the upcoming patches:
- Some of the script actors do some funky stuff (such as ThreadActor manually   
  sending a response packet before entering a nested event loop), so the plan is 
  to attack these actors one at a time.
- I'm not going to refactor the client fronts yet: doing so would interfere with 
  jlongster's refactor of the frontend. I will file a followup for refactoring   
  the fronts once this bug is fixed.

Comment 11

[Eddy Bruel [:ejpbruel]]

Attachment: Change BreakpointActor to protocol.js

Comment 12

[Eddy Bruel [:ejpbruel]]

Attachment: Change FrameActor to protocol.js

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8664980 [details] [diff] [review]
Change EnvironmentActor to protocol.js

Review of attachment 8664980 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/script.js
@@ +3416,5 @@
>   *        The lexical environment that will be used to create the actor.
>   * @param ThreadActor aThreadActor
>   *        The parent thread actor that contains this environment.
>   */
> +let EnvironmentActor = ActorClass({

I guess these actor will manage their lifetimes outside of p.js's usual flow?  I see this one is added a to a pool, for example.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8664982 [details] [diff] [review]
Change BreakpointActor to protocol.js

Review of attachment 8664982 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/script.js
@@ +3255,2 @@
>  
> +  initialize: function (aThreadActor, aOriginalLocation) {

You could convert away from `aArg` style to `arg` if you wanted...

@@ +3392,5 @@
> +  delete: method(function () {
> +    return this._delete();
> +  }),
> +
> +  _delete: function () {

You don't need 2 separate functions.  It's safe for an actor to call a function defined via p.js `method(...)` directly, because `method(...)` returns the function itself, which will be available on the actor object like normal.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8664978 [details] [diff] [review]
Implement ProtocolError.

Review of attachment 8664978 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/protocol.js
@@ +907,5 @@
>      console.error(err);
>      if (err.stack) {
>        dump(err.stack);
>      }
> +    if (err instanceof ProtocolError) {

Please add a test of this new protocol.js behavior.  See devtools/server/tests/unit/test_protocol_*.

@@ +1438,5 @@
> + *   Optional. Other fields to be included in the error packet.
> + * @constructor
> + */
> +var ProtocolError = Class({
> +  initialize: function (error, message = "", packet = {}) {

Hmm, any reason to prefer this style over just accepting an object for the whole thing including error and message?

Does it make sense to extend from the Error object?

Comment 16

[Lin Clark [:linclark]]

I'm splitting this out into separate issues and turning this into a META. I've moved the attached patches to:

- bug 1235370
- bug 1235371
- bug 1235374
- bug 1235375

Please add yourself to the CC list to those issues if you want to follow their progress.

Comment 17

[Lin Clark [:linclark]]

Un-assigning since I'm focusing on web console atm.

Comment 18

[Alexandre Poirot [:ochameau]]

ThreadActor is the last actor to not be using protocol.js
A proof of that is this usage of requestTypes:
https://searchfox.org/mozilla-central/rev/c61720a7d0c094d772059f9d6a7844eb7619f107/devtools/server/actors/thread.js#2245-2256
And many usages of .form() in the actor, which means that we are not using protocol.js automatic marshalling.