CountryNames: Separate data creation from usage

CountryNames: Separate data creation from usage

==Goal==
Fix a race condition in CountryNames-related code without complicating
it significantly.

==Non-goal==
Any deeper refactoring not necessary to fix the above race
condition. A discussion is needed to see whether once could avoid the
Singleton business and use some more common ways to store the country names
data (a database?), as well as whether it is necessary to compute it in
run-time. But that is explicitly out of scope of this CL.

==Content==
This CL makes CountryNames create its data (mappings from country names to
country codes and the appropriate collators for hashing the country names) on
construction, rather than on demand.

The main purpose of this change is to eliminate race conditions. Before this
CL, multiple threads could have trigger the data creation, and it is believed
to have caused data corruption and crashes.

Because the CountryNames creation is guaranteed by Singleton to only happen
once, it solves the threading issues.

However, there are caveats:
(Caveat A) CountryNames cannot get constructor arguments,
being created through Singleton's get(). But it needs to know which locale
Chrome is using. To circumvent this, it gets the information from a global
static string, and relies on the callsites of CountryNames to ensure
initialisation of this string.

To make that happen, every instance of AutofillManager ensures on its
construction, that said string is initialised. This is a bit clumsy (there is
an AutofillManager for each tab, but this pointer only needs to be initialised
once per Chrome's execution), but works -- CountryNames are never used earlier
than the first AutofillManager is constructed.

Also, while initialising the string is not guarded by any mutex, it is
safe -- AutofillManager gets constructed on the UI thread, so there are not
more concurrent constructions. And once the string is set, it is never set
again, so a constructed CountryNames can read it safely on any thread.

(Caveat B) Before this CL, the code was capable of generating more data for a
newly added locale on the fly. After this CL, it will only work for the locale
of the program during the CountryNames creation (and en_US as a fallback).
Note that Chrome does not seem to be able to change the language without
restart, so this should be no concern. The only unknown for me is CrOS -- I'm
not sure whether a new CountryNames instance is created for each user session.
But my suggestion is to wait with making the code more flexible until the
more systematic fix to the way it generates and stores the CountryNames data.

Finally, this CL adds a way to construct CountryNames with an explicit locale
in tests, and uses it in the appropriate unit tests.

BUG=571610
Committed: https://crrev.com/f39cbace358fe626f0f07e32f4e66b78ca589038
Cr-Commit-Position: refs/heads/master@{#371226}

[vabr (Chromium), 2016-01-15 19:37:24]

Description was changed from

==========
CountryNames: Separate data creation from usage

==Goal==
Fix a race condition in CountryNames-related code without complicating
it significantly.

==Non-goal==
Any deeper refactoring not necessary to fix the above race
condition. A discussion is needed to see whether once could avoid the
Singleton business and use some more common ways to store the country names
data (a database?), as well as whether it is necessary to compute it in
run-time. But that is explicitly out of scope of this CL.

==Content==
This CL makes CountryNames create its data (mappings from country names to
country codes and the appropriate collators for hashing the country names) on
construction, rather than on demand.

The main purpose of this change is to eliminate race conditions. Before this
CL, multiple threads could have trigger the data creation, and it is believed
to have caused data corruption and crashes.

Because the CountryNames creation is guaranteed by Singleton to only happen
once, it solves the threading issues.

However, there are caveats: CountryNames cannot get constructor arguments,
being created through Singleton's get(). But it needs to know which locale
Chrome is using. To circumvent this, it gets the information from a global
static string, and relies on the callsites of CountryNames to ensure
initialisation of this string.

To make that happen, every instance of AutofillManager ensures on its
construction, that said string is initialised. This is a bit clumsy (there is
an AutofillManager for each tab, but this pointer only needs to be initialised
once per Chrome's execution), but works -- CountryNames are never used earlier
than the first AutofillManager is constructed.

Also, while initialising the string is not guarded by any mutex, it is
safe -- AutofillManager gets constructed on the UI thread, so there are not
more concurrent constructions. And once the string is set, it is never set
again, so a constructed CountryNames can read it safely on any thread.

To get the application locale, this CL also adds a method to AutofillClient to
provide it independently of platform.

Finally, this CL adds a way to construct CountryNames with an explicit locale
in tests, and uses it in the appropriate unit tests.

BUG=571610
==========

to

==========
CountryNames: Separate data creation from usage

==Goal==
Fix a race condition in CountryNames-related code without complicating
it significantly.

==Non-goal==
Any deeper refactoring not necessary to fix the above race
condition. A discussion is needed to see whether once could avoid the
Singleton business and use some more common ways to store the country names
data (a database?), as well as whether it is necessary to compute it in
run-time. But that is explicitly out of scope of this CL.

==Content==
This CL makes CountryNames create its data (mappings from country names to
country codes and the appropriate collators for hashing the country names) on
construction, rather than on demand.

The main purpose of this change is to eliminate race conditions. Before this
CL, multiple threads could have trigger the data creation, and it is believed
to have caused data corruption and crashes.

Because the CountryNames creation is guaranteed by Singleton to only happen
once, it solves the threading issues.

However, there are caveats:
(Caveat A) CountryNames cannot get constructor arguments,
being created through Singleton's get(). But it needs to know which locale
Chrome is using. To circumvent this, it gets the information from a global
static string, and relies on the callsites of CountryNames to ensure
initialisation of this string.

To make that happen, every instance of AutofillManager ensures on its
construction, that said string is initialised. This is a bit clumsy (there is
an AutofillManager for each tab, but this pointer only needs to be initialised
once per Chrome's execution), but works -- CountryNames are never used earlier
than the first AutofillManager is constructed.

Also, while initialising the string is not guarded by any mutex, it is
safe -- AutofillManager gets constructed on the UI thread, so there are not
more concurrent constructions. And once the string is set, it is never set
again, so a constructed CountryNames can read it safely on any thread.

To get the application locale, this CL also adds a method to AutofillClient to
provide it independently of platform.

(Caveat B) Before this CL, the code was capable of generating more data for a
newly added locale on the fly. After this CL, it will only work for the locale
of the program during the CountryNames creation (and en_US as a fallback).
Note that Chrome does not seem to be able to change the language without
restart, so this should be no concern. The only unknown for me is CrOS -- I'm
not sure whether a new CountryNames instance is created for each user session.
But my suggestion is to wait with making the code more flexible until the
more systematic fix to the way it generates and stores the CountryNames data.

Finally, this CL adds a way to construct CountryNames with an explicit locale
in tests, and uses it in the appropriate unit tests.

BUG=571610
==========

[vabr (Chromium), 2016-01-15 19:43:27]

vabr@chromium.org changed reviewers:
+ boliu@chromium.org, isherman@chromium.org, jdonnelly@chromium.org,
pavely@chromium.org

[vabr (Chromium), 2016-01-15 19:43:27]

Hello reviewers!

boliu@ -- please review android_webview/native/aw_autofill_client.*

pavely@ -- please review
chrome/browser/sync/profile_sync_service_autofill_unittest.cc

jdonnelly@ -- please review ios/chrome/browser/ui/autofill/autofill_client_ios.h

isherman@ -- please review the rest

mathp@ -- your review is optional, because covered by isherman's, but you are
certainly welcome to do one if you have any comments to share

I acknowledge that Monday might be holiday for some of you. This CL does not
need to be reviewed before Monday, so no rush.

Cheers,
Vaclav

[vabr (Chromium), 2016-01-15 19:48:05]

Small correction:

On 2016/01/15 19:43:27, vabr (Chromium) wrote:
> jdonnelly@ -- please review
ios/chrome/browser/ui/autofill/autofill_client_ios.h
Actually also ios/chrome/browser/ui/autofill/autofill_client_ios.mm, please. :)

[pavely, 2016-01-15 23:13:39]

chrome/browser/sync/profile_sync_service_autofill_unittest.cc: lgtm

[boliu, 2016-01-19 19:44:48]

There are android compile errors. Also kinda waiting for actual reviewer to
approve before stamping.

[Ilya Sherman, 2016-01-20 01:31:54]

Did you consider ensuring that the method is only called from a single thread or
using a lock rather than changing the timing?  I'm a little concerned that
changing the timing might create extra work at startup, which is a time when
resources are under heavy contention.

[vabr (Chromium), 2016-01-20 16:43:11]

@boliu: the Android failures are now fixed.

@isherman:
On 2016/01/20 01:31:54, Ilya Sherman wrote:
> Did you consider ensuring that the method is only called from a single thread
or
> using a lock rather than changing the timing?
That's actually what is happening -- the expensive method is called when
Singleton
decides to create CountryNames, and Singleton guarantees that it happens on a
single
thread. Adding custom locks would just create a second place like that, but with
additional code complexity.

> I'm a little concerned that
> changing the timing might create extra work at startup, which is a time when
> resources are under heavy contention.
Note that this did not really change the timing -- the extra work is now done on
CountryNames creation, and used to be done during its first use, which are
almost
the same moments.

What I do earlier (approximately during tab creation, in AutofillManager, is to
set
the locale string to be used by CountryNames later. But that's a quick job,
nothing
heavy.



One way to look at this CL, and how it resolves the threading issue is
understanding
what it sacrifices to fix the issue: it sacrifices the flexibility to compute
more
country-names-data for additional locales. After this CL, it only will have the
data
for the current locale and en-US, without the possibility to compute more on the
fly
later than on the first use. This bought me the possibility to guarantee
computing
that data before its use, solving the threading issue.
The sacrifice was not pushing the heavy computation at Chrome's start, though --
it
happens on the CountryNames' construction. And because Singleton's construction
is
on-demand, the timing was not affected by this.

Happy to answer more questions!
Vaclav

[Ilya Sherman, 2016-01-21 02:07:57]

Okay, I think I see your point about the timing not really changing.  In that
case, I'm okay with this approach, in general.

https://codereview.chromium.org/1582353006/diff/140001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/1582353006/diff/140001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:33: #include
"chrome/browser/browser_process.h"
nit: Can this include be removed from this, or any other, files?

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
File components/autofill/core/browser/country_names.cc (right):

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:113: // empty string, to
account for the very rare cases when the collator fails to
nit: s/string/map

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:145: if (default_locale !=
locale_object)
If you pass the default_collotor_ into this method, then comparisons against
"en_US" are unnecessary.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:158: }
Tracking |initialized| here seems not necessary -- you could just check whether
the locale is set or not.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:158: }
I think it would be good to DCHECK that the locale never changes during Chrome's
runtime.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:173:
GetDefaultLocalizedNames(locale_name, collator_.get())) {}
Why doesn't this use the default_collator_?

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:175:
CountryNames::CountryNames() : CountryNames(g_application_locale.Get()) {}
Please DCHECK that g_application_locale is non-empty by this point.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:195:
DCHECK(default_collator_);
nit: You could just write "if (default_collator_)" and drop the default_locale
entirely.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
File components/autofill/core/browser/country_names.h (right):

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.h:28: void
SetLocaleString(std::string locale);
Please define this as a static method on CountryNames.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.h:28: void
SetLocaleString(std::string locale);
nit: Pass by reference.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.h:38: // can be expensive so use
judiciously.
Can this function still be expensive?  It's probably GetInstance() that is now
potentially expensive, right?

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.h:54: // Looks up |country_name|
in |localized_names|, using |collator| andd
nit: s/andd/and

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
File components/autofill/core/browser/country_names_unittest.cc (right):

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names_unittest.cc:33: TestCountryNames
nl_names("nl");
Let's go ahead and split this test up into several tests -- mebbe one per
commented section?

[vabr (Chromium), 2016-01-22 13:46:45]

Description was changed from

==========
CountryNames: Separate data creation from usage

==Goal==
Fix a race condition in CountryNames-related code without complicating
it significantly.

==Non-goal==
Any deeper refactoring not necessary to fix the above race
condition. A discussion is needed to see whether once could avoid the
Singleton business and use some more common ways to store the country names
data (a database?), as well as whether it is necessary to compute it in
run-time. But that is explicitly out of scope of this CL.

==Content==
This CL makes CountryNames create its data (mappings from country names to
country codes and the appropriate collators for hashing the country names) on
construction, rather than on demand.

The main purpose of this change is to eliminate race conditions. Before this
CL, multiple threads could have trigger the data creation, and it is believed
to have caused data corruption and crashes.

Because the CountryNames creation is guaranteed by Singleton to only happen
once, it solves the threading issues.

However, there are caveats:
(Caveat A) CountryNames cannot get constructor arguments,
being created through Singleton's get(). But it needs to know which locale
Chrome is using. To circumvent this, it gets the information from a global
static string, and relies on the callsites of CountryNames to ensure
initialisation of this string.

To make that happen, every instance of AutofillManager ensures on its
construction, that said string is initialised. This is a bit clumsy (there is
an AutofillManager for each tab, but this pointer only needs to be initialised
once per Chrome's execution), but works -- CountryNames are never used earlier
than the first AutofillManager is constructed.

Also, while initialising the string is not guarded by any mutex, it is
safe -- AutofillManager gets constructed on the UI thread, so there are not
more concurrent constructions. And once the string is set, it is never set
again, so a constructed CountryNames can read it safely on any thread.

To get the application locale, this CL also adds a method to AutofillClient to
provide it independently of platform.

(Caveat B) Before this CL, the code was capable of generating more data for a
newly added locale on the fly. After this CL, it will only work for the locale
of the program during the CountryNames creation (and en_US as a fallback).
Note that Chrome does not seem to be able to change the language without
restart, so this should be no concern. The only unknown for me is CrOS -- I'm
not sure whether a new CountryNames instance is created for each user session.
But my suggestion is to wait with making the code more flexible until the
more systematic fix to the way it generates and stores the CountryNames data.

Finally, this CL adds a way to construct CountryNames with an explicit locale
in tests, and uses it in the appropriate unit tests.

BUG=571610
==========

to

==========
CountryNames: Separate data creation from usage

==Goal==
Fix a race condition in CountryNames-related code without complicating
it significantly.

==Non-goal==
Any deeper refactoring not necessary to fix the above race
condition. A discussion is needed to see whether once could avoid the
Singleton business and use some more common ways to store the country names
data (a database?), as well as whether it is necessary to compute it in
run-time. But that is explicitly out of scope of this CL.

==Content==
This CL makes CountryNames create its data (mappings from country names to
country codes and the appropriate collators for hashing the country names) on
construction, rather than on demand.

The main purpose of this change is to eliminate race conditions. Before this
CL, multiple threads could have trigger the data creation, and it is believed
to have caused data corruption and crashes.

Because the CountryNames creation is guaranteed by Singleton to only happen
once, it solves the threading issues.

However, there are caveats:
(Caveat A) CountryNames cannot get constructor arguments,
being created through Singleton's get(). But it needs to know which locale
Chrome is using. To circumvent this, it gets the information from a global
static string, and relies on the callsites of CountryNames to ensure
initialisation of this string.

To make that happen, every instance of AutofillManager ensures on its
construction, that said string is initialised. This is a bit clumsy (there is
an AutofillManager for each tab, but this pointer only needs to be initialised
once per Chrome's execution), but works -- CountryNames are never used earlier
than the first AutofillManager is constructed.

Also, while initialising the string is not guarded by any mutex, it is
safe -- AutofillManager gets constructed on the UI thread, so there are not
more concurrent constructions. And once the string is set, it is never set
again, so a constructed CountryNames can read it safely on any thread.

(Caveat B) Before this CL, the code was capable of generating more data for a
newly added locale on the fly. After this CL, it will only work for the locale
of the program during the CountryNames creation (and en_US as a fallback).
Note that Chrome does not seem to be able to change the language without
restart, so this should be no concern. The only unknown for me is CrOS -- I'm
not sure whether a new CountryNames instance is created for each user session.
But my suggestion is to wait with making the code more flexible until the
more systematic fix to the way it generates and stores the CountryNames data.

Finally, this CL adds a way to construct CountryNames with an explicit locale
in tests, and uses it in the appropriate unit tests.

BUG=571610
==========

[vabr (Chromium), 2016-01-22 15:50:29]

Status update:

Still fixing failing tests, but after pruning the change, there are no longer
changes to //ios and Android webview, so I'll drop boliu@ and jdonnelly@ from
the reviewers list.

Will ping the CL once it is ready for review by Ilya again.

Cheers,
Vaclav

[vabr (Chromium), 2016-01-22 15:50:46]

vabr@chromium.org changed reviewers:
- boliu@chromium.org, jdonnelly@chromium.org

[vabr (Chromium), 2016-01-22 17:36:42]

Thanks for your review comments, Ilya.

I addressed some of them, and pushed back on others, please have a look.

To explain why I added the locale string initialisation in PersonalDataManager
as well: AutofillManager is responsible for the string initialisation normally,
but on Android apparently the settings can use CountryNames via PDM without
creating a tab with AutofillManager.

Cheers,
Vaclav

https://codereview.chromium.org/1582353006/diff/140001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/1582353006/diff/140001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:33: #include
"chrome/browser/browser_process.h"
On 2016/01/21 02:07:56, Ilya Sherman wrote:
> nit: Can this include be removed from this, or any other, files?

It is still used by FormGroups (see, e.g., line 192). I tried to check this in
the other affected files as well, and did not find an instance where I could
remove the #include.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
File components/autofill/core/browser/country_names.cc (right):

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:113: // empty string, to
account for the very rare cases when the collator fails to
On 2016/01/21 02:07:56, Ilya Sherman wrote:
> nit: s/string/map

Done.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:145: if (default_locale !=
locale_object)
On 2016/01/21 02:07:56, Ilya Sherman wrote:
> If you pass the default_collotor_ into this method, then comparisons against
> "en_US" are unnecessary.

You are correct, thanks for catching this!
Removed the whole function.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:158: }
On 2016/01/21 02:07:56, Ilya Sherman wrote:
> Tracking |initialized| here seems not necessary -- you could just check
whether
> the locale is set or not.

I'm not sure how to do that. LazyInstance does not seem to have a way to tell
whether the instance has been already created (without creating it as a
side-effect).

So I assumed you meant treating the empty locale as a special value meaning
"initialized == false". This should work as long as nobody passes an empty
string to this method, which should not happen (e.g., on Linux, even if the
environment variable LANGUAGE is empty, the browser process reports the default
English locale). (Setting |g_application_locale| multiple times might result in
race conditions again, because at the same time |g_application_locale| is set
for the second time, CountryNames could be being constructed on another thread.)

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:158: }
On 2016/01/21 02:07:56, Ilya Sherman wrote:
> I think it would be good to DCHECK that the locale never changes during
Chrome's
> runtime.

So apparently, Chrome might be expected to be able to change locale on some
platforms after all, looking at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/....

While there is the API (used by the linked test) to change the locale, it is
apparently not working on all platforms, and I could not find a way to trigger
this via UI on the platforms I checked (Linux, Win, Android).

Judging the trade-offs, for losing this flexibility:
 (+) we gain the fix for the race condition (which relies on computing the
locale-based data once and at start, thus avoiding races later),
 (-) we make autofill's heuristics slightly worse in the case the user manages
to switch locale while Chrome is running.

I see (-) dwarfing (+) because of the following:
 * The situation from (-) is unlikely to happen (I have no idea how it can be
triggered, if at all).
 * The impact of (-) is questionable -- it is perfectly normal for a user to
have, say, English locale, yet browse, e.g., Czech pages. In cases like this,
autofill is equally bad at understanding the names of the countries anyway.
 * http://crbug.com/579971 should restore this feature eventually

In the light of this, I dropped the check, and put a TODO inside instead. Does
that sound reasonable?

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:173:
GetDefaultLocalizedNames(locale_name, collator_.get())) {}
On 2016/01/21 02:07:56, Ilya Sherman wrote:
> Why doesn't this use the default_collator_?

