Fix leaking windows in some of memory tool's browser mochitests

NEW
Unassigned

Status

()

Firefox
Developer Tools: Memory
P2
normal
2 years ago
2 years ago

People

(Reporter: fitzgen, Unassigned)

Tracking

(Blocks: 1 bug, {leave-open})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 10 obsolete attachments)

1.95 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
2.00 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
2.45 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
2.37 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
3.92 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
1.66 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
1.25 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
2.00 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
1.21 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
Pretty sure whatever is causing these windows to leak until shutdown on mochitests already existed, but we didn't have many browser mochitests.

In either case, it is worth it to land those patches now but disabled on DEBUG builds so we at least have the sanity test coverage on builds that don't do leak checking.
STR:

Run browser mochitests on a DEBUG build.
Has STR: --- → yes
Blocks: 1221517
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
I thought bug 1226217 would fix this, but it turns out not to.
See Also: → bug 1226217
The leak itself isn't hugely worrying to me, but this is pretty important since it is reducing our test coverage.
Priority: -- → P1
This is really strange: the only CC log I'm finding the leaking windows in are saying that they have no roots, which IIUC means they are garbage.
Created attachment 8715351 [details] [diff] [review]
Enable the browser_memory_simple_01.js test on DEBUG builds
Attachment #8715351 - Flags: review?(jsantell)
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5511fee015f5
Keywords: leave-open
Created attachment 8715377 [details] [diff] [review]
Enable the browser_memory_breakdowns_01.js test on DEBUG builds
Attachment #8715377 - Flags: review?(jsantell)
I haven't actually figured out what it is about waitUntilSnapshotState that sometimes causes leaks, but not all the time. I tried rewriting it in ways to avoid closing over variables as much as possible, no luck. I tried moving its definition inside the test function, no luck.

Going to just manually get all the tests running on DEBUG builds without leaks.

Try push for the new patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d11c11462931
Created attachment 8715387 [details] [diff] [review]
Enable the browser_memory_clear_snapshots.js test on DEBUG builds
Attachment #8715387 - Flags: review?(jsantell)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbb90b00eb9a
Created attachment 8715392 [details] [diff] [review]
Enable the browser_memory_diff_01.js test on DEBUG builds
Attachment #8715392 - Flags: review?(jsantell)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb33e2926c51
Created attachment 8715394 [details] [diff] [review]
Enable the browser_memory_filter_01.js test on DEBUG builds
Attachment #8715394 - Flags: review?(jsantell)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3393b697d2e1
Created attachment 8715409 [details] [diff] [review]
Enable the browser_memory_no_allocation_stacks.js test on DEBUG builds
Attachment #8715409 - Flags: review?(jsantell)
(Reporter)

Comment 16

2 years ago
memory-tests-on-fleek
This last one actually worked on DEBUG without changes to the test, but I cleaned up some unused imports and wrong summary comment.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c8771f3538f
Attachment #8715409 - Flags: review?(jsantell) → review+
Attachment #8715351 - Flags: review?(jsantell) → review+
Attachment #8715377 - Flags: review?(jsantell) → review+
Attachment #8715387 - Flags: review?(jsantell) → review+
Attachment #8715392 - Flags: review?(jsantell) → review+
Attachment #8715394 - Flags: review?(jsantell) → review+
This is weird. We should remove the utility function completely if it's leaking though so we don't have this again
Created attachment 8715442 [details] [diff] [review]
Remove the unused waitUntilSnapshotState test utility
Attachment #8715442 - Flags: review?(jsantell)
Created attachment 8715445 [details] [diff] [review]
Follow up: clean up memory panel open/destroy

This commit removes the assignment expression in a return statement thing that
was going on. It is easy to misread and we don't usually use this idiom anywhere
else in the codebase. Additionally, it makes the destroy method clean up some
more members set when opening the tool.
Attachment #8715445 - Flags: review?(jsantell)
Created attachment 8715449 [details] [diff] [review]
Follow up: HeapAnalysesClient should set its Worker reference null upon destruction

This makes attempts to use the HeapAnalysesClient after the worker has already
been destroyed into an earlier, less quiet error so we can catch more of them
and catch them sooner.
Attachment #8715449 - Flags: review?(jsantell)
Attachment #8715442 - Flags: review?(jsantell) → review+
Attachment #8715445 - Flags: review?(jsantell) → review+
Attachment #8715449 - Flags: review?(jsantell) → review+
Created attachment 8715968 [details] [diff] [review]
Follow up: HeapAnalysesClient should set its Worker reference null upon destruction

