Add a browsertest for bug 75604

Add a browsertest for bug 75604

This browser test tickles the webkit bug described in crbug.com/75604 .
The bug is very hard to repliably reproduce in a layout test, so my plan is
to land this browser_test, update webkit to fix the bug, and update this
test as we garden past the fix.

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

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

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

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

[gavinp, 2011-10-27 03:23:55]

jam, please review this test, and help me figure out if I've put it in the right
place.

japhet: double check that I'm testing the right condition?

[Paweł Hajdan Jr., 2011-10-27 08:52:57]

Drive-by.

http://codereview.chromium.org/8404001/diff/7001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8404001/diff/7001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:222:
'../content/browser/net/url_request_abort_on_end_job.h',
Let's move those to test_support_content. Bonus points if you can also move the
below lines.

http://codereview.chromium.org/8404001/diff/7001/content/browser/net/url_requ...
File content/browser/net/url_request_abort_on_end_job.h (right):

http://codereview.chromium.org/8404001/diff/7001/content/browser/net/url_requ...
content/browser/net/url_request_abort_on_end_job.h:20: virtual void Start();
nit: OVERRIDE on those please (base/compiler_specific).

http://codereview.chromium.org/8404001/diff/7001/content/browser/net/url_requ...
content/browser/net/url_request_abort_on_end_job.h:40: };
And DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/8404001/diff/7001/content/browser/webkit_brows...
File content/browser/webkit_browsertest.cc (right):

http://codereview.chromium.org/8404001/diff/7001/content/browser/webkit_brows...
content/browser/webkit_browsertest.cc:29: // TODO(gavinp): after bug 75604 is
fixed in WebKit, update expectations
I think it'd be safer strategy to omit this check for now and when the bug is
fixed just insert EXPECT_FALSE. Up to you.

[gavinp, 2011-10-27 12:52:53]

As always, thanks for the review Paweł!

http://codereview.chromium.org/8404001/diff/7001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8404001/diff/7001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:222:
'../content/browser/net/url_request_abort_on_end_job.h',
On 2011/10/27 08:52:58, Paweł Hajdan Jr. wrote:
> Let's move those to test_support_content. Bonus points if you can also move
the
> below lines.

Done.

I'll move those other lines in a separate CL, and add you as a reviewer.

http://codereview.chromium.org/8404001/diff/7001/content/browser/net/url_requ...
File content/browser/net/url_request_abort_on_end_job.h (right):

http://codereview.chromium.org/8404001/diff/7001/content/browser/net/url_requ...
content/browser/net/url_request_abort_on_end_job.h:20: virtual void Start();
On 2011/10/27 08:52:58, Paweł Hajdan Jr. wrote:
> nit: OVERRIDE on those please (base/compiler_specific).

Done.

http://codereview.chromium.org/8404001/diff/7001/content/browser/net/url_requ...
content/browser/net/url_request_abort_on_end_job.h:40: };
On 2011/10/27 08:52:58, Paweł Hajdan Jr. wrote:
> And DISALLOW_COPY_AND_ASSIGN.

Done.

http://codereview.chromium.org/8404001/diff/7001/content/browser/webkit_brows...
File content/browser/webkit_browsertest.cc (right):

http://codereview.chromium.org/8404001/diff/7001/content/browser/webkit_brows...
content/browser/webkit_browsertest.cc:29: // TODO(gavinp): after bug 75604 is
fixed in WebKit, update expectations
On 2011/10/27 08:52:58, Paweł Hajdan Jr. wrote:
> I think it'd be safer strategy to omit this check for now and when the bug is
> fixed just insert EXPECT_FALSE. Up to you.

I'd really like to see this test pass as it is on all platforms; this bug has
been notoriously hard to reproduce.  But you're right: I will, after this check
has rolled through, remove it before landing the WebKit change, then add it
inverted after, in order to make gardening that much easier.

[gavinp, 2011-10-27 13:02:52]

Just publishing my second upload.

[gavinp, 2011-10-27 13:04:49]

And now publishing my third upload, wherein we make test data available for your
perusal.

[Paweł Hajdan Jr., 2011-10-27 15:11:26]

Code I commented in the drive-by LGTM, thanks.

[gavinp, 2011-10-28 15:02:58]

jam: *ping*

[jam, 2011-10-28 16:24:11]

this lgtm although i'm not that familiar with this code. you'll need to get an
owners lgtm from someone in content/browser/net/OWNERS as well

apologies for not seeing this earlier

[gavinp, 2011-10-28 16:31:50]

On 2011/10/28 16:24:11, John Abd-El-Malek wrote:
> this lgtm although i'm not that familiar with this code. you'll need to get an
> owners lgtm from someone in content/browser/net/OWNERS as well
> 
> apologies for not seeing this earlier

