1041537 - browser_Browser.js leaks windows frequently on linux and windows debug runs when running the fuel/test/ directory by itself

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

in bug 992911, we have been doing work to get our unit tests running per directory so we can split things up easier in the future when needed.  In testing this out on try server, we find that on linux (32+64) we get a perma fail (bc2):
https://tbpl.mozilla.org/?tree=Try&rev=017a84179f10

where we have leaked 1 window until shutdown:
https://tbpl.mozilla.org/php/getParsedLog.php?id=44177338&tree=Try

This is common in windows debug (mostly windows 7) as well in other try pushes (example: https://tbpl.mozilla.org/?tree=Try&rev=677baf422c94)

In 2 weeks we want to switch to this per directory mode, I would like to resolve as many of the few remaining tests as possible so we don't disable them.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Gavin, Can you help find an owner for this bug and prioritize it as you see fit?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

I'm adding this bug to our priority backlog for the next iteration, so we can expect it to be assigned August 5th.

Comment 3

[Dão Gottwald [:dao]]

It looks like this doesn't have much to do with browser_Browser.js per se. This is the leaked window:

data:application/vnd.mozilla.xul+xml;charset=utf-8,<window%20id='win'/>

Both BrowserNewTabPreloader.jsm and CustomizationTabPreloader.jsm create such windows. I suspect the former is to blame here.

Tim, as the creator and de-facto owner of BrowserNewTabPreloader.jsm, do you have an idea about what exactly might be going on here?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

I dug a little deeper into this and I'm pretty sure that bug 886570 disabled the leak detector. The leaks might be due to ShutdownLeaks.process() being called earlier now than before. Not sure though, need to try to fix the stuff from bug 886570 first...

Comment 5

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #4)
> I dug a little deeper into this and I'm pretty sure that bug 886570 disabled
> the leak detector. The leaks might be due to ShutdownLeaks.process() being
> called earlier now than before. Not sure though, need to try to fix the
> stuff from bug 886570 first...

It would probably make sense to file that as a new bug? If so can you please do that and make it block this one?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

I unfortunately wasn't able to make much progress here. Fixing the shutdown leak detector didn't yield any more leaks but maybe that's just pure luck. I'll file a bug on getting this fixed. I can reproduce some of the leaks locally but can't figure out why they occur.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Filed bug 1052371 and attached a patch.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Pushed to try again with the leak detector and some DT leaks fixed:

https://tbpl.mozilla.org/?tree=Try&rev=f36e2f158b00

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

we do have one failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46250852&tree=Try

I kicked off some retriggers, we can see how consistent it is.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

The leaks we see in the try push all affect the last test in a test directory:

browser/fuel/test/browser_Browser.js
content/base/test/browser_bug902350.js
browser/components/safebrowsing/content/test/browser_bug400731.js
toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js

except:

browser/components/loop/test/mochitest/browser_mozLoop_appVersionInfo.js

I'm thinking whether it might be something like bug 995266... but I can't see how that might be possible.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Pushed to try with another guess:

https://tbpl.mozilla.org/?tree=Try&rev=5c0f2d010e53

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Ok, that didn't work. Pushed again to try to find out which preloader's frame we're leaking here:

https://tbpl.mozilla.org/?tree=Try&rev=80e316611627

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Awesome stuff ttaubert! This might knock out a few top leaks.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Stealing, talked to Dão about this before.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Turns out it's the BrowserNewTabPreloader. Attempt to fix:

https://tbpl.mozilla.org/?tree=Try&rev=66b7a011e775

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Was able to reproduce it locally after lots of trying. Seems to be caused by about:newtab's search field. Need to dig into it more but this shouldn't be too hard from here on.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Let's see:

https://tbpl.mozilla.org/?tree=Try&rev=18262f50339b

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1041537-Prevent-ContentSearch-from-leaking-the-b.patch

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1041537-Prevent-ContentSearch-from-leaking-the-b.patch, v2

How's that? We don't really need to do anything for normal shutdowns. We however must give the test suite the possibility to destroy ContentSearch. By making that async we can just wait until the current message is processed.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=345a4505777b

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

this latest try push is pretty solid.

Comment 22

[Drew Willcoxon :adw]

Comment on attachment 8476541 [details] [diff] [review]
0001-Bug-1041537-Prevent-ContentSearch-from-leaking-the-b.patch, v2

Review of attachment 8476541 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, I like this better.

I took a look at how leaks are observed in browser-test.js.  To free permanent resources held onto by services, I wonder if it might be better for services to be able to register cleanup functions that happen before the harness's leak detection, like an AsyncShutdown.beforeTestLeakDetection (or call it beforeEarlyShutdown or something), instead of having to make the harness know about them.  And to free things that are not permanent but are in use, like the ContentSearch promises here, I wonder if the harness could somehow drain the app's event queue and add a barrier.  Guess that would require deep changes.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Drew Willcoxon :adw from comment #22)
> I took a look at how leaks are observed in browser-test.js.  To free
> permanent resources held onto by services, I wonder if it might be better
> for services to be able to register cleanup functions that happen before the
> harness's leak detection, like an AsyncShutdown.beforeTestLeakDetection (or
> call it beforeEarlyShutdown or something), instead of having to make the
> harness know about them.

I'm not exactly sure how to do that so that it doesn't break in non-test mode but using AsyncShutdown might indeed be an option. I can file a follow-up.

> And to free things that are not permanent but are
> in use, like the ContentSearch promises here, I wonder if the harness could
> somehow drain the app's event queue and add a barrier.  Guess that would
> require deep changes.

Yeah that sounds like it would require bigger changes. Might not be worth it.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/885c0c395a10

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/885c0c395a10