Fixed a few data races on reference counters.

Fixed a few data races on reference counters.
BUG=18488
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26476

[brettw, 2009-09-17 18:16:18]

LGTM

[Timur Iskhodzhanov, 2009-09-17 18:21:46]

On 2009/09/17 18:16:18, brettw wrote:
> LGTM
Do I need more LGTMs?

Waiting for the tree to open...

[cpu_(ooo_6.6-7.5), 2009-09-17 18:22:37]

LGTM

On 2009/09/17 18:21:46, Timur Iskhodzhanov wrote:
> On 2009/09/17 18:16:18, brettw wrote:
> > LGTM
> Do I need more LGTMs?
> 
> Waiting for the tree to open...

[eroman, 2009-09-17 18:57:30]

The refcountedthreadsafe for HostResolver is unfortunately necessary because
although it is supposed to live on the IO thread, we construct the
URLRequestContext dependencies on the UI thread :(

However I don't understand why you have made LoadLog refcountedthreadsafe.

Otherwise LGTM.

[laforge, 2009-09-17 19:05:31]

Please ping me as soon as this gets committed, I'd like to pull it on to what
will be this week's Dev channel build.

On 2009/09/17 18:57:30, eroman wrote:
> The refcountedthreadsafe for HostResolver is unfortunately necessary because
> although it is supposed to live on the IO thread, we construct the
> URLRequestContext dependencies on the UI thread :(
> 
> However I don't understand why you have made LoadLog refcountedthreadsafe.
> 
> Otherwise LGTM.

[Timur Iskhodzhanov, 2009-09-17 19:08:44]

On 2009/09/17 18:57:30, eroman wrote:
> However I don't understand why you have made LoadLog refcountedthreadsafe.
I don't remember the place exactly, but I saw the same instance of LoadLog used
by two threads recently.
If this is performance critical I can cancel the LoadLog change and (once I get
the log again) create a separate issue.

[eroman, 2009-09-17 19:14:41]

> If this is performance critical I can cancel the LoadLog change and (once I
get
> the log again) create a separate issue.

I am not worried about the performance impact, but rather the fact that it was
accessed from multiple threads.

If it is being used across threads then there is a bigger problem.

[Timur Iskhodzhanov, 2009-09-17 19:18:50]

On 2009/09/17 19:14:41, eroman wrote:
> > If this is performance critical I can cancel the LoadLog change and (once I
> get
> > the log again) create a separate issue.
> 
> I am not worried about the performance impact, but rather the fact that it was
> accessed from multiple threads.
> 
> If it is being used across threads then there is a bigger problem.
I'll un-do the change to LoadLog and try to reproduce the report, ok?

[eroman, 2009-09-17 19:20:49]

Sounds good, thanks!

[laforge, 2009-09-17 20:32:26]

For the record, I merged these changes on to the 211 branch that I'm kicking off
right now.  I had no more time to wait patiently for the tree to re-open (8
hours now...yeesh).

On 2009/09/17 19:20:49, eroman wrote:
> Sounds good, thanks!

[Timur Iskhodzhanov, 2009-09-17 20:47:08]

On 2009/09/17 20:32:26, laforge wrote:
> For the record, I merged these changes on to the 211 branch that I'm kicking
off
> right now.  I had no more time to wait patiently for the tree to re-open (8
> hours now...yeesh).
The changelist has been submitted into trunk a couple of minutes later

[Aaron Boodman, 2009-09-17 22:59:34]

http://codereview.chromium.org/215011/diff/7005/4006
File chrome/browser/extensions/user_script_master.h (right):

http://codereview.chromium.org/215011/diff/7005/4006#newcode25
Line 25: class UserScriptMaster : public
base::RefCountedThreadSafe<UserScriptMaster>,
Does this class actually get called on multiple threads? It seems like it is
only called on the UI thread.

http://codereview.chromium.org/215011/diff/7005/4006#newcode72
Line 72: : public base::RefCounted<UserScriptMaster::ScriptReloader> {
It seems like this guy needs to be base::RefCountedThreadSafe.

[Timur Iskhodzhanov, 2009-09-18 18:40:06]

On 2009/09/17 19:20:49, eroman wrote:
> Sounds good, thanks!
I've reproduced the report on LoadLog
http://build.chromium.org/buildbot/waterfall.fyi/builders/Modules%20Linux%20(...
It's a bit damaged due to inlining.
However, you may notice net/base/host_resolver_impl.cc:152 out there.
On that line there is a few Request objects deleted and it contains LoadLog.
ThreadSanitizer saw acceses to this reference counter from at least two threads.

I'll re-run the test locally and will have complete (not-inlined) stacktraces of
both accesses soon.

[eroman, 2009-09-18 19:00:32]

Ah okay. I think I understand the problem now, thanks!

(If a request is cancelled, the HostResolverImpl::Job will be freed on the
worker thread, so in turn the HostResolverImpl::Request gets freed on the worker
thread, and calls LoadLog::Release()).

I would like to solve this by clearing the LoadLog within
HostResolverImpl::CancelRequest(), rather than making it RefCountedThreadSafe.
Can file this bug against me?