1184825 - Too many FxA WebChannels?

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 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.

Comment 3

[Shane Tomlinson [: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?

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5f1d69c03e3d

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f1d69c03e3d