Add client for background testing of HTTP pipelining.

Add client for background testing of HTTP pipelining.

Note this isn't hooked up, so it won't affect users yet. Once the servers are up, it'll be
hooked up to a field test, much like the UDP echo test. Also missing is a force-pipelining option for requests. I'll add that in a separate CL.

BUG=110794
TEST=unit_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121572

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121766

[mmenke, 2012-02-01 19:08:06]

One unfortunate thing about our network stack is that we can't make it probable
that we'll send both requests in a single packet, but I don't see any way around
that.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
File chrome/browser/net/http_pipelining_compatibility_client.cc (right):

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.cc:66: :
request_id_(request_id),
nit:  4-space indent.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.cc:77:
net::LOAD_DO_NOT_SEND_AUTH_DATA);
Think you might want to add net::DO_NOT_PROMPT_FOR_LOGIN.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.cc:141:
Finished(TOO_SMALL);
Is there a meaningful difference between when we get this and when we get
CONTENT_MISMATCH?  May want to check if the response is a subset of what we
expect.

Also, what if the response is too long?  I don't think this will happen, but the
Internet can be a scary place when you're facing non-compliant HTTP proxies,
armed with nothing but an RFC.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.cc:155: if
(status.status() == net::URLRequestStatus::FAILED) {
It seems a little weird to me that we're completely ignoring |result| here.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
File chrome/browser/net/http_pipelining_compatibility_client.h (right):

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
nit:  All these files should be updated to 2012.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.h:91: void
Finished(Status result);
nit:  Suggest you call this RequestFinished or OnRequestFinished, to indicate
that each call to it doesn't exactly correspond to a call to Start.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.h:93: int request_id_;
nit:  this can be const.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.h:102: friend class
Request;
nit:  Don't think this is needed.  An inner classes should have access to its
outer class's private members.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
(right):

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:55: if
(sample.TotalCount() > 0) {
If you removed this check, you could also remove the ContainsKey check below,
making things a little simpler.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:72:
//client.Start("http://127.0.0.1:7777/",
Remove this.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:117:
histograms_["NetConnectivity.Pipeline.0.Status"];
Not a big fan of repeating these strings everywhere.  Seems like too much chance
for a typo.

A couple ways to do this.  You add a "GeHistogramValue" function that mimics
GetMetricName from the client, and EXPECT its description is one of the 3
allowed.  Or you skip the expect, and use constants instead, either defined in
this file or the client file.

Or could just make all 9 of them consts.

Or ditch the maps and go with arrays and enums.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:372: } //
anonymous namespace
nit:  2 spaces before comment.

http://codereview.chromium.org/9302024/diff/1/chrome/test/base/run_all_unitte...
File chrome/test/base/run_all_unittests.cc (right):

http://codereview.chromium.org/9302024/diff/1/chrome/test/base/run_all_unitte...
chrome/test/base/run_all_unittests.cc:10: base::StatisticsRecorder recorder;
You're going to need an OWNER of chrome/test/ to review this.  I suspect that
chrome/test/base/chrome_test_suite.h would actually be the place to put this.

http://codereview.chromium.org/9302024/diff/1/net/tools/testserver/testserver.py
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/9302024/diff/1/net/tools/testserver/testserver...
net/tools/testserver/testserver.py:301: print '%s %s %s' % (self.path, args,
kwargs)
You only added this for your own debugging, right?

http://codereview.chromium.org/9302024/diff/1/net/tools/testserver/testserver...
net/tools/testserver/testserver.py:931: print 'file_path %s' % file_path
This was just for your own debugging, right?

[James Simonsen, 2012-02-07 00:01:37]

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
File chrome/browser/net/http_pipelining_compatibility_client.cc (right):

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.cc:66: :
request_id_(request_id),
On 2012/02/01 19:08:07, Matt Menke wrote:
> nit:  4-space indent.

Done.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.cc:77:
net::LOAD_DO_NOT_SEND_AUTH_DATA);
On 2012/02/01 19:08:07, Matt Menke wrote:
> Think you might want to add net::DO_NOT_PROMPT_FOR_LOGIN.

Done.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.cc:141:
Finished(TOO_SMALL);
On 2012/02/01 19:08:07, Matt Menke wrote:
> Is there a meaningful difference between when we get this and when we get
> CONTENT_MISMATCH?  May want to check if the response is a subset of what we
> expect.

