1492475 - Dont block async init of nsSearchService on region fetch

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Dale Harvey (:daleharvey)]

As I understand it we would like to use the async constructor for nsSearchService everywhere, however it currently awaits on a network request for region fetching during construction which can make it take far too long.

For usage purposes it Mike said fire and forget is likely fine for the region check, when it returns it can do its update and not much reason to coordinate that with anyone else. For testing purposes we are probably going to want a way to entire we have waited for the region fetch to complete

Comment 1

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1492475-Do-not-wait-for-region-lookup-in-async-i.patch

This introduces a new checkRegion function that is called from async init but not awaited on, if someone wants to know the region check is complete they can call it themselves and await on the result, it caches the promise so it wont retrigger.

There are a few tests broken here, (test_hidden.js), I just wanted to check this was in the right direction before going to far with fixing tests

Comment 2

[Mike Kaply [:mkaply]]

Comment on attachment 9011482 [details] [diff] [review]
0001-Bug-1492475-Do-not-wait-for-region-lookup-in-async-i.patch

Florian has more of the history here, so I'd like to defer to him.

Comment 3

[Dale Harvey (:daleharvey)]

Comment on attachment 9011482 [details] [diff] [review]
0001-Bug-1492475-Do-not-wait-for-region-lookup-in-async-i.patch

Was meant to be f?, but this breaks some stuff so got a patch passing tests shortly

Comment 4

[Dale Harvey (:daleharvey)]

