Closed Bug 1539502 Opened 5 years ago Closed 3 years ago

The toolbox is leaking its React components after closing

Categories

(DevTools :: Framework, defect, P2)

defect

Tracking

(firefox95 fixed)

RESOLVED FIXED
95 Branch
Tracking Status
firefox95 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: dt-perf-stability-mvp)

Attachments

(2 files)

After we close the toolbox, the Toolbox class instance is correctly freed, as well as most of what it manipulates, including its document (toolbox.xul).
But, some part of the toolbox are being leaked and stay alive after closing it.

There is at least the react components like ToolboxController which are being kept around. It is instanciated from here:
https://searchfox.org/mozilla-central/rev/a7315d78417179b151fef6108f2bce14786ba64d/devtools/client/framework/toolbox.js#1181

Then, there is some other objects that are being kept alive, like the markup view iframe, but this is specific to the panel you are opening.

Component: General → Framework
Depends on: 1539518
Priority: -- → P2
Blocks: 1553178
No longer blocks: devtools-performance
Blocks: dt-leak
No longer blocks: 1553178
Type: enhancement → defect
Blocks: 1729925

This is two reason for leaking the globals:

  • The JIT/CachedIR that are introduced by trackingAllocationSites=true
    and can be fixed by calling minimizeMemoryUsage to purge CachedIR objects.
  • The Debugger::allocationsLog which relates to Debugger.Memory.drainAllocationLog
    and can be fixed by calling drainAllocationLog to clear allocationsLog.
    We also have to be careful about disable allocation site recording while doing the GCs.
    On start and on stop.

Both were keeping strong references to the globals.

In this patch, I'm also extending the coverage of the AllocationTracker
to better assert what precise leaks are reported.

Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED

After some more investigation, it looks more like a false report where the Memory API, used to investigate the leaks, was holding strong references to all browser-loader globals and ultimately leading to leak all React components as well as the Redux store.

Or, we might also have fixed a real leak recently. But after fixing the false report, I no longer see leaks involving React components!

Depends on: 1733480
Whiteboard: dt-perf-stability-mvp

These patches mostly impact browser_toolbox and toolbox tests, which are creating transient globals.
The browser-loader modules and the toolbox and panel documents are create and destroyed while running the test.
Without this, all these globals were kept alive and reported as being leaked.

Here is the raw number:

With these two patches

 0:20.30 INFO The browser-console test leaked 14648 objects (-41643 with missing allocation site) in the parent process
 0:52.89 INFO The reload-debugger test leaked 86 objects (-77028 with missing allocation site) in the parent process
 0:52.89 INFO The reload-debugger test leaked 1281 objects (-315 with missing allocation site) in the content process
 1:24.31 INFO The reload-inspector test leaked 115 objects (-68004 with missing allocation site) in the parent process
 1:24.31 INFO The reload-inspector test leaked 3788 objects (-2297 with missing allocation site) in the content process
 1:54.98 INFO The reload-netmonitor test leaked 121 objects (-45561 with missing allocation site) in the parent process
 1:54.98 INFO The reload-netmonitor test leaked 949 objects (-306 with missing allocation site) in the content process
 2:22.38 INFO The reload-no-devtools test leaked 0 objects (2717 with missing allocation site) in the parent process
 2:22.38 INFO The reload-no-devtools test leaked 552 objects (-488 with missing allocation site) in the content process
 2:53.07 INFO The reload-webconsole test leaked 271 objects (-42975 with missing allocation site) in the parent process
 2:53.07 INFO The reload-webconsole test leaked 932 objects (-358 with missing allocation site) in the content process
 3:05.25 INFO The target test leaked 0 objects (-10450 with missing allocation site) in the parent process
 3:22.67 INFO The toolbox test leaked -22333 objects (-106380 with missing allocation site) in the parent process

On mozilla-central

 0:19.69 INFO The browser-console test leaked 54996 objects (257096 with missing allocation site) in the parent process
 0:51.84 INFO The reload-debugger test leaked 86 objects (10850 with missing allocation site) in the parent process
 0:51.84 INFO The reload-debugger test leaked 1304 objects (4444 with missing allocation site) in the content process
 1:23.08 INFO The reload-inspector test leaked 115 objects (23524 with missing allocation site) in the parent process
 1:23.08 INFO The reload-inspector test leaked 3825 objects (6708 with missing allocation site) in the content process
 1:53.92 INFO The reload-netmonitor test leaked 100 objects (7765 with missing allocation site) in the parent process
 1:53.92 INFO The reload-netmonitor test leaked 972 objects (4436 with missing allocation site) in the content process
 2:21.05 INFO The reload-no-devtools test leaked 0 objects (2722 with missing allocation site) in the parent process
 2:21.05 INFO The reload-no-devtools test leaked 572 objects (1059 with missing allocation site) in the content process
 2:51.63 INFO The reload-webconsole test leaked 272 objects (8482 with missing allocation site) in the parent process
 2:51.63 INFO The reload-webconsole test leaked 955 objects (4361 with missing allocation site) in the content process
 3:04.18 INFO The target test leaked 0 objects (2728 with missing allocation site) in the parent process
 3:21.61 INFO The toolbox test leaked 66999 objects (191020 with missing allocation site) in the parent process

I'll investigate the toolbox test as we shouldn't have a negative leak count!

I fixed the negative count for toolbox, and added some test coverage. This was related to a missing call to flushAllocation from startRecording.
This is required for the same reason we are calling it from stopRecording. To avoid leaking globals which have been destroyed and only hold by the allocation log.

With one very small additional fix (to do an additional GC in the parent process when we start the record in the content process), I end up with these numbers, which all look correct now (without this last fix, netmonitor was reporting negative leaks in the parent):

 0:20.44 INFO The browser-console test leaked 24708 objects (50994 with missing allocation site) in the parent process
 0:57.60 INFO The reload-debugger test leaked 86 objects (2868 with missing allocation site) in the parent process
 0:57.60 INFO The reload-debugger test leaked 1281 objects (-315 with missing allocation site) in the content process
 1:33.32 INFO The reload-inspector test leaked 115 objects (3494 with missing allocation site) in the parent process
 1:33.32 INFO The reload-inspector test leaked 3788 objects (-2297 with missing allocation site) in the content process
 2:07.73 INFO The reload-netmonitor test leaked 287 objects (3281 with missing allocation site) in the parent process
 2:07.73 INFO The reload-netmonitor test leaked 949 objects (-359 with missing allocation site) in the content process
 2:38.46 INFO The reload-no-devtools test leaked 0 objects (2767 with missing allocation site) in the parent process
 2:38.46 INFO The reload-no-devtools test leaked 552 objects (-473 with missing allocation site) in the content process
 3:12.66 INFO The reload-webconsole test leaked 271 objects (3458 with missing allocation site) in the parent process
 3:12.66 INFO The reload-webconsole test leaked 932 objects (-307 with missing allocation site) in the content process
 3:24.86 INFO The target test leaked 0 objects (117 with missing allocation site) in the parent process
 3:42.32 INFO The toolbox test leaked 0 objects (250 with missing allocation site) in the parent process
Attachment #9244712 - Attachment description: Bug 1539502 - [devtools] Assert that no late allocation happen when recording memory leaking. → Bug 1539502 - [devtools] Warn when late allocations happen when recording memory leaking.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d22b45edbf35
[devtools] Prevent leaking all transient global while recording memory leaks. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/cf29fbe4e56f
[devtools] Warn when late allocations happen when recording memory leaking. r=jdescottes
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
Regressions: 1735624
Regressions: 1735632
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: