Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 18488 Possible data race on HistoryAddPageArgs
Starred by 7 users Reported by timur...@gmail.com, Aug 5 2009 Back to list
Status: Fixed
Owner: timurrrr@chromium.org
Closed: Oct 2009
Cc: sh...@chromium.org, cpu@chromium.org, huanr@chromium.org, dank@chromium.org, brettw@chromium.org
Components:
OS: All
Pri: 1
Type: Bug-Regression

Restricted
  • Only users with Commit permission may comment.


Sign in to add a comment
Chrome Version       : r22179
OS + version : Linux, possibly Mac as well

Hi,
I've been running ui_tests under ThreadSanitizer and got the following race
report:

==21334== INFO: T0 is program's main thread
==21334== INFO: T7 has been created by T0 at this point: {{{
==21334==     #0  clone /lib32/libc-2.7.so
==21334==     #1  pthread_create@@GLIBC_2.1 /lib32/libpthread-2.7.so
==21334==     #2  pthread_create@*
/home/timurrrr/valgrind-patches/valgrind-10695/tsan/ts_valgrind_intercepts.c:556
==21334==     #3  (anonymous namespace)::CreateThread(unsigned int, bool,
PlatformThread::Delegate*, unsigned long*) base/platform_thread_posix.cc:93
==21334==     #4  PlatformThread::Create(unsigned int,
PlatformThread::Delegate*, unsigned long*) base/platform_thread_posix.cc:105
==21334==     #5  base::Thread::StartWithOptions(base::Thread::Options
const&) base/thread.cc:84
==21334==     #6  base::Thread::Start() base/thread.cc:73
==21334==     #7  HistoryService::Init(FilePath const&, BookmarkService*)
chrome/browser/history/history.cc:128
==21334==     #8 
ProfileImpl::GetHistoryService(Profile::ServiceAccessType)
chrome/browser/profile.cc:862
==21334==     #9  VisitedLinkMaster::RebuildTableFromHistory()
chrome/browser/visitedlink_master.cc:868
==21334==     #10 VisitedLinkMaster::InitFromScratch(bool)
chrome/browser/visitedlink_master.cc:622
==21334==     #11 VisitedLinkMaster::Init()
chrome/browser/visitedlink_master.cc:265
==21334==     #12 ProfileImpl::GetVisitedLinkMaster()
chrome/browser/profile.cc:721
==21334==     #13 BrowserRenderProcessHost::InitVisitedLinks()
chrome/browser/renderer_host/browser_render_process_host.cc:576
==21334==     #14 BrowserRenderProcessHost::Init()
chrome/browser/renderer_host/browser_render_process_host.cc:492
==21334==     #15 RenderViewHost::CreateRenderView()
chrome/browser/renderer_host/render_view_host.cc:174
==21334== WARNING: Possible data race during read of size 4 at 0xB79C8F0: {{{
==21334==    T0 (locks held: {}):
==21334==     #0  std::vector<GURL, std::allocator<GURL> >::~vector()
/usr/include/c++/4.2/bits/stl_vector.h:268
==21334==     #1  history::HistoryAddPageArgs::~HistoryAddPageArgs()
chrome/browser/history/history_marshaling.h:21
==21334==     #2  base::RefCounted<history::HistoryAddPageArgs>::Release()
base/ref_counted.h:80
==21334==     #3 
scoped_refptr<history::HistoryAddPageArgs>::~scoped_refptr()
base/ref_counted.h:196
==21334==     #4  HistoryService::AddPage(GURL const&, base::Time, void
const*, int, GURL const&, unsigned int, std::vector<GURL,
std::allocator<GURL> > const&, bool) chro»
==21334==     #5  HistoryService::AddPage(GURL const&, void const*, int,
GURL const&, unsigned int, std::vector<GURL, std::allocator<GURL> > const&,
bool) chrome/browser/h»
==21334==     #6  TabContents::UpdateHistoryForNavigation(GURL const&,
NavigationController::LoadCommittedDetails const&,
ViewHostMsg_FrameNavigate_Params const&) chrome/b»
==21334==     #7  TabContents::DidNavigate(RenderViewHost*,
ViewHostMsg_FrameNavigate_Params const&)
chrome/browser/tab_contents/tab_contents.cc:1848
==21334==     #8  RenderViewHost::OnMsgNavigate(IPC::Message const&)
chrome/browser/renderer_host/render_view_host.cc:944
==21334==     #9  RenderViewHost::OnMessageReceived(IPC::Message const&)
chrome/browser/renderer_host/render_view_host.cc:723
==21334==   Concurrent write(s) happened at (OR AFTER) these points:
==21334==    T7 (locks held: {}):
==21334==     #0  std::vector<GURL, std::allocator<GURL>
>::erase(__gnu_cxx::__normal_iterator<GURL*, std::vector<GURL,
std::allocator<GURL> > >) /usr/include/c++/4.2/bits»
==21334==     #1 
history::HistoryBackend::AddPage(scoped_refptr<history::HistoryAddPageArgs>) chrome/browser/history/history_backend.cc:404
==21334==     #2  void DispatchToMethod<history::HistoryBackend, void
(history::HistoryBackend::*)(scoped_refptr<history::HistoryAddPageArgs>),
scoped_refptr<history::Hist»
==21334==     #3  RunnableMethod<history::HistoryBackend, void
(history::HistoryBackend::*)(scoped_refptr<history::HistoryAddPageArgs>),
Tuple1<scoped_refptr<history::Hist»
==21334==     #4  MessageLoop::RunTask(Task*) base/message_loop.cc:316
==21334==     #5 
MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&)
base/message_loop.cc:324
==21334==     #6  MessageLoop::DoWork() base/message_loop.cc:435
==21334==     #7 
base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base/message_pump_default.cc:23
==21334==     #8  MessageLoop::RunInternal() base/message_loop.cc:199
==21334==     #9  MessageLoop::RunHandler() base/message_loop.cc:181
==21334== }}}

