Skip generation of stats, since code is now shared across threads

Skip generation of stats, since code is now shared across threads

The stats function was not written to be shared across threads, and
recent use by sync on a second thread is causing it to malfunction
(because it uses function level statics).


r=wtc
bug=74467
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76989

[willchan no longer on Chromium, 2011-03-04 19:15:34]

http://codereview.chromium.org/6624023/diff/1/net/base/connection_type_histog...
File net/base/connection_type_histograms.cc (right):

http://codereview.chromium.org/6624023/diff/1/net/base/connection_type_histog...
net/base/connection_type_histograms.cc:26:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Unfortunately, net/ is not aware of browser stuff. This is a layering violation.

Oh, I see the bug thread now. The sync code is running from a different thread
(the sync thread) and it uses TCPClientSocketLibevent. So yes, I guess this code
here is definitely racy.

[cbentzel, 2011-03-04 19:43:12]

I added code in net to ensure that a histogram was always accessed from the
same thread, without forcing it to be the IO thread.

The implementation was actually your suggestion, jar.

http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/http/http...

On Fri, Mar 4, 2011 at 2:15 PM, <willchan@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6624023/diff/1/net/base/connection_type_histog...
> File net/base/connection_type_histograms.cc (right):
>
>
>
http://codereview.chromium.org/6624023/diff/1/net/base/connection_type_histog...
> net/base/connection_type_histograms.cc:26:
>
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
> Unfortunately, net/ is not aware of browser stuff. This is a layering
> violation.
>
> Oh, I see the bug thread now. The sync code is running from a different
> thread (the sync thread) and it uses TCPClientSocketLibevent. So yes, I
> guess this code here is definitely racy.
>
>
> http://codereview.chromium.org/6624023/
>

[jar (doing other things), 2011-03-04 20:59:51]

The code you pointed at does indeed ensure (at least DCHECK) that the function
is only run on one thread.  That is indeed sufficient to validate and hopefully
preclude the class of problems faced by this CL.

This CL addresses the issue that the code *IS* being run on multiple threads.

On 2011/03/04 19:43:12, cbentzel wrote:
> I added code in net to ensure that a histogram was always accessed from the
> same thread, without forcing it to be the IO thread.
> 
> The implementation was actually your suggestion, jar.
> 
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/http/http...
> 
> On Fri, Mar 4, 2011 at 2:15 PM, <mailto:willchan@chromium.org> wrote:
> 
> >
> >
> >
>
http://codereview.chromium.org/6624023/diff/1/net/base/connection_type_histog...
> > File net/base/connection_type_histograms.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6624023/diff/1/net/base/connection_type_histog...
> > net/base/connection_type_histograms.cc:26:
> >
> > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
> > Unfortunately, net/ is not aware of browser stuff. This is a layering
> > violation.
> >
> > Oh, I see the bug thread now. The sync code is running from a different
> > thread (the sync thread) and it uses TCPClientSocketLibevent. So yes, I
> > guess this code here is definitely racy.
> >
> >
> > http://codereview.chromium.org/6624023/
> >

[wtc, 2011-03-04 21:05:24]

LGTM.

http://codereview.chromium.org/6624023/diff/2002/net/base/connection_type_his...
File net/base/connection_type_histograms.cc (right):

http://codereview.chromium.org/6624023/diff/2002/net/base/connection_type_his...
net/base/connection_type_histograms.cc:24: // TODO(wtc): 74467 Move these stats
up to a higher level, where the explicit
Nit: please add "bug" before "74467".

[commit-bot: I haz the power, 2011-03-04 23:00:04]

Try job failure for 6624023-8001 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...