I made a try run that: * In FxAccountsCommon.js defaulted the FxA log to "Debug" level. * In FxAccountsWebChannel.jsm logged a debug message in tearDown: log.debug("FxAccountsWebChannel unregistered: " + this._webChannelId); to match the one when it is registered. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=41e537d83214 and the raw log of the failure. The logs have 114 occurrences of "FxAccountsWebChannel registered" but only 3 of "FxAccountsWebChannel unregistered". I also locally instrumented WebChannel.jsm to show how many channels are in its map and that too reflects the channels being created many times and never torn down. I haven't dug into how representative this is in a normal Firefox run, but at face value it seems bad.
It looks like browser-fxaccounts loads a new channel and never destroys it. Thus each new browser window leaks it. The simplest option is probably to avoid exposing the FxAccountsWebChannel (although tests will still be able to reach into it) and replace it with a function like ensureChannelListening() which creates a single instance - it's probably fine for that to never be destroyed.
Created attachment 8636886 [details] [diff] [review] 0001-Bug-1184825-treat-FxAccountsWebChannel-as-a-singleto.patch FxAccountsWebChannel.jsm now exports a function EnsureFxAccountsWebChannel that ensures only one instance is ever created. Tests can still access the constructor.
Assignee: nobody → markh
Status: NEW → ASSIGNED
Attachment #8636886 - Flags: review?(stomlinson)
Comment on attachment 8636886 [details] [diff] [review] 0001-Bug-1184825-treat-FxAccountsWebChannel-as-a-singleto.patch Review of attachment 8636886 [details] [diff] [review]: ----------------------------------------------------------------- Aside from the request to add more context about why a singleton is needed, r+. Thanks for cleaning up my mess Mark. ::: browser/base/content/test/general/browser_fxa_web_channel.js @@ +11,5 @@ > > XPCOMUtils.defineLazyModuleGetter(this, "WebChannel", > "resource://gre/modules/WebChannel.jsm"); > > +// FxAccountsWebChannel isn't explicitly exported by FxAccountsWebChannel.jsm I found this a bit weird, but I understand the intention of only exporting "EnsureFxAccountsWebChannel" in FxAccountsWebChannel.jsm. This is a strong indicator to folks that they should not directly create a FxAccountsWebChannel. ::: services/fxaccounts/FxAccountsWebChannel.jsm @@ +313,5 @@ > }; > + > +let singleton; > +// The entry-point for this module. It initializes a singleton > +// FxAccountsWebChannel. Can you make a note about why a singleton is needed, so nobody regresses this behavior?
Attachment #8636886 - Flags: review?(stomlinson) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.