Please note that HistoryAddPageArgs subclasses base::RefCounted, NOT
base::RefCountedThreadSafe
(chrome/browser/history/history_marshaling.h)
 20 // Marshalling structure for AddPage.
 21 class HistoryAddPageArgs : public base::RefCounted<HistoryAddPageArgs> {
 22  public:

and scoped_ptr<HistoryAddPageArgs> are shared between multiple threads:
(chrome/browser/history/history.cc, executed in Thread #0)
 295 
 296   scoped_refptr<history::HistoryAddPageArgs> request(
 297       new history::HistoryAddPageArgs(url, time, id_scope, page_id,
 298                                       referrer, redirects, transition,
 299                                       did_replace_entry));
 300   ScheduleAndForget(PRIORITY_NORMAL, &HistoryBackend::AddPage, request);
 301 }

(chrome/browser/history/history_backend.cc, executed in Thread #7)
 321 
 322 void HistoryBackend::AddPage(scoped_refptr<HistoryAddPageArgs> request) {
 323   DLOG(INFO) << "Adding page " << request->url.possibly_invalid_spec();
 324 

I believe the fix is simple: just replace base::RefCounted with
base::RefCountedThreadSafe

Thank you,
Timur Iskhodzhanov
 
A cl with your suggested fix is up at http://codereview.chromium.org/160642

Do you know which ui tests in particular tickle this?
(It's nice to include a single test name in bug reports to make reproducing
easier, if possible.  That's why I use tools/valgrind/shard-all-tests.sh
or the like to run the tests one at a time when fishing for valgrind warnings.)

Comment 2 by timur...@gmail.com, Aug 6 2009
It seems like HostResolver in net/base/host_resolver.h has a similar data race
Comment 3 by timur...@gmail.com, Aug 13 2009
Same as UserScriptMaster in chrome/browser/extensions/user_script_master.h
Comment 4 by timur...@gmail.com, Aug 14 2009
Same as LoadLog in net/base/load_log.h
Labels: -Area-Misc Area-BrowserBackend
Status: Started
Marking started since there's a patch out for review. 

@brettw: BTW, you're listed as the reviewer for that patch :)
Comment 6 by brettw@chromium.org, Aug 22 2009
I don't see any patches in my queue or email for this. If you sent it out, please check 
my address and re-send the email. Thanks!
Comment 8 by brettw@chromium.org, Sep 17 2009
What is the status of this bug? It's been open for a month and a half. Is it being 
worked on?
Comment 9 by timur...@gmail.com, Sep 17 2009
The bugs are still opened.

I'll prepare a changelist now for all 4 races described above.
Comment 10 by timur...@gmail.com, Sep 17 2009
http://codereview.chromium.org/215011 reviewers are welcome
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=26473 

------------------------------------------------------------------------
r26473 | laforge@chromium.org | 2009-09-17 13:24:45 -0700 (Thu, 17 Sep 2009) | 3 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/211/src/net/base/host_resolver.h?r1=26473&r2=26472

Fixed a few data races on reference counters.
BUG= 18488 
http://codereview.chromium.org/215011/
------------------------------------------------------------------------

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=26474 

------------------------------------------------------------------------
r26474 | laforge@chromium.org | 2009-09-17 13:25:00 -0700 (Thu, 17 Sep 2009) | 3 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/211/src/chrome/browser/extensions/user_script_master.h?r1=26474&r2=26473
   M http://src.chromium.org/viewvc/chrome/branches/211/src/chrome/browser/history/history_marshaling.h?r1=26474&r2=26473

Fixed a few data races on reference counters.
BUG= 18488 
http://codereview.chromium.org/215011/
------------------------------------------------------------------------

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=26476 

------------------------------------------------------------------------
r26476 | timurrrr@chromium.org | 2009-09-17 13:36:29 -0700 (Thu, 17 Sep 2009) | 3 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/user_script_master.h?r1=26476&r2=26475
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/history/history_marshaling.h?r1=26476&r2=26475
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/base/host_resolver.h?r1=26476&r2=26475

Fixed a few data races on reference counters.
BUG= 18488 
Review URL: http://codereview.chromium.org/215011
------------------------------------------------------------------------

Comment 14 by sh...@chromium.org, Sep 17 2009
I find myself wondering if cases like RefCounted when you should have RefCountedThreadSafe couldn't be 
enforced using template magic.  But after experimenting, I find that my C++ Fu is not nearly up to it.  Is that 
kind of thing possible?
Pawel is looking at asserting that RefCounted is being used from the same thread.
Nice detective work, Timur.
Do you have any theory on how this could cause corruption (such as 
http://code.google.com/p/chromium/issues/detail?id=15577) ?

If both threads collide in accessing the reference object, is there a case whereby a 
double-free can occur? Or a use-after-free?
My reading of the code is that Thread 0 elevates the reference count to 2 (local 
stack frame scoped_refptr and scoped_refptr copy in the new Task* object) before a 
second thread becomes involved.
Admittedly it is hard to reason about because the scoped_refptr is passed around a 
lot to various templates and functions -- sometimes by value (will increment) and 
sometimes by references (will not increment).
Comment 17 by timur...@gmail.com, Sep 18 2009
> Pawel is looking at asserting that RefCounted is being used from the same thread.
Please, NO.
I'm sure there are dozens of classes using RefCounted in multi-threaded environment
properly in Chromium.
For example, by guarding it using a Lock.

Maybe a better idea is to add a couple of "DCHECK(refcount > 0)" and also set
"refcount = -1000" before "delete this"
Comment 18 by bre...@gmail.com, Sep 18 2009
I thought about whether there would be a lot of legitimate uses, and decided it's 
unlikely. In your example, we don't use locks much. It would be interesting to know if 
we do do this a lot.
Comment 19 by timur...@gmail.com, Sep 18 2009
Consider two threads running the same code (counter is a shared variable, initially
set to 0):

void ThreadProc() {
  for (int i = 0; i < 100000; i++)
    counter++;
}

experiments show that counter becomes 19XXXX, not 200000.

Now consider a data race on reference counter.
If two increments or decrements happen simultaneously, the reference counter may
become +1 or -1 to the correct value.
In case of "+1", we are unlikely to achieve zero refcount at all, so we are likely to
have a memory leak.
In case of "-1", reference counter may become zero more than once; and it can also
become -1.

For example, consider the following interleaving (T1, T2 = thread{1,2})
T1|T2| refcount
XXXXX|  0
++|  |  1
++|++|!!2, not 3!!
--|  |  1, not 2
  |--|  0, not 1   -> free is called
++|  |  1, not 2
--|  |  0, not 1   -> free is called the second time
--|  | -1, not 0

Please note that there is an equal number of "++" and "--" in this interleaving
Comment 20 by timur...@gmail.com, Sep 18 2009
the last comment is for scarybeasts :-)
It might still be interesting to get a list of RefCounted instances
which are used from multiple threads.  We could add an optional warning
for that and check the log from a full test suite run.  If the list
isn't horribly huge, maybe it'd be easy to eyeball the callers and
see which need locks or the like.
Comment 22 by timur...@gmail.com, Sep 18 2009
You can get it by running ThreadSanitizer :-)
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=26593 

------------------------------------------------------------------------
r26593 | timurrrr@chromium.org | 2009-09-18 11:09:05 -0700 (Fri, 18 Sep 2009) | 2 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/tools/valgrind/tsan/suppressions.txt?r1=26593&r2=26592

Remove a suppression for bug 18488 since it should be fixed already.
Review URL: http://codereview.chromium.org/215021
------------------------------------------------------------------------

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=26755 

------------------------------------------------------------------------
r26755 | cpu@chromium.org | 2009-09-21 15:39:09 -0700 (Mon, 21 Sep 2009) | 8 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/user_script_master.h?r1=26755&r2=26754

Making UserScriptMaster::ScriptReloader refcounted thread safe
- Being addref() in two different threads.

BUG= 18488 
TEST=none


Review URL: http://codereview.chromium.org/213025
------------------------------------------------------------------------

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=26883 

------------------------------------------------------------------------
r26883 | laforge@chromium.org | 2009-09-22 16:45:59 -0700 (Tue, 22 Sep 2009) | 6 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/195/src/chrome/browser/extensions/user_script_master.h?r1=26883&r2=26882
   M http://src.chromium.org/viewvc/chrome/branches/195/src/chrome/browser/history/history_marshaling.h?r1=26883&r2=26882
   M http://src.chromium.org/viewvc/chrome/branches/195/src/net/base/host_resolver.h?r1=26883&r2=26882

Merge 26476 - Fixed a few data races on reference counters.
BUG= 18488 
Review URL: http://codereview.chromium.org/215011

TBR=timurrrr@chromium.org
Review URL: http://codereview.chromium.org/209073
------------------------------------------------------------------------

Comment 26 by timur...@gmail.com, Sep 23 2009
Please note that some data races on reference counters in LoadLog are still present.
I've created a separate issue for them
http://code.google.com/p/chromium/issues/detail?id=22272
Issue 11492 has been merged into this issue.
Labels: -OS-Linux OS-All Valgrind Crash Pri-1
Comment 29 by huanr@chromium.org, Oct 13 2009
Labels: Valgrind-Tsan
Comment 30 by sky@chromium.org, Oct 13 2009
Shouldn't this bug be marked fixed no?
Status: Fixed
I think so. Thanks for mentioning!
Labels: -Valgrind-Tsan ThreadSanitizer
Labels: -Area-BrowserBackend Area-Internals
Labels: -Valgrind bulkmove Stability-Valgrind
Chrome Version       : r22179
OS + version : Linux, possibly Mac as well

Hi,
I've been running ui_tests under ThreadSanitizer and got the following race
report:

==21334== INFO: T0 is program's main thread
==21334== INFO: T7 has been created by T0 at this point: {{{
==21334==     #0  clone /lib32/libc-2.7.so
==21334==     #1  pthread_create@@GLIBC_2.1 /lib32/libpthread-2.7.so
==21334==     #2  pthread_create@*
/home/timurrrr/valgrind-patches/valgrind-10695/tsan/ts_valgrind_intercepts.c:556
==21334==     #3  (anonymous namespace)::CreateThread(unsigned int, bool,
PlatformThread::Delegate*, unsigned long*) base/platform_thread_posix.cc:93
==21334==     #4  PlatformThread::Create(unsigned int,
PlatformThread::Delegate*, unsigned long*) base/platform_thread_posix.cc:105
==21334==     #5  base::Thread::StartWithOptions(base::Thread::Options
const&amp;) base/thread.cc:84
==21334==     #6  base::Thread::Start() base/thread.cc:73
==21334==     #7  HistoryService::Init(FilePath const&amp;, BookmarkService*)
chrome/browser/history/history.cc:128
==21334==     #8 
ProfileImpl::GetHistoryService(Profile::ServiceAccessType)
chrome/browser/profile.cc:862
==21334==     #9  VisitedLinkMaster::RebuildTableFromHistory()
chrome/browser/visitedlink_master.cc:868
==21334==     #10 VisitedLinkMaster::InitFromScratch(bool)
chrome/browser/visitedlink_master.cc:622
==21334==     #11 VisitedLinkMaster::Init()
chrome/browser/visitedlink_master.cc:265
==21334==     #12 ProfileImpl::GetVisitedLinkMaster()
chrome/browser/profile.cc:721
==21334==     #13 BrowserRenderProcessHost::InitVisitedLinks()
chrome/browser/renderer_host/browser_render_process_host.cc:576
==21334==     #14 BrowserRenderProcessHost::Init()
chrome/browser/renderer_host/browser_render_process_host.cc:492
==21334==     #15 RenderViewHost::CreateRenderView()
chrome/browser/renderer_host/render_view_host.cc:174
==21334== WARNING: Possible data race during read of size 4 at 0xB79C8F0: {{{
==21334==    T0 (locks held: {}):
==21334==     #0  std::vector&lt;GURL, std::allocator&lt;GURL&gt; &gt;::~vector()
/usr/include/c++/4.2/bits/stl_vector.h:268
==21334==     #1  history::HistoryAddPageArgs::~HistoryAddPageArgs()
chrome/browser/history/history_marshaling.h:21
==21334==     #2  base::RefCounted&lt;history::HistoryAddPageArgs&gt;::Release()
base/ref_counted.h:80
==21334==     #3 
scoped_refptr&lt;history::HistoryAddPageArgs&gt;::~scoped_refptr()
base/ref_counted.h:196
==21334==     #4  HistoryService::AddPage(GURL const&amp;, base::Time, void
const*, int, GURL const&amp;, unsigned int, std::vector&lt;GURL,
std::allocator&lt;GURL&gt; &gt; const&amp;, bool) chro»
==21334==     #5  HistoryService::AddPage(GURL const&amp;, void const*, int,
GURL const&amp;, unsigned int, std::vector&lt;GURL, std::allocator&lt;GURL&gt; &gt; const&amp;,
bool) chrome/browser/h»
==21334==     #6  TabContents::UpdateHistoryForNavigation(GURL const&amp;,
NavigationController::LoadCommittedDetails const&amp;,
ViewHostMsg_FrameNavigate_Params const&amp;) chrome/b»
==21334==     #7  TabContents::DidNavigate(RenderViewHost*,
ViewHostMsg_FrameNavigate_Params const&amp;)
chrome/browser/tab_contents/tab_contents.cc:1848
==21334==     #8  RenderViewHost::OnMsgNavigate(IPC::Message const&amp;)
chrome/browser/renderer_host/render_view_host.cc:944
==21334==     #9  RenderViewHost::OnMessageReceived(IPC::Message const&amp;)
chrome/browser/renderer_host/render_view_host.cc:723
==21334==   Concurrent write(s) happened at (OR AFTER) these points:
==21334==    T7 (locks held: {}):
==21334==     #0  std::vector&lt;GURL, std::allocator&lt;GURL&gt;
&gt;::erase(__gnu_cxx::__normal_iterator&lt;GURL*, std::vector&lt;GURL,
std::allocator&lt;GURL&gt; &gt; &gt;) /usr/include/c++/4.2/bits»
==21334==     #1 
history::HistoryBackend::AddPage(scoped_refptr&lt;history::HistoryAddPageArgs&gt;) chrome/browser/history/history_backend.cc:404
==21334==     #2  void DispatchToMethod&lt;history::HistoryBackend, void
(history::HistoryBackend::*)(scoped_refptr&lt;history::HistoryAddPageArgs&gt;),
scoped_refptr&lt;history::Hist»
==21334==     #3  RunnableMethod&lt;history::HistoryBackend, void
(history::HistoryBackend::*)(scoped_refptr&lt;history::HistoryAddPageArgs&gt;),
Tuple1&lt;scoped_refptr&lt;history::Hist»
==21334==     #4  MessageLoop::RunTask(Task*) base/message_loop.cc:316
==21334==     #5 
MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&amp;)
base/message_loop.cc:324
==21334==     #6  MessageLoop::DoWork() base/message_loop.cc:435
==21334==     #7 
base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base/message_pump_default.cc:23
==21334==     #8  MessageLoop::RunInternal() base/message_loop.cc:199
==21334==     #9  MessageLoop::RunHandler() base/message_loop.cc:181
==21334== }}}

