[TSAN] Fix TSAN error in i18n RTL.

[TSAN] Fix TSAN error in i18n RTL.

Use an atomic to protect the global g_icu_text_direction cached value
from multiple readers/writers.

Also reduce acccess to g_icu_text_diretion so it only has a single write
location (in SetICUDefaultLocal) and a single read location (ICUIsRTL),
to simplify the race-y-ness.

Outside of test code, SetICUDefaultLocal is only used in two locations:
ppapi_plugin_main.cc and l10n_util.cc, whereas IsRTL is used in >100
places outside of test code, so it seemed reasonable to move the heavy
logic (updating g_icu_text_direction when it changes) into what I imagine to be
a function that is called rarely and let the one that is used a lot
(IsRTL/ICUIsRTL) just do a simple read.

This still still leaves a race condition in that IsRTL may give in
inaccurate result if SetICUDefaultLocale is executed at the same time.
In particular, IsRTL may return the value for the previously set default
locale after the default locale has been updated.

BUG=695929
TEST=cast_shell_browsertests under TSAN before/after

Review-Url: https://codereview.chromium.org/2877983004
Cr-Commit-Position: refs/heads/master@{#472890}
Committed: https://chromium.googlesource.com/chromium/src/+/89c24d4a46c1f0e7ad03891a659e6d23dc776c36

[mbjorge, 2017-05-14 07:44:25]

mbjorge@chromium.org changed reviewers:
+ jshin@chromium.org

[mbjorge, 2017-05-15 18:11:14]

Description was changed from

==========
[TSAN] Fix TSAN error in i18n RTL.

Use an atomic to protect the global g_icu_text_direction cached value
from multiple readers/writers.

Also reduce acccess to g_icu_text_diretion so it only has a single write
location (in SetICUDefaultLocal) and a single read location (ICUIsRTL),
to simplify the race-y-ness.

Outside of test code, SetICUDefaultLocal is only used in two locations:
ppapi_plugin_main.cc and l10n_util.cc, whereas IsRTL is used in >100
places outside of test code, so it seemed reasonable to move the heavy
logic (updating g_icu_text_direction when it changes) into what I imagine to be
a function that is called rarely and let the one that is used a lot
(IsRTL/ICUIsRTL) just do a simple read.

This still still leaves a race condition in that IsRTL may give in
inaccurate result if SetICUDefaultLocale is executed at the same time.
In particular, IsRTL may return the value for the previously set default
locale after the default locale has been updated.

BUG=695929
TEST=cast_shell_browsertests under TSAN before/after

Change-Id: I1ea12d5111823253a5564157b1b7a4e68320904c
==========

to

==========
[TSAN] Fix TSAN error in i18n RTL.

Use an atomic to protect the global g_icu_text_direction cached value
from multiple readers/writers.

Also reduce acccess to g_icu_text_diretion so it only has a single write
location (in SetICUDefaultLocal) and a single read location (ICUIsRTL),
to simplify the race-y-ness.

Outside of test code, SetICUDefaultLocal is only used in two locations:
ppapi_plugin_main.cc and l10n_util.cc, whereas IsRTL is used in >100
places outside of test code, so it seemed reasonable to move the heavy
logic (updating g_icu_text_direction when it changes) into what I imagine to be
a function that is called rarely and let the one that is used a lot
(IsRTL/ICUIsRTL) just do a simple read.

This still still leaves a race condition in that IsRTL may give in
inaccurate result if SetICUDefaultLocale is executed at the same time.
In particular, IsRTL may return the value for the previously set default
locale after the default locale has been updated.

BUG=695929
TEST=cast_shell_browsertests under TSAN before/after
==========

[jungshik at Google, 2017-05-17 22:18:59]

Didn't I review this already (another CL? )?   Or, am I dreaming? :-) 

Anyway, thanks. LGTM

[mbjorge, 2017-05-17 22:34:34]

On 2017/05/17 at 22:18:59, jshin wrote:
> Didn't I review this already (another CL? )?   Or, am I dreaming? :-) 
> 
> Anyway, thanks. LGTM

I landed another CL recently that just suppressed the issue
(https://chromium-review.googlesource.com/c/503790/) and a couple months ago
tried my hand at a mutext-based solution
(https://codereview.chromium.org/2716003002, reverted), so perhaps one of those.
(If this sticks, then the suppression one can get reverted though.)

I think we all dream of a world with no race conditions :)

[mbjorge, 2017-05-17 22:34:37]

The CQ bit was checked by mbjorge@chromium.org

[commit-bot: I haz the power, 2017-05-17 22:35:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 04:50:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 04:50:58]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mbjorge, 2017-05-18 17:11:24]

The CQ bit was checked by mbjorge@chromium.org

[commit-bot: I haz the power, 2017-05-18 17:12:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 18:53:58]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495127484856000,
"parent_rev": "a8946999597874d4773c3bc74a3d20ec8b63410b", "commit_rev":
"89c24d4a46c1f0e7ad03891a659e6d23dc776c36"}

