Closed Bug 1729925 Opened 3 years ago Closed 3 years ago

Fronts, commands and a few class of inspector are leaked after toolbox is closed

Categories

(DevTools :: General, enhancement)

enhancement

Tracking

(firefox94 fixed)

RESOLVED FIXED
94 Branch
Tracking Status
firefox94 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Whiteboard: dt-perf-stability-mvp)

Attachments

(9 files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

The memory test dedicated to toolbox open and close:
https://searchfox.org/mozilla-central/source/devtools/client/framework/test/allocations/browser_allocations_toolbox.js
is reporting many leaks today.

Today, on mozilla-central, it reports 44082 objects being leaked.
The leaks mostly come from bug 1539502, where we have a leak that is keeping the toolbox and/or React elements in memory. Both are being cross referenced, so it is unlear which one of the two are the real source of leak.

Until we figure out this leak, we can clear references here and there slightly better so that, if we happen to leak toolbox, inspector, targets, ... we aren't leaking much more.

We can especially focus on avoiding to leak: commands, fronts and sub classes of the inspector.

Whiteboard: dt-perf-stability-mvp

Currently, we do leak the Toolbox/Inspector and some Documents when
closing the toolbox.
So that these classes are kept in memory, while not nullifying all its references
to client classes.

By adding these few nullications we can at least avoid leaking the fronts and commands.

(also fix a failure due to null markup.walker in browser_rules_add-rule-with-menu.js)

We still do leak Toolbox/Inspector and some Documents.
Unmounting the React components and nullifying references to them
seems to allow clearing a few React component instances.

We weren't unmounting the splitBox/toolsidebar, nor destroying ruleViewSideBar instance.

Also avoid creating a NodePicker during toolbox's destroy.

We do set resourceCommand from onTargetAvailable, without ever clearing it.
If we happen to leak targets, we end up leaking commands.

Now another stack of patches to focus on toolbox leaks.
The number display "toolbox: xxxx" is the numbe of objects being reported as leaked by:
https://searchfox.org/mozilla-central/source/devtools/client/framework/test/allocations/browser_allocations_toolbox.js

Avoid leaking Commands/DevToolsClient/Fronts when closing the toolbox

Ensure clearing references and unmounting all React components

Ensure unregistering some DOM event listeners in the inspector

Avoid leaking children fronts of target in case target is leaked

Avoid leaking commands from NodePicker

Avoid leaking commands from TargetFront

Avoid leaking commands from toolbox

Avoid leaking toolbox's redux store and ultimately targets

Avoid leaking the rule panel via a click listener

Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/955138a73fa4
[devtools] Avoid leaking Commands/DevToolsClient/Fronts when closing the toolbox. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/9b1eb369d679
[devtools] Ensure clearing references and unmounting all React components. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/61dfd48f3409
[devtools] Ensure unregistering some DOM event listeners in the inspector. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/14619c743301
[devtools] Avoid leaking children fronts of target in case target is leaked. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/e9f40c89753f
[devtools] Avoid leaking commands from NodePicker. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/22097d7a00c2
[devtools] Avoid leaking commands from TargetFront. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/331ffa0bd2f9
[devtools] Avoid leaking commands from toolbox. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/ac9a15005a5b
[devtools] Avoid leaking toolbox's redux store and ultimately targets. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/509a0601481a
[devtools] Avoid leaking the rule panel via a click listener. r=nchevobbe

It looks like it comes from bug 1728072, so let's try to land only these patches first.

With only patches from this bug, it looks green:
https://treeherder.mozilla.org/jobs?repo=try&revision=d8abd17ca97273bd690b00f93b64eb577a78e3ea

Flags: needinfo?(poirot.alex)
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/87456fe4679c
[devtools] Avoid leaking Commands/DevToolsClient/Fronts when closing the toolbox. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/68037ba707f5
[devtools] Ensure clearing references and unmounting all React components. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/1ae31c877141
[devtools] Ensure unregistering some DOM event listeners in the inspector. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/bfe04d9f4203
[devtools] Avoid leaking children fronts of target in case target is leaked. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/d7c34f64e35e
[devtools] Avoid leaking commands from NodePicker. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/e1f15379815f
[devtools] Avoid leaking commands from TargetFront. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/55a0f637c042
[devtools] Avoid leaking commands from toolbox. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/3e8fac84e076
[devtools] Avoid leaking toolbox's redux store and ultimately targets. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/e7567d7b60a3
[devtools] Avoid leaking the rule panel via a click listener. r=nchevobbe
Regressions: 1731793
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: