723953 - Fennec chrome scripts and tests leak global variables

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

+++ This bug was initially created as a clone of Bug #720985 +++

XUL-Fennec browser-chrome tests went permaorange when bug 720985 landed because they leak global variables:
https://tbpl.mozilla.org/php/getParsedLog.php?id=9059420&tree=Firefox

These variables have been temporarily added to the whitelist, but we should fix the tests to stop them from leaking.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch

I haven't tested this yet.  Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=046cb8306f36

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 594244 [details] [diff] [review]
patch

So DownloadUtils and AddonRepository should remain whitelisted? Why?

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to Dão Gottwald [:dao] from comment #2)
> Comment on attachment 594244 [details] [diff] [review]
> patch
> 
> So DownloadUtils and AddonRepository should remain whitelisted? Why?

These are loaded lazily by Fennec chrome after the first content document has loaded.  Because of this, they are showing up as "leaked" by whichever test runs first.  I'm still thinking about any better way around this.  Suggestions?

Comment 4

[Dão Gottwald [:dao]]

How and where are they loaded lazily? An ordinary lazy getter wouldn't have this problem since getters appear in the keys list, right?

Comment 5

[Matt Brubeck (:mbrubeck)]

One is imported from downloads.js:
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/downloads.js#38

And the other from a lazy importer in extensions.js:
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/extensions.js#57

The problem is that these files are themselves lazily loaded after startup, here:
https://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser-scripts.js#99

Creating the DownloadUtils and AddonRepository getters directly in browser-scripts.js at startup, instead of within other lazily-loaded files, would probably solve this problem.  I'll try that next.

My Try push still had two errors, so it looks like I also failed to fix these tests:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/mobile/chrome/tests/browser_localerepository.js | leaked window property: LocaleRepository
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/mobile/chrome/tests/browser_vkb.js | leaked window property: VKBstateHasChanged

http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-046cb8306f36/try-android-xul/try_tegra_android-xul_test-browser-chrome-build506.txt.gz

Comment 6

[Dão Gottwald [:dao]]

(In reply to Matt Brubeck (:mbrubeck) from comment #5)
> One is imported from downloads.js:
> http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/
> downloads.js#38
> 
> And the other from a lazy importer in extensions.js:
> http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/
> extensions.js#57
> 
> The problem is that these files are themselves lazily loaded after startup,
> here:
> https://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/
> browser-scripts.js#99

They are explicitly loaded in a sandbox, though, so the getters leaking outside seems unintentional.

> Creating the DownloadUtils and AddonRepository getters directly in
> browser-scripts.js at startup, instead of within other lazily-loaded files,
> would probably solve this problem.  I'll try that next.

Yes, if you actually want these objects there.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Matt Brubeck (:mbrubeck) from comment #5)
> My Try push still had two errors, so it looks like I also failed to fix
> these tests:

> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/mobile/chrome/tests/browser_vkb.js |
> leaked window property: VKBstateHasChanged

This is a typo, VKBstateHasChanged vs. VKBStateHasChanged.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 612975 [details] [diff] [review]
patch v2

Thanks for following up on this, Dão!

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bb1eebb47fc3

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/bb1eebb47fc3