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

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P2)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

STR:

Run browser mochitests on a DEBUG build.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I thought bug 1226217 would fix this, but it turns out not to.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

The leak itself isn't hugely worrying to me, but this is pretty important since it is reducing our test coverage.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Enable the browser_memory_simple_01.js test on DEBUG builds

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5511fee015f5

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Enable the browser_memory_breakdowns_01.js test on DEBUG builds

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Enable the browser_memory_clear_snapshots.js test on DEBUG builds

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbb90b00eb9a

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Enable the browser_memory_diff_01.js test on DEBUG builds

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb33e2926c51

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Enable the browser_memory_filter_01.js test on DEBUG builds

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3393b697d2e1

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Enable the browser_memory_no_allocation_stacks.js test on DEBUG builds

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

This is weird. We should remove the utility function completely if it's leaking though so we don't have this again

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Remove the unused waitUntilSnapshotState test utility

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: 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.

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: 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.

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: 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.

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Enable the browser_memory_simple_01.js test on DEBUG builds

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Enable the browser_memory_breakdowns_01.js test on DEBUG builds

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Enable the browser_memory_clear_snapshots.js test on DEBUG builds

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Enable the browser_memory_diff_01.js test on DEBUG builds

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Enable the browser_memory_filter_01.js test on DEBUG builds

Comment 27

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Clean up the browser_memory_no_auto_expand.js test

Comment 28

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Remove the unused waitUntilSnapshotState test utility

Comment 29

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: 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.

Comment 30

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: 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.

Comment 31

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b729094af3a0

Comment 32

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

checkin-needed for all patches attached, please.

Comment 33

[Pulsebot]

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

Comment 34

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Since we have most of the tests on DEBUG builds now, I don't think this is quite as urgent as it previously was.

Comment 35

[Phil Ringnalda (:philor)]

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

Comment 36

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:julienw, maybe it's time to close this bug?

Comment 37

[Julien Wajsberg [:julienw]]

I believe all patches landed here, let's close this and file new bugs if needed.