1155168 - The Network and Console displays HTTP requests twice

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P2)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: [e-mail]

This problem happens when a custom actor is registered dynamically.

STR:
1. Install the attached extension (it registers one global actor)
2. Open the Toolbox (F12)
3. Load https://getfirebug.com/tests/head/net/601/issue601.html
4. Select the Console panel
5. Click the POST button on the page, the Console displays one request -> OK
6. Select the Network panel and go back to the Console panel
7. Click the POST button on the page, the Console displays two requests -> BUG

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1155168-1.patch

I did some analyses and the problem is caused by the fact that the WebConsoleActor is instantiated twice and so, also creating two instances of NetworkMonitor() - and these instances send two "networkEvent"'s (for one HTTP request).

The reason why the WebConsoleActor is created twice is that DebuggerServerConnection._getOrCreateActor() doesn't find the existing instance of WebConsoleActor (when requested the second time). The pool contains ObservedActorFactory instead and the actual instance created previously is lost. 

I believe that it's lost because the entire parent pool is lost - rewritten by another pool created in TabActor.form() 

See here:
https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/webbrowser.js#L864

A new instance of ActorPool() is created every time. And this method can be executed quite often e.g. when a custom actor is registered.

I am attaching a patch that shows what fixes the problem.
I don't know if this is the best way, but I've already found a problem with this code, see my comment here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1119794#c10

It's related to tab actors, but it's the same problem as with global actors I believe. Both should be fixed (my patch here does only global actors)

Alex, to reply your comment in bug 1119794:

> I think you will end up registering duplicated factories here,
> because of this check in createExtraActors (from common.js):
>     if (!aPool.has(actor.actorID)) {
>       aPool.addActor(actor);
>     }

I checked this out and there are no duplications.

In any case, there might be a better way, but we should fix it.

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Joe, this doesn't have devedition-40 flag, but I think we should fix it.
I'll let you to set the priority.

Honza

Comment 3

[Alexandre Poirot [:ochameau]]

I imagine you are using a new client in the addon?
That's the same discussion we had in another bug with :past.
I thought we would/should share the same client to avoid such issue, but he had some concerns about states store in actors.

So at the end it is "expected" that you get another webconsole client/actors.
But duplicated instances shouldn't end up with such behavior either.
The bottleneck here may be more about the NetworkMonitor,
may be that's the previce thing that should be instanciated only once, no matter how many webconsole actor are created?

TBH, I imagine you will find random similar issues in actors are they weren't designed with such usage in mind (multiple instances).

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #3)
> I imagine you are using a new client in the addon?
No, I am using toolbox.target.client

> That's the same discussion we had in another bug with :past.
> I thought we would/should share the same client to avoid such issue, but he
> had some concerns about states store in actors.
I share these concerns.

> So at the end it is "expected" that you get another webconsole client/actors.
> But duplicated instances shouldn't end up with such behavior either.
> The bottleneck here may be more about the NetworkMonitor,
> may be that's the previce thing that should be instanciated only once, no
> matter how many webconsole actor are created?
I'd prefer keeping actor's life cycle predictable (i.e. one tab actor instance
per tab, one global actor instance per browser). This will make easy to
maintain state in actors. Not sure why more instances
should be expected...

But, anyway, this problem breaks extensions that are registering backend actors
(or rather they are breaking the HTTP requests) and I think we should fix it asap
(whatever solution we use).

> TBH, I imagine you will find random similar issues in actors are they
> weren't designed with such usage in mind (multiple instances).
Precisely

