Debugger Protocol needs a generic "observer notified" unsolicited notification

RESOLVED FIXED in Firefox 33

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jimb, Assigned: ochameau)

Tracking

Trunk
Firefox 33
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

6 years ago
The UnsolicitedNotifications table in toolkit/devtools/debugger/dbg-client.jsm needs a generic "observer was notified" packet. We'll be adding packets to report notifications to various kinds of observers in the near future, and it's dumb to have to extend that table for every one --- but since the table affects the behavior of DebuggerClient.prototype.onPacket, whatever type we do use must appear there.

Updated

6 years ago
Blocks: 758697
For the record, bug 793672 has a generic'ish implementation of observer notifications through rdp, at the profiler actor level. That'll be enough for our immediate needs, so we don't really depend on an actual proper solution for the profiler addon.
I think we just want to grab the implementation from bug 793672 and move it to somewhere reusable from other actors, like the web console's. I'm thinking about adding helper methods like DebuggerServer.addObserver(topic, actor) and DebuggerClient.addObserver(topic, handler).
Depends on: 793672
No longer depends on: 798047
Assignee: nobody → past
Status: NEW → ASSIGNED
Priority: -- → P2
I'm not actually working on this.
Assignee: past → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3

Updated

4 years ago
Flags: firefox-backlog-
(Assignee)

Comment 4

4 years ago
Created attachment 8441317 [details] [diff] [review]
patch

I understood the original bug report differently.
I'm not sure the original issue was about nsIObserverService,
but more about UnsolicitedNotifications we keep modifying.
Is is painful as each time we add something in this list,
we have to update all clients (think about nodejs, python clients).

So given that, here is a patch to introduce a generic way to send event from the actors to the clients.
It patches monitor actor, which isn't landed yet to see how we could use this API.

The actor would do:

  connection.sendActorEvent(actorID, "event-name", eventObject);

And the client would listen to such event like this:

  let actorClient = function MyClient() {
    EventEmitter.decorate(this);
  };
  actorClient.on("event-name", function (type, eventObject) {
    // type = "event-name"

  });
  client.registerActorClient(actorID, actorClient);

I tried to come up with something that matches dbg-client.jsm practices,
but I'm open to any kind of API that would prevent having to tweak UnsolicitedNotifications
for new events.
Attachment #8441317 - Flags: feedback?(past)
Comment on attachment 8441317 [details] [diff] [review]
patch

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

I think this approach could work too, assuming we don't have an actual need to send observer notifications across the protocol without any payload munging by the actor. Looking through the existing uses of observer notifications in our actors, it appears to be the case. This is one more case where the YAGNI principle proves its worth :-)

Since this functionality is only for old-style actors we could just decorate the existing clients and not require an additional front object, right?

Also, could we use a more descriptive name, like perhaps DebuggerClient.registerEventListener or addEventObserver? registerActorClient sounds too generic for what it actually accomplishes and additionally conflates actors and clients. And one final note: I think we can just use the client object as the single argument to the registration method, as the actor ID can be retrieved from actorClient.actor.
Attachment #8441317 - Flags: feedback?(past) → feedback+
(Assignee)

Comment 6

