856232 - initialize globalActorFactories upon declaring DebuggerServer

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: initializes globalActorFactories etc. immediately

DebuggerServer delays initialization of globalActorFactories, but that is unnecessary, as DebuggerServer could simply initialize it to a new empty object upon declaring it.  (That wouldn't be true if DebuggerServer were a prototype for multiple instances, since then they would share that object, which is probably not what we would want.  But it's a singleton, so there's no such issue.)

And doing so would make the API safer and easier to use.  Currently, one must be careful to call init() before calling addGlobalActor(), because the first init() call discards any actors already added by addGlobalActor().  This change would remove that dangerous limitation and make it possible to call addGlobalActor() anytime after importing dbg-server.jsm.  (Although calling destroy() will still discard actors; but that's a footgun for another bug).

Here's a patch that should do the needful.  I'm not requesting review yet, as I haven't tested it sufficiently.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: lets addons add DebuggerServer actors anytime

Here's a patch with tests.

I also ran the existing xpcshell tests in toolkit/devtools/debugger/tests/unit, the mochitest-chrome tests in toolkit/devtools/debugger/tests/mochitest, and the mochitest-browser tests in browser/devtools/debugger/test/ and browser/devtools/profiler/test/, which all passed.

And I ran the xpcshell tests against a B2G build, where four tests failed with or without the change.


This change seems necessary to make it possible for addons to register actors without having to know when DebuggerServer is initialized.  But I don't think it's sufficient to remove all the DebuggerServer hacks in the Simulator.  It looks like we'll need to do more work for that.

Unfortunately, the Simulator doesn't currently work with a B2G build from mozilla-central, which makes it hard to figure out exactly what additional work is required.


Aside: DebuggerServer.destroy() is a footgun, since it'll remove addon actors without telling them nor readding them when the DebuggerServer is reinited.  It also doesn't seem to be used anywhere in production code, just in two test files:

  http://mxr.mozilla.org/mozilla-central/search?string=debuggerserver.destroy

So it isn't clear why it exists, when to use it, and how addons should interact with it.

If it's important to be able to initialize/destroy/reinitialize DebuggerServer independently from the loading/unloading of its component, and if we want addons to be able to extend DebuggerServer, then we'll need to clear that up.

Comment 2

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

Comment on attachment 744314 [details] [diff] [review]
patch v1: lets addons add DebuggerServer actors anytime

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

Looks good to me, but I know that Dave has been touching this code in his devtools loader work, which I haven't looked at yet, and might be changing the singleton status of the debugger server.

::: toolkit/devtools/debugger/tests/unit/test_add_actors.js
@@ +3,5 @@
> +
> +var gClient;
> +var gActors;
> +
> +function run_test()

Could you add a short comment about the purpose of the test?

Comment 3

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #1)
> Aside: DebuggerServer.destroy() is a footgun, since it'll remove addon
> actors without telling them nor readding them when the DebuggerServer is
> reinited.  It also doesn't seem to be used anywhere in production code, just
> in two test files:
> 
>   http://mxr.mozilla.org/mozilla-central/search?string=debuggerserver.destroy
> 
> So it isn't clear why it exists, when to use it, and how addons should
> interact with it.
> 
> If it's important to be able to initialize/destroy/reinitialize
> DebuggerServer independently from the loading/unloading of its component,
> and if we want addons to be able to extend DebuggerServer, then we'll need
> to clear that up.

DebuggerServer.destroy() was added in order to avoid memory leaks in tests, because letting the server clean up on its own didn't prove to be fast enough for the way our test harness shuts down. Normal browser usage never produced such leaks, so we didn't need to call it from production code.

After the server is destroyed, the onus is on the entity that controls the server's lifecycle to reinitialize it, adding any actors as necessary. Any client connections should have been already closed before destroy() was called, triggering the actors' disconnect() methods to perform any necessary cleanup.

Please let me know if there is a use case that is not covered by the above workflow.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: add comment explaining reason for tests

(In reply to Panos Astithas [:past] from comment #2)
> Looks good to me, but I know that Dave has been touching this code in his
> devtools loader work, which I haven't looked at yet, and might be changing
> the singleton status of the debugger server.

In that case we'll want to do something different.  But then perhaps initialization can happen during construction, and DebuggerServer can notify observers of initialization so they can add actors at that time.


> Could you add a short comment about the purpose of the test?

Yes, I added this comment:

/**
 * The purpose of these tests is to verify that it's possible to add actors
 * both before and after the DebuggerServer has been initialized, so addons
 * that add actors don't have to poll the object for its initialization state
 * in order to add actors after initialization but rather can add actors anytime
 * regardless of the object's state.
 */


(In reply to Panos Astithas [:past] from comment #3)
> DebuggerServer.destroy() was added in order to avoid memory leaks in tests,
> because letting the server clean up on its own didn't prove to be fast
> enough for the way our test harness shuts down. Normal browser usage never
> produced such leaks, so we didn't need to call it from production code.

In that case, it seems like we should fence it off from use by production code.  Ideally, we would hide it completely, so production code couldn't call it, although I'm not sure how to do that.

But at the very least we should add a comment to the interface description explaining that production code shouldn't call that method, because the method is intended to be used only by tests to suppress false positives.


> After the server is destroyed, the onus is on the entity that controls the
> server's lifecycle to reinitialize it, adding any actors as necessary. Any
> client connections should have been already closed before destroy() was
> called, triggering the actors' disconnect() methods to perform any necessary
> cleanup.
> 
> Please let me know if there is a use case that is not covered by the above
> workflow.

If an addon adds an actor to the server, then it is necessary for the "entity that controls the server's lifecycle" to readd the actor after reinitializing the server; otherwise, reinitialization will break addons.

But if that entity is responsible for doing that, then it has to maintain a reference to the addon's actor after destroying the server in order to readd the actor after reinitializing the server.  Which seems likely to defeat the purpose of destroying the server (removing references to it, so it can be garbage-collected).

This doesn't particularly matter if "destroy" is only to work around test problems.  But it will matter if we ever use it in production code.  And it seems brittle.


If we really expect the server to be inited and destroyed multiple times over the course of an application session, or we're adding support for multiple servers, then I would make it easier for addons to observe the server state and react accordingly, something like:

1. When the server is initialized, it notifies observers, who can then add actors.
2. When the server is destroyed, the actors' disconnect() methods are triggered to perform any necessary cleanup (as is currently the case).
3. When the server is reinitialized, it again notifies observers, who can then readd actors.

Then an addon can always ensure its actors are added by doing two things:

1. On addon startup, if the server is already initialized, add actors (covers the case of restartless addons that get installed/enabled in the middle of an application session).
2. On observation of "server initialized", add actors (covers the case of server not yet initialized when addon started on application startup).

Comment 5

[Dave Camp (:dcamp)]

Comment on attachment 745263 [details] [diff] [review]
patch v2: add comment explaining reason for tests

I have a patch I'm working on that gets rid of the "initialized" state altogether.  Configuring the server (such as by registering actors etc) can happen anytime, and any heavyweight initialization should just be done the first time a client connects.

That patch isn't already landed, so landing this won't hurt or conflict.

As for destroying/restarting: my patch moves the server to a jetpack loader.  We'll be able to implement destroy/re-init by killing the compartments in the old loader and starting a new one, re-importing the actor URIs into the new loader.  That should just mean that from the perspective of a given actor global, its DebuggerServer will always last as long as the actor global does. So no complicated observer handling, a simple addTabActor() will work and get cleaned up correctly.

Seem reasonable?

r+, but this line:

+  DebuggerServer.addActors("resource://test/testactors.js");

is unnecessary if you're using the real browser actors.

Comment 6

[Dave Camp (:dcamp)]

In case you're curious, my patch moves the allowConnection callback from init() to openListener().

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: removes unnecessary testactors from 'add actors' test

(In reply to Dave Camp (:dcamp) from comment #5)
> I have a patch I'm working on that gets rid of the "initialized" state
> altogether.  Configuring the server (such as by registering actors etc) can
> happen anytime, and any heavyweight initialization should just be done the
> first time a client connects.
> 
> That patch isn't already landed, so landing this won't hurt or conflict.

Ok, then I'm going to land this patch, although it's unclear whether it'll still be needed after your work lands.


> As for destroying/restarting: my patch moves the server to a jetpack loader.
> We'll be able to implement destroy/re-init by killing the compartments in
> the old loader and starting a new one, re-importing the actor URIs into the
> new loader.  That should just mean that from the perspective of a given
> actor global, its DebuggerServer will always last as long as the actor
> global does. So no complicated observer handling, a simple addTabActor()
> will work and get cleaned up correctly.
> 
> Seem reasonable?

It seems very reasonable!


> r+, but this line:
> 
> +  DebuggerServer.addActors("resource://test/testactors.js");
> 
> is unnecessary if you're using the real browser actors.

Ah, indeed, removed!


Carrying forward reviews.  This is the patch I'll land if the tryserver run looks good:

https://tbpl.mozilla.org/?tree=Try&rev=b400575edd35

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 745336 [details] [diff] [review]
patch v3: removes unnecessary testactors from 'add actors' test

https://hg.mozilla.org/integration/mozilla-inbound/rev/e65f2839b0a2

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e65f2839b0a2