Closed Bug 1346854 Opened 7 years ago Closed 7 years ago

GCLI starts network listeners for seemingly no reason

Tracking

(Performance Impact:high)

Status:

RESOLVED FIXED

Milestone:

Firefox 55

Project Flags:

Performance Impact

high

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1346854 - Delay network listening until toolbox open. 7 years ago J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) 59 bytes, text/x-review-board-request	ochameau : review+	Details
Bug 1346854 - Adjust network monitor test expectations. 7 years ago J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) 59 bytes, text/x-review-board-request	Honza : review+	Details

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

Assignee

Description

•

7 years ago

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

We should disable this feature when only GCLI is used.

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

Assignee

Updated

•

7 years ago

Priority: -- → P1

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

Assignee

Updated

•

7 years ago

Blocks: dt-leak

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

Assignee

Comment 1

•

7 years ago

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.

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

Assignee

Updated

•

7 years ago

Assignee: nobody → jryans

Status: NEW → ASSIGNED

Comment hidden (mozreview-request)

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/

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

Assignee

Comment 3

•

7 years ago

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

Alexandre Poirot [:ochameau]

Comment 4

•

7 years ago

mozreview-review

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.

Attachment #8850656 - Flags: review?(poirot.alex) → review+

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

Assignee

Comment 5

•

7 years ago

mozreview-review-reply

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

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

Assignee

Comment 8

•

7 years ago

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

Comment hidden (mozreview-request)

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/

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

Assignee

Comment 10

•

7 years ago

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

Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26

Comment 11

•

7 years ago

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

Honza

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

Assignee

Comment 12

•

7 years ago

(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.

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

Assignee

Updated

•

7 years ago

Depends on: 1352699

Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26

Comment 13

•

7 years ago

(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

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

Assignee

Comment 14

•

7 years ago

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

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26

Comment 17

•

7 years ago

mozreview-review

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

Attachment #8854927 - Flags: review?(odvarko) → review+

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

Assignee

Updated

•

7 years ago

Whiteboard: [qf]

Comment hidden (mozreview-request)

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/

Mozilla Autoland

Comment 19

•

7 years ago

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Pulsebot

Comment 22

•

7 years ago

Pushed by jryans@gmail.com:
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

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

Comment 23

•

7 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox55: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 55

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

Assignee

Updated

•

7 years ago

Whiteboard: [qf] → [qf:p1]

BMO Automation

Updated

•

6 years ago

Product: Firefox → DevTools

BMO Automation

Updated

•

6 years ago

Product: DevTools → DevTools Graveyard

Dave Hunt [:davehunt] [he/him] ⌚BST

Updated

•

2 years ago

Performance Impact: --- → P1

status-firefox55: fixed → ---

Whiteboard: [qf:p1]

You need to log in before you can comment on or make changes to this bug.