1305836 - Remove CCAnalyzer leak checker

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

When reviewing, looking at the initial commit that added the CC analyzer might be useful: https://hg.mozilla.org/mozilla-central/rev/c3b7cfd70d378d78e84cfcb256f3b6962eef3970

Comment 4

[Andrew McCreight [:mccr8]]

Try run with the 6 GC+CC at the end:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cee87aef5a4

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1305836, part 1 - Remove CCAnalyzer leak checker.

This leak checker may be triggering a shutdown leak on Windows,
doesn't work with e10s, and should not be needed now that ttaubert
fixed the ++DOMWINDOW leak detector to work.

The additional GCs and CCs this patch adds used to be run as part of
cc-analyzer.js, and are needed to avoid window leaks in tests.

Review commit: https://reviewboard.mozilla.org/r/81896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81896/

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Bug 1305836, part 2 - Remove openedWindows, openedURLs and onDocumentCreated.

These are now unused.

Review commit: https://reviewboard.mozilla.org/r/81898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81898/

Comment 7

[Jonathan Griffin (:jgriffin)]

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

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8795930 [details]
Bug 1305836, part 2 - Remove openedWindows, openedURLs and onDocumentCreated.

https://reviewboard.mozilla.org/r/81898/#review80692

Comment 9

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8795929 [details]
Bug 1305836, part 1 - Remove CCAnalyzer leak checker.

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

Comment 10

[Andrew McCreight [:mccr8]]

(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 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8795929 [details]
Bug 1305836, part 1 - Remove CCAnalyzer leak checker.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81896/diff/1-2/

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 8795930 [details]
Bug 1305836, part 2 - Remove openedWindows, openedURLs and onDocumentCreated.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81898/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Andrew McCreight [:mccr8]]

Ryan, you could do a beta try run if you want to see if this fixes the permaorange in bug 1220517.

Comment 15

[Phil Ringnalda (:philor)]

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?

Comment 16

[Andrew McCreight [:mccr8]]

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!

Comment 17

[Andrew McCreight [:mccr8]]

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.

Comment 18

[Andrew McCreight [:mccr8]]

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.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

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?!)

Comment 20

[Andrew McCreight [:mccr8]]

It turns out Blake just removed that test entirely in bug 1304531. That solves that problem!

Comment 21

[Andrew McCreight [:mccr8]]

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.

Comment 22

[Mike Conley (: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!

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/releases/mozilla-beta/rev/69df4d73eeee9b696581a0936ce71e939812641d - it's already gone (and gone on aurora too).

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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 25

[Andrew McCreight [:mccr8]]

Comment on attachment 8795929 [details]
Bug 1305836, part 1 - Remove CCAnalyzer leak checker.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81896/diff/2-3/

Comment 26

[Andrew McCreight [:mccr8]]

Comment on attachment 8795930 [details]
Bug 1305836, part 2 - Remove openedWindows, openedURLs and onDocumentCreated.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81898/diff/2-3/

Comment 27

[Andrew McCreight [:mccr8]]

I'm asking for review for the second part to just make sure I removed things correctly from your patch, Blake.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8795930 [details]
Bug 1305836, part 2 - Remove openedWindows, openedURLs and onDocumentCreated.

https://reviewboard.mozilla.org/r/81898/#review82314

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9657b1da6f78
https://hg.mozilla.org/mozilla-central/rev/0251849dcbb7

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d1882f207837
https://hg.mozilla.org/releases/mozilla-aurora/rev/dfa484769f77

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ea37484255cb
https://hg.mozilla.org/releases/mozilla-beta/rev/419f4f6f0f68

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/5d79acd6454f
https://hg.mozilla.org/releases/mozilla-esr45/rev/21c83140b2a3