Remove CCAnalyzer leak checker

RESOLVED FIXED in Firefox -esr45

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox-esr45 fixed, firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

3 years ago
The CCAnalyzer is a neat in-browser leak checker that tracks what windows are open, and then looks at the CC logs to try to figure out whether the windows are alive or not. I believe it was initially implemented because of problems with the window leak checker based on ++DOMWINDOW, but ttaubert eventually fixed those issues. The CCAnalyzer was never updated to work with e10s (bug 1096614), and may be causing shut down leaks in automation (bug 1220517) by dumping a ton of events into the queue late in shut down, so I'm going to remove it and hope that helps with the intermittent.
Assignee

Comment 1

3 years ago
This whole mechanism ends up GCing up to 8 times, and CCing up to 6 times (though only in the parent process). It is an open question if I can remove any of these without causing leaks in browser chrome tests. 

I tried removing them all, but I ended up with leaks in the GPU tests, plus some other bc test directory I ran locally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75b487c4efdd

A version that does 2 GCs but drops the rest appears to be a little leaky already: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f8db49a2adf&selectedJob=28132091

Of course, since the number of times is variable, I may end up increasing how long tests take to run if I do them every time. Hopefully a few extra CCs and GCs isn't too much. I could also try checking if we ever actually do the second cycle in practice, or if we almost always do, in which case I don't need to worry about it too much.
Assignee

Comment 2

3 years ago
In local testing, adding 6 GC+CC did not make the test run any longer, and fixed some leaking windows, when running browser/components/privatebrowsing/test/ browser-chrome. Hopefully this will resolve the issues.
Assignee

Comment 3

3 years ago
When reviewing, looking at the initial commit that added the CC analyzer might be useful: https://hg.mozilla.org/mozilla-central/rev/c3b7cfd70d378d78e84cfcb256f3b6962eef3970
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

3 years ago
mozreview-review
Comment on attachment 8795929 [details]
Bug 1305836, part 1 - Remove CCAnalyzer leak checker.

https://reviewboard.mozilla.org/r/81896/#review80688