Please note that HistoryAddPageArgs subclasses base::RefCounted, NOT
base::RefCountedThreadSafe
(chrome/browser/history/history_marshaling.h)
 20 // Marshalling structure for AddPage.
 21 class HistoryAddPageArgs : public base::RefCounted&lt;HistoryAddPageArgs&gt; {
 22  public:

and scoped_ptr&lt;HistoryAddPageArgs&gt; are shared between multiple threads:
(chrome/browser/history/history.cc, executed in Thread #0)
 295 
 296   scoped_refptr&lt;history::HistoryAddPageArgs&gt; request(
 297       new history::HistoryAddPageArgs(url, time, id_scope, page_id,
 298                                       referrer, redirects, transition,
 299                                       did_replace_entry));
 300   ScheduleAndForget(PRIORITY_NORMAL, &amp;HistoryBackend::AddPage, request);
 301 }

(chrome/browser/history/history_backend.cc, executed in Thread #7)
 321 
 322 void HistoryBackend::AddPage(scoped_refptr&lt;HistoryAddPageArgs&gt; request) {
 323   DLOG(INFO) &lt;&lt; &quot;Adding page &quot; &lt;&lt; request-&gt;url.possibly_invalid_spec();
 324 

I believe the fix is simple: just replace base::RefCounted with
base::RefCountedThreadSafe

Thank you,
Timur Iskhodzhanov
Labels: -ThreadSanitizer Type-Regression
Chrome Version       : r22179
OS + version : Linux, possibly Mac as well

Hi,
I've been running ui_tests under ThreadSanitizer and got the following race
report:

==21334== INFO: T0 is program's main thread
==21334== INFO: T7 has been created by T0 at this point: {{{
==21334==     #0  clone /lib32/libc-2.7.so
==21334==     #1  pthread_create@@GLIBC_2.1 /lib32/libpthread-2.7.so
==21334==     #2  pthread_create@*
/home/timurrrr/valgrind-patches/valgrind-10695/tsan/ts_valgrind_intercepts.c:556
==21334==     #3  (anonymous namespace)::CreateThread(unsigned int, bool,
PlatformThread::Delegate*, unsigned long*) base/platform_thread_posix.cc:93
==21334==     #4  PlatformThread::Create(unsigned int,
PlatformThread::Delegate*, unsigned long*) base/platform_thread_posix.cc:105
==21334==     #5  base::Thread::StartWithOptions(base::Thread::Options
const&amp;) base/thread.cc:84
==21334==     #6  base::Thread::Start() base/thread.cc:73
==21334==     #7  HistoryService::Init(FilePath const&amp;, BookmarkService*)
chrome/browser/history/history.cc:128
==21334==     #8 
ProfileImpl::GetHistoryService(Profile::ServiceAccessType)
chrome/browser/profile.cc:862
==21334==     #9  VisitedLinkMaster::RebuildTableFromHistory()
chrome/browser/visitedlink_master.cc:868
==21334==     #10 VisitedLinkMaster::InitFromScratch(bool)
chrome/browser/visitedlink_master.cc:622
==21334==     #11 VisitedLinkMaster::Init()
chrome/browser/visitedlink_master.cc:265
==21334==     #12 ProfileImpl::GetVisitedLinkMaster()
chrome/browser/profile.cc:721
==21334==     #13 BrowserRenderProcessHost::InitVisitedLinks()
chrome/browser/renderer_host/browser_render_process_host.cc:576
==21334==     #14 BrowserRenderProcessHost::Init()
chrome/browser/renderer_host/browser_render_process_host.cc:492
==21334==     #15 RenderViewHost::CreateRenderView()
chrome/browser/renderer_host/render_view_host.cc:174
==21334== WARNING: Possible data race during read of size 4 at 0xB79C8F0: {{{
==21334==    T0 (locks held: {}):
==21334==     #0  std::vector&lt;GURL, std::allocator&lt;GURL&gt; &gt;::~vector()
/usr/include/c++/4.2/bits/stl_vector.h:268
==21334==     #1  history::HistoryAddPageArgs::~HistoryAddPageArgs()
chrome/browser/history/history_marshaling.h:21
==21334==     #2  base::RefCounted&lt;history::HistoryAddPageArgs&gt;::Release()
base/ref_counted.h:80
==21334==     #3 
scoped_refptr&lt;history::HistoryAddPageArgs&gt;::~scoped_refptr()
base/ref_counted.h:196
==21334==     #4  HistoryService::AddPage(GURL const&amp;, base::Time, void
const*, int, GURL const&amp;, unsigned int, std::vector&lt;GURL,
std::allocator&lt;GURL&gt; &gt; const&amp;, bool) chro»
==21334==     #5  HistoryService::AddPage(GURL const&amp;, void const*, int,
GURL const&amp;, unsigned int, std::vector&lt;GURL, std::allocator&lt;GURL&gt; &gt; const&amp;,
bool) chrome/browser/h»
==21334==     #6  TabContents::UpdateHistoryForNavigation(GURL const&amp;,
NavigationController::LoadCommittedDetails const&amp;,
ViewHostMsg_FrameNavigate_Params const&amp;) chrome/b»
==21334==     #7  TabContents::DidNavigate(RenderViewHost*,
ViewHostMsg_FrameNavigate_Params const&amp;)
chrome/browser/tab_contents/tab_contents.cc:1848
==21334==     #8  RenderViewHost::OnMsgNavigate(IPC::Message const&amp;)
chrome/browser/renderer_host/render_view_host.cc:944
==21334==     #9  RenderViewHost::OnMessageReceived(IPC::Message const&amp;)
chrome/browser/renderer_host/render_view_host.cc:723
==21334==   Concurrent write(s) happened at (OR AFTER) these points:
==21334==    T7 (locks held: {}):
==21334==     #0  std::vector&lt;GURL, std::allocator&lt;GURL&gt;
&gt;::erase(__gnu_cxx::__normal_iterator&lt;GURL*, std::vector&lt;GURL,
std::allocator&lt;GURL&gt; &gt; &gt;) /usr/include/c++/4.2/bits»
==21334==     #1 
history::HistoryBackend::AddPage(scoped_refptr&lt;history::HistoryAddPageArgs&gt;) chrome/browser/history/history_backend.cc:404
==21334==     #2  void DispatchToMethod&lt;history::HistoryBackend, void
(history::HistoryBackend::*)(scoped_refptr&lt;history::HistoryAddPageArgs&gt;),
scoped_refptr&lt;history::Hist»
==21334==     #3  RunnableMethod&lt;history::HistoryBackend, void
(history::HistoryBackend::*)(scoped_refptr&lt;history::HistoryAddPageArgs&gt;),
Tuple1&lt;scoped_refptr&lt;history::Hist»
==21334==     #4  MessageLoop::RunTask(Task*) base/message_loop.cc:316
==21334==     #5 
MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&amp;)
base/message_loop.cc:324
==21334==     #6  MessageLoop::DoWork() base/message_loop.cc:435
==21334==     #7 
base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base/message_pump_default.cc:23
==21334==     #8  MessageLoop::RunInternal() base/message_loop.cc:199
==21334==     #9  MessageLoop::RunHandler() base/message_loop.cc:181
==21334== }}}

