Closed Bug 966823 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

(Whiteboard: [qa?])

Attachments

(1 file)

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)
Check if sync is initialized before referencing Weave.Service
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Attachment #8369271 - Flags: review?(ttaubert)
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 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?
Attachment #8369271 - Flags: review?(ttaubert) → review+
Opened bug 966824 for the latent valgrind problem.

Checked into central: https://hg.mozilla.org/mozilla-central/rev/5e41335ed0a4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [qa?]
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
Attachment #8369271 - Flags: approval-mozilla-aurora?
Attachment #8369271 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No need to uplift, looks like this made it into 29.
Target Milestone: --- → mozilla29
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.