Closed Bug 1184825 Opened 4 years ago Closed 4 years ago

Too many FxA WebChannels?


(Firefox :: Firefox Accounts, defect, P1)




Firefox 42
Tracking Status
firefox42 --- fixed


(Reporter: markh, Assigned: markh)



(1 file)

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 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.
FxAccountsWebChannel.jsm now exports a function EnsureFxAccountsWebChannel that ensures only one instance is ever created. Tests can still access the constructor.
Assignee: nobody → markh
Attachment #8636886 - Flags: review?(stomlinson)
Comment on attachment 8636886 [details] [diff] [review]

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+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Product: Core → Firefox
Target Milestone: mozilla42 → Firefox 42
You need to log in before you can comment on or make changes to this bug.