Closed Bug 1041537 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 34
Iteration:
34.3

People

(Reporter: jmaher, Assigned: ttaubert)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Gavin, Can you help find an owner for this bug and prioritize it as you see fit?
Flags: needinfo?(gavin.sharp)
I'm adding this bug to our priority backlog for the next iteration, so we can expect it to be assigned August 5th.
Flags: needinfo?(gavin.sharp)
Flags: firefox-backlog+
Points: --- → 5
QA Whiteboard: [qa-]
Assignee: nobody → dao
Status: NEW → ASSIGNED
Iteration: --- → 34.2
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?
Component: General → New Tab Page
Flags: needinfo?(ttaubert)
Summary: browser_Browser.js fails quite frequently on linux and windows debug runs when running the fuel/test/ directory by itself → browser_Browser.js leaks windows frequently on linux and windows debug runs when running the fuel/test/ directory by itself
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...
Flags: needinfo?(ttaubert)
(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?
Flags: needinfo?(ttaubert)
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.
Flags: needinfo?(ttaubert)
Filed bug 1052371 and attached a patch.
Component: New Tab Page → General
OS: Linux → All
Pushed to try again with the leak detector and some DT leaks fixed:

https://tbpl.mozilla.org/?tree=Try&rev=f36e2f158b00
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.
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.
Iteration: 34.2 → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
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
Awesome stuff ttaubert! This might knock out a few top leaks.
Stealing, talked to Dão about this before.
Assignee: dao → ttaubert
Turns out it's the BrowserNewTabPreloader. Attempt to fix:

https://tbpl.mozilla.org/?tree=Try&rev=66b7a011e775
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.
Component: General → New Tab Page
Component: New Tab Page → Search
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.
Attachment #8476139 - Attachment is obsolete: true
Attachment #8476139 - Flags: review?(adw)
Attachment #8476541 - Flags: review?(adw)
this latest try push is pretty solid.
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.
Attachment #8476541 - Flags: review?(adw) → review+
Duplicate of this bug: 1041544
(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.
Depends on: 1057386
Duplicate of this bug: 1017187
Duplicate of this bug: 1041549
Duplicate of this bug: 1041583
Duplicate of this bug: 1041569
https://hg.mozilla.org/mozilla-central/rev/885c0c395a10
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Depends on: 1060639
You need to log in before you can comment on or make changes to this bug.