This makes attempts to use the HeapAnalysesClient after the worker has already
been destroyed into an earlier, less quiet error so we can catch more of them
and catch them sooner.
Attachment #8715968 - Flags: review+
Attachment #8715351 - Attachment is obsolete: true
Attachment #8715377 - Attachment is obsolete: true
Attachment #8715387 - Attachment is obsolete: true
Attachment #8715392 - Attachment is obsolete: true
Attachment #8715394 - Attachment is obsolete: true
Attachment #8715409 - Attachment is obsolete: true
Attachment #8715442 - Attachment is obsolete: true
Attachment #8715445 - Attachment is obsolete: true
Attachment #8715449 - Attachment is obsolete: true
Created attachment 8715969 [details] [diff] [review]
Enable the browser_memory_simple_01.js test on DEBUG builds
Attachment #8715969 - Flags: review+
Attachment #8715968 - Attachment is obsolete: true
Created attachment 8715970 [details] [diff] [review]
Enable the browser_memory_breakdowns_01.js test on DEBUG builds
Attachment #8715970 - Flags: review+
Created attachment 8715971 [details] [diff] [review]
Enable the browser_memory_clear_snapshots.js test on DEBUG builds
Attachment #8715971 - Flags: review+
Created attachment 8715972 [details] [diff] [review]
Enable the browser_memory_diff_01.js test on DEBUG builds
Attachment #8715972 - Flags: review+
Created attachment 8715973 [details] [diff] [review]
Enable the browser_memory_filter_01.js test on DEBUG builds
Attachment #8715973 - Flags: review+
Created attachment 8715974 [details] [diff] [review]
Clean up the browser_memory_no_auto_expand.js test
Attachment #8715974 - Flags: review+
Created attachment 8715975 [details] [diff] [review]
Remove the unused waitUntilSnapshotState test utility
Attachment #8715975 - Flags: review+
Created attachment 8715976 [details] [diff] [review]
Follow up: clean up memory panel open/destroy

This commit removes the assignment expression in a return statement thing that
was going on. It is easy to misread and we don't usually use this idiom anywhere
else in the codebase. Additionally, it makes the destroy method clean up some
more members set when opening the tool.
Attachment #8715976 - Flags: review+
Created attachment 8715977 [details] [diff] [review]
Follow up: HeapAnalysesClient should set its Worker reference null upon destruction

This makes attempts to use the HeapAnalysesClient after the worker has already
been destroyed into an earlier, less quiet error so we can catch more of them
and catch them sooner.
Attachment #8715977 - Flags: review+
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b729094af3a0
checkin-needed for all patches attached, please.
Keywords: checkin-needed

Comment 33

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/ab299cfe29c8
https://hg.mozilla.org/integration/fx-team/rev/59172c886f4e
https://hg.mozilla.org/integration/fx-team/rev/bbdae693b8db
https://hg.mozilla.org/integration/fx-team/rev/06908e33c209
https://hg.mozilla.org/integration/fx-team/rev/dc5a5cd40d67
https://hg.mozilla.org/integration/fx-team/rev/aaa82c7efa4e
https://hg.mozilla.org/integration/fx-team/rev/03c8e1c949a4
https://hg.mozilla.org/integration/fx-team/rev/61b3d4d0dfd4
https://hg.mozilla.org/integration/fx-team/rev/50e29526c717
Keywords: checkin-needed
Attachment #8715969 - Flags: checkin+
Attachment #8715970 - Flags: checkin+
Attachment #8715971 - Flags: checkin+
Attachment #8715972 - Flags: checkin+
Attachment #8715973 - Flags: checkin+
Attachment #8715974 - Flags: checkin+
Attachment #8715975 - Flags: checkin+
Attachment #8715976 - Flags: checkin+
Attachment #8715977 - Flags: checkin+
Depends on: 1245739
Since we have most of the tests on DEBUG builds now, I don't think this is quite as urgent as it previously was.
Priority: P1 → P2

Comment 35

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab299cfe29c8
https://hg.mozilla.org/mozilla-central/rev/59172c886f4e
https://hg.mozilla.org/mozilla-central/rev/bbdae693b8db
https://hg.mozilla.org/mozilla-central/rev/06908e33c209
https://hg.mozilla.org/mozilla-central/rev/dc5a5cd40d67
https://hg.mozilla.org/mozilla-central/rev/aaa82c7efa4e
https://hg.mozilla.org/mozilla-central/rev/03c8e1c949a4
https://hg.mozilla.org/mozilla-central/rev/61b3d4d0dfd4
https://hg.mozilla.org/mozilla-central/rev/50e29526c717
Assignee: nfitzgerald → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.