4 years ago
(In reply to Panos Astithas [:past] from comment #5)
> Since this functionality is only for old-style actors we could just decorate
> the existing clients and not require an additional front object, right?

Client... front... For me, it is two different implementations of the same thing?
But today, fronts take several more hundreds of kB in memory (because of its dependencies and its big module) :/

But yes, we just need a listener object, it can be a client, a front or any arbitraty object with an `emit` method.
Sorry for adding to the confusion, what I meant by "an additional front object" was having an extra MyThreadClient in addition to ThreadClient, for example, but as you say this is not the case.
(Assignee)

Comment 8

4 years ago
Created attachment 8442890 [details] [diff] [review]
patch registerEventListener

I tried to addressed your comments in this patch,
but while modifying it, it appeared to me that we can
make an API that would allow using this new method
with existing Client/events, without changing the protocol behavior
(i.e. keep compatibility between older/newer geckos)
Attachment #8441317 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Created attachment 8442895 [details] [diff] [review]
patch registerClient

So in this patch, you register clients to the DebuggerClient.
In this patch it is only meant to forward events to clients,
but I've seen that keeping client references is something we already do manually
with _tabClients, _addonClients, ...
I can add _monitorClients and _webappsClients,
but that would be handy to have a generic way to do that.
Then I can call `close` method of these clients on detach.

Otherwise, this patch allows to stop using UnsolicitedNotifications.
Instead we declare events on clients.
  function MyClient(form) {
    this.actor = form.myActor;
    this.events = ["event-name"];
    EventEmitter.decorate(this);
    ...
    this.on("event-name", function (type, eventObject) {
      // type = "event-name"

    });
  };
  let client = new MyClient(form);
  client.registerClient(client);

In french we say 'one stone two hits',
but it looks like in english it is 'one stone two birds'.
I don't want to hit birds, but this patch seems to embrace the existing clients.
It should allow us switching existing client to that API,
without changing the protocol behavior at all \o/
Attachment #8442895 - Flags: feedback?(past)
Comment on attachment 8442895 [details] [diff] [review]
patch registerClient

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

Yep, still like it.

::: toolkit/devtools/client/dbg-client.jsm
@@ +265,5 @@
>  
>    this._pendingRequests = [];
>    this._activeRequests = new Map;
>    this._eventsEnabled = true;
> +  this._clients = {};

Better use a Map for consistency, and I also think "_clients" is too broad for a file that already has half a dozen other "_fooClients" properties. It kind of implies that it's some sort of superset of those. How about "_eventClients"?

@@ +874,5 @@
> +        if (clients.length > 0) {
> +          clients.forEach(function (client) {
> +            client.emit(type, aPacket);
> +          });
> +          // we ignore the rest, as the client expected to handle this packet.

Typo: "...the client is expected..."

@@ +983,5 @@
>    onClosed: function (aStatus) {
>      this.notify("closed");
>    },
>  
> +  registerClient: function (client) {

Same naming comment here, registerEventListener sounds better to me.

@@ +991,5 @@
> +                      "a Client instance with an `actor` attribute.");
> +    }
> +    if (!client.events) {
> +      throw new Error("DebuggerServer.registerClient expect " +
> +                      "a Client instance with an `events` attribute.");

If you are doing all this sanity checking, might as well check that client.events is an actual array. That sounds like an easy mistake to make.

We could also allow client.events to be a string in case a single event is observed, at the expense of a slightly more complicated onPacket implementation, but that doesn't seem like the right tradeoff to me.

@@ +1003,5 @@
> +  unregisterClient: function (client) {
> +    let actorID = client.actor;
> +    if (!actorID) {
> +      throw new Error("DebuggerServer.unregisterClient expect " +
> +                      "a Client instance with a `actor` attribute.");

Typo: "...expects a client instance with an |actor|..."
Attachment #8442895 - Flags: feedback?(past) → feedback+
(Assignee)

Comment 11

4 years ago
Created attachment 8448758 [details] [diff] [review]
patch, with tests

(In reply to Panos Astithas [:past] from comment #10)
> Comment on attachment 8442895 [details] [diff] [review]
> patch registerClient
> 
> Review of attachment 8442895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yep, still like it.
> 
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +265,5 @@
> >  
> >    this._pendingRequests = [];
> >    this._activeRequests = new Map;
> >    this._eventsEnabled = true;
> > +  this._clients = {};
> 
> Better use a Map for consistency, and I also think "_clients" is too broad
> for a file that already has half a dozen other "_fooClients" properties. It
> kind of implies that it's some sort of superset of those. How about
> "_eventClients"?

That's exactly what I had in mind: start registering clients of any kind the same way to DebuggerClient.
As far as I can tell client should be unique given an actor ID (am I right to think that?).
So there isn't much point in segregating each "client kind" in a dedicated Map.
Also, my first patch only covers events, but I had in mind to support more client features,
like calling their "detach" method when the main DebuggerClient closes.
I went ahead and added generic support of detach in this patch,
that, to highlight the value of registering all clients once for all.

Still like it ? ;)
I wont argue much if you don't, having just an helper for events is enough for me.

try: https://tbpl.mozilla.org/?tree=Try&rev=a302fb468762
Attachment #8442890 - Attachment is obsolete: true
Attachment #8442895 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8448758 - Flags: review?(past)
(Assignee)

Updated

4 years ago
Blocks: 1025804
Comment on attachment 8448758 [details] [diff] [review]
patch, with tests

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

So you want to replace the various maps with a unified one? I would support such a change in this patch, if it worked as expected.

You are correct that the client maps were created to cache clients, so that we don't create more than one for each actor, but I'm not sure they are all respect that (somewhat recent) guideline. From a quick look, a new AddonClient seems to be created for each attachAddon request, but the code may rely on an implicit assumption by the UI that it's not possible to call it twice for the same add-on. But for the purposes of your unification goal, that would be just a bug to fix at most.

The only real problem I see is that detaching is not necessarily stateless, and the current code guarantees that the child clients get closed/detached before their parents. With the detachment loop as you have modified it, clients for tab-scoped actors that use the registerClient API will have to ensure that they don't depend on their parent tab to exist while detaching, which sounds rather limiting and I'm pretty sure doesn't (or didn't until recently) happen to be the case.

So I would say, if you really want to unify the client maps, then do it in this patch (removing the old ones) and make sure that nothing breaks (make detachClients smarter or remove implicit dependencies from client.detach methods). If you'd rather just do the event client thing in this patch, then use _eventClients/registerEventListener and the rest is good to go.

::: toolkit/devtools/client/dbg-client.jsm
@@ +996,5 @@
> +                      "a client instance with an `actor` attribute.");
> +    }
> +    if (!Array.isArray(client.events)) {
> +      throw new Error("DebuggerServer.registerClient expects " +
> +                      "a client instance with an `events` attribute.");

"...an 'events' attribute that is an array."

Otherwise people will be perplexed by this message when the property is present, but of a different type.

::: toolkit/devtools/server/tests/unit/test_client.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Test DebuggerClient API

"Test the DebuggerClient.registerClient API" would be more specific and accurate.
Attachment #8448758 - Flags: review?(past)
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
(Assignee)

Comment 13

4 years ago
Created attachment 8451681 [details] [diff] [review]
patch

Ok, I tried to also refactor the existing code.

To be honest I don't quite picture the child/parent relation for clients.
The only complex relation I found is the ThreadClient that plays with
both DebuggerClient or TabClient instance.
The good point is that the map is always on the DebuggerClient.

I tried to be safe and call each client's `detach` method one by one
and starts by calling the clients being registered last.
As we call registerClient as soon as client is instanciated,
and "child clients" are created after that by calling attachXxxx,
we should be calling child detach method first.

Everything seems to still work, I especially tried to test the ThreadClient
by stressing the debugger in tab and browser toolboxes.
But a green try would be great:
  https://tbpl.mozilla.org/?tree=Try&rev=a5f833a78ff1
(Assignee)

Updated

4 years ago
Attachment #8448758 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Created attachment 8451694 [details] [diff] [review]
patch
Attachment #8451681 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8451694 - Flags: review?(past)
(Assignee)

Comment 15

4 years ago
Created attachment 8451972 [details] [diff] [review]
patch

Added calls to DebuggerClient.unregisterClient from each Client's detach method.
That to allow calling detach method individually (not just DebuggerClient.close)

https://tbpl.mozilla.org/?tree=Try&rev=9c99aaad482b
Attachment #8451694 - Attachment is obsolete: true
Attachment #8451694 - Flags: review?(past)
(Assignee)

Updated

4 years ago
Attachment #8451972 - Flags: review?(past)
Comment on attachment 8451972 [details] [diff] [review]
patch

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

I'm glad that the parent-child dependencies proved to be easy to deal with.

::: toolkit/devtools/server/actors/webbrowser.js
@@ -1381,5 @@
>      if (!this.attached) {
>        return { error: "wrongState" };
>      }
>  
> -    this._contextPool.remoteActor(this._threadActor);

Holy shit!
Attachment #8451972 - Flags: review?(past) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/59a72bc5db00
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/59a72bc5db00
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
You need to log in before you can comment on or make changes to this bug.