Please note that HistoryAddPageArgs subclasses base::RefCounted, NOT
base::RefCountedThreadSafe
(chrome/browser/history/history_marshaling.h)
 20 // Marshalling structure for AddPage.
 21 class HistoryAddPageArgs : public base::RefCounted&lt;HistoryAddPageArgs&gt; {
 22  public:

and scoped_ptr&lt;HistoryAddPageArgs&gt; are shared between multiple threads:
(chrome/browser/history/history.cc, executed in Thread #0)
 295 
 296   scoped_refptr&lt;history::HistoryAddPageArgs&gt; request(
 297       new history::HistoryAddPageArgs(url, time, id_scope, page_id,
 298                                       referrer, redirects, transition,
 299                                       did_replace_entry));
 300   ScheduleAndForget(PRIORITY_NORMAL, &amp;HistoryBackend::AddPage, request);
 301 }

(chrome/browser/history/history_backend.cc, executed in Thread #7)
 321 
 322 void HistoryBackend::AddPage(scoped_refptr&lt;HistoryAddPageArgs&gt; request) {
 323   DLOG(INFO) &lt;&lt; &quot;Adding page &quot; &lt;&lt; request-&gt;url.possibly_invalid_spec();
 324 

I believe the fix is simple: just replace base::RefCounted with
base::RefCountedThreadSafe

Thank you,
Timur Iskhodzhanov
Labels: -Crash Stability-Crash
Chrome Version       : r22179
OS + version : Linux, possibly Mac as well

Hi,
I've been running ui_tests under ThreadSanitizer and got the following race
report:

==21334== INFO: T0 is program's main thread
==21334== INFO: T7 has been created by T0 at this point: {{{
==21334==     #0  clone /lib32/libc-2.7.so
==21334==     #1  pthread_create@@GLIBC_2.1 /lib32/libpthread-2.7.so
==21334==     #2  pthread_create@*
/home/timurrrr/valgrind-patches/valgrind-10695/tsan/ts_valgrind_intercepts.c:556
==21334==     #3  (anonymous namespace)::CreateThread(unsigned int, bool,
PlatformThread::Delegate*, unsigned long*) base/platform_thread_posix.cc:93
==21334==     #4  PlatformThread::Create(unsigned int,
PlatformThread::Delegate*, unsigned long*) base/platform_thread_posix.cc:105
==21334==     #5  base::Thread::StartWithOptions(base::Thread::Options
const&amp;) base/thread.cc:84
==21334==     #6  base::Thread::Start() base/thread.cc:73
==21334==     #7  HistoryService::Init(FilePath const&amp;, BookmarkService*)
chrome/browser/history/history.cc:128
==21334==     #8 
ProfileImpl::GetHistoryService(Profile::ServiceAccessType)
chrome/browser/profile.cc:862
==21334==     #9  VisitedLinkMaster::RebuildTableFromHistory()
chrome/browser/visitedlink_master.cc:868
==21334==     #10 VisitedLinkMaster::InitFromScratch(bool)
chrome/browser/visitedlink_master.cc:622
==21334==     #11 VisitedLinkMaster::Init()
chrome/browser/visitedlink_master.cc:265
==21334==     #12 ProfileImpl::GetVisitedLinkMaster()
chrome/browser/profile.cc:721
==21334==     #13 BrowserRenderProcessHost::InitVisitedLinks()
chrome/browser/renderer_host/browser_render_process_host.cc:576
==21334==     #14 BrowserRenderProcessHost::Init()
chrome/browser/renderer_host/browser_render_process_host.cc:492
==21334==     #15 RenderViewHost::CreateRenderView()
chrome/browser/renderer_host/render_view_host.cc:174
==21334== WARNING: Possible data race during read of size 4 at 0xB79C8F0: {{{
==21334==    T0 (locks held: {}):
==21334==     #0  std::vector&lt;GURL, std::allocator&lt;GURL&gt; &gt;::~vector()
/usr/include/c++/4.2/bits/stl_vector.h:268
==21334==     #1  history::HistoryAddPageArgs::~HistoryAddPageArgs()
chrome/browser/history/history_marshaling.h:21
==21334==     #2  base::RefCounted&lt;history::HistoryAddPageArgs&gt;::Release()
base/ref_counted.h:80
==21334==     #3 
scoped_refptr&lt;history::HistoryAddPageArgs&gt;::~scoped_refptr()
base/ref_counted.h:196
==21334==     #4  HistoryService::AddPage(GURL const&amp;, base::Time, void
const*, int, GURL const&amp;, unsigned int, std::vector&lt;GURL,
std::allocator&lt;GURL&gt; &gt; const&amp;, bool) chro»
==21334==     #5  HistoryService::AddPage(GURL const&amp;, void const*, int,
GURL const&amp;, unsigned int, std::vector&lt;GURL, std::allocator&lt;GURL&gt; &gt; const&amp;,
bool) chrome/browser/h»
==21334==     #6  TabContents::UpdateHistoryForNavigation(GURL const&amp;,
NavigationController::LoadCommittedDetails const&amp;,
ViewHostMsg_FrameNavigate_Params const&amp;) chrome/b»
==21334==     #7  TabContents::DidNavigate(RenderViewHost*,
ViewHostMsg_FrameNavigate_Params const&amp;)
chrome/browser/tab_contents/tab_contents.cc:1848
==21334==     #8  RenderViewHost::OnMsgNavigate(IPC::Message const&amp;)
chrome/browser/renderer_host/render_view_host.cc:944
==21334==     #9  RenderViewHost::OnMessageReceived(IPC::Message const&amp;)
chrome/browser/renderer_host/render_view_host.cc:723
==21334==   Concurrent write(s) happened at (OR AFTER) these points:
==21334==    T7 (locks held: {}):
==21334==     #0  std::vector&lt;GURL, std::allocator&lt;GURL&gt;
&gt;::erase(__gnu_cxx::__normal_iterator&lt;GURL*, std::vector&lt;GURL,
std::allocator&lt;GURL&gt; &gt; &gt;) /usr/include/c++/4.2/bits»
==21334==     #1 
history::HistoryBackend::AddPage(scoped_refptr&lt;history::HistoryAddPageArgs&gt;) chrome/browser/history/history_backend.cc:404
==21334==     #2  void DispatchToMethod&lt;history::HistoryBackend, void
(history::HistoryBackend::*)(scoped_refptr&lt;history::HistoryAddPageArgs&gt;),
scoped_refptr&lt;history::Hist»
==21334==     #3  RunnableMethod&lt;history::HistoryBackend, void
(history::HistoryBackend::*)(scoped_refptr&lt;history::HistoryAddPageArgs&gt;),
Tuple1&lt;scoped_refptr&lt;history::Hist»
==21334==     #4  MessageLoop::RunTask(Task*) base/message_loop.cc:316
==21334==     #5 
MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&amp;)
base/message_loop.cc:324
==21334==     #6  MessageLoop::DoWork() base/message_loop.cc:435
==21334==     #7 
base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base/message_pump_default.cc:23
==21334==     #8  MessageLoop::RunInternal() base/message_loop.cc:199
==21334==     #9  MessageLoop::RunHandler() base/message_loop.cc:181
==21334== }}}