In practice, it probably doesn't matter. I just thought it'd be interesting to
see if we were getting truncated as opposed to some other failure. The subset
idea is good, so I added that.

> Also, what if the response is too long?  I don't think this will happen, but
the
> Internet can be a scary place when you're facing non-compliant HTTP proxies,
> armed with nothing but an RFC.

There's a Finished(TOO_LARGE) earlier that'll bail on reading if it gets more
bytes than it's expecting. We won't crash.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.cc:155: if
(status.status() == net::URLRequestStatus::FAILED) {
On 2012/02/01 19:08:07, Matt Menke wrote:
> It seems a little weird to me that we're completely ignoring |result| here.

I figured the network errors trump all others. For instance on a suddenly closed
connection while reading the body, I'd rather see NETWORK_ERROR than TOO_SMALL.

I think the distinction for me is whether we can detect problems with real
content. We can detect anything that falls under NETWORK_ERROR, but if content
is missing even though all the headers are correct (TOO_SMALL), then pipelining
will happily return broken content to WebKit and we'll break pages.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
File chrome/browser/net/http_pipelining_compatibility_client.h (right):

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2012/02/01 19:08:07, Matt Menke wrote:
> nit:  All these files should be updated to 2012.

Done.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.h:91: void
Finished(Status result);
On 2012/02/01 19:08:07, Matt Menke wrote:
> nit:  Suggest you call this RequestFinished or OnRequestFinished, to indicate
> that each call to it doesn't exactly correspond to a call to Start.

Done.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.h:93: int request_id_;
On 2012/02/01 19:08:07, Matt Menke wrote:
> nit:  this can be const.

Done.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.h:102: friend class
Request;
On 2012/02/01 19:08:07, Matt Menke wrote:
> nit:  Don't think this is needed.  An inner classes should have access to its
> outer class's private members.

Done.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
(right):

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:55: if
(sample.TotalCount() > 0) {
On 2012/02/01 19:08:07, Matt Menke wrote:
> If you removed this check, you could also remove the ContainsKey check below,
> making things a little simpler.

It CHECK fails. Basically, before we've executed any tests, it doesn't know the
size of the ENUM histograms and there's a CHECK that those match during
Subtract(). It's probably not worth taking that out for this (hopefully)
short-lived test.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:72:
//client.Start("http://127.0.0.1:7777/",
On 2012/02/01 19:08:07, Matt Menke wrote:
> Remove this.

Done.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:117:
histograms_["NetConnectivity.Pipeline.0.Status"];
On 2012/02/01 19:08:07, Matt Menke wrote:
> Not a big fan of repeating these strings everywhere.  Seems like too much
chance
> for a typo.
> 
> A couple ways to do this.  You add a "GeHistogramValue" function that mimics
> GetMetricName from the client, and EXPECT its description is one of the 3
> allowed.  Or you skip the expect, and use constants instead, either defined in
> this file or the client file.
> 
> Or could just make all 9 of them consts.
> 
> Or ditch the maps and go with arrays and enums.

Done.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:372: } //
anonymous namespace
On 2012/02/01 19:08:07, Matt Menke wrote:
> nit:  2 spaces before comment.

Done.

http://codereview.chromium.org/9302024/diff/1/chrome/test/base/run_all_unitte...
File chrome/test/base/run_all_unittests.cc (right):

http://codereview.chromium.org/9302024/diff/1/chrome/test/base/run_all_unitte...
chrome/test/base/run_all_unittests.cc:10: base::StatisticsRecorder recorder;
On 2012/02/01 19:08:07, Matt Menke wrote:
> You're going to need an OWNER of chrome/test/ to review this.  I suspect that
> chrome/test/base/chrome_test_suite.h would actually be the place to put this. 


Done.

http://codereview.chromium.org/9302024/diff/1/net/tools/testserver/testserver.py
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/9302024/diff/1/net/tools/testserver/testserver...
net/tools/testserver/testserver.py:301: print '%s %s %s' % (self.path, args,
kwargs)
On 2012/02/01 19:08:07, Matt Menke wrote:
> You only added this for your own debugging, right?

Oops. Yeah.

http://codereview.chromium.org/9302024/diff/1/net/tools/testserver/testserver...
net/tools/testserver/testserver.py:931: print 'file_path %s' % file_path
On 2012/02/01 19:08:07, Matt Menke wrote:
> This was just for your own debugging, right?

Done.

[mmenke, 2012-02-07 15:17:39]

What happens if a proxy sends us HTTP/1.1 responses instead of HTTP/1.0?

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
File chrome/browser/net/http_pipelining_compatibility_client.cc (right):

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.cc:155: if
(status.status() == net::URLRequestStatus::FAILED) {
On 2012/02/07 00:01:37, James Simonsen wrote:
> On 2012/02/01 19:08:07, Matt Menke wrote:
> > It seems a little weird to me that we're completely ignoring |result| here.
> 
> I figured the network errors trump all others. For instance on a suddenly
closed
> connection while reading the body, I'd rather see NETWORK_ERROR than
TOO_SMALL.
> 
> I think the distinction for me is whether we can detect problems with real
> content. We can detect anything that falls under NETWORK_ERROR, but if content
> is missing even though all the headers are correct (TOO_SMALL), then
pipelining
> will happily return broken content to WebKit and we'll break pages.

Suggest you at least put a small comment here about that.

http://codereview.chromium.org/9302024/diff/10001/chrome/browser/net/http_pip...
File chrome/browser/net/http_pipelining_compatibility_client.cc (right):

http://codereview.chromium.org/9302024/diff/10001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client.cc:151:
Finished(TOO_SMALL);
Think it would be a good idea to document these, either inline or where they're
declared in the header.

[mmenke, 2012-02-07 16:24:38]

On 2012/02/07 15:17:39, Matt Menke wrote:
> What happens if a proxy sends us HTTP/1.1 responses instead of HTTP/1.0?

Err... 1.0 instead of 1.1, rather

[James Simonsen, 2012-02-07 22:40:40]

Good idea. I added the HTTP/1.0 check too.

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
File chrome/browser/net/http_pipelining_compatibility_client.cc (right):

http://codereview.chromium.org/9302024/diff/1/chrome/browser/net/http_pipelin...
chrome/browser/net/http_pipelining_compatibility_client.cc:155: if
(status.status() == net::URLRequestStatus::FAILED) {
On 2012/02/07 15:17:39, Matt Menke wrote:
> On 2012/02/07 00:01:37, James Simonsen wrote:
> > On 2012/02/01 19:08:07, Matt Menke wrote:
> > > It seems a little weird to me that we're completely ignoring |result|
here.
> > 
> > I figured the network errors trump all others. For instance on a suddenly
> closed
> > connection while reading the body, I'd rather see NETWORK_ERROR than
> TOO_SMALL.
> > 
> > I think the distinction for me is whether we can detect problems with real
> > content. We can detect anything that falls under NETWORK_ERROR, but if
content
> > is missing even though all the headers are correct (TOO_SMALL), then
> pipelining
> > will happily return broken content to WebKit and we'll break pages.
> 
> Suggest you at least put a small comment here about that.

Done.

http://codereview.chromium.org/9302024/diff/10001/chrome/browser/net/http_pip...
File chrome/browser/net/http_pipelining_compatibility_client.cc (right):

http://codereview.chromium.org/9302024/diff/10001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client.cc:151:
Finished(TOO_SMALL);
On 2012/02/07 15:17:39, Matt Menke wrote:
> Think it would be a good idea to document these, either inline or where
they're
> declared in the header.

Done.

[James Simonsen, 2012-02-07 22:41:08]

[+Paweł]

Paweł, you were the most recent OWNER to touch chrome_test_suite.h. Can you
review that?

[mmenke, 2012-02-08 15:50:50]

LGTM

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
File chrome/browser/net/http_pipelining_compatibility_client.h (right):

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client.h:19: // Class for
performing a background test of users' internet connections.
nit:  Internet.  Doesn't really matter, but more we stick to the Google strings
guide in comments, the more likely we are to do the same in user-visible
strings.

[willchan no longer on Chromium, 2012-02-08 15:58:19]

I'm curious how the pipelining will get forced. I look forward to that later
changelist. Just skimmed the changelist and had a few nits, nothing blocking.

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
(right):

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:14: #include
"chrome/browser/net/http_pipelining_compatibility_client.h"
should go first

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:27: static
const char* kHistogramNames[] = {
static const char* const

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:119:
private:
it shouldn't go protected=>private=>protected=>private. you should move the data
members up to the appropriate sections and ditch the extra protected/private
sections.

[mmenke1, 2012-02-08 16:07:46]

And another quick nit

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
(right):

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:54: virtual
void SetUp() {
nit:  These should be OVERRIDE.

[James Simonsen, 2012-02-10 01:28:47]

I had to tweak the test again to make that StatisticsRecorder work. There are
some Mac tests that create their own StatisticsRecorders while running. So, we
can't have one global one registered for all tests. This is probably better
anyway, because now you won't see other tests' statistics.

Now, we construct our own StatisticsRecorder for each test. Unfortunately, the
histogram macros cache the Histogram objects. So, even though the
StatisticsRecorder is reset between runs, we're still writing to the old
Histograms. That's okay, because those are never deleted. But, we now need to
cache them during the first run too.

Someone should really make the Histograms easier to use in unit tests. Maybe
I'll do that in another CL. Until then, this seems to work. And I no longer need
to change chrome/test/base.

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
File chrome/browser/net/http_pipelining_compatibility_client.h (right):

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client.h:19: // Class for
performing a background test of users' internet connections.
On 2012/02/08 15:50:51, Matt Menke wrote:
> nit:  Internet.  Doesn't really matter, but more we stick to the Google
strings
> guide in comments, the more likely we are to do the same in user-visible
> strings.

Done.

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
(right):

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:14: #include
"chrome/browser/net/http_pipelining_compatibility_client.h"
On 2012/02/08 15:58:19, willchan wrote:
> should go first

Done.

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:27: static
const char* kHistogramNames[] = {
On 2012/02/08 15:58:19, willchan wrote:
> static const char* const

Done.

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:54: virtual
void SetUp() {
On 2012/02/08 16:07:47, mmenke wrote:
> nit:  These should be OVERRIDE.

Done.

http://codereview.chromium.org/9302024/diff/19001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:119:
private:
On 2012/02/08 15:58:19, willchan wrote:
> it shouldn't go protected=>private=>protected=>private. you should move the
data
> members up to the appropriate sections and ditch the extra protected/private
> sections.

Done.

[commit-bot: I haz the power, 2012-02-10 22:13:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/9302024/26001

[commit-bot: I haz the power, 2012-02-11 00:02:45]

Change committed as 121572

[szym, 2012-02-11 02:01:17]

While I'm not sure what caused the leak http://crbug.com/113769 , I've already
been staring at this for a while, so...

http://codereview.chromium.org/9302024/diff/26001/chrome/browser/net/http_pip...
File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
(right):

http://codereview.chromium.org/9302024/diff/26001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:72:
message_loop_.RunAllPending();
Is |message_loop_| the same loop that ReleaseSoon posts to?
Shouldn't this rather be: 
content::BrowserThread::GetMessageLoopProxyForThread(content::BrowserThread::IO)->RunAllPending();

[mmenke, 2012-02-11 02:06:31]

http://codereview.chromium.org/9302024/diff/26001/chrome/browser/net/http_pip...
File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
(right):

http://codereview.chromium.org/9302024/diff/26001/chrome/browser/net/http_pip...
chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:72:
message_loop_.RunAllPending();
On 2012/02/11 02:01:17, szym wrote:
> Is |message_loop_| the same loop that ReleaseSoon posts to?
> Shouldn't this rather be: 
>
content::BrowserThread::GetMessageLoopProxyForThread(content::BrowserThread::IO)->RunAllPending();

I was thinking the issue was we didn't have a:

BrowserThreadImpl io_thread_(BrowserThread::IO, &message_loop_);

Though I'm not terribly familiar with just what that really does.

[James Simonsen, 2012-02-11 02:48:57]

On 2012/02/11 02:06:31, Matt Menke wrote:
>
http://codereview.chromium.org/9302024/diff/26001/chrome/browser/net/http_pip...
> File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
> (right):
> 
>
http://codereview.chromium.org/9302024/diff/26001/chrome/browser/net/http_pip...
> chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:72:
> message_loop_.RunAllPending();
> On 2012/02/11 02:01:17, szym wrote:
> > Is |message_loop_| the same loop that ReleaseSoon posts to?
> > Shouldn't this rather be: 
> >
>
content::BrowserThread::GetMessageLoopProxyForThread(content::BrowserThread::IO)->RunAllPending();
> 
> I was thinking the issue was we didn't have a:
> 
> BrowserThreadImpl io_thread_(BrowserThread::IO, &message_loop_);
> 
> Though I'm not terribly familiar with just what that really does.

Thanks to both of you for looking into it. Adding the thread fixed it.

I've uploaded a new patch. I'll land it next week if it's okay with you.

[mmenke, 2012-02-11 02:58:53]

On 2012/02/11 02:48:57, James Simonsen wrote:
> On 2012/02/11 02:06:31, Matt Menke wrote:
> >
>
http://codereview.chromium.org/9302024/diff/26001/chrome/browser/net/http_pip...
> > File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
> > (right):
> > 
> >
>
http://codereview.chromium.org/9302024/diff/26001/chrome/browser/net/http_pip...
> > chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:72:
> > message_loop_.RunAllPending();
> > On 2012/02/11 02:01:17, szym wrote:
> > > Is |message_loop_| the same loop that ReleaseSoon posts to?
> > > Shouldn't this rather be: 
> > >
> >
>
content::BrowserThread::GetMessageLoopProxyForThread(content::BrowserThread::IO)->RunAllPending();
> > 
> > I was thinking the issue was we didn't have a:
> > 
> > BrowserThreadImpl io_thread_(BrowserThread::IO, &message_loop_);
> > 
> > Though I'm not terribly familiar with just what that really does.
> 
> Thanks to both of you for looking into it. Adding the thread fixed it.
> 
> I've uploaded a new patch. I'll land it next week if it's okay with you.

Sure.  Also, could you store the pointer in a scoped_refptr, and then just reset
it (And still call message_loop_.RunAllPending(); afterwards), instead of
manually calling AddRef?  My understanding is that the reset will post the
deletion even to the IOThread's message loop in that case.  If that works, think
that makes it a little clearer we have an owning reference, though if it doesn't
work, or if you prefer to leave it as-is, I can live with it.

[James Simonsen, 2012-02-13 22:17:47]

On 2012/02/11 02:58:53, Matt Menke wrote:
> On 2012/02/11 02:48:57, James Simonsen wrote:
> > On 2012/02/11 02:06:31, Matt Menke wrote:
> > >
> >
>
http://codereview.chromium.org/9302024/diff/26001/chrome/browser/net/http_pip...
> > > File chrome/browser/net/http_pipelining_compatibility_client_unittest.cc
> > > (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9302024/diff/26001/chrome/browser/net/http_pip...
> > > chrome/browser/net/http_pipelining_compatibility_client_unittest.cc:72:
> > > message_loop_.RunAllPending();
> > > On 2012/02/11 02:01:17, szym wrote:
> > > > Is |message_loop_| the same loop that ReleaseSoon posts to?
> > > > Shouldn't this rather be: 
> > > >
> > >
> >
>
content::BrowserThread::GetMessageLoopProxyForThread(content::BrowserThread::IO)->RunAllPending();
> > > 
> > > I was thinking the issue was we didn't have a:
> > > 
> > > BrowserThreadImpl io_thread_(BrowserThread::IO, &message_loop_);
> > > 
> > > Though I'm not terribly familiar with just what that really does.
> > 
> > Thanks to both of you for looking into it. Adding the thread fixed it.
> > 
> > I've uploaded a new patch. I'll land it next week if it's okay with you.
> 
> Sure.  Also, could you store the pointer in a scoped_refptr, and then just
reset
> it (And still call message_loop_.RunAllPending(); afterwards), instead of
> manually calling AddRef?  My understanding is that the reset will post the
> deletion even to the IOThread's message loop in that case.  If that works,
think
> that makes it a little clearer we have an owning reference, though if it
doesn't
> work, or if you prefer to leave it as-is, I can live with it.

Didn't work. I'll send the current version to the commit queue, feel free to
stop it if you want to try something else.

[commit-bot: I haz the power, 2012-02-13 22:18:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/9302024/35002

[commit-bot: I haz the power, 2012-02-13 23:56:17]

Change committed as 121766