966823 - browser-fxaccounts.js causes sync to fully initialize too early

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

Checking if there is a login error inadvertently causes sync to fully initialize as the browser starts up.  It shouldn't.  This is causing valgrind tests to report leaks (bug 966136)

Comment 1

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

Attachment: 0001-Bug-966823-ensure-checking-if-there-is-a-login-error.patch

Check if sync is initialized before referencing Weave.Service

Comment 2

[Nicholas Nethercote [inactive]]

I tested this patch locally and it caused the leak report to go away, which is good.

Having said that, the leak is still present, and probably has been for a long time. (What changed was the Sync code being run at start-up, and thus falling under the coverage of the Valgrind test job.) A follow-up for that would be good.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8369271 [details] [diff] [review]
0001-Bug-966823-ensure-checking-if-there-is-a-login-error.patch

Review of attachment 8369271 [details] [diff] [review]:
-----------------------------------------------------------------

thx!

::: browser/base/content/browser-fxaccounts.js
@@ +50,5 @@
>    get loginFailed() {
> +    let service = Cc["@mozilla.org/weave/service;1"]
> +                  .getService(Components.interfaces.nsISupports)
> +                  .wrappedJSObject;
> +    if (!service.ready) {

if (!this.weave.ready) {

Also can you please add a comment that this is to avoid early sync init?

Comment 4

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

Opened bug 966824 for the latent valgrind problem.

Checked into central: https://hg.mozilla.org/mozilla-central/rev/5e41335ed0a4

Comment 5

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

Comment on attachment 8369271 [details] [diff] [review]
0001-Bug-966823-ensure-checking-if-there-is-a-login-error.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FxA Sync
User impact if declined: Slower startup time for all users.
Testing completed (on m-c, etc.): m-c along time ago!
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

No need to uplift, looks like this made it into 29.