Don't keep old network events around after a tab navigation occurs.

Harald, Could you think about any existing telemetry which may help assert the impact of this patch?
I'm pretty confident it will address serious memory leaks from DevTools, and it would be great to take an overview of existing metrics we have to see if one highlights this fix. If one do pick this up, we may take it as good candidate for the performance initiative.

I was wondering for example if we have a metrics about OOM crashes? If yet, could we categorize the one related to DevTools?
Or telemetry about memory usage, also categorized by DevTools usages?
Or the number of restart of Firefox per day/week? Or Firefox uptime for DevTools users?

Not sure about OOM crashes as we only get very little information. Bug 1297525 was filed for DevTools because of OOM, but we don't have the means to track those reliably over time (afaik).

The best we can do without additional instrumentation is relying on existing probes. For leaks especially we can track our memory telemetry; but also GC related metrics (leak = long GC). Leaks slow specific parts of GC but also cause hangs with mostly GC.

Preliminary chat with sfink about this yields:

For DevTools users:

1. MEMORY_JS_GC_HEAP should improve.
2. GC_MS should improve.
3. GC_MARK_MS should improve.
4. GC_SWEEP_MS should not change much as leaks prevent sweeping (maybe)

As you said (and Sorin brought it up as well) open/close count for DevTools should improve if this has big impact, as we trained developers that DevTools leaks are fixed by closing the tools (also comes up in feedback).

Most of these we should be able to plot in stmo (with some help from data science).

It is a bit surprising, but DAMP doesn't highlight any improvement:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&newProject=try&newRevision=0f7e1a3c96ad5512e356006e3a19af287d75527c&originalSignature=1759151&newSignature=1759151&framework=12&originalRevision=f77921e426e801272780129a8cdff3121c3d6620
I would expect us to free some more memory between page reload while running the netmonitor test, leading to less GC pauses in the following tests. But nothing visible.
Note that netmonitor isn't having a stress test document. We only test it against bild.de, which may not trigger enough requests?

Last time I checked the DAMP version of bild.de it seemed pretty lightweight by today's standards. That's why I chose Mastodon to test (it's pretty heavyweight with React, Redux, Immutable and tons of other libraries).

Agreed. An SPA would probably best represent a modern heavy JS payload. Mastodon seems good – Riot, Firefox Profiler would also be open source alternatives. If we want to go towards content, we could have Discord and Reddit.

We can also continue that discussion in bug 1539195.

Note that bug 1615283 might help improve test helpers. At least, when we navigate to a new URL, there is a shared helper that we can tweak.
But here, we may want to also tweak initNetmonitor all all netmonitor tests, using various wait methods for no clear reason.

Honza, could you please have a look at the comments on phabricator?
https://phabricator.services.mozilla.com/D54800#1815934
https://phabricator.services.mozilla.com/D54800#1906061
https://phabricator.services.mozilla.com/D54800#inline-377350

I believe that, in order to fix this issue and have tests passing, we would need to review all existing helpers around netmonitor.
We have way too many different wait mechanisms:

• waitForAllNetworkUpdateEvents
• waitForNetworkEvents
• waitForAllRequestsFinished
• waitForRequestsFinished
• and probably some others...
  This patch highlights that some are weak and waitForAllRequestsFinished looks slightly better.
  Should we use this one everywhere? Would it work? Or is it weak against another edge case?
  Why are we having so many different ways to wait for updates?

Then, once we can pick the best way to wait for updates, we can go over netmonitor test and use the right one.
This may involve tweaking initNetmonitor and/or navigateTo.
It looks like we are explicitely avoiding to check the behavior during toolbox opening and always reload the page.
This creates a lot of duplicated lines in each test, if we have to keep this, we should introduce an helper to do all of that in one function call.

Could we maybe create another issue to track this refactoring of the tests? It seems to be a more systemic problem than what the revision tries to address.

(In reply to Sorin Davidoi from comment #22)

Could we maybe create another issue to track this refactoring of the tests? It seems to be a more systemic problem than what the revision tries to address.

Yes. Your patch raise a systemic issue which has to be addressed before being able to fix this leak.

I opened bug 1622043 and will start submitting patches to it.

@ochameau Since the test issues are fixed, can you take another look at the patch?

I was looking at this yesterday and pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75a94dd42d446e96e21309e64deb8e7553ed17f2

I am seeing some test failures:

• devtools/client/netmonitor/test/browser_net_block-context.js
• devtools/client/netmonitor/test/browser_net_block-csp.js
• devtools/client/netmonitor/test/browser_net_block.js
• devtools/client/netmonitor/test/browser_net_cause_source_map.js
• devtools/client/netmonitor/test/browser_net_charts-**.js
• devtools/client/netmonitor/src/har/test/browser_net_har_copy_all_as_har.js
• devtools/client/framework/test/browser_ignore_toolbox_network_requests.js

I was doing some analysis of the Network monitor backend and I am hoping to get also more time to work on this.

Honza

I've rebased the patch. The test failure are still there, and I'm not sure how to fix them. I suspect that they are related to the test setup (probably similar issues to the ones @ochameau has fixed).

Cleaning up dt-leak dependencies

I'm curious to know if the test failures in this bug highlighted a real issue with the fix or if they are only test artifacts.
I wonder if it would be acceptable to put this new behavior behind a pref so that we can toggle it off for the tests?

The main challenge here is to still support persist-logs feature of the netmonitor.
When this feature is enabled we have to keep around all the Network Events as before this patch.

Fission refactoring and the migration to server side NETWORK_EVENT watchers made
this fix significantly easier as we can now more easily reach out the parent process
code which is keeping things allocated.

Note that with the current patch, we still leak one request. The navigation request,
which is slightly special as it is flaged with previous document innerWindowId
and is meant to be inspectable from the next document load.

DAMP reports various small improvements (3% to 6%) on many tests:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=be53546902553b0e585b42f253d6ceb0f67876c8&originalSignature=3148563&newSignature=3130913&framework=12&selectedTimeRange=172800&page=1&showOnlyImportant=1
The most important improvements impact tests others than netmonitor one, which probably means that the leaked objects were slowing down the following tests.

Surprisingly, I no longer see notable improvement reported by DAMP:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=f08cb89717e5fb10f940ffc0ef6e84d596646fd6&originalSignature=3130913&newSignature=3130913&framework=12&originalRevision=37234b235e647e5814d4eb24fe19d873ad284f9f&page=1

Let's see if it impacts the metrics on mozilla-central.

But it does still fix browser_allocation_reload.js, which sees its number of leaks go down to 0 objects leaked with stack (with all the target fronts and markupview fixes [still in flight]).

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