[commit-bot: I haz the power, 2017-05-18 18:54:10]

Description was changed from

==========
[TSAN] Fix TSAN error in i18n RTL.

Use an atomic to protect the global g_icu_text_direction cached value
from multiple readers/writers.

Also reduce acccess to g_icu_text_diretion so it only has a single write
location (in SetICUDefaultLocal) and a single read location (ICUIsRTL),
to simplify the race-y-ness.

Outside of test code, SetICUDefaultLocal is only used in two locations:
ppapi_plugin_main.cc and l10n_util.cc, whereas IsRTL is used in >100
places outside of test code, so it seemed reasonable to move the heavy
logic (updating g_icu_text_direction when it changes) into what I imagine to be
a function that is called rarely and let the one that is used a lot
(IsRTL/ICUIsRTL) just do a simple read.

This still still leaves a race condition in that IsRTL may give in
inaccurate result if SetICUDefaultLocale is executed at the same time.
In particular, IsRTL may return the value for the previously set default
locale after the default locale has been updated.

BUG=695929
TEST=cast_shell_browsertests under TSAN before/after
==========

to

==========
[TSAN] Fix TSAN error in i18n RTL.

Use an atomic to protect the global g_icu_text_direction cached value
from multiple readers/writers.

Also reduce acccess to g_icu_text_diretion so it only has a single write
location (in SetICUDefaultLocal) and a single read location (ICUIsRTL),
to simplify the race-y-ness.

Outside of test code, SetICUDefaultLocal is only used in two locations:
ppapi_plugin_main.cc and l10n_util.cc, whereas IsRTL is used in >100
places outside of test code, so it seemed reasonable to move the heavy
logic (updating g_icu_text_direction when it changes) into what I imagine to be
a function that is called rarely and let the one that is used a lot
(IsRTL/ICUIsRTL) just do a simple read.

This still still leaves a race condition in that IsRTL may give in
inaccurate result if SetICUDefaultLocale is executed at the same time.
In particular, IsRTL may return the value for the previously set default
locale after the default locale has been updated.

BUG=695929
TEST=cast_shell_browsertests under TSAN before/after

Review-Url: https://codereview.chromium.org/2877983004
Cr-Commit-Position: refs/heads/master@{#472890}
Committed:
https://chromium.googlesource.com/chromium/src/+/89c24d4a46c1f0e7ad03891a659e...
==========

[commit-bot: I haz the power, 2017-05-18 18:54:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/89c24d4a46c1f0e7ad03891a659e...

[mbjorge, 2017-05-31 00:55:42]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2917523002/ by mbjorge@chromium.org.

The reason for reverting is: Appears to break the Force UI Direction / Force
Text Direction / FOrce RTL flags on mac.

BUG=727229.