Honza

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #4)
> (In reply to Alexandre Poirot [:ochameau] from comment #3)
> > I imagine you are using a new client in the addon?
> No, I am using toolbox.target.client

Ah! I haven't looked at the addon sources.
(Could be handy to post a link to github/whatever repo if you are hosting addon sources somewhere)
I thought you were connecting manually...

It is very different if you are hacking on top of toolbox target/client!

I think the issue is with this code:
  target.client.listTabs(response => {}
You shouldn't call listTabs and instead, use target.form.
target.form is the TabActor.form() result.

Also I don't see why you are calling target.activeTab.attachThread({}, (response, threadClient) => {}) ?
I imagine we are already doing it somewhere in toolbox codepath. Is this a workaround? Leftover?

Please do not hesitate to ping us. It looks like you are trying to navigate alone in a dense and obscur forest by yourself whereas some of us have a good picture of it.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> (In reply to Jan Honza Odvarko [:Honza] from comment #4)
> > (In reply to Alexandre Poirot [:ochameau] from comment #3)
> > > I imagine you are using a new client in the addon?
> > No, I am using toolbox.target.client
> 
> Ah! I haven't looked at the addon sources.
> (Could be handy to post a link to github/whatever repo if you are hosting
> addon sources somewhere)
> I thought you were connecting manually...
Yep, it's public example, here is the repo:
https://github.com/firebug/devtools-extension-examples/tree/master/GlobalActor

> 
> It is very different if you are hacking on top of toolbox target/client!
> 
> I think the issue is with this code:
>   target.client.listTabs(response => {}
> You shouldn't call listTabs and instead, use target.form.
> target.form is the TabActor.form() result.
I see and how can I get access to the global form (list of global actors)?

It looks like the target.form is initialized here (from the listTabs response),
but I don't see where global actor list would be stored.
https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/framework/target.js#L431

Also, even if I can use target.form (to get tab actors) - listTabs packet can be sent any time (e.g. by an extension) and it shouldn't break the list of actors/pools (as I described in comment #1), no?

> 
> Also I don't see why you are calling target.activeTab.attachThread({},
> (response, threadClient) => {}) ?
> I imagine we are already doing it somewhere in toolbox codepath. Is this a
> workaround? Leftover?
Yep, leftover. I'll remove it.

> 
> Please do not hesitate to ping us. It looks like you are trying to navigate
> alone in a dense and obscur forest by yourself whereas some of us have a
> good picture of it.
Great, thanks.

Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Panos, what do you think about the patch I provided?

Honza

Comment 8

[Alexandre Poirot [:ochameau]]

This code/cleanup has been introduced in bug 753401.
I haven't seen any argument debated in that bug about this particular code, except that we expect to really cleanup these previous actor, rob even suggested to call actor pool cleanup() method.

Also, Panos, this isn't related to this bug, but in bug 753401 comment 6, you suggested to return tab actor grip from attach request, that makes a lot of sense, and I worked around the current listTabs behavior in e10s and getTab.

Comment 9

[Alexandre Poirot [:ochameau]]

Also, there isn't much documentation about listTabs behavior there:
  https://wiki.mozilla.org/Remote_Debugging_Protocol#Listing_Browser_Tabs
Is this out official doc, or is there another one?

Comment 10

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

I'm still trying to wrap my head around the new dynamic actor registration API that is used in this case and it's taking me a while to understand how it interacts with this code and the old server extension API, but let me clarify a couple of issues that were raised above.

About not needing target.client.listTabs() when a target is available: since the toolbox is already open (by the time onToolboxReady is called), it has already determined its debugging (connection) target and is already connected to it. It may correspond to a tab or to a root actor form (if it's a browser toolbox debugging chrome code). In the former case that tab form will contain any tab-scoped actors, while in the latter case it will contain the global-scoped actors. That said, calling listTabs() should not do anything unexpected however and I'm trying to understand what is going on in your case.

About multiple connections to a single debugger server: it was a design goal from the beginning to support multiple clients debugging the same thing (e.g. tab). In order to have these separate debugging sessions not interfere with each other (to the extent that they could - e.g. while not modifying shared browser state) and at the same time minimize actor complexity, the design chosen was to have separate actor instances per connection, unaware of each others existence. This way the cost of maintaining multiple connections is centralized to the DebuggerServerConnection code and amortized among the various actor definitions. For example, one could connect both Firebug and Firefox console to a tab, do some expression evaluations and maintain a separate set of history entries and sandbox properties (at least while console code doesn't actually modify the page on evaluation).

Hope this is helpful.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Panos Astithas [:past] from comment #10)

> That said, calling listTabs() should not do anything unexpected
Precisely

> and I'm trying to understand what is going on in your case.
Thanks!

My gut feeling is that the pool with extra actors shouldn't be
reset every time 'listTabs' packet is received.
(and my patch is trying to avoid it)


Honza

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@dcamp: do you have any insights for the problem/patch?

Honza

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> My gut feeling is that the pool with extra actors shouldn't be
> reset every time 'listTabs' packet is received.
> (and my patch is trying to avoid it)

Yes, but should we recreate new tab actors each time we call listTabs?
i.e. each time listTabs call TabActor.grip()?
Shouldn't we just cache the grip for a given TabActor instance?
The behavior of listTabs isn't documented, if it was, we would take decisions more easily.

listTabs is expensive, you should try to avoid calling it.
target.js no longer call it.
But you are right, you can't use target.form as your main goal here is registering a global actor.
May be, in parallel of fixing listTabs itself, we should expose a new request dedicated to fetching global actors...

I'm curious, do you have a similar addon for tab actors?
I would expect it to be much simplier?

Otherwise I think at this point you should just ask for review for this patch.
That would be really great if you could come up with a unit test that currently fail without your patch and pass with it.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1155168-2.patch

(In reply to Alexandre Poirot [:ochameau] from comment #13)
> (In reply to Jan Honza Odvarko [:Honza] from comment #11)
> > My gut feeling is that the pool with extra actors shouldn't be
> > reset every time 'listTabs' packet is received.
> > (and my patch is trying to avoid it)
> 
> Yes, but should we recreate new tab actors each time we call listTabs?
> i.e. each time listTabs call TabActor.grip()?
No, see the new (updated) path that covers both global as well as tab actors
(it also comes from bug 1119794)

> Shouldn't we just cache the grip for a given TabActor instance?
> The behavior of listTabs isn't documented, if it was, we would take
> decisions more easily.
> 
> listTabs is expensive, you should try to avoid calling it.
> target.js no longer call it.
> But you are right, you can't use target.form as your main goal here is
> registering a global actor.
> May be, in parallel of fixing listTabs itself, we should expose a new
> request dedicated to fetching global actors...
Sounds good (and listTabs fixed first)

> I'm curious, do you have a similar addon for tab actors?
Yes: https://github.com/firebug/devtools-extension-examples/tree/master/CustomActor
This example registers custom *tab* actor - it also causes double-HTTP-request issue, since it also sends 'listTabs' requests to get the global ActorRegistryActor.

My patch fixes this example as well.

> I would expect it to be much simplier?
Registering tab and global actors is pretty much the same - since the ActorRegistryActor (global actor) is needed in both cases.

> Otherwise I think at this point you should just ask for review for this
> patch.
OK, who should I ask for review?

> That would be really great if you could come up with a unit test that
> currently fail without your patch and pass with it.
Sure, I'll come up with something.

Honza

Comment 15

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

I've pored over the referenced bugs and the webbrowser.js history and I can't think of a good reason to not make the change you are proposing. I don't remember exactly why I wrote this code like that, but I suspect that I relied a little too much on the implementation of onListTabs() when modifying grip().

For context, the idiom that we were using in this case stems from the old onListTabs implementation, which replaced its actor pool with a new one after iterating over the open tabs. This was a simple code optimization that ensured any actors for tabs that were closed since the previous call to onListTabs would be GCed automatically.

The only interesting thing in onListTabs (now getList) contract is that it is guaranteed to keep the actors alive between 2 calls to it, in order to ensure the client wouldn't have an actor disappear from it because its tab was closed. Nowadays we are using tabListChanged events to immediately notify the client, so there is even less point in doing what we used to do before.

Alex can review this, or you could send it to me if he is too busy.

(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Yes, but should we recreate new tab actors each time we call listTabs?
> i.e. each time listTabs call TabActor.grip()?
> Shouldn't we just cache the grip for a given TabActor instance?

createExtraActors doesn't do much if it has already been called once, unless an actor-providing add-on was installed in the meantime. It's nice that we already support this case.

> That would be really great if you could come up with a unit test that
> currently fail without your patch and pass with it.

You can look for inspiration in test_register_actor.js and test_registerClient.js.

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1155168-test-1.patch

Here is mochitest that passes if my patch is applied; fails otherwise.

Note that it depends on:
https://hg.mozilla.org/integration/fx-team/rev/998c4e53c1f8
(which has been backed out recently)


Honza

Comment 17

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #16) 
> Note that it depends on:
> https://hg.mozilla.org/integration/fx-team/rev/998c4e53c1f8
> (which has been backed out recently)

I'll try to reland it without landing the offending patch.

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #17)
> (In reply to Jan Honza Odvarko [:Honza] from comment #16) 
> > Note that it depends on:
> > https://hg.mozilla.org/integration/fx-team/rev/998c4e53c1f8
> > (which has been backed out recently)
> 
> I'll try to reland it without landing the offending patch.
Great, thanks!

Honza

Comment 19

[Alexandre Poirot [:ochameau]]

Comment on attachment 8595921 [details] [diff] [review]
bug1155168-2.patch

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

Looks good, sounds like a sane fix once you get the whole picture in mind!

::: toolkit/devtools/server/actors/childtab.js
@@ +77,2 @@
>    return response;
>  };

Note that I recently removed this ContentActor.form() method overload in childtab.js in order to just use TabActor one.
You shouldn't need to patch as it doesn't exists anymore ;)

::: toolkit/devtools/server/actors/webbrowser.js
@@ +865,2 @@
>        this.conn.addActorPool(this._tabActorPool);
>      }

I imagine it would be worth saying that we have to ensure using the same ActorPool, always, in order to ensure that createExtraActors won't instanciate duplicated actor instance.

@@ +865,3 @@
>        this.conn.addActorPool(this._tabActorPool);
>      }
> + 

nit: There is a trailing space on this line

@@ +865,4 @@
>        this.conn.addActorPool(this._tabActorPool);
>      }
> + 
> +    // Walk over tab actors added by extensions and add them to a new ActorPool.

I think this comment is misleading, isn't it?
This isn't just actors added by extensions, right?
But all the actors registered with DebuggerServer.add{Tab|Global}Actor or DebuggerServer.registerModule.

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8596040 [details] [diff] [review]
bug1155168-test-1.patch

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b3b8024b08c

::: toolkit/devtools/server/tests/mochitest/test_registerActor.html
@@ +43,5 @@
> +        type: { tab: true }
> +      };
> +
> +      var registry = ActorRegistryFront(gClient, aResponse);
> +      registry.registerActor(actorURL, options).then(actorFront => {

This has nothing to do with this patch, but I'm wondering if we could somehow return the real actor front from registerActor?
`actorFront` here is just the ActorActorFront, but that would be handy to also return the actual front for this actor, i.e. HelloActorFront (if implemented).
As here, in your test, you have to use the old API, with client.request to communicate with your actor, instead of using protocol.js stuffs.

@@ +74,5 @@
> +    getCount(helloActor, response => {
> +      ok(!response.error, "No error");
> +      is(response.count, 2, "The counter must be valid");
> +
> +      gClient.listTabs(({}) => {

It would be safe to check that listTabs always returns your helloActor id.

@@ +83,5 @@
> +          callback();
> +        });
> +      });
> +    });
> +  });

nit: with a bit of Task, you could make that code less nested. Up to you, not a big deal.
There is just gClient.listTabs that doesn't return a promise and would need:
  let deferred = promise.defer();
  gClient.listTabs(deferred.resolve);
  yield deferred.promise;

@@ +90,5 @@
> +function getCount(actor, callback) {
> +  gClient.request({
> +    to: actor,
> +    type: "count"
> +  }, callback);

nit: note that gClient.request also returns a promise

Comment 21

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #16)
> Created attachment 8596040 [details] [diff] [review]
> https://hg.mozilla.org/integration/fx-team/rev/998c4e53c1f8

I landed just this changeset, it shouldn't be backed out this time.
And it looks like your patch, is going to allow me landed the other patches from bug 1137285 !

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1155168-3.patch

I am attaching updated patch (changes based on Alex's review comments)
It also includes the test.

- ContentActor.form() related code removed 
- Couple of comments added
- Trailing spaces removed
- One more check in the mochitest

Push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=666009a4734c


> And it looks like your patch, is going to allow me landed
> the other patches from bug 1137285 !
Great!

Honza

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

> This has nothing to do with this patch, but I'm wondering if we could somehow return
> the real actor front from registerActor? `actorFront` here is just the ActorActorFront,
> but that would be handy to also return the actual front for this actor, i.e. HelloActorFront
> (if implemented).
Note that registerActor just registers an actor-factory. The instance is created lazily later if/when the first packet is sent to it - e.g. "attach".

Three initialization steps:
1. Register actor factory
2. Get actor factory ID (through listTabs or getTab)
3. Send the first packet to the actor factory ID to instantiate it (e.g. "attach").

The ActorActorFront instance is needed too since used to unregister the actor factory at the end (and Bug 1119794 should make sure that instances are removed too).

Honza

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

> Push to try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=666009a4734c
Alex, looks good, should I set checkin needed?

Honza

Comment 25

[Alexandre Poirot [:ochameau]]

yes, the related changeset stick on fx-team so you are good to land.

Thanks!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f0da78f90011

Comment 27

[Carsten Book [:Tomcat]]

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