Closed Bug 855604 Opened 9 years ago Closed 9 years ago

SearchCountMeasurement2 should wait for nsSearchService to initialize asynchronously

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect, P4)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file, 2 obsolete files)

We don't currently hit this, because FHR is initialized after a ten-second timer.

I was playing around with dropping that to 50ms, and I got this in a talos dump:

[JavaScript Error: "DEPRECATION WARNING: Search service falling back to deprecated synchronous initializer.
You may find more details about this deprecation at: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIBrowserSearchService#async_warning
jar:jar:file:///data/app/org.mozilla.fennec-1.apk!/omni.ja!/components/nsSearchService.js 3825 epsSyncInit
jar:jar:file:///data/app/org.mozilla.fennec-1.apk!/omni.ja!/components/nsSearchService.js 2588 SRCH_SVC__ensureInitialized
jar:jar:file:///data/app/org.mozilla.fennec-1.apk!/omni.ja!/components/nsSearchService.js 3337 SRCH_SVC_getDefault
resource://gre/modules/HealthReport.jsm 6354 SearchCountMeasurement2.prototype<.getDefaultEngines
resource://gre/modules/HealthReport.jsm 6371 SearchCountMeasurement2.prototype<._initialize
resource://gre/modules/HealthReport.jsm 6390 SearchCountMeasurement2.prototype<.fields

That is, SearchCountMeasurement2 causes nsSearchService to init implicitly.

We should switch SCM2 to chain off nsSearchService.init.
Good catch.
Attached patch Proposed patch. v1 (obsolete) — Splinter Review
Note that measurements get touched really early in the provider lifecycle, and don't themselves have a good async place to hang stuff without spinning. See comments.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #730554 - Flags: review?(gps)
(In reply to Richard Newman [:rnewman] from comment #0)
> We don't currently hit this, because FHR is initialized after a ten-second
> timer.
> 
> I was playing around with dropping that to 50ms, and I got this in a talos
> dump:

That suggests that it's causing the search service to initialize before the search bar does, which probably has negative effects on painting the first window. This particular change should be made either way for API consistency, but it doesn't seem like actually moving ahead FHR initialization that much is feasible, right?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)

> This particular change should be made either way for API
> consistency, but it doesn't seem like actually moving ahead FHR
> initialization that much is feasible, right?

Not feasible without solving bigger problems, like "we shouldn't be initializing large chunks of functionality by using timers and luck" (I'm sure Greg has a bug somewhere for building something like Solaris's SMF for Firefox…), and as you mentioned, not desirable for UX reasons.

This was basically me trying to see if I could get Talos to regress by getting FHR to do some work during a test.
Comment on attachment 730554 [details] [diff] [review]
Proposed patch. v1

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

So, we either have onInit() or we overload init() in child classes. Let's not do both.

My vote is for a preInit hook because I like sanctioned hook points when writing plugin-like software (which is what providers are). Perhaps we should also rename onInit to postInit or something.
Attachment #730554 - Flags: review?(gps) → review-
(In reply to Richard Newman [:rnewman] from comment #4)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #3)
> 
> > This particular change should be made either way for API
> > consistency, but it doesn't seem like actually moving ahead FHR
> > initialization that much is feasible, right?
> 
> Not feasible without solving bigger problems, like "we shouldn't be
> initializing large chunks of functionality by using timers and luck" (I'm
> sure Greg has a bug somewhere for building something like Solaris's SMF for
> Firefox…), and as you mentioned, not desirable for UX reasons.

I have no such bug :) Although, I really wish we had a sane service initialization dependency graph to avoid stuff like this.

I think a fundamental problem is many IDL types hang the onInit() method off the same type as the object being initialized. You have to know to use onInit() instead of being forced to go through it. Even FHR's HealthReporter is guilty of this!

If a service is lazy initialized or initialized asynchronously, it's access pattern should be:

let service = Cc["@mozilla.org/..."].getService(Ci.nsIMyAsyncService);
let instance = service.onInit(function gotIt() { ... });