So I thought this would be a simpler change, however getting the tests fixed up is taking a while and running into some issues, we want fetchRegion to be triggered by the asyncinit even if it is interrupted by a sync init (https://searchfox.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#2693) which isnt compatible with moving the fetchRegion to after |gInitialized = true|, also if we move fetchRegion to before loadEngines, we need to make sure to recalculate the engines.

From an API perspective it also seems messy, this still means async consumers need to (potentially) wait on the network request from happening when as far as I understand it nobody cares? (as they would just end up with sync init anyway)

Can work around the above issues, but it seems cleaner imo to remove fetchRegion from the init flow completely and have it called from browser.js in a requestIdleCallback, we can expose a way for tests to make sure to wait for it to complete if they depend on it

Florian what do you think about that or is having it moved to the end still the best option in your opinion?

Comment 5

[Florian Quèze [:florian]]

(In reply to Dale Harvey (:daleharvey) from comment #4)

> this still means async
> consumers need to (potentially) wait on the network request from happening
> when as far as I understand it nobody cares? (as they would just end up with
> sync init anyway)

Can you explain this some more? In which case do we not care about the region being correct?

Comment 6

[Dale Harvey (:daleharvey)]

> Can you explain this some more? In which case do we not care about the region being correct?

In any case where we would block showing UI on a network connection right? as you mentioned in the other bug if a user does a search before asyncinit is complete, we will do the sync init path which does not do the region fetch.

We should get the correct region as soon as we can, but havent heard / seen any reason why we would block anything on fetching that, and if there isnt I dont see why we would have it in the init path

Comment 7

[Florian Quèze [:florian]]

(In reply to Dale Harvey (:daleharvey) from comment #6)
> > Can you explain this some more? In which case do we not care about the region being correct?
> 
> In any case where we would block showing UI on a network connection right?

Do these case exist? I think we do a network request to check the region mostly for new profiles, and maybe also after an upgrade. In both of these cases, we show a "welcome" tab that means about:home isn't visible at startup, and won't be until the user closes the first tab.

> as you mentioned in the other bug if a user does a search before asyncinit
> is complete, we will do the sync init path which does not do the region
> fetch.

This is supposed to be a rare case. If we hit this frequently, maybe our region check request gets blocked by a corporate firewall or something.

Comment 8

[Dale Harvey (:daleharvey)]

> Do these case exist? 

That is the same case as below (user does a search)

> This is supposed to be a rare case.

Certainly sounds like it should be, and this bug wasnt opened because we are seeing it hit a lot. Mike identified this as a potential complication for the work in https://bugzilla.mozilla.org/show_bug.cgi?id=1486820 (switching nsSearchService from loading OpenSearch files to WebExtentions) as well as a bit of complexity that doesnt need to exist.

Its entirely possible/likely I am missing something not having worked on this code before, but it seems https://bugzilla.mozilla.org/show_bug.cgi?id=1492475#c4 would make things simpler, is there any downsides to it?

Comment 9

[Florian Quèze [:florian]]

(In reply to Dale Harvey (:daleharvey) from comment #8)

> Its entirely possible/likely I am missing something not having worked on
> this code before, but it seems
> https://bugzilla.mozilla.org/show_bug.cgi?id=1492475#c4 would make things
> simpler, is there any downsides to it?

So let me give you more context:

In some locales, the default engine is different depending on the region.
I think the Russian locale is a good example of this, where the default will be Google unless we are in some specific regions: https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/browser/components/search/searchplugins/list.json#691,697-708
The en-US locale is a less visible example, where will use a different partner code if we are in the US region: https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/browser/components/search/searchplugins/list.json#9-12
The en-US case has no user-visible consequences. The ru case makes the default engine exposed to users different. I think we had general agreement that sending any user typed data to the wrong engine was unacceptable. Has this changed?

So currently we delay the initialization until we have all the information needed to pick an engine.

Another assumption that we made and that you may be challenging was that changing the user's search engine in the middle of a browser session was bad. This is the reason why currently if we happen to do a sync init that ignores the region check, we'll take into account the regional settings only at the next restart of the browser.

I think we decided this both to avoid user surprise, and to avoid flicker in the UI.


So I'm not sure if you are proposing that to simplify the code we accept sending some queries to the wrong engine, or that we accept some flicker, or something else.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Florian Quèze [:florian] from comment #9)
> So I'm not sure if you are proposing that to simplify the code we accept
> sending some queries to the wrong engine, or that we accept some flicker, or
> something else.

We'll accept some flicker. We won't accept sending some queries to the wrong engine, but I don't see a reason why that can only be ensured using a sync init fallback.

Comment 11

[Dale Harvey (:daleharvey)]

I feel like there is something fundemental I am missing here

> I think we had general agreement that sending any user typed 
> data to the wrong engine was unacceptable. Has this changed?

> So currently we delay the initialization until we have all the information needed to pick an engine.

As far as I understand from the code, if we do the sync init we will be sending the searches to the wrong engine (as in, the one defined in the build default and not affected by the region check), so while rare it is accepted that we send searches to the wrong engine right?

Comment 12

[Florian Quèze [:florian]]

(In reply to Dale Harvey (:daleharvey) from comment #11)

> As far as I understand from the code, if we do the sync init we will be
> sending the searches to the wrong engine (as in, the one defined in the
> build default and not affected by the region check), so while rare it is
> accepted that we send searches to the wrong engine right?

It's accepted as a last resort when the user is attempting to search. But it's something we avoid when it's just some code (eg. activity stream) trying to initialize itself.

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> (In reply to Florian Quèze [:florian] from comment #9)
> > So I'm not sure if you are proposing that to simplify the code we accept
> > sending some queries to the wrong engine, or that we accept some flicker, or
> > something else.
> 
> We'll accept some flicker.

Startup flicker is something we tried very hard to avoid for 57 / Photon. I'm assuming you think it's fine because the region check will typically happen in cases where we show a welcome page, so the default engine change that will occur a few seconds after startup won't be user-visible, except in obscure edge cases we haven't thought about. Correct?

Comment 13

[Mike de Boer [:mikedeboer]]

(In reply to Florian Quèze [:florian] from comment #12)
> Startup flicker is something we tried very hard to avoid for 57 / Photon.
> I'm assuming you think it's fine because the region check will typically
> happen in cases where we show a welcome page, so the default engine change
> that will occur a few seconds after startup won't be user-visible, except in
> obscure edge cases we haven't thought about. Correct?

Correct.

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 1: Stop using .defaultEngine, in favor of .currentEngine

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 2: Convert unit tests to stop using .defaultEngine, in favor of .currentEngine

Depends on D7892

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 1: Migrate most, if not all nsSearchService consumers to use async APIs. r?florian!

Depends on D7893

Comment 17

[Mike de Boer [:mikedeboer]]

Florian, Dale, I've talked to you both a bunch on IRC about this and I figured I might as well put up a proposal in patch-form to make things easier to talk about.

Part 1: This is probably something that may belong in another bug (I've got the feeling that I saw something similar in my bugmail), but this is simply about cleaning up some old cruft - we don't really need a mention of 'defaultEngine' anymore. 'currentEngine' will do fine and we'll figure out appropriate names for other, new features when we need them.
Part 2: This is my attempt at cleaning up all the unit tests as a result from Part 1.

Part 3: The meat of the feedback request here. There's a Part 4..6 as well, but I'm keeping those for later on purpose. What I did here was fork the current nsIBrowserSearchService to nsIBrowserSearchService2, to save our Android browser in-tree the head-ache, because dealing with Promises in its current structure ain't fun.
The idea is that nsIBrowserSearchService2 will eventually obsolete the need for an explicit call to `Services.search.init(cb)` before doing anything and also nicely gets rid of the weird callback hackery that exists for `addEngine()`.
In nsSearchService.js I made all relevant methods that rely on an initialized search service state await the init and it'll of course take the fast path out if that's already done. Results are returned/ encapsulated in Promises as async APIs do nowadays.
The sync init fallback is commented for now and throws an error if that code path is hit. This made it easy to debug. A follow-up part will yank the sync code out.
Taking care of the region check code is something for a later part as well, since I'm looking for early feedback here.

Thanks!

Comment 18

[Chris H-C :chutten]

Hi, Data Steward here. Is currentEngine different from defaultEngine in content? We want to be careful with these strings sent in every ping that includes an Environment.

Comment 19

[Mike Kaply [:mkaply]]

> Hi, Data Steward here. Is currentEngine different from defaultEngine in content? We want to be careful with these strings sent in every ping that includes an Environment.

No. They were unified into the same thing. It's really just about cleaning up variable names.

Comment 20

[Chris H-C :chutten]

Spiffy. Just checking that the collection didn't change, so Data Collection Review isn't needed. (Though we can go through it if you'd like. The search engine data has only been covered under implicit review. )

Comment 21

[Dale Harvey (:daleharvey)]

Already answered cheers

Comment 22

[Dale Harvey (:daleharvey)]

Comment on attachment 9014826 [details]
Bug 1492475 - Part 1: Migrate most, if not all nsSearchService consumers to use async APIs. r?florian!

Gave feedback in meeting, but the tl;dr is that splitting into nsSearchService2 to avoid android incompatibilities seems like a good idea and the code changes here look good

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 9014826 [details]
Bug 1492475 - Part 1: Migrate most, if not all nsSearchService consumers to use async APIs. r?florian!

I gave feedback on phabricator.

Comment 24

[Mike de Boer [:mikedeboer]]

(In reply to Florian Quèze [:florian] from comment #23)
> I gave feedback on phabricator.

Thanks! I replied to the questions/ issues raised in Phabricator :)

Comment 25

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfef908f8aa6d77f2c32dcde3139b46583b9db94

Comment 26

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09f80a37b5c261dc46b358279640b0a62dd1e105

Comment 28

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 2: Move nsIBrowserSearchService.idl to toolkit/components/search/nsISearchService.idl and update references. r?florian!

Depends on D7894

Comment 29

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 3: The search service init() method should simply return a Promise. r?florian!

Depends on D9275

Comment 30

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 4: Remove the synchronous initialization flow. r?florian!

Depends on D9277

Comment 31

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 5: Since async initialization of the search service now is implicit behavior, remove the distinctive verbiage used internally. r?florian!

Depends on D9278

Comment 32

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 6: Update the cache build task to work with an actual Promise and re-initialize only once at the same time - all to fix race conditions here. r?florian!

Depends on D9279

Comment 33

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 8: Update unit tests to stop initializing the search service explicitly and switch to the asynchronous API wholesale.

Depends on D9280

Comment 34

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 10: Update unit tests to be fully aware of the new, async signatures of the search service API and remove sync init flow tests. r?mkaply!,florian!

Depends on D9281

Comment 35

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 7: Make the region fetch not block the init flow, to ensure it's as fast as possible. r?florian!

Depends on D9280

Comment 36

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 9: Update unit tests to stop using 'currentEngine', in favor of 'defaultEngine'. r?Standard8!

Depends on D9281

Comment 37

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddb1038f5b907f7d5405d5836af93b97312dc627

Comment 38

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86fa0515c78dcbfcc13afbd7453b998120972b99

Comment 39

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1273f2d47a6dd0f2072f58469d118e1c742ca17

Comment 40

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc6e5cd3b631b619faf719ba4aff7ea498541698

Comment 41

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 11: Repair incorrect usage of the `identifier` property of nsISearchEngine instances. r?florian!

Depends on D9282

Comment 42

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d21aedb3cde9fde2b73bc3bcf59ea410fe3f7fc9

Comment 43

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f81268938a34b80f5ba6477268be8ec6fcabbb1e

Comment 44

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb04f528363d38554f487bcbae39a028fb77e85e

Comment 45

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca799c483f6f32faf381136db9b96c1504f9da2c

Comment 46

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95376c9d21c0b44ca00b485a941f19efdbe8f3d3

Comment 47

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f8915e0aad37a7db4e8b4ec4164404c4f6c1728

Comment 48

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1492475 - Part 8: Introduce an init flag, which can only be used privately, that allows to explicitly skip waiting for the region check process to complete. r?florian!

Depends on D11444

Comment 49

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c9d676dee4698db8cbc1ad896434027951e0549

Comment 50

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/projects/cedar/rev/891252fdd0b1a3e6b129025d94952ac30d922c7e
Bug 1492475 - Part 1: Migrate most, if not all nsSearchService consumers to use async APIs. r=florian

https://hg.mozilla.org/projects/cedar/rev/79b2eb2367aab104669bbc75c3b42290f7de1570
Bug 1492475 - Part 2: Move nsIBrowserSearchService.idl to toolkit/components/search/nsISearchService.idl and update references. r=florian

https://hg.mozilla.org/projects/cedar/rev/a947d3cdf078032614edaa491ec3db1d046b55f4
Bug 1492475 - Part 3: The search service init() method should simply return a Promise. r=florian

https://hg.mozilla.org/projects/cedar/rev/c1e172dfacad4b14ebdb352bee2fd946716acd59
Bug 1492475 - Part 4: Remove the synchronous initialization flow. r=florian

https://hg.mozilla.org/projects/cedar/rev/cd41189eac88aa6023af1b0a060c15ddcd407952
Bug 1492475 - Part 5: Since async initialization of the search service now is implicit behavior, remove the distinctive verbiage used internally. r=florian

https://hg.mozilla.org/projects/cedar/rev/2ae7189dfaa63cab0e264e7a2796b1610505c40a
Bug 1492475 - Part 6: Update the cache build task to work with an actual Promise and re-initialize only once at the same time - all to fix race conditions here. r=florian

https://hg.mozilla.org/projects/cedar/rev/c8ee92973f24a44496f2bee23c13e0c74b6e11d8
Bug 1492475 - Part 7: Make the region fetch not block the init flow, to ensure it's as fast as possible. r=florian

https://hg.mozilla.org/projects/cedar/rev/c44e674e160ebab49ea5ba1ed5821bb8d3c30e53
Bug 1492475 - Part 8: Introduce an init flag, which can only be used privately, that allows to explicitly skip waiting for the region check process to complete. r=florian

https://hg.mozilla.org/projects/cedar/rev/6c79eaf1d349638258d542ced0229d786f022683
Bug 1492475 - Part 9: Update unit tests to stop using 'currentEngine', in favor of 'defaultEngine'. r=Standard8

https://hg.mozilla.org/projects/cedar/rev/21b3aa17ee43dd0efd3c08564bbc7d747d4628b9
Bug 1492475 - Part 10: Update unit tests to be fully aware of the new, async signatures of the search service API and remove sync init flow tests. r=mkaply,florian

https://hg.mozilla.org/projects/cedar/rev/ce5ba6901957903ade31888cdc6a52e2b828dac0
Bug 1492475 - Part 11: Repair incorrect usage of the `identifier` property of nsISearchEngine instances. r=florian

Comment 51

[Mark Banner (:standard8)]

To clarify, bug 1524593 fixed this, by taking the patches and rolling them up into:

https://hg.mozilla.org/mozilla-central/rev/add76bbdce4e