Delay Weave import

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Sync
P2
normal
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: eoger, Assigned: eoger)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

59 bytes, text/x-review-board-request
kitcambridge
: review+
Details | Review
(Assignee)

Description

a month ago
See bug 1358798, we don't need to initialize Sync that early, we can do it in nsBrowserGlue#_finalUIStartup instead.
Comment hidden (mozreview-request)
Comment on attachment 8867354 [details]
Bug 1364571 - Delay Weave startup.

https://reviewboard.mozilla.org/r/138880/#review142218

LGTM! We don't use this on Android, SeaMonkey is already broken because it doesn't support FxA, and Graphene and B2G are no longer a thing.

::: browser/components/nsBrowserGlue.js:21
(Diff revision 1)
>  
>  XPCOMUtils.defineLazyServiceGetter(this, "WindowsUIUtils", "@mozilla.org/windows-ui-utils;1", "nsIWindowsUIUtils");
>  XPCOMUtils.defineLazyServiceGetter(this, "AlertsService", "@mozilla.org/alerts-service;1", "nsIAlertsService");
> +XPCOMUtils.defineLazyGetter(this, "WeaveService", () => {
> +  return Components.classes["@mozilla.org/weave/service;1"]
> +                   .getService(Components.interfaces.nsISupports)

Nit: `Cc` instead of `Components.classes`, `Ci` instead of `Components.interfaces`?
Attachment #8867354 - Flags: review?(kit) → review+

Comment 3

a month ago
mozreview-review
Comment on attachment 8867354 [details]
Bug 1364571 - Delay Weave startup.

https://reviewboard.mozilla.org/r/138880/#review142234

Thanks for looking at this! final-ui-startup is right before the first window is shown, that's still too early. Given that all this does is add a 10s timer to then do stuff later, it should really wait until the first window is done initializing. "browser-delayed-startup-finished" /  _onFirstWindowLoaded in nsBrowserGlue.js would be a much better place to start this.

::: browser/components/nsBrowserGlue.js:21
(Diff revision 1)
>  
>  XPCOMUtils.defineLazyServiceGetter(this, "WindowsUIUtils", "@mozilla.org/windows-ui-utils;1", "nsIWindowsUIUtils");
>  XPCOMUtils.defineLazyServiceGetter(this, "AlertsService", "@mozilla.org/alerts-service;1", "nsIAlertsService");
> +XPCOMUtils.defineLazyGetter(this, "WeaveService", () => {
> +  return Components.classes["@mozilla.org/weave/service;1"]
> +                   .getService(Components.interfaces.nsISupports)

nit: I think getService() (ie. without parameter) is equivalent to getting the nsISupports interface.

::: browser/components/nsBrowserGlue.js:670
(Diff revision 1)
>      TabCrashHandler.init();
>      if (AppConstants.MOZ_CRASHREPORTER) {
>        PluginCrashReporter.init();
>        UnsubmittedCrashHandler.init();
>      }
> +    WeaveService.init();

Can we check if sync is enabled before accessing 'WeaveService', and if not completely avoid loading this component?
From a quick look at the code, it seems Weave.js does a check on this.enabled before doing any real initialization... and that's just a check for Services.prefs.prefHasUserValue("services.sync.username").

::: services/sync/Weave.js:120
(Diff revision 1)
>          if (isConfigured) {
>            getHistogramById("WEAVE_CONFIGURED_MASTER_PASSWORD").add(Utils.mpEnabled());
>            this.ensureLoaded();
>          }
>        }
>      }, 10000, Ci.nsITimer.TYPE_ONE_SHOT);

We really need to get rid of this timer. But if this is in an xpcom component, I'm afraid we'll need to wait until bug 1353206 is fixed to do it.
Attachment #8867354 - Flags: review-
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a month ago
Thanks Florian!

Comment 6

a month ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/135031bdb7d0
Delay Weave startup. r=kitcambridge

Comment 7

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/135031bdb7d0
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.