Closed
Bug 1184825
Opened 9 years ago
Closed 9 years ago
Too many FxA WebChannels?
Categories
(Firefox :: Firefox Accounts, defect, P1)
Firefox
Firefox Accounts
Tracking
()
RESOLVED
FIXED
Firefox 42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: markh, Assigned: markh)
Details
Attachments
(1 file)
5.30 KB,
patch
|
stomlinson
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
FxAccountsWebChannel.jsm now exports a function EnsureFxAccountsWebChannel that ensures only one instance is ever created. Tests can still access the constructor.
Comment 3•9 years ago
|
||
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+
Comment 5•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Updated•7 years ago
|
Product: Core → Firefox
Updated•7 years ago
|
Target Milestone: mozilla42 → Firefox 42
You need to log in
before you can comment on or make changes to this bug.
Description
•