+mmenke

[mmenke, 2011-10-28 17:29:52]

LGTM, modulo minor comments.

http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_requ...
File content/browser/net/url_request_abort_on_end_job.cc (right):

http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_requ...
content/browser/net/url_request_abort_on_end_job.cc:86:
&URLRequestAbortOnEndJob::StartAsync));
You should use base::Bind here instead.  The simplest way to do this is replace
|method_factory_| with a weak pointer factory, and then use base::bind(method
pointer, weak_factory_.GetWeakPtr()) here.

http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_requ...
File content/browser/net/url_request_abort_on_end_job.h (right):

http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_requ...
content/browser/net/url_request_abort_on_end_job.h:17: class
URLRequestAbortOnEndJob : public net::URLRequestJob {
Think it would be nice to have some short description up here.

[gavinp, 2011-10-28 18:25:53]

Thanks mmenke.

I've uploaded my remediation to your review, and I will land this first thing
monday morning.

http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_requ...
File content/browser/net/url_request_abort_on_end_job.cc (right):

http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_requ...
content/browser/net/url_request_abort_on_end_job.cc:86:
&URLRequestAbortOnEndJob::StartAsync));
On 2011/10/28 17:29:53, Matt Menke wrote:
> You should use base::Bind here instead.  The simplest way to do this is
replace
> |method_factory_| with a weak pointer factory, and then use base::bind(method
> pointer, weak_factory_.GetWeakPtr()) here.

Done.

http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_requ...
File content/browser/net/url_request_abort_on_end_job.h (right):

http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_requ...
content/browser/net/url_request_abort_on_end_job.h:17: class
URLRequestAbortOnEndJob : public net::URLRequestJob {
On 2011/10/28 17:29:53, Matt Menke wrote:
> Think it would be nice to have some short description up here.

Done.

[mmenke, 2011-10-28 18:32:09]

http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_requ...
File content/browser/net/url_request_abort_on_end_job.h (right):

http://codereview.chromium.org/8404001/diff/8002/content/browser/net/url_requ...
content/browser/net/url_request_abort_on_end_job.h:17: class
URLRequestAbortOnEndJob : public net::URLRequestJob {
On 2011/10/28 18:25:53, gavinp wrote:
> On 2011/10/28 17:29:53, Matt Menke wrote:
> > Think it would be nice to have some short description up here.
> 
> Done.

Doesn't seem to be in the last uploaded CL, but I'll take your word for it (Or
does this comment imply otherwise?  I'm not quite sure).

http://codereview.chromium.org/8404001/diff/3013/content/browser/net/url_requ...
File content/browser/net/url_request_abort_on_end_job.h (right):

http://codereview.chromium.org/8404001/diff/3013/content/browser/net/url_requ...
content/browser/net/url_request_abort_on_end_job.h:43:
base::WeakPtrFactory<URLRequestAbortOnEndJob> weak_factory_;
nit:  Probably a good idea to include src/base/memory/weak_ptr.h for this.

[gavinp, 2011-10-31 14:34:02]

my comment didn't make it into the upload.  Now it is in this upload.  I'm
landing this now.

http://codereview.chromium.org/8404001/diff/3013/content/browser/net/url_requ...
File content/browser/net/url_request_abort_on_end_job.h (right):

http://codereview.chromium.org/8404001/diff/3013/content/browser/net/url_requ...
content/browser/net/url_request_abort_on_end_job.h:43:
base::WeakPtrFactory<URLRequestAbortOnEndJob> weak_factory_;
On 2011/10/28 18:32:10, Matt Menke wrote:
> nit:  Probably a good idea to include src/base/memory/weak_ptr.h for this.

Done.

[gavinp, 2011-10-31 15:24:41]

Mac fun, I reverted this.  Crashed, but didn't register in the is_crashed()
test.  I'll fix and reland.

[gavinp, 2011-11-07 14:34:26]

And here's what I'm going to land later today.  The upstream webkit issue is now
landed, so this test can land with an expectation of NO crash.

I've run try jobs with the webkit fixed patched out, and this test caught the
regression, so that's good.

[commit-bot: I haz the power, 2011-11-07 15:55:48]

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

[commit-bot: I haz the power, 2011-11-07 17:04:00]

Change committed as 108878

[mmenke, 2011-11-09 02:09:26]

http://codereview.chromium.org/8404001/diff/26001/content/browser/net/url_req...
File content/browser/net/url_request_abort_on_end_job.cc (right):

http://codereview.chromium.org/8404001/diff/26001/content/browser/net/url_req...
content/browser/net/url_request_abort_on_end_job.cc:93: *bytes_read =
std::max(size_t(max_bytes), sizeof(kPageContent));
This should be min, methinks.