Bug 702559 is a good modern example of this pattern in action. There's no way to mess it up and access members before the object is initialized because you don't see the object until it has been initialized!
(In reply to Gregory Szorc [:gps] from comment #5)

> My vote is for a preInit hook because I like sanctioned hook points when
> writing plugin-like software (which is what providers are). Perhaps we
> should also rename onInit to postInit or something.

preInit works for me. I'll revise the patch later today.
Attached patch Proposed patch. v2 (obsolete) — Splinter Review
Attachment #730554 - Attachment is obsolete: true
Attachment #730966 - Flags: review?(gps)
Comment on attachment 730966 [details] [diff] [review]
Proposed patch. v2

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

Holding review until questions are answered.

::: services/healthreport/providers.jsm
@@ +1338,5 @@
> +      Services.search.init({
> +        onInitComplete: function () {
> +          deferred.resolve(Services.search.getDefaultEngines());
> +        },
> +      });

Can onInitComplete's callback ever not fire?

Can Services.search.getDefaultEngines() ever throw?

Why do we call getDefaultEngines()?

::: services/metrics/dataprovider.jsm
@@ +560,5 @@
> +      if (!pre || typeof(pre.then) != "function") {
> +        throw new Error("preInit() does not return a promise.");
> +      }
> +      yield pre;
> +      

Nit: trailing whitespace

@@ +607,5 @@
> +   */
> +  preInit: function () {
> +    return CommonUtils.laterTickResolvingPromise();
> +  },
> +    

Nite: trailing whitespace
Comment on attachment 730966 [details] [diff] [review]
Proposed patch. v2

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

Holding review until questions are answered.

::: services/healthreport/providers.jsm
@@ +1335,5 @@
> +    return Task.spawn(function searchPreInit() {
> +      // Initialize search service.
> +      let deferred = Promise.defer();
> +      Services.search.init({
> +        onInitComplete: function () {

You don't need a full object - just a function will work. If you are feeling adventurous and don't need to uplift this:

Services.search.init(() => deferred.resolve(Services.search.getDefaultEngines()));

@@ +1338,5 @@
> +      Services.search.init({
> +        onInitComplete: function () {
> +          deferred.resolve(Services.search.getDefaultEngines());
> +        },
> +      });

Can onInitComplete's callback ever not fire?

Can Services.search.getDefaultEngines() ever throw?

Why do we call getDefaultEngines()?

::: services/metrics/dataprovider.jsm
@@ +560,5 @@
> +      if (!pre || typeof(pre.then) != "function") {
> +        throw new Error("preInit() does not return a promise.");
> +      }
> +      yield pre;
> +      

Nit: trailing whitespace

@@ +607,5 @@
> +   */
> +  preInit: function () {
> +    return CommonUtils.laterTickResolvingPromise();
> +  },
> +    

Nite: trailing whitespace
(In reply to Gregory Szorc [:gps] from comment #10)

> Can onInitComplete's callback ever not fire?

Only if there's a problem with the promise or task implementation, or Cu.reportError throws.

See <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#3293>.


> Can Services.search.getDefaultEngines() ever throw?

Not from my reading of the code.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#3351


> Why do we call getDefaultEngines()?

To get the list of built-in engines from the search service. Those will be the ones we ship with the distribution, which include those we wish to track in FHR.

(As far as I know, there are no search providers that we don't ship with Firefox.)

We then intersect that small list with the set of partner providers to get the "live" field list. This allows us to have the minimal set that could be recorded.

The alternatives for getting a list of engines are:

* getEngines (all), which includes those the user has installed or created. We don't want to do that.
* getVisibleEngines, which is the above with hidden engines removed. We don't care about this distinction.
* Just use *all* of the partners, which will leave us with 61 * 4 fields.
Fixed whitespace, and switched an indexOf to a contains.
Attachment #730966 - Attachment is obsolete: true
Attachment #730966 - Flags: review?(gps)
Attachment #731008 - Flags: review?(gps)
Comment on attachment 731008 [details] [diff] [review]
Proposed patch. v3

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

Not sure if you were on something when you implemented preInit() or I am still too tipsy to review.

While this code is correct, it's not ideal.

::: services/healthreport/providers.jsm
@@ +1335,5 @@
> +    return Task.spawn(function searchPreInit() {
> +      // Initialize search service.
> +      let deferred = Promise.defer();
> +      Services.search.init(function onInitComplete () {
> +        deferred.resolve(Services.search.getDefaultEngines());

Isn't the resolved value of preInit unused? Again, not sure why you are retrieving the default engines here.

@@ +1338,5 @@
> +      Services.search.init(function onInitComplete () {
> +        deferred.resolve(Services.search.getDefaultEngines());
> +      });
> +      yield deferred.promise;
> +    }.bind(this));

I'm not sure why you are creating a task. Isn't it sufficient to return the promise you create? The yield in the main init() body will wait for it. This just creates a useless wrapper promise with generator/task overhead.
Attachment #731008 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #13)

> Not sure if you were on something when you implemented preInit() or I am
> still too tipsy to review.

Yeah, I was on something.
Landed with braino removed.

https://hg.mozilla.org/services/services-central/rev/b742e556917b
Whiteboard: [fixed in services]
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
https://hg.mozilla.org/mozilla-central/rev/b742e556917b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.