Closed Bug 1346854 Opened 4 years ago Closed 4 years ago

GCLI starts network listeners for seemingly no reason

Categories

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

Product:
DevTools Graveyard
Component:
Graphic Commandline and Toolbar
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p1])

Attachments

(2 files)

Bug 1346854 - Delay network listening until toolbox open.
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1346854 - Adjust network monitor test expectations.
59 bytes, text/x-review-board-request
Honza
: review+
Details 
From bug 1297525 comment 10, GCLI is starting network listeners, which gobble up memory.

We should disable this feature when only GCLI is used.
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.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
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+
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.
I am seeing some collisions with patch from bug 1352699 that just landed (integration/autoland)

Honza
(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.
(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 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+
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)
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
https://hg.mozilla.org/mozilla-central/rev/87c2e570f1c6
https://hg.mozilla.org/mozilla-central/rev/7857690c1399
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Whiteboard: [qf] → [qf:p1]
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.