New issue
Advanced search Search tips

Issue 695929 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 734093



Sign in to add a comment

ThreadSanitizer data race in base::i18n::IsRTL

Project Member Reported by mbjorge@chromium.org, Feb 24 2017

Issue description

WARNING: ThreadSanitizer: data race (pid=584)
  Read of size 4 at 0x000009ce5398 by main thread:
    #0 ICUIsRTL base/i18n/rtl.cc:161:7 (cast_test+0x00000332e00a)
    #1 base::i18n::IsRTL() base/i18n/rtl.cc:157 (cast_test+0x00000332e00a)
    #2 (anonymous namespace)::AdjustParagraphDirectionality(std::__1::basic_string<unsigned short, base::string16_char_traits, std::__1::allocator<unsigned short> >*) ui/base/l10n/l10n_util.cc:271:7 (cast_test+0x000003449ef4)
    #3 l10n_util::GetStringUTF16(int) ui/base/l10n/l10n_util.cc:680:3 (cast_test+0x000003449ed0)
    #4 chromecast::shell::CastContentClient::GetLocalizedString(int) const chromecast/common/cast_content_client.cc:91:10 (cast_test+0x00000058d2c2)
    #5 content::WebUIDataSourceImpl::AddLocalizedString(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int) content/browser/webui/web_ui_data_source_impl.cc:131:33 (cast_test+0x00000116fcbf)
    #6 non-virtual thunk to content::WebUIDataSourceImpl::AddLocalizedString(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int) content/browser/webui/web_ui_data_source_impl.cc (cast_test+0x00000116fe0f)
    [...snip...]

  Previous write of size 4 at 0x000009ce5398 by thread T32:
    #0 ICUIsRTL base/i18n/rtl.cc:163:26 (cast_test+0x00000332e03c)
    #1 base::i18n::IsRTL() base/i18n/rtl.cc:157 (cast_test+0x00000332e03c)
    #2 chromecast::GetDisplayStringForLocale(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) chromecast/.../icu_utils.cc:138:14 (cast_test+0x0000023e193d)
    [...snip...]

  Location is global 'base::i18n::g_icu_text_direction' of size 4 at 0x000009ce5398 (cast_test+0x000009ce5398)

  Thread T32 'Setup_IOThread' (tid=627, running) created by main thread at:
    #0 pthread_create <null> (cast_test+0x00000045c1a5)
    #1 base::(anonymous namespace)::CreateThread(unsigned long, bool, base::PlatformThread::Delegate*, base::PlatformThreadHandle*, base::ThreadPriority) base/threading/platform_thread_posix.cc:110:13 (cast_test+0x000002e0c956)
    #2 base::PlatformThread::CreateWithPriority(unsigned long, base::PlatformThread::Delegate*, base::PlatformThreadHandle*, base::ThreadPriority) base/threading/platform_thread_posix.cc:191:10 (cast_test+0x000002e0c815)
    #3 base::Thread::StartWithOptions(base::Thread::Options const&) base/threading/thread.cc:112:15 (cast_test+0x000002e16f33)
    [...snip...]

