Closed Bug 1219554 Opened 4 years ago Closed Last year

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

Categories

(DevTools :: Memory, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 10 obsolete files)

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
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
I thought bug 1226217 would fix this, but it turns out not to.
See Also: → 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.
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
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
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)
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+
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
Attachment #8715968 - Attachment is obsolete: true
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+
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+
checkin-needed for all patches attached, please.
Keywords: checkin-needed
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
Assignee: nfitzgerald → nobody
Status: ASSIGNED → NEW
Product: Firefox → DevTools
The leave-open keyword is there and there is no activity for 6 months.
:julienw, maybe it's time to close this bug?
Flags: needinfo?(felash)
I believe all patches landed here, let's close this and file new bugs if needed.
Status: NEW → RESOLVED
Closed: Last year
Flags: needinfo?(felash)
Resolution: --- → FIXED
Assignee: nobody → nfitzgerald
You need to log in before you can comment on or make changes to this bug.