::: testing/mochitest/browser-test.js:641
(Diff revision 1)
>          // use a shrinking GC so that the JS engine will discard JIT code and
>          // JIT caches more aggressively.
>  
> -        let checkForLeakedGlobalWindows = aCallback => {
> +        let shutdownCleanup = aCallback => {
>            Cu.schedulePreciseShrinkingGC(() => {
> -            let analyzer = new CCAnalyzer();
> +            let numCycles = 3;

Why is 3 the magic number here? Can we have a comment?

Comment 8

3 years ago
mozreview-review
Comment on attachment 8795930 [details]
Bug 1305836, part 2 - Remove openedWindows, openedURLs and onDocumentCreated.

https://reviewboard.mozilla.org/r/81898/#review80692
Attachment #8795930 - Flags: review?(jgriffin) → review+

Comment 9

3 years ago
mozreview-review
Comment on attachment 8795929 [details]
Bug 1305836, part 1 - Remove CCAnalyzer leak checker.

https://reviewboard.mozilla.org/r/81896/#review80694
Attachment #8795929 - Flags: review?(jgriffin) → review+
Assignee

Comment 10

3 years ago
(In reply to Jonathan Griffin (:jgriffin) from comment #7)
> Why is 3 the magic number here? Can we have a comment?

There's not too much of a reason that it has to be 3 in particular, but I'll add a comment about how we need to run it a few times, so it doesn't look like total magic.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

3 years ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89a22098629e
part 1 - Remove CCAnalyzer leak checker. r=jgriffin
https://hg.mozilla.org/integration/autoland/rev/578a4588fb7c
part 2 - Remove openedWindows, openedURLs and onDocumentCreated. r=jgriffin
Assignee

Comment 14

3 years ago
Ryan, you could do a beta try run if you want to see if this fixes the permaorange in bug 1220517.
Flags: needinfo?(ryanvm)
Clownshoes, nothing but clownshoes, clownshoes all the way down.

This landed on bc7 permaorange, which makes it sort of difficult to look at, but https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=2a1bee2daee5fa69e56458047d4032c08b08dfe8&tochange=578a4588fb7cb7c6140edeb268c9f0c3cc13edc3&fromchange=28d81d95566ba38cb985b671b27a8cb021386b99 is this and its parent, with the parent having permaorange in browser_trackingUI_1.js, and this having that permaorange in every run, but having four runs which also hit the intermittent bug 1298475, which is a complete explosion in the failure rate for it, https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1298475

Backed out in https://hg.mozilla.org/integration/autoland/rev/6f31b6753379 for, um, for, uh, for making e10s browser-chrome a little faster than it was, maybe?
Assignee

Comment 16

3 years ago
Argh. That even showed up in the try run, I just saw the existing bug, and the orange didn't look related to my patch. I guess it was!
Flags: needinfo?(ryanvm)
Assignee

Comment 17

3 years ago
When I run the test by itself locally, it always fails, unless I disable e10s. The bug that added the test has the comment "I think, in an e10s world the whole problem we're fixing here would actually not exist" so maybe this test should be disabled with e10s? I'll have to investigate that.
Assignee

Comment 18

3 years ago
Felipe, can I disable browser/base/content/test/general/browser_bug880101.js with e10s? Looking at the test, it looks like it would be pretty racey under e10s (it doesn't use any of the BrowserTestUtils functions to load things, for instance), and I suspect my patch that makes finishing a test faster somehow just makes it fail all the time when run with other tests, rather than just failing all the time when run by itself. I started looking at fixing it myself, but the test has a comment "on the next tick, but before the window has finished loading, access the window's gBrowser property to force the tabbrowser constructor early" that I don't understand at all, so I'm not sure what it is even really testing. I could file a bug about re-enabling it. Thanks.
Flags: needinfo?(felipc)
I think you can, but let me redirect the question to mconley who has been more involved with this code lately (and also oversaw another similar test removed today?!)
Flags: needinfo?(felipc) → needinfo?(mconley)
Assignee

Comment 20

3 years ago
It turns out Blake just removed that test entirely in bug 1304531. That solves that problem!
Flags: needinfo?(mconley)
Assignee

Updated

3 years ago
Depends on: 1304531
Assignee

Comment 21

3 years ago
Oh right, but I want this on earlier branches. mconley, is it okay if I backport the deletion of browser_bug880101.js to Aurora and Beta? Or just disable it for e10s there? My patch here (which is intended to fix a permaorange on Beta) is causing the test to fail with e10s.
Flags: needinfo?(mconley)
(In reply to Andrew McCreight [:mccr8] from comment #21)
> is it okay if I
> backport the deletion of browser_bug880101.js to Aurora and Beta?

Let's remove it. I don't think that test has any value anymore. Thanks!
Flags: needinfo?(mconley)
This needs a bit of rebasing around bug 1304531, but a Try push of these patches on top of Beta is looking encouraging!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1edef766059b&noautoclassify
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 27

3 years ago
I'm asking for review for the second part to just make sure I removed things correctly from your patch, Blake.
Comment on attachment 8795930 [details]
Bug 1305836, part 2 - Remove openedWindows, openedURLs and onDocumentCreated.

https://reviewboard.mozilla.org/r/81898/#review82314
Attachment #8795930 - Flags: review?(mrbkap) → review+

Comment 29

3 years ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9657b1da6f78
part 1 - Remove CCAnalyzer leak checker. r=jgriffin
https://hg.mozilla.org/integration/autoland/rev/0251849dcbb7
part 2 - Remove openedWindows, openedURLs and onDocumentCreated. r=jgriffin,mrbkap

Comment 30

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9657b1da6f78
https://hg.mozilla.org/mozilla-central/rev/0251849dcbb7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1308998
You need to log in before you can comment on or make changes to this bug.