Too many FxA WebChannels?

RESOLVED FIXED in Firefox 42

Status

()

P1
normal
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: markh, Assigned: markh)

Tracking

unspecified
Firefox 42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

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+
https://hg.mozilla.org/mozilla-central/rev/5f1d69c03e3d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

10 months ago
Product: Core → Firefox
Target Milestone: mozilla42 → Firefox 42
You need to log in before you can comment on or make changes to this bug.