1346854 - GCLI starts network listeners for seemingly no reason

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, enhancement, P1)

Description

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

From bug 1297525 comment 10, GCLI is starting network listeners, which gobble up memory.

We should disable this feature when only GCLI is used.

Comment 1

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

I can confirm that GCLI enables networking listening for all tabs.  So, this is in effect much worse than bug 1297525, because it affects all tabs instead of a single one.

Comment 2

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

Attachment: Bug 1346854 - Delay network listening until toolbox open.

Once upon a time (bug 862341), we decided to enable network listening by
default in DevTools.

In a general sense, that's fine.  When you open a toolbox, we listen to that
tab and stop listening when the toolbox closes.

GCLI / Developer Toolbar is quite different, though.  It connects to the whole
browser.  This meant that enabling GCLI would start listening to network
activity in *every* tab (even though it doesn't have any way to even use that
data).  This of course will slow down performance with all the extra tracking
and eat up memory with the tracked request data.

In this change, we move the step to enable network listening into the toolbox,
which seems more like what we intended anyway.

Review commit: https://reviewboard.mozilla.org/r/123204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123204/

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50a6791a3db913a47a8fd125df62df3c2b2425ad

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8850656 [details]
Bug 1346854 - Delay network listening until toolbox open.

https://reviewboard.mozilla.org/r/123204/#review126230

Good catch! Thanks for the fix.

::: devtools/client/framework/toolbox.js:416
(Diff revision 1)
>        yield this._target.makeRemote();
>  
> +      // Start tracking network activity on toolbox open
> +      yield this._target.client.attachConsole(this._target.form.consoleActor, [
> +        "NetworkActivity",
> +      ]);

It looks wrong to call attachConsole multiple times, we are going to create two instances of WebConsoleClient.
I think it would be better to call:
target.activeConsole.startListeners instead of client.attachConsole.

Comment 5

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

Comment on attachment 8850656 [details]
Bug 1346854 - Delay network listening until toolbox open.

https://reviewboard.mozilla.org/r/123204/#review126230

> It looks wrong to call attachConsole multiple times, we are going to create two instances of WebConsoleClient.
> I think it would be better to call:
> target.activeConsole.startListeners instead of client.attachConsole.

Thanks, I actually wanted to do that first, but then I thought the method didn't exist...  Turns out the `WebConsoleClient` was just in a separate module. :S

I'll make the change.

Comment 6

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

Attachment: Bug 1346854 - Adjust network monitor test expectations.

After the previous commit, we're now starting network listening at a different
point in toolbox startup.  We need to rework some test expectations to account
for the new timing.

Review commit: https://reviewboard.mozilla.org/r/126852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126852/

Comment 7

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

Comment on attachment 8850656 [details]
Bug 1346854 - Delay network listening until toolbox open.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123204/diff/1-2/

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99a338d6a50005f18dbbbb90a1defac14e2e21f1

Comment 9

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

Comment on attachment 8854927 [details]
Bug 1346854 - Adjust network monitor test expectations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126852/diff/1-2/

Comment 10

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37404c9be64d3ff03d900fad265217aa39ffcaa9

Comment 11

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

I am seeing some collisions with patch from bug 1352699 that just landed (integration/autoland)

Honza

Comment 12

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

(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> I am seeing some collisions with patch from bug 1352699 that just landed
> (integration/autoland)

Okay, I'll rebase after this merges.

Comment 13

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> (In reply to Jan Honza Odvarko [:Honza] from comment #11)
> > I am seeing some collisions with patch from bug 1352699 that just landed
> > (integration/autoland)
> 
> Okay, I'll rebase after this merges.
Just a note that patches from bug 1352699 landed in m-c.

Honza

Comment 14

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

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

Comment 15

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

Comment on attachment 8850656 [details]
Bug 1346854 - Delay network listening until toolbox open.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123204/diff/2-3/

Comment 16

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

Comment on attachment 8854927 [details]
Bug 1346854 - Adjust network monitor test expectations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126852/diff/2-3/

Comment 17

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

Comment on attachment 8854927 [details]
Bug 1346854 - Adjust network monitor test expectations.

https://reviewboard.mozilla.org/r/126852/#review132626

Looks good to me!

Thanks Ryan!

Honza

Comment 18

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

Comment on attachment 8854927 [details]
Bug 1346854 - Adjust network monitor test expectations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126852/diff/3-4/

Comment 19

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s dc639ea60ada -d a8a257ba1903: rebasing 389953:dc639ea60ada "Bug 1346854 - Delay network listening until toolbox open. r=ochameau"
rebasing 389954:52565493af19 "Bug 1346854 - Adjust network monitor test expectations. r=Honza" (tip)
merging devtools/client/netmonitor/test/head.js
warning: conflicts while merging devtools/client/netmonitor/test/head.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 20

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

Comment on attachment 8850656 [details]
Bug 1346854 - Delay network listening until toolbox open.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123204/diff/3-4/

Comment 21

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

Comment on attachment 8854927 [details]
Bug 1346854 - Adjust network monitor test expectations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126852/diff/4-5/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87c2e570f1c6
Delay network listening until toolbox open. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/7857690c1399
Adjust network monitor test expectations. r=Honza

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/87c2e570f1c6
https://hg.mozilla.org/mozilla-central/rev/7857690c1399