I got myself confused with all the default stuff, but you helped me prune it. :)
Fixed now.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:175:
CountryNames::CountryNames() : CountryNames(g_application_locale.Get()) {}
On 2016/01/21 02:07:56, Ilya Sherman wrote:
> Please DCHECK that g_application_locale is non-empty by this point.

Done.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.cc:195:
DCHECK(default_collator_);
On 2016/01/21 02:07:56, Ilya Sherman wrote:
> nit: You could just write "if (default_collator_)" and drop the default_locale
> entirely.

Done.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
File components/autofill/core/browser/country_names.h (right):

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.h:28: void
SetLocaleString(std::string locale);
On 2016/01/21 02:07:57, Ilya Sherman wrote:
> nit: Pass by reference.

This is by value on purpose. Looking in the SetLocaleString definition, you will
see that |locale| is moved. So if the argument to this method is a temporary,
there will be no copy. If it is not a temporary (including when it is a
refererence), the copy will happen on calling, but that's the same as if it
happened by assigining a const ref in the function body.

So in summary, this is in some cases as expensive, and in other cheaper than
using a const ref.

(This technique was one of the main takeaways of the C++11 course for me.)

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.h:28: void
SetLocaleString(std::string locale);
On 2016/01/21 02:07:57, Ilya Sherman wrote:
> Please define this as a static method on CountryNames.

