Closed
Bug 1018240
Opened 10 years ago
Closed 10 years ago
Bundled locale specific search engines not loaded until next browser restart
Categories
(Firefox for Android Graveyard :: Locale switching and selection, defect)
Firefox for Android Graveyard
Locale switching and selection
All
Android
Tracking
(firefox32 fixed, firefox33 fixed, firefox34 verified, fennec32+)
VERIFIED
FIXED
Firefox 34
People
(Reporter: aaronmt, Assigned: rnewman)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
adw
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bnicholson
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Currently when one changes the browser language to a locale with a bundled search engine, the engine list shown in the browser will not change until next browser restart.
E.g, change to Chinese Simplified and you'd expect to see Baidu but you see our default en-US listing (Google/Yahoo/Bing/Amazon/Twitter/Wikipedia).
We should fetch the appropriate engines without the need to restart.
Assignee | ||
Comment 1•10 years ago
|
||
Pretty much a sibling of Bug 997589.
Updated•10 years ago
|
tracking-fennec: ? → 32+
Assignee | ||
Comment 2•10 years ago
|
||
This ought to do it. Untested.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
This ought to do it, except that BrowserSearch isn't listening.
06-09 22:59:08.470 W/GeckoEventDispatcher(23059): No listeners for SearchEngines:Data
Reporter | ||
Updated•10 years ago
|
status-firefox33:
--- → affected
Assignee | ||
Comment 4•10 years ago
|
||
Raymond, Ryan: you two have the most blame on nsSearchService.
From reading code, it looks to me like the correct course of action is for the search service itself to observe the general.useragent.locale pref, calling _asyncLoadEngines when the locale changes, just as if it were being initialized for the first time, and notifying observers accordingly.
Do you agree with this analysis? The alternative, as far as I can see, is to have code elsewhere specifically ask nsSearchService to reinitialize itself.
If you do agree, would you folks like to do that work, or are you free to review a patch from me?
Flags: needinfo?(rflint)
Flags: needinfo?(raymond)
Assignee | ||
Comment 5•10 years ago
|
||
This patch makes nsSearchService watch for changes in the
general.useragent.locale pref, asynchronously reiniting itself when the pref
changes.
reinit-complete and reinit-failed events are broadcast on the usual search
service topic.
Assignee | ||
Comment 6•10 years ago
|
||
This builds on top of part 1, assuming that:
* If we're redisplaying the search bar, and
* The locale has changed since we last fetched search results, then
* We should re-request, and nsSearchService will be up-to-date, because humans don't click fast enough for this assumption to be validated.
If the assumption chain is false, we'll get stale or no search engines until we fetch again. That's acceptable IMO.
Attachment #8454790 -
Flags: review?(bnicholson)
Assignee | ||
Updated•10 years ago
|
Attachment #8437438 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8454780 -
Flags: review?(MattN+bmo)
Updated•10 years ago
|
Attachment #8454780 -
Flags: feedback?(gavin.sharp)
Comment 7•10 years ago
|
||
Comment on attachment 8454790 [details] [diff] [review]
Part 2: invalidate BrowserSearch engine list when locale has changed. v1
Review of attachment 8454790 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/home/BrowserSearch.java
@@ +108,5 @@
> private volatile SuggestClient mSuggestClient;
>
> + // List of search engines from Gecko.
> + // Do not mutate this list.
> + private volatile List<SearchEngine> mSearchEngines = new ArrayList<SearchEngine>();
Why change this to volatile? If we actually access this on multiple threads, we're in trouble since we aren't using any form of synchronization, so I don't think volatile buys us anything.
It looks like we're requiring mSearchEngines to be on the UI thread, so I think it would be preferable to:
* Add a comment that this be used only on the UI thread.
* Add ThreadUtils.assertOnUiThread() to any methods using it whose thread is unclear.
@@ +233,5 @@
> super.onResume();
>
> + // Fetch engines if we need to.
> + if (mSearchEngines.isEmpty() ||
> + !Locale.getDefault().equals(mLastLocale)) {
Nit: Keep this on the same line? It makes my eyes bleed when a multi-line condition is indented at the same level as the body.
Attachment #8454790 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #7)
> Why change this to volatile? If we actually access this on multiple threads,
> we're in trouble since we aren't using any form of synchronization, so I
> don't think volatile buys us anything.
We write to it from the message loop (background), and check it in onResume (UI).
We don't mutate the interior of the list (indeed, it's unmodifiable), and it's never accessed via two places until long after it's built, so volatile is exactly the primitive we want: atomic dereference and assignment.
Comment 9•10 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #8)
> We write to it from the message loop (background)
I don't think so -- see http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java?rev=36853e536d68#351. Note that mAdapter is tied to mSearchEngines, so we must follow modifications to mSearchEngines with notifyDataSetChanged() on the UI thread.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #9)
> (In reply to Richard Newman [:rnewman] from comment #8)
> > We write to it from the message loop (background)
>
> I don't think so -- see
Oh, you're right! That's what I get for making assumptions. Good spot.
Assignee | ||
Comment 11•10 years ago
|
||
Updated•10 years ago
|
Attachment #8454780 -
Flags: review?(MattN+bmo) → review?(adw)
Comment 12•10 years ago
|
||
Comment on attachment 8454780 [details] [diff] [review]
Part 1: reinitialize nsSearchService when the browser locale changes. v1.
Review of attachment 8454780 [details] [diff] [review]:
-----------------------------------------------------------------
I don't know this code super well, but there are a couple of races here that I'm a little worried about.
(1) If the locale is changed while the initial init is ongoing, it looks like we could run into trouble, at least with checkForSyncCompletion and the error it throws. That seems unlikely to happen in practice, though, but who knows.
(2) If clients use the service while re-init is ongoing, then it looks like what will happen is it'll fall back to sync init, which logs a warning, and then the _asyncLoadEngines call from _asyncReInit may end up throwing an error during one of its several calls to checkForSyncCompletion. That seems more likely to happen than (1).
It doesn't look like the consequences of either case are too bad. We'd end up with warnings or errors logged, but the service would ultimately be (re)inited. But it is a little dodgy, so I'm kind of inclined to not r+ this in favor of a more sound fix. But what do you think, Richard?
Comment 13•10 years ago
|
||
Clients are supposed to call nsIBrowserSearchService.init themselves -- in the one client I've written since async init was added, it was easiest just to assume the service had never been inited, so all my calls to the service are guarded by init calls. If that's a common pattern for clients, then maybe it's enough for the service to simply uninit itself when the locale changes and let clients re-init it. Just a thought.
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #12)
Thanks for the review!
> It doesn't look like the consequences of either case are too bad. We'd end
> up with warnings or errors logged, but the service would ultimately be
> (re)inited. But it is a little dodgy, so I'm kind of inclined to not r+
> this in favor of a more sound fix. But what do you think, Richard?
The main trigger for this (and the typical place where we'd see timing issues) would be Fennec's locale switching, which sets the newly observed pref in response to user action.
It's not possible to switch locales until Gecko is up and running (=> the search service should either be initialized or not yet begun, depending on how lazy its consumers are), and locale switching is human-initiated, so it's impractical for a race to occur there. (Fingers are slow.)
On desktop, my guess is that that pref isn't changed often (or else someone would have fixed this bug sooner!), but if it is changed it's likely to be changed by a human, with similar race characteristics.
If I understand the desktop distribution story correctly, desktop users download a single-locale build, so they should have no cause to ever change general.useragent.locale.
I agree wholeheartedly that this is something of an inexpert fix -- it pretty much stomps on state until it stops moving, then triggers init, and it has few better options given that it's in an observer. It would be great if someone with more knowledge of this code would make it bulletproof.
My inclination is to kick that to a follow-up and get this into 32 with the rest of the locale switching code -- AFAICS the risk for desktop users is ~0, and the risk for Fennec users seems acceptable to me after testing. The alternative would be that we reach into the guts of nsSearchService from Fennec's code, which seems even more fragile.
Re Comment 13: I deliberately triggered the init immediately, rather than relying on a consumer causing init, because it simply produces a better user experience to not have to wait for the search engines to load. We even have a bug on file to pre-load them the first time: Bug 927692. The reason for lazy init is to avoid doing work when cycles are precious; locale change probably isn't such a case, so it's better to front-load the work.
Comment 15•10 years ago
|
||
Comment on attachment 8454780 [details] [diff] [review]
Part 1: reinitialize nsSearchService when the browser locale changes. v1.
Review of attachment 8454780 [details] [diff] [review]:
-----------------------------------------------------------------
OK.
Attachment #8454780 -
Flags: review?(adw)
Attachment #8454780 -
Flags: review+
Attachment #8454780 -
Flags: feedback?(gavin.sharp)
Assignee | ||
Comment 16•10 years ago
|
||
Assignee | ||
Comment 17•10 years ago
|
||
Misread my bugmail as requesting f? from Gavin, not clearing it, so missed 33. Darn.
Will give this a few days to bake then request uplift. Thanks for the reviews, folks!
Flags: needinfo?(rflint)
Flags: needinfo?(raymond)
Hardware: ARM → All
Target Milestone: --- → Firefox 34
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(rnewman)
https://hg.mozilla.org/mozilla-central/rev/0234ad055a7b
https://hg.mozilla.org/mozilla-central/rev/476a0455e0e8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 19•10 years ago
|
||
Let's get this on Beta
Status: RESOLVED → VERIFIED
status-firefox34:
--- → verified
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8454780 [details] [diff] [review]
Part 1: reinitialize nsSearchService when the browser locale changes. v1.
** Approval request for both parts **
Approval Request Comment
[Feature/regressing bug #]:
Long-standing limitation in nsSearchService and BrowserSearch. Needed to support locale switching work in 32.
[User impact if declined]:
Users switching locales will see the default search engines from their previous locale until Android next relaunches Fennec.
[Describe test coverage new/current, TBPL]:
Manually verified by QA.
[Risks and why]:
Low risk of incorrect reinitialization due to pref changes and races (nothing practical). Otherwise seems pretty robust.
[String/UUID change made/needed]:
None.
Attachment #8454780 -
Flags: approval-mozilla-beta?
Attachment #8454780 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(rnewman)
Comment 21•10 years ago
|
||
Oops - I wanted to look at this before it landed (hence the feedback), and I missed that being cleared.
Can we make this re-init behavior Fennec-only? Reinitialization is a bit scary to me and I'd prefer if it not happen on Desktop, where there little value.
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #21)
> Oops - I wanted to look at this before it landed (hence the feedback), and I
> missed that being cleared.
>
> Can we make this re-init behavior Fennec-only?
We can. How would you prefer to do this?
Options, of varying complexities:
* Preprocess nsSearchService.js.
* Introspect at runtime to decide which app we are. Not sure if that's done in toolkit code right now.
* Switch to using some explicit observer notification that desktop doesn't send (e.g., "locale-changed-reinit") instead of pref observing.
* Moving internals-aware logic from nsSearchService into Fennec. Doesn't seem like a good solution from a maintenance standpoint.
* Fork nsSearchService for Fennec.
Flags: needinfo?(gavin.sharp)
Comment 23•10 years ago
|
||
nsSearchService.js is already preprocessed (for fennec-specific stuff even), you can just throw an #ifdef ANDROID in there. I filed bug 1043627.
Flags: needinfo?(gavin.sharp)
Comment 24•10 years ago
|
||
Comment on attachment 8454780 [details] [diff] [review]
Part 1: reinitialize nsSearchService when the browser locale changes. v1.
Please also fill the uplift request in bug 1043627 when it is ready.
Attachment #8454780 -
Flags: approval-mozilla-beta?
Attachment #8454780 -
Flags: approval-mozilla-beta+
Attachment #8454780 -
Flags: approval-mozilla-aurora?
Attachment #8454780 -
Flags: approval-mozilla-aurora+
Comment 25•10 years ago
|
||
Comment on attachment 8454790 [details] [diff] [review]
Part 2: invalidate BrowserSearch engine list when locale has changed. v1
> ** Approval request for both parts **
As requested.
Attachment #8454790 -
Flags: approval-mozilla-beta+
Attachment #8454790 -
Flags: approval-mozilla-aurora+
Comment 26•10 years ago
|
||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•