ThreadSanitizer data race in base::i18n::IsRTL |
|||||
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
,
Feb 24 2017
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&)
,
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
,
Mar 2 2017
,
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
,
May 12 2017
,
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
,
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
,
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
,
Jun 14 2017
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
,
Jul 10 2017
,
Dec 19
jshin, what's the status here? |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by mbjorge@chromium.org
, Feb 24 2017