Good call!
Done.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.h:38: // can be expensive so use
judiciously.
On 2016/01/21 02:07:57, Ilya Sherman wrote:
> Can this function still be expensive?  It's probably GetInstance() that is now
> potentially expensive, right?

Correct, GetCountryCode is nowhere as expensive as the creation of CountryNames.
So I moved that note to GetInstance.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.h:54: // Looks up |country_name|
in |localized_names|, using |collator| andd
On 2016/01/21 02:07:56, Ilya Sherman wrote:
> nit: s/andd/and

Done.

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
File components/autofill/core/browser/country_names_unittest.cc (right):

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names_unittest.cc:33: TestCountryNames
nl_names("nl");
On 2016/01/21 02:07:57, Ilya Sherman wrote:
> Let's go ahead and split this test up into several tests -- mebbe one per
> commented section?

Done.

[Ilya Sherman, 2016-01-23 01:41:04]

LGTM % a couple more nits:

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
File components/autofill/core/browser/country_names.h (right):

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.h:28: void
SetLocaleString(std::string locale);
On 2016/01/22 17:36:42, vabr (Chromium) wrote:
> On 2016/01/21 02:07:57, Ilya Sherman wrote:
> > nit: Pass by reference.
> 
> This is by value on purpose. Looking in the SetLocaleString definition, you
will
> see that |locale| is moved. So if the argument to this method is a temporary,
> there will be no copy. If it is not a temporary (including when it is a
> refererence), the copy will happen on calling, but that's the same as if it
> happened by assigining a const ref in the function body.
> 
> So in summary, this is in some cases as expensive, and in other cheaper than
> using a const ref.
> 
> (This technique was one of the main takeaways of the C++11 course for me.)