Please note that HistoryAddPageArgs subclasses base::RefCounted, NOT
base::RefCountedThreadSafe
(chrome/browser/history/history_marshaling.h)
 20 // Marshalling structure for AddPage.
 21 class HistoryAddPageArgs : public base::RefCounted&lt;HistoryAddPageArgs&gt; {
 22  public:

and scoped_ptr&lt;HistoryAddPageArgs&gt; are shared between multiple threads:
(chrome/browser/history/history.cc, executed in Thread #0)
 295 
 296   scoped_refptr&lt;history::HistoryAddPageArgs&gt; request(
 297       new history::HistoryAddPageArgs(url, time, id_scope, page_id,
 298                                       referrer, redirects, transition,
 299                                       did_replace_entry));
 300   ScheduleAndForget(PRIORITY_NORMAL, &amp;HistoryBackend::AddPage, request);
 301 }

(chrome/browser/history/history_backend.cc, executed in Thread #7)
 321 
 322 void HistoryBackend::AddPage(scoped_refptr&lt;HistoryAddPageArgs&gt; request) {
 323   DLOG(INFO) &lt;&lt; &quot;Adding page &quot; &lt;&lt; request-&gt;url.possibly_invalid_spec();
 324 

I believe the fix is simple: just replace base::RefCounted with
base::RefCountedThreadSafe

Thank you,
Timur Iskhodzhanov
Project Member Comment 37 by bugdroid1@chromium.org, Oct 13 2012
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member Comment 38 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Area-Internals -Stability-Valgrind -Type-Regression Type-Bug-Regression Performance-Valgrind Cr-Internals
Project Member Comment 39 by bugdroid1@chromium.org, Apr 1 2013
Labels: -Performance-Valgrind Stability-Valgrind
Sign in to add a comment