SUMMARY: ThreadSanitizer: data race base/i18n/rtl.cc:161:7 in ICUIsRTL



 
ICUIsRTL (https://cs.chromium.org/chromium/src/base/i18n/rtl.cc?dr=Ss&l=160) gets called into from 2 places, but ICUIsRTL is reading/writing the global g_icu_text_direction to lazily initialize it, which is causing the race.

Similarly:

WARNING: ThreadSanitizer: data race (pid=33599)
  Write of size 4 at 0x000009cda1c8 by main thread:
    #0 base::i18n::SetICUDefaultLocale(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) base/i18n/rtl.cc:153:24 (cast_shell_browsertests+0x0000032fdb81)
    #1 chromecast::(anonymous namespace)::LoadLocaleResources(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >*) chromecast/internal/l10n/locale_manager.cc:150:3 (cast_shell_browsertests+0x0000023d970e)
    #2 chromecast::LocaleManager::Initialize(scoped_refptr<base::SingleThreadTaskRunner>, PrefService*, chromecast::receiver::ApplicationManager*, net::URLRequestContextGetter*) chromecast/internal/l10n/locale_manager.cc:338:5 (cast_shell_browsertests+0x0000023d8eaa)
    [...snip...]

  Previous read of size 4 at 0x000009cda1c8 by thread T13:
    #0 ICUIsRTL base/i18n/rtl.cc:161:7 (cast_shell_browsertests+0x0000032fdbda)
    #1 base::i18n::IsRTL() base/i18n/rtl.cc:157 (cast_shell_browsertests+0x0000032fdbda)
    #2 (anonymous namespace)::AdjustParagraphDirectionality(std::__1::basic_string<unsigned short, base::string16_char_traits, std::__1::allocator<unsigned short> >*) ui/base/l10n/l10n_util.cc:271:7 (cast_shell_browsertests+0x0000034194b4)
    #3 GetStringUTF16 ui/base/l10n/l10n_util.cc:680:3 (cast_shell_browsertests+0x000003419422)
    #4 l10n_util::GetStringUTF8(int) ui/base/l10n/l10n_util.cc:674 (cast_shell_browsertests+0x000003419422)
    [...snip...]

  Location is global 'base::i18n::g_icu_text_direction' of size 4 at 0x000009cda1c8 (cast_shell_browsertests+0x000009cda1c8)

  Thread T13 'Chrome_IOThread' (tid=33615, running) created by main thread at:
    #0 pthread_create <null> (cast_shell_browsertests+0x00000045c5a5)
    #1 base::(anonymous namespace)::CreateThread(unsigned long, bool, base::PlatformThread::Delegate*, base::PlatformThreadHandle*, base::ThreadPriority) base/threading/platform_thread_posix.cc:110:13 (cast_shell_browsertests+0x00000279b8d6)
    #2 base::PlatformThread::CreateWithPriority(unsigned long, base::PlatformThread::Delegate*, base::PlatformThreadHandle*, base::ThreadPriority) base/threading/platform_thread_posix.cc:191:10 (cast_shell_browsertests+0x00000279b795)
    #3 base::Thread::StartWithOptions(base::Thread::Options const&) base/threading/thread.cc:112:15 (cast_shell_browsertests+0x0000027a5eb3)
    [...snip...]

SUMMARY: ThreadSanitizer: data race base/i18n/rtl.cc:153:24 in base::i18n::SetICUDefaultLocale(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d27e590ad6471e444e164597ee22a80c7801451d

commit d27e590ad6471e444e164597ee22a80c7801451d
Author: mbjorge <mbjorge@chromium.org>
Date: Thu Mar 02 00:07:45 2017

[TSAN] Fix data race with base::i18n text direction.

The global g_icu_text_direction could be read/written from
any thread that called base::i18n::IsRTL resulting in a data race.
Guard read/writes with a mutex.

BUG=695929
TEST=base_unittests

Review-Url: https://codereview.chromium.org/2716003002
Cr-Commit-Position: refs/heads/master@{#454108}

[modify] https://crrev.com/d27e590ad6471e444e164597ee22a80c7801451d/base/i18n/rtl.cc

Status: Fixed (was: Untriaged)
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c019d90a9527169ba4ed43f0cc80b40c49dbc28

commit 8c019d90a9527169ba4ed43f0cc80b40c49dbc28
Author: mbjorge <mbjorge@chromium.org>
Date: Wed Mar 08 06:53:20 2017

Revert of [TSAN] Fix data race with base::i18n text direction. (patchset #1 id:1 of https://codereview.chromium.org/2716003002/ )

Reason for revert:
IsRTL is called a lot and this caused serious performance impact.

Original issue's description:
> [TSAN] Fix data race with base::i18n text direction.
>
> The global g_icu_text_direction could be read/written from
> any thread that called base::i18n::IsRTL resulting in a data race.
> Guard read/writes with a mutex.
>
> BUG=695929
> TEST=base_unittests
>
> Review-Url: https://codereview.chromium.org/2716003002
> Cr-Commit-Position: refs/heads/master@{#454108}
> Committed: https://chromium.googlesource.com/chromium/src/+/d27e590ad6471e444e164597ee22a80c7801451d

TBR=jshin@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=695929

Review-Url: https://codereview.chromium.org/2722413004
Cr-Commit-Position: refs/heads/master@{#455392}

[modify] https://crrev.com/8c019d90a9527169ba4ed43f0cc80b40c49dbc28/base/i18n/rtl.cc

Status: Assigned (was: Fixed)
Project Member

Comment 7 by bugdroid1@chromium.org, May 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80429170bb3de6b4639f42fc630a2d1f8b8a7117

commit 80429170bb3de6b4639f42fc630a2d1f8b8a7117
Author: Mike Bjorge <mbjorge@chromium.org>
Date: Sun May 14 08:23:31 2017

[TSAN] Temporarily suppress data race in ICUIsRTL.

It's harder to see if new errors come up since this triggers
TSAN failures in so many places. Suppress for now until a there is a
solution.

BUG=695929

Change-Id: I2729c069a29ab595208f8f04695cace3877c1083
Reviewed-on: https://chromium-review.googlesource.com/503790
Reviewed-by: Jungshik Shin <jshin@chromium.org>
Commit-Queue: Mike Bjorge <mbjorge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#471622}
[modify] https://crrev.com/80429170bb3de6b4639f42fc630a2d1f8b8a7117/build/sanitizers/tsan_suppressions.cc

Project Member

Comment 8 by bugdroid1@chromium.org, May 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89c24d4a46c1f0e7ad03891a659e6d23dc776c36

commit 89c24d4a46c1f0e7ad03891a659e6d23dc776c36
Author: mbjorge <mbjorge@chromium.org>
Date: Thu May 18 18:53:54 2017

[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}

[modify] https://crrev.com/89c24d4a46c1f0e7ad03891a659e6d23dc776c36/base/i18n/rtl.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Jun 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/27175dace3576a3e351348cf8272c20b0d3aceec

commit 27175dace3576a3e351348cf8272c20b0d3aceec
Author: mbjorge <mbjorge@chromium.org>
Date: Mon Jun 05 20:12:55 2017

Revert of [TSAN] Fix TSAN error in i18n RTL. (patchset #3 id:40001 of https://codereview.chromium.org/2877983004/ )

Reason for revert:
Appears to break the Force UI Direction / Force Text Direction / FOrce RTL flags on mac.

BUG= 727229 

Original issue's description:
> [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

TBR=jshin@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=695929

Review-Url: https://codereview.chromium.org/2917523002
Cr-Commit-Position: refs/heads/master@{#477064}

[modify] https://crrev.com/27175dace3576a3e351348cf8272c20b0d3aceec/base/i18n/rtl.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 14 2017

Labels: merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eeb12a2d4f56714eafd9e71dba80ca378ace13e9

commit eeb12a2d4f56714eafd9e71dba80ca378ace13e9
Author: Mike Bjorge <mbjorge@chromium.org>
Date: Wed Jun 14 20:44:20 2017

Revert of [TSAN] Fix TSAN error in i18n RTL. (patchset #3 id:40001 of https://codereview.chromium.org/2877983004/ )

Reason for revert:
Appears to break the Force UI Direction / Force Text Direction / FOrce RTL flags on mac.

BUG= 727229 

Original issue's description:
> [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

BUG=695929

(cherry picked from commit 27175dace3576a3e351348cf8272c20b0d3aceec)

Review-Url: https://codereview.chromium.org/2917523002
Cr-Original-Commit-Position: refs/heads/master@{#477064}
Change-Id: I7e06fc64d20b673766f2bed3f084326e590024f1
Reviewed-on: https://chromium-review.googlesource.com/527328
Reviewed-by: Jungshik Shin <jshin@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#345}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/eeb12a2d4f56714eafd9e71dba80ca378ace13e9/base/i18n/rtl.cc

Comment 11 by js...@chromium.org, Jul 10 2017

Blocking: 734093
jshin, what's the status here?

Sign in to add a comment