Boy, this seems like a super duper micro-optimization.  I'd recommend emailing
chromium-dev@ to ask whether this style is preferred -- it certainly surprised
me to see std::move() being used to move strings of ~5 characters in length. 
But maybe I'm just behind the times =)

https://codereview.chromium.org/1582353006/diff/360001/components/autofill/co...
File components/autofill/core/browser/country_names.cc (right):

https://codereview.chromium.org/1582353006/diff/360001/components/autofill/co...
components/autofill/core/browser/country_names.cc:149: std::string* const
storage = g_application_locale.Pointer();
nit: The const here seems not very necessary.

https://codereview.chromium.org/1582353006/diff/360001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/1582353006/diff/360001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:103:
CountryNames::SetLocaleString("en-US");
Hmm, why is this needed, given that the PDM sets the locale internally?

[vabr (Chromium), 2016-01-25 10:30:39]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-25 10:30:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1582353006/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1582353006/380001

[vabr (Chromium), 2016-01-25 10:34:19]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2016-01-25 10:36:44]

Thanks, Ilya!

I will send this to the CQ, and e-mail chromium-dev as discussed below in the
comments.

Cheers,
Vaclav

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
File components/autofill/core/browser/country_names.h (right):

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.h:28: void
SetLocaleString(std::string locale);
On 2016/01/23 01:41:04, Ilya Sherman wrote:
> On 2016/01/22 17:36:42, vabr (Chromium) wrote:
> > On 2016/01/21 02:07:57, Ilya Sherman wrote:
> > > nit: Pass by reference.
> > 
> > This is by value on purpose. Looking in the SetLocaleString definition, you
> will
> > see that |locale| is moved. So if the argument to this method is a
temporary,
> > there will be no copy. If it is not a temporary (including when it is a
> > refererence), the copy will happen on calling, but that's the same as if it
> > happened by assigining a const ref in the function body.
> > 
> > So in summary, this is in some cases as expensive, and in other cheaper than
> > using a const ref.
> > 
> > (This technique was one of the main takeaways of the C++11 course for me.)
> 
> Boy, this seems like a super duper micro-optimization.  I'd recommend emailing
> chromium-dev@ to ask whether this style is preferred -- it certainly surprised
> me to see std::move() being used to move strings of ~5 characters in length. 
> But maybe I'm just behind the times =)

