Closed Bug 723953 Opened 8 years ago Closed 8 years ago

Fennec chrome scripts and tests leak global variables

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 14

People

(Reporter: mbrubeck, Assigned: dao)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ 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.
Attached patch patch (obsolete) — Splinter Review
I haven't tested this yet.  Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=046cb8306f36
Attachment #594244 - Attachment is patch: true
Comment on attachment 594244 [details] [diff] [review]
patch

So DownloadUtils and AddonRepository should remain whitelisted? Why?
(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?
How and where are they loaded lazily? An ordinary lazy getter wouldn't have this problem since getters appear in the keys list, right?
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/mbrubeck@mozilla.com-046cb8306f36/try-android-xul/try_tegra_android-xul_test-browser-chrome-build506.txt.gz
Status: NEW → ASSIGNED
(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.
(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.
Attached patch patch v2Splinter Review
Assignee: mbrubeck → dao
Attachment #612975 - Flags: review?(mbrubeck)
Attachment #594244 - Attachment is obsolete: true
Version: Firefox 13 → Trunk
Comment on attachment 612975 [details] [diff] [review]
patch v2

Thanks for following up on this, Dão!
Attachment #612975 - Flags: review?(mbrubeck) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb1eebb47fc3
Summary: Fennec tests leak global variables → Fennec chrome scripts and tests leak global variables
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/bb1eebb47fc3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.