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.)

It seems like HostResolver in net/base/host_resolver.h has a similar data race

Same as UserScriptMaster in chrome/browser/extensions/user_script_master.h

Same as LoadLog in net/base/load_log.h

Marking started since there's a patch out for review. 

@brettw: BTW, you're listed as the reviewer for that patch :)

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!

This may be related to http://code.google.com/p/chromium/issues/detail?id=15577

What is the status of this bug? It's been open for a month and a half. Is it being 
worked on?

The bugs are still opened.

I'll prepare a changelist now for all 4 races described above.

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

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).

> 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"

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.

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

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.

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

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.

(No comment was entered for this change.)

(No comment was entered for this change.)

Shouldn't this bug be marked fixed no?

I think so. Thanks for mentioning!

(No comment was entered for this change.)

(No comment was entered for this change.)