E-mailing chromium-dev seems like a good idea, I'll do that. I will still
proceed with landing the current code, because the crasher needs fixing, and
this is just a nit. If there is a conclusive feedback against pass by value +
move, I am happy to make a follow-up CL.

Back to the issue itself -- IMO this is not an optimization, it is a safe
default practice. It was not done before because there was no moving in C++, not
because it has any disadvantages. The point is that one can write this and not
think about how long the strings actually are.

https://codereview.chromium.org/1582353006/diff/360001/components/autofill/co...
File components/autofill/core/browser/country_names.cc (right):

https://codereview.chromium.org/1582353006/diff/360001/components/autofill/co...
components/autofill/core/browser/country_names.cc:149: std::string* const
storage = g_application_locale.Pointer();
On 2016/01/23 01:41:04, Ilya Sherman wrote:
> nit: The const here seems not very necessary.

A matter of taste, I guess :), but removed.

https://codereview.chromium.org/1582353006/diff/360001/components/autofill/co...
File components/autofill/core/browser/personal_data_manager_unittest.cc (right):

https://codereview.chromium.org/1582353006/diff/360001/components/autofill/co...
components/autofill/core/browser/personal_data_manager_unittest.cc:103:
CountryNames::SetLocaleString("en-US");
On 2016/01/23 01:41:04, Ilya Sherman wrote:
> Hmm, why is this needed, given that the PDM sets the locale internally?

Good point, I forgot this here when I modified PDM, I guess.
Removed.

[vabr (Chromium), 2016-01-25 10:37:28]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-01-25 10:37:29]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1582353006/#ps420001
(title: "Just rebased")

[commit-bot: I haz the power, 2016-01-25 10:38:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1582353006/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1582353006/420001

[commit-bot: I haz the power, 2016-01-25 11:34:14]

Description was changed from

==========
CountryNames: Separate data creation from usage

==Goal==
Fix a race condition in CountryNames-related code without complicating
it significantly.

==Non-goal==
Any deeper refactoring not necessary to fix the above race
condition. A discussion is needed to see whether once could avoid the
Singleton business and use some more common ways to store the country names
data (a database?), as well as whether it is necessary to compute it in
run-time. But that is explicitly out of scope of this CL.

==Content==
This CL makes CountryNames create its data (mappings from country names to
country codes and the appropriate collators for hashing the country names) on
construction, rather than on demand.

The main purpose of this change is to eliminate race conditions. Before this
CL, multiple threads could have trigger the data creation, and it is believed
to have caused data corruption and crashes.

Because the CountryNames creation is guaranteed by Singleton to only happen
once, it solves the threading issues.

However, there are caveats:
(Caveat A) CountryNames cannot get constructor arguments,
being created through Singleton's get(). But it needs to know which locale
Chrome is using. To circumvent this, it gets the information from a global
static string, and relies on the callsites of CountryNames to ensure
initialisation of this string.

To make that happen, every instance of AutofillManager ensures on its
construction, that said string is initialised. This is a bit clumsy (there is
an AutofillManager for each tab, but this pointer only needs to be initialised
once per Chrome's execution), but works -- CountryNames are never used earlier
than the first AutofillManager is constructed.

Also, while initialising the string is not guarded by any mutex, it is
safe -- AutofillManager gets constructed on the UI thread, so there are not
more concurrent constructions. And once the string is set, it is never set
again, so a constructed CountryNames can read it safely on any thread.

(Caveat B) Before this CL, the code was capable of generating more data for a
newly added locale on the fly. After this CL, it will only work for the locale
of the program during the CountryNames creation (and en_US as a fallback).
Note that Chrome does not seem to be able to change the language without
restart, so this should be no concern. The only unknown for me is CrOS -- I'm
not sure whether a new CountryNames instance is created for each user session.
But my suggestion is to wait with making the code more flexible until the
more systematic fix to the way it generates and stores the CountryNames data.

Finally, this CL adds a way to construct CountryNames with an explicit locale
in tests, and uses it in the appropriate unit tests.

BUG=571610
==========

to

==========
CountryNames: Separate data creation from usage

==Goal==
Fix a race condition in CountryNames-related code without complicating
it significantly.

==Non-goal==
Any deeper refactoring not necessary to fix the above race
condition. A discussion is needed to see whether once could avoid the
Singleton business and use some more common ways to store the country names
data (a database?), as well as whether it is necessary to compute it in
run-time. But that is explicitly out of scope of this CL.

==Content==
This CL makes CountryNames create its data (mappings from country names to
country codes and the appropriate collators for hashing the country names) on
construction, rather than on demand.

The main purpose of this change is to eliminate race conditions. Before this
CL, multiple threads could have trigger the data creation, and it is believed
to have caused data corruption and crashes.

Because the CountryNames creation is guaranteed by Singleton to only happen
once, it solves the threading issues.

However, there are caveats:
(Caveat A) CountryNames cannot get constructor arguments,
being created through Singleton's get(). But it needs to know which locale
Chrome is using. To circumvent this, it gets the information from a global
static string, and relies on the callsites of CountryNames to ensure
initialisation of this string.

To make that happen, every instance of AutofillManager ensures on its
construction, that said string is initialised. This is a bit clumsy (there is
an AutofillManager for each tab, but this pointer only needs to be initialised
once per Chrome's execution), but works -- CountryNames are never used earlier
than the first AutofillManager is constructed.

Also, while initialising the string is not guarded by any mutex, it is
safe -- AutofillManager gets constructed on the UI thread, so there are not
more concurrent constructions. And once the string is set, it is never set
again, so a constructed CountryNames can read it safely on any thread.

(Caveat B) Before this CL, the code was capable of generating more data for a
newly added locale on the fly. After this CL, it will only work for the locale
of the program during the CountryNames creation (and en_US as a fallback).
Note that Chrome does not seem to be able to change the language without
restart, so this should be no concern. The only unknown for me is CrOS -- I'm
not sure whether a new CountryNames instance is created for each user session.
But my suggestion is to wait with making the code more flexible until the
more systematic fix to the way it generates and stores the CountryNames data.

Finally, this CL adds a way to construct CountryNames with an explicit locale
in tests, and uses it in the appropriate unit tests.

BUG=571610
==========

[commit-bot: I haz the power, 2016-01-25 11:34:15]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2016-01-25 11:35:12]

Description was changed from

==========
CountryNames: Separate data creation from usage

==Goal==
Fix a race condition in CountryNames-related code without complicating
it significantly.

==Non-goal==
Any deeper refactoring not necessary to fix the above race
condition. A discussion is needed to see whether once could avoid the
Singleton business and use some more common ways to store the country names
data (a database?), as well as whether it is necessary to compute it in
run-time. But that is explicitly out of scope of this CL.

==Content==
This CL makes CountryNames create its data (mappings from country names to
country codes and the appropriate collators for hashing the country names) on
construction, rather than on demand.

The main purpose of this change is to eliminate race conditions. Before this
CL, multiple threads could have trigger the data creation, and it is believed
to have caused data corruption and crashes.

Because the CountryNames creation is guaranteed by Singleton to only happen
once, it solves the threading issues.

However, there are caveats:
(Caveat A) CountryNames cannot get constructor arguments,
being created through Singleton's get(). But it needs to know which locale
Chrome is using. To circumvent this, it gets the information from a global
static string, and relies on the callsites of CountryNames to ensure
initialisation of this string.

To make that happen, every instance of AutofillManager ensures on its
construction, that said string is initialised. This is a bit clumsy (there is
an AutofillManager for each tab, but this pointer only needs to be initialised
once per Chrome's execution), but works -- CountryNames are never used earlier
than the first AutofillManager is constructed.

Also, while initialising the string is not guarded by any mutex, it is
safe -- AutofillManager gets constructed on the UI thread, so there are not
more concurrent constructions. And once the string is set, it is never set
again, so a constructed CountryNames can read it safely on any thread.

(Caveat B) Before this CL, the code was capable of generating more data for a
newly added locale on the fly. After this CL, it will only work for the locale
of the program during the CountryNames creation (and en_US as a fallback).
Note that Chrome does not seem to be able to change the language without
restart, so this should be no concern. The only unknown for me is CrOS -- I'm
not sure whether a new CountryNames instance is created for each user session.
But my suggestion is to wait with making the code more flexible until the
more systematic fix to the way it generates and stores the CountryNames data.

Finally, this CL adds a way to construct CountryNames with an explicit locale
in tests, and uses it in the appropriate unit tests.

BUG=571610
==========

to

==========
CountryNames: Separate data creation from usage

==Goal==
Fix a race condition in CountryNames-related code without complicating
it significantly.

==Non-goal==
Any deeper refactoring not necessary to fix the above race
condition. A discussion is needed to see whether once could avoid the
Singleton business and use some more common ways to store the country names
data (a database?), as well as whether it is necessary to compute it in
run-time. But that is explicitly out of scope of this CL.

==Content==
This CL makes CountryNames create its data (mappings from country names to
country codes and the appropriate collators for hashing the country names) on
construction, rather than on demand.

The main purpose of this change is to eliminate race conditions. Before this
CL, multiple threads could have trigger the data creation, and it is believed
to have caused data corruption and crashes.

Because the CountryNames creation is guaranteed by Singleton to only happen
once, it solves the threading issues.

However, there are caveats:
(Caveat A) CountryNames cannot get constructor arguments,
being created through Singleton's get(). But it needs to know which locale
Chrome is using. To circumvent this, it gets the information from a global
static string, and relies on the callsites of CountryNames to ensure
initialisation of this string.

To make that happen, every instance of AutofillManager ensures on its
construction, that said string is initialised. This is a bit clumsy (there is
an AutofillManager for each tab, but this pointer only needs to be initialised
once per Chrome's execution), but works -- CountryNames are never used earlier
than the first AutofillManager is constructed.

Also, while initialising the string is not guarded by any mutex, it is
safe -- AutofillManager gets constructed on the UI thread, so there are not
more concurrent constructions. And once the string is set, it is never set
again, so a constructed CountryNames can read it safely on any thread.

(Caveat B) Before this CL, the code was capable of generating more data for a
newly added locale on the fly. After this CL, it will only work for the locale
of the program during the CountryNames creation (and en_US as a fallback).
Note that Chrome does not seem to be able to change the language without
restart, so this should be no concern. The only unknown for me is CrOS -- I'm
not sure whether a new CountryNames instance is created for each user session.
But my suggestion is to wait with making the code more flexible until the
more systematic fix to the way it generates and stores the CountryNames data.

Finally, this CL adds a way to construct CountryNames with an explicit locale
in tests, and uses it in the appropriate unit tests.

BUG=571610

Committed: https://crrev.com/f39cbace358fe626f0f07e32f4e66b78ca589038
Cr-Commit-Position: refs/heads/master@{#371226}
==========

[commit-bot: I haz the power, 2016-01-25 11:35:13]

Patchset 22 (id:??) landed as
https://crrev.com/f39cbace358fe626f0f07e32f4e66b78ca589038
Cr-Commit-Position: refs/heads/master@{#371226}

[vabr (Chromium), 2016-01-26 09:13:11]

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
File components/autofill/core/browser/country_names.h (right):

https://codereview.chromium.org/1582353006/diff/140001/components/autofill/co...
components/autofill/core/browser/country_names.h:28: void
SetLocaleString(std::string locale);
On 2016/01/25 10:36:44, vabr (Chromium) wrote:
> On 2016/01/23 01:41:04, Ilya Sherman wrote:
> > On 2016/01/22 17:36:42, vabr (Chromium) wrote:
> > > On 2016/01/21 02:07:57, Ilya Sherman wrote:
> > > > nit: Pass by reference.
> > > 
> > > This is by value on purpose. Looking in the SetLocaleString definition,
you
> > will
> > > see that |locale| is moved. So if the argument to this method is a
> temporary,
> > > there will be no copy. If it is not a temporary (including when it is a
> > > refererence), the copy will happen on calling, but that's the same as if
it
> > > happened by assigining a const ref in the function body.
> > > 
> > > So in summary, this is in some cases as expensive, and in other cheaper
than
> > > using a const ref.
> > > 
> > > (This technique was one of the main takeaways of the C++11 course for me.)
> > 
> > Boy, this seems like a super duper micro-optimization.  I'd recommend
emailing
> > chromium-dev@ to ask whether this style is preferred -- it certainly
surprised
> > me to see std::move() being used to move strings of ~5 characters in length.

> > But maybe I'm just behind the times =)
> 
> E-mailing chromium-dev seems like a good idea, I'll do that. I will still
> proceed with landing the current code, because the crasher needs fixing, and
> this is just a nit. If there is a conclusive feedback against pass by value +
> move, I am happy to make a follow-up CL.
> 
> Back to the issue itself -- IMO this is not an optimization, it is a safe
> default practice. It was not done before because there was no moving in C++,
not
> because it has any disadvantages. The point is that one can write this and not
> think about how long the strings actually are.

Follow-up filed in https://codereview.chromium.org/1635813003/.