Dont block async init of nsSearchService on region fetch

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P1
normal
RESOLVED FIXED
7 months ago
16 days ago

People

(Reporter: daleharvey, Assigned: mikedeboer)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 unaffected, firefox66 unaffected, firefox67 fixed)

Details

Attachments

(9 attachments, 6 obsolete attachments)

3.83 KB, patch
Details | Diff | Splinter Review
46 bytes, text/x-phabricator-request
florian
: feedback+
daleharvey
: feedback+
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
(Reporter)

Description

7 months ago
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
(Reporter)

Updated

7 months ago
Assignee: nobody → dharvey
(Reporter)

Comment 1

7 months ago
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
Attachment #9011482 - Flags: review?(mozilla)
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.
Attachment #9011482 - Flags: review?(mozilla) → review?(florian)
(Reporter)

Comment 3

7 months ago
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
Attachment #9011482 - Flags: review?(florian)

Updated

7 months ago
Priority: -- → P1
(Reporter)

Comment 4

7 months ago
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?
Flags: needinfo?(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?
(Reporter)

Comment 6

7 months ago
> 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
(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.
(Reporter)

Comment 8

7 months ago
> 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?
(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.
Flags: needinfo?(florian)
(Assignee)

Comment 10

7 months ago
(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.
(Reporter)

Comment 11

7 months ago
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?
(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?
(Assignee)

Comment 13

7 months ago
(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.
(Assignee)

Updated

7 months ago
Blocks: 1486811
(Assignee)

Comment 17

7 months ago
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!
QA Contact: adw
(Assignee)

Updated

7 months ago
Attachment #9014826 - Flags: feedback?(florian)
Attachment #9014826 - Flags: feedback?(dharvey)
QA Contact: adw
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.
Flags: needinfo?(dharvey)
> 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.
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. )
(Reporter)

Comment 21

7 months ago
Already answered cheers
Flags: needinfo?(dharvey)
(Reporter)

Comment 22

6 months ago
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
Attachment #9014826 - Flags: feedback?(dharvey) → feedback+
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.
Attachment #9014826 - Flags: feedback?(florian) → feedback+
(Assignee)

Comment 24

6 months ago
(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 :)
(Reporter)

Updated

6 months ago
Assignee: dharvey → mdeboer
(Reporter)

Updated

6 months ago
Duplicate of this bug: 1490334
Attachment #9014826 - Attachment description: Bug 1492475 - Part 3: Migrate most, if not all nsSearchService consumers to use async APIs. → Bug 1492475 - Part 1: Migrate most, if not all nsSearchService consumers to use async APIs.
Attachment #9018664 - Attachment description: Bug 1492475 - Part 4: Move nsIBrowserSearchService.idl to toolkit/components/search/nsISearchService.idl and update references. → Bug 1492475 - Part 2: Move nsIBrowserSearchService.idl to toolkit/components/search/nsISearchService.idl and update references.
Attachment #9018666 - Attachment description: Bug 1492475 - Part 5: The search service init() method should simply return a Promise. → Bug 1492475 - Part 3: The search service init() method should simply return a Promise.
Attachment #9018667 - Attachment description: Bug 1492475 - Part 6: Remove the synchronous initialization flow. → Bug 1492475 - Part 4: Remove the synchronous initialization flow.
Attachment #9018668 - Attachment description: Bug 1492475 - Part 7: Since async initialization of the search service now is implicit behavior, remove the distinctive verbiage used internally. → Bug 1492475 - Part 5: Since async initialization of the search service now is implicit behavior, remove the distinctive verbiage used internally.
Attachment #9018669 - Attachment description: Bug 1492475 - Part 8: 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. → 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.
Attachment #9018670 - Attachment description: Bug 1492475 - Part 9: Update unit tests to stop initializing the search service explicitly and switch to the asynchronous API wholesale. → Bug 1492475 - Part 8: Update unit tests to stop initializing the search service explicitly and switch to the asynchronous API wholesale.
Attachment #9018671 - Attachment description: 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. → Bug 1492475 - Part 9: Update unit tests to be fully aware of the new, async signatures of the search service API and remove sync init flow tests.
Attachment #9014823 - Attachment is obsolete: true
Attachment #9014824 - Attachment is obsolete: true
Attachment #9018671 - Attachment description: Bug 1492475 - Part 9: Update unit tests to be fully aware of the new, async signatures of the search service API and remove sync init flow tests. → 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.
Attachment #9023983 - Attachment description: Bug 1492475 - Part 9: Update unit tests to stop using 'currentEngine', in favor of 'defaultEngine'. → Bug 1492475 - Part 8: Update unit tests to stop using 'currentEngine', in favor of 'defaultEngine'.
Attachment #9018671 - Attachment description: 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. → Bug 1492475 - Part 9: Update unit tests to be fully aware of the new, async signatures of the search service API and remove sync init flow tests.
Attachment #9018670 - Attachment is obsolete: true
Attachment #9014826 - Attachment description: Bug 1492475 - Part 1: Migrate most, if not all nsSearchService consumers to use async APIs. → Bug 1492475 - Part 1: Migrate most, if not all nsSearchService consumers to use async APIs. r?florian!
Attachment #9018664 - Attachment description: Bug 1492475 - Part 2: Move nsIBrowserSearchService.idl to toolkit/components/search/nsISearchService.idl and update references. → Bug 1492475 - Part 2: Move nsIBrowserSearchService.idl to toolkit/components/search/nsISearchService.idl and update references. r?florian!
Attachment #9018666 - Attachment description: Bug 1492475 - Part 3: The search service init() method should simply return a Promise. → Bug 1492475 - Part 3: The search service init() method should simply return a Promise. r?florian!
Attachment #9018667 - Attachment description: Bug 1492475 - Part 4: Remove the synchronous initialization flow. → Bug 1492475 - Part 4: Remove the synchronous initialization flow. r?florian!
Attachment #9018668 - Attachment description: Bug 1492475 - Part 5: Since async initialization of the search service now is implicit behavior, remove the distinctive verbiage used internally. → Bug 1492475 - Part 5: Since async initialization of the search service now is implicit behavior, remove the distinctive verbiage used internally. r?florian!
Attachment #9018669 - Attachment description: 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. → 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?mkaply!,florian!
Attachment #9023887 - Attachment description: Bug 1492475 - Part 7: Make the region fetch not block the init flow, to ensure it's as fast as possible. → Bug 1492475 - Part 7: Make the region fetch not block the init flow, to ensure it's as fast as possible. r?mkaply!,florian!
Attachment #9023983 - Attachment description: Bug 1492475 - Part 8: Update unit tests to stop using 'currentEngine', in favor of 'defaultEngine'. → Bug 1492475 - Part 8: Update unit tests to stop using 'currentEngine', in favor of 'defaultEngine'. r?Standard8!
Attachment #9018671 - Attachment description: Bug 1492475 - Part 9: Update unit tests to be fully aware of the new, async signatures of the search service API and remove sync init flow tests. → Bug 1492475 - Part 9: 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!
Attachment #9026452 - Attachment description: Bug 1492475 - Part 10: Repair incorrect usage of the `identifier` property of nsISearchEngine instances and fixup tests that checked the wrong properties. r?florian! → Bug 1492475 - Part 10: Repair incorrect usage of the `identifier` property of nsISearchEngine instances. r?florian!
(Reporter)

Updated

5 months ago
Blocks: 1486820
Attachment #9023983 - Attachment description: Bug 1492475 - Part 8: Update unit tests to stop using 'currentEngine', in favor of 'defaultEngine'. r?Standard8! → Bug 1492475 - Part 9: Update unit tests to stop using 'currentEngine', in favor of 'defaultEngine'. r?Standard8!
Attachment #9018671 - Attachment description: Bug 1492475 - Part 9: 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! → 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!
Attachment #9026452 - Attachment description: Bug 1492475 - Part 10: Repair incorrect usage of the `identifier` property of nsISearchEngine instances. r?florian! → Bug 1492475 - Part 11: Repair incorrect usage of the `identifier` property of nsISearchEngine instances. r?florian!
Attachment #9030701 - Attachment description: Bug 1492475 - Part 8: Introduce an nsISearchService flag that allows to explicitly await the region check process to complete. r?florian! → Bug 1492475 - Part 8: Introduce an nsISearchService::init flag that allows to explicitly skip waiting for the region check process to complete. r?florian!
Attachment #9030701 - Attachment description: Bug 1492475 - Part 8: Introduce an nsISearchService::init flag that allows to explicitly skip waiting for the region check process to complete. r?florian! → 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!
Attachment #9018669 - Attachment description: 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?mkaply!,florian! → 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!
Attachment #9023887 - Attachment description: Bug 1492475 - Part 7: Make the region fetch not block the init flow, to ensure it's as fast as possible. r?mkaply!,florian! → Bug 1492475 - Part 7: Make the region fetch not block the init flow, to ensure it's as fast as possible. r?florian!
(Assignee)

Updated

4 months ago
Blocks: 1517486
(Assignee)

Comment 50

3 months ago
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
(Assignee)

Updated

3 months ago
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
(Assignee)

Updated

3 months ago
Depends on: 1518543
(Assignee)

Updated

3 months ago
Blocks: 1518545
(Assignee)

Updated

3 months ago
Blocks: 1518546
(Assignee)

Updated

3 months ago
Blocks: 1518548
(Assignee)

Updated

3 months ago
Blocks: 1518551
(Assignee)

Updated

3 months ago
Depends on: 1523708

Updated

2 months ago
Depends on: 1526211

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

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

Blocks: 1524593
Target Milestone: --- → Firefox 67
Attachment #9023887 - Attachment is obsolete: true
Attachment #9018671 - Attachment is obsolete: true
Attachment #9018669 - Attachment is obsolete: true
Duplicate of this bug: 1405671
Duplicate of this bug: 888958
You need to log in before you can comment on or make changes to this bug.