877295 - Allow actors to be loaded as addon-sdk modules

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Dave Camp (:dcamp)]

Attachment: v1, no tests

In advance of the whole debugger server loading with the jetpack loader, it should be easy to allow specific modules to be loaded using the loader, which will let us land inspector work separately from the push to the jetpack loader on the debugger server.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 755527 [details] [diff] [review]
v1, no tests

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

::: toolkit/devtools/server/main.js
@@ +193,5 @@
> +      return;
> +    }
> +    let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +    let mod = devtools.require(id);
> +    mod.register();

I understand we can't do

  this.createRootActor = devtools.require(id).createRootActor;

because some modules just define actors but not a root actor, but I don't get why we are relying on implicit state changes by just calling

  mod.register()

Couldn't we make this more explicit by having like a shared namespace for actors and doing something like

  mod.register(actors)

and let the module define it's actors onto the shared namespace?

I don't know, maybe I am being nitpick-y but something about this implicit-ness is really rubbing me the wrong way.

Comment 2

[Dave Camp (:dcamp)]

Depends on bug 877413 for testing so that we can register resource://test/ urls with our loader.

Comment 3

[Dave Camp (:dcamp)]

These modules have full access to the server API.  As long as we load modules we're open to side effects.  A required register() and unregister() pair at least makes it clear when we expect those side effects to happen.

There are some commonly-expected side effects, particularly registration of actors.  We could maybe do more to show that this is a commonly-expected side effect by having the explicit shared namespace.  But that won't prevent side effects, just give a false sense that there are none.

Comment 4

[Dave Camp (:dcamp)]

But it might be nice to pass in the extension API object (DebuggerServer) rather than making each imported actor reimport the Debugger server.  Then down the road it would be easy to trim that object down if it's helpful.

Comment 5

[Dave Camp (:dcamp)]

Attachment: WIP 2

This version passes in a module-unique extension API object to each module's register/unregister functions.  The module should use this method to add and remove actors.  If they do so, modules that still exist after unregister() can be cleaned up by the server.

For now you can also manually add via DebuggerServer, but we could remove that once everything's modularized if we want.

Comment 6

[Dave Camp (:dcamp)]

Attachment: WIP 2.1

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I'm so happy (๑╹ڡ╹)╭ ～ ♡

Comment 8

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

Comment on attachment 755645 [details] [diff] [review]
WIP 2.1

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

::: toolkit/devtools/server/main.js
@@ +239,5 @@
> +   */
> +  unregisterModule: function DS_unregisterModule(id) {
> +    let mod = gRegisteredModules[id];
> +    if (!mod) {
> +      throw new Error("Tried to unregister a module that was not previously registered.");

If we throw when unregistering a non-existent module then maybe we should also throw when trying to register another module with the same id? If we prefer to be lenient then maybe not throw in either case?

If being strict doesn't cause problems with updating add-ons that register modules, then I vote for throwing in both cases. Or maybe abide by Postel's law and log a warning (so the dev can find out why it didn't work) instead of throwing.

Comment 9

[Dave Camp (:dcamp)]

Attachment: WIP 3

Yeah, let's throw and see if it causes problems.

Comment 10

[Dave Camp (:dcamp)]

Attachment: v1

Comment 11

[Joe Walker [:jwalker]]

Attachment: v1.00001

Rebase. Minor change to xpcshell.ini
Not obsoleting v1 because dcamp should see what's going on.

Comment 12

[Joe Walker [:jwalker]]

fwiw, the interdiff looks sane:

diff attachment.cgi\?id=759486 .hg/patches/dcamp-877295.patch
2c2
< # Parent e32eee2ed2f965fcfacaad6518c27586520a8dcb
---
> # Parent 855a29c9dd686ddeb5fdb485a24ca975589d445e
101c101
< @@ -155,16 +207,23 @@ var DebuggerServer = {
---
> @@ -154,16 +206,23 @@ var DebuggerServer = {
125c125
< @@ -177,16 +236,55 @@ var DebuggerServer = {
---
> @@ -176,16 +235,55 @@ var DebuggerServer = {
270c270,273
< @@ -35,16 +35,17 @@ reason = bug 821285
---
> @@ -32,16 +32,17 @@ reason = bug 821285
>  [test_frameclient-02.js]
>  [test_nativewrappers.js]
>  [test_eval-01.js]
275,277d277
<  [test_protocol_simple.js]
<  [test_protocol_longstring.js]
<  [test_protocol_children.js]

Comment 13

[Joe Walker [:jwalker]]

Scratch that - the problem isn't bitrot, it's patch application order.
Adding dependency on bug 866305, which is only a 'to cleanly apply xpcshell.ini' dependency.

Patch queues are 1D where dependency trees are 2D.

Comment 14

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/21075f02a219
https://hg.mozilla.org/integration/fx-team/rev/5c76c23df424

Comment 15

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/5c76c23df424 was from a different bug, will be backing that out and relanding with the right commit message.

Comment 16

[Dave Camp (:dcamp)]

Backed out the bad commit message in https://hg.mozilla.org/integration/fx-team/rev/deff9b455b24

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21075f02a219

Comment 18

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> I'm so happy (๑╹ڡ╹)╭ ～ ♡

Yeah, I like the new system too.

Comment 19

[Jim Blandy :jimb]

Comment on attachment 759486 [details] [diff] [review]
v1

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

::: toolkit/devtools/server/main.js
@@ +61,5 @@
> + * destroyed.
> + */
> +function ModuleAPI() {
> +  let activeTabActors = new Set();
> +  let activeGlobalActors = new Set();

This is great; I like the idea of having an object whose scope is exactly the lifetime of these extensions.

But: will the same actor factory function ever get passed to a ModuleAPI's addTabActor or addGlobalActor method twice? Then the first 'destroy' will remove it. I guess the checks for duplicate module ids should cover this.

It might be nice to let ModuleAPI instances *own* the factories, and just have DebuggerServer maintain a set of ModuleAPI instances. But that would require a bunch of work on CommonCreateExtraActors etc; probably a pain.

Separately: I don't really care, but aside from avoiding 'this.' everywhere, is there a reason not to make this a normal constructor, with methods on its prototypes instead of its instances? It's what people expect to see; less importantly, it's fewer objects allocated (one instance pointing to a shared prototype, versus one new closure per method per instance).