When cross-site navigations are cancelled, delete the request being transferred

When cross-site navigations are cancelled, delete the
ResourceLoader that would have been trasferred to the
new renderer. These were being leaked, and would keep
a lock on disk cache entries, making a page impossible
to visit after a cancelled cross site navigation to it.

BUG=341134
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251561

[mmenke, 2014-02-13 14:33:43]

creis:  Please review everything but the prerendering change.
davidben:  Please review the prerendering change.

Gone through a bunch of iterations of this fix.  I think keeping a pointer to
the CrossSiteResourceHandler is the best approach - if we want to handle cases
where the render process dies, or otherwise never starts using the request, we
can switch to using a weak pointer, and having the new render_frame_host take
ownership of the request transfer.  Then it can try to delete it at some point,
without releasing it - figuring out at just what point may take some thought.

[davidben, 2014-02-13 21:08:46]

chrome/browser/prerender LGTM.

Also adding creis@chromium.org instead of creis@google.com. :-)

https://codereview.chromium.org/143183009/diff/640001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/143183009/diff/640001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:3638:
content::SetBrowserClientForTesting(original_browser_client);
Is there any particular reason to revert to the original ContentBrowserClient
before navigating? I assume it doesn't matter much either way, but it seems a
little strange.

[mmenke, 2014-02-13 21:13:56]

https://codereview.chromium.org/143183009/diff/640001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/143183009/diff/640001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:3638:
content::SetBrowserClientForTesting(original_browser_client);
On 2014/02/13 21:08:47, David Benjamin wrote:
> Is there any particular reason to revert to the original ContentBrowserClient
> before navigating? I assume it doesn't matter much either way, but it seems a
> little strange.

I did that because I wanted to test a "normal" navigation, and a navigation with
a weird browser client isn't very normal.  The content browser test I added
*also* use it to do a weird navigation, of course.

I did go back and forth on this, though, and I agree it's also weird.  I've gone
ahead and switched.

[Charlie Reis, 2014-02-13 22:12:18]

Thanks a ton for working out a solution!  I see that this handles the case that
the WebContents is deleted (case 1 from comment 18 of http://crbug.com/341134). 
What about the other cases, like OpenURLFromTab returning null?

Please include the bug number in the description.  Also, there's some newline
errors that lint caught.

A few questions below, since I'd like to avoid storing an IO object on the UI
thread if at all possible.

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1224: // either the old
renderer is about to to reclaim it, or a new one is going
nit: to to

I'm not sure I follow this.  Do you mean cancel or abort by "reclaim"?  Can you
specify where the new renderer claims ownership?  (I think you mean
RDH::UpdateRequestForTransfer?)

Also, is there a reason this code is here?  It's currently in the middle of the
decision of whether to swap processes, but that doesn't seem to matter for it. 
If you want it to happen on any navigation that passes through RFHM, maybe
RFHM::Navigate is a better spot?

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:285: // and thus
|request_transfer|, will cancel the request being transferred,
nit: request_transfer doesn't match the name of the parameter.

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:302: // If a pending
request needs to be transfferred to another process, this
nit: transferred

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:304: // cleaned up if teh
navigation is cancelled.  Otherwise, this is NULL.
nit: the

https://codereview.chromium.org/143183009/diff/640001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:34: bool
leak_requests_for_testing_ = false;
This seems unfortunate, but I don't have a better idea off the top of my head.

https://codereview.chromium.org/143183009/diff/640001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:281: // Cancelling the
request is not strictly necessary, since nothing is
Is it not done by the CancelRequest call below?  (Or is it skipped there because
it's considered a transfer?)

I'm wondering if there's an alternative that avoids passing the CSRH to the UI
thread.  For example, add a ResourceLoader::CancelTransfer, which then lets us
call RDH::CancelRequest with the child ID and request ID to clean it up.  Would
be nice to stick to passing IDs between threads rather than references to
objects.

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
File content/browser/renderer_host/cross_site_request_transfer.h (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
content/browser/renderer_host/cross_site_request_transfer.h:6: #define
CONTENT_BROWSER_RENDERER_HOST_CROSS_SITE_REQUEST_TRANSFER_H_
I think this would belong in frame_host rather than renderer_host if we decide
to keep it.  All navigation stuff is ending up there.

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
content/browser/renderer_host/cross_site_request_transfer.h:19: class
CONTENT_EXPORT CrossSiteRequestTransfer {
CrossSiteRequest or CrossSiteTransferringRequest, perhaps?  The Request is the
main noun, not the Transfer.

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
content/browser/renderer_host/cross_site_request_transfer.h:33:
CrossSiteResourceHandler* cross_site_resource_handler_;
I'm not yet convinced this is safe, because CrossSiteResourceHandler is only
supposed to be accessed from the IO thread.  Couldn't it be deleted on the IO
thread when canceled in some way?

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:97: //
|request_transfer| is NULL if a request is not being transferred between
nit: request_transfer doesn't match parameter name

https://codereview.chromium.org/143183009/diff/640001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:170: class
TrackingResourceDispatcherHostDelegate
Thanks for figuring out how to test this.

I'm getting the feeling that we're vastly overloading this test class, since it
was designed to test things specific to --site-per-process, while these transfer
tests matter today (as demonstrated by these bugs).  We can split them out to a
new test class in a separate CL if that's too much work for now, but I don't
think they should stay in SitePerProcessBrowserTest.

https://codereview.chromium.org/143183009/diff/640001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:864: // Don't want for the
navigation to complete, since that never happens in
nit: wait

https://codereview.chromium.org/143183009/diff/640001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/143183009/diff/640001/content/content_browser...
content/content_browser.gypi:825:
'browser/renderer_host/cross_site_request_transfer.h',
nit: cc files come before h files

[mmenke, 2014-02-14 16:30:01]

On 2014/02/13 22:12:18, Charlie Reis wrote:
> Thanks a ton for working out a solution!  I see that this handles the case
that
> the WebContents is deleted (case 1 from comment 18 of
http://crbug.com/341134). 
> What about the other cases, like OpenURLFromTab returning null?

Both of the new tests actually only test the case where OpenURLFromTab returns
NULL, it's actually the deletion case (And the others) that I don't test.  I'm
not quite sure how to do the others.  I basically have to either stop the
navigation or delete the WebContents while the unload handler of the first site
is running, or after the CrossSiteResourceHandler has started passing the
request to the UI thread but before it's been passed on to the RVH, and I'm not
sure how to do either cleanly.

M34 branch looms, and given the complexity of this CL, I'd really like to get it
in before then.  Happy to spend today on adding another quick test, but I'm not
either lends itself to one.

> Please include the bug number in the description.  Also, there's some newline
> errors that lint caught.

Added the bug number.  Lint's newline errors always seem to be bogus.

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1224: // either the old
renderer is about to to reclaim it, or a new one is going
On 2014/02/13 22:12:19, Charlie Reis wrote:
> nit: to to
> 
> I'm not sure I follow this.  Do you mean cancel or abort by "reclaim"?  Can
you
> specify where the new renderer claims ownership?  (I think you mean
> RDH::UpdateRequestForTransfer?)

I mean the current_instance can take ownership - I assume this happens in the
case where we think we have a cross-process navigation, but it turns out we
don't, so the first renderer takes back ownership of the request.  I've
simplified the comment.

> Also, is there a reason this code is here?  It's currently in the middle of
the
> decision of whether to swap processes, but that doesn't seem to matter for it.

> If you want it to happen on any navigation that passes through RFHM, maybe
> RFHM::Navigate is a better spot?

You're right...  I had thought this should only run on cross-site navigations,
and since we had special logic for that case here, I put it here.  But then I
found out we sometimes transfer the request to current_instance.  By moving it,
we also now handle the case where CreateRenderFrame fails.

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:285: // and thus
|request_transfer|, will cancel the request being transferred,
On 2014/02/13 22:12:19, Charlie Reis wrote:
> nit: request_transfer doesn't match the name of the parameter.

Done.  I've noticed my number of typos varies directly with how long I've spend
revising a CL.  :(

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:302: // If a pending
request needs to be transfferred to another process, this
On 2014/02/13 22:12:19, Charlie Reis wrote:
> nit: transferred

Done.

https://codereview.chromium.org/143183009/diff/640001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:304: // cleaned up if teh
navigation is cancelled.  Otherwise, this is NULL.
On 2014/02/13 22:12:19, Charlie Reis wrote:
> nit: the

Done.

https://codereview.chromium.org/143183009/diff/640001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:34: bool
leak_requests_for_testing_ = false;
On 2014/02/13 22:12:19, Charlie Reis wrote:
> This seems unfortunate, but I don't have a better idea off the top of my head.

I agree.  I couldn't think of something better to do, either.  We could avoid
making it a global, but it's a bit of a pain to pass it around, and it's still
ugly.

https://codereview.chromium.org/143183009/diff/640001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:281: // Cancelling the
request is not strictly necessary, since nothing is
On 2014/02/13 22:12:19, Charlie Reis wrote:
> Is it not done by the CancelRequest call below?  (Or is it skipped there
because
> it's considered a transfer?)

No, it's not.  Despite its name, ResourceDispatcherHostImpl::CancelRequest
deletes a request without cancelling it.  In the test, the throttle, on
deletion, checks the status of the request.  If we just delete the request,
while not waiting for a callback (Like when it's paused after reading the
headers), we get a status indicating success, which makes for bizarre and
regression prone test.

It looks like I can check "is_pending" instead, so I've removed the real cancel.

> I'm wondering if there's an alternative that avoids passing the CSRH to the UI
> thread.  For example, add a ResourceLoader::CancelTransfer, which then lets us
> call RDH::CancelRequest with the child ID and request ID to clean it up. 
Would
> be nice to stick to passing IDs between threads rather than references to
> objects.

I initially just kept the IDs, and used RDH::RemovePendingRequest.

I was thinking that this design would make it easier to handle cleanup in the
deletion case - just have the RenderFrameHost keep ownership of the cross site
request transfer, until some point at which is must have been claimed or is no
longer needed.  Then hop over to the UI thread, and if it exists, see if it was
reclaimed, etc....

That could also be done through the RDH, now that I think about it, if we wanted
to.  I've switched back to going through the RDH.  I've added a new function to
the RDH to specifically cancel a pending transfer, just so we can make sure it's
a transfer (And it exists).  I think that the extra check is important, as a
check there's no case where we accidentally delete a request we shouldn't.

I'm open to other proposals.

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
File content/browser/renderer_host/cross_site_request_transfer.h (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
content/browser/renderer_host/cross_site_request_transfer.h:6: #define
CONTENT_BROWSER_RENDERER_HOST_CROSS_SITE_REQUEST_TRANSFER_H_
On 2014/02/13 22:12:19, Charlie Reis wrote:
> I think this would belong in frame_host rather than renderer_host if we decide
> to keep it.  All navigation stuff is ending up there.

I tried to have it in frame_host.  Unfortunately, renderer_host can't depend on
frame_host files, and we currently pass this through renderer_host files.

We could just have this object exist in frame_host, and pass it around
unprotected in renderer_host, but that makes me very nervous about regressions.

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
content/browser/renderer_host/cross_site_request_transfer.h:19: class
CONTENT_EXPORT CrossSiteRequestTransfer {
On 2014/02/13 22:12:19, Charlie Reis wrote:
> CrossSiteRequest or CrossSiteTransferringRequest, perhaps?  The Request is the
> main noun, not the Transfer.

I considered "CrossSiteRequest", but I didn't like it because the actual request
object is on the IO thread, and this lives for the duration of the transfer,
rather than the duration of the request.

I've switched to CrossSiteTransferringRequest.

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
content/browser/renderer_host/cross_site_request_transfer.h:33:
CrossSiteResourceHandler* cross_site_resource_handler_;
On 2014/02/13 22:12:19, Charlie Reis wrote:
> I'm not yet convinced this is safe, because CrossSiteResourceHandler is only
> supposed to be accessed from the IO thread.  Couldn't it be deleted on the IO
> thread when canceled in some way?

If it can be, then we don't actually own it, and there may be a bug.  I actually
want the crashing behavior, as a way to ensure there aren't any bugs.  In this
case, crashing isn't a bug of the implementation, it's a feature!

We don't delete transferring requests on process shutdown (We have a DCHECK to
make sure of that), and the only case we do delete them, with the exception of
them being claimed, is when the RDH shuts down, which only happens after we've
closed all tabs, which would have already cancelled any request owned by one of
these.

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:97: //
|request_transfer| is NULL if a request is not being transferred between
On 2014/02/13 22:12:19, Charlie Reis wrote:
> nit: request_transfer doesn't match parameter name

Done.

https://codereview.chromium.org/143183009/diff/640001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:170: class
TrackingResourceDispatcherHostDelegate
On 2014/02/13 22:12:19, Charlie Reis wrote:
> Thanks for figuring out how to test this.

This is pretty similar to how the network-related browser tests work, except we
test by injecting URLRequestJobs instead.  It's a bit ugly, but it works.

> I'm getting the feeling that we're vastly overloading this test class, since
it
> was designed to test things specific to --site-per-process, while these
transfer
> tests matter today (as demonstrated by these bugs).  We can split them out to
a
> new test class in a separate CL if that's too much work for now, but I don't
> think they should stay in SitePerProcessBrowserTest.

I think it makes sense to separate them, but I don't think it makes sense to do
it in this CL.  I'm happy to do a followup CL after this lands.

https://codereview.chromium.org/143183009/diff/640001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:864: // Don't want for the
navigation to complete, since that never happens in
On 2014/02/13 22:12:19, Charlie Reis wrote:
> nit: wait

Done.

https://codereview.chromium.org/143183009/diff/640001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/143183009/diff/640001/content/content_browser...
content/content_browser.gypi:825:
'browser/renderer_host/cross_site_request_transfer.h',
On 2014/02/13 22:12:19, Charlie Reis wrote:
> nit: cc files come before h files

Done.

[mmenke, 2014-02-14 17:10:20]

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
File content/browser/renderer_host/cross_site_request_transfer.h (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
content/browser/renderer_host/cross_site_request_transfer.h:6: #define
CONTENT_BROWSER_RENDERER_HOST_CROSS_SITE_REQUEST_TRANSFER_H_
On 2014/02/14 16:30:02, mmenke wrote:
> On 2014/02/13 22:12:19, Charlie Reis wrote:
> > I think this would belong in frame_host rather than renderer_host if we
decide
> > to keep it.  All navigation stuff is ending up there.
> 
> I tried to have it in frame_host.  Unfortunately, renderer_host can't depend
on
> frame_host files, and we currently pass this through renderer_host files.
> 
> We could just have this object exist in frame_host, and pass it around
> unprotected in renderer_host, but that makes me very nervous about
regressions.

"Pass it around" == Pass the request ID around without having anything own it.

[Charlie Reis, 2014-02-15 01:25:44]

Apologies for the delay!  I got hit with a Stable blocker bug that needed to be
fixed today.

I like the changes here.  LGTM once AbortTransfer is gone.

On 2014/02/14 16:30:01, mmenke wrote:
> On 2014/02/13 22:12:18, Charlie Reis wrote:
> > Thanks a ton for working out a solution!  I see that this handles the case
> that
> > the WebContents is deleted (case 1 from comment 18 of
> http://crbug.com/341134). 
> > What about the other cases, like OpenURLFromTab returning null?
> 
> Both of the new tests actually only test the case where OpenURLFromTab returns
> NULL, it's actually the deletion case (And the others) that I don't test.

Oh good, I'm glad they test that.  While a test for the tab deletion would be
nice, that was the easier one for me to reason about (i.e., RFHM's destructor
will take CSTR with it).

>  I'm
> not quite sure how to do the others.  I basically have to either stop the
> navigation or delete the WebContents while the unload handler of the first
site
> is running, or after the CrossSiteResourceHandler has started passing the
> request to the UI thread but before it's been passed on to the RVH, and I'm
not
> sure how to do either cleanly.
> 
> M34 branch looms, and given the complexity of this CL, I'd really like to get
it
> in before then.  Happy to spend today on adding another quick test, but I'm
not
> either lends itself to one.

I'm very sorry if I made you miss the branch.  I think this is ready with the
small update.  Thanks again for working through it.

https://codereview.chromium.org/143183009/diff/640001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.cc (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:281: // Cancelling the
request is not strictly necessary, since nothing is
On 2014/02/14 16:30:02, mmenke wrote:
> On 2014/02/13 22:12:19, Charlie Reis wrote:
> > Is it not done by the CancelRequest call below?  (Or is it skipped there
> because
> > it's considered a transfer?)
> 
> No, it's not.  Despite its name, ResourceDispatcherHostImpl::CancelRequest
> deletes a request without cancelling it.  In the test, the throttle, on
> deletion, checks the status of the request.  If we just delete the request,
> while not waiting for a callback (Like when it's paused after reading the
> headers), we get a status indicating success, which makes for bizarre and
> regression prone test.

Ah.

> It looks like I can check "is_pending" instead, so I've removed the real
cancel.
> 
> > I'm wondering if there's an alternative that avoids passing the CSRH to the
UI
> > thread.  For example, add a ResourceLoader::CancelTransfer, which then lets
us
> > call RDH::CancelRequest with the child ID and request ID to clean it up. 
> Would
> > be nice to stick to passing IDs between threads rather than references to
> > objects.
> 
> I initially just kept the IDs, and used RDH::RemovePendingRequest.
> 
> I was thinking that this design would make it easier to handle cleanup in the
> deletion case - just have the RenderFrameHost keep ownership of the cross site
> request transfer, until some point at which is must have been claimed or is no
> longer needed.  Then hop over to the UI thread, and if it exists, see if it
was
> reclaimed, etc....
> 
> That could also be done through the RDH, now that I think about it, if we
wanted
> to.  I've switched back to going through the RDH.  I've added a new function
to
> the RDH to specifically cancel a pending transfer, just so we can make sure
it's
> a transfer (And it exists).  I think that the extra check is important, as a
> check there's no case where we accidentally delete a request we shouldn't.

Ah, I like that better.  It looks like you've left AbortTransfer in the code
unused, though.  Can you remove it?

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
File content/browser/renderer_host/cross_site_request_transfer.h (right):

https://codereview.chromium.org/143183009/diff/640001/content/browser/rendere...
content/browser/renderer_host/cross_site_request_transfer.h:6: #define
CONTENT_BROWSER_RENDERER_HOST_CROSS_SITE_REQUEST_TRANSFER_H_
On 2014/02/14 17:10:20, mmenke wrote:
> On 2014/02/14 16:30:02, mmenke wrote:
> > On 2014/02/13 22:12:19, Charlie Reis wrote:
> > > I think this would belong in frame_host rather than renderer_host if we
> decide
> > > to keep it.  All navigation stuff is ending up there.
> > 
> > I tried to have it in frame_host.  Unfortunately, renderer_host can't depend
> on
> > frame_host files, and we currently pass this through renderer_host files.
> > 
> > We could just have this object exist in frame_host, and pass it around
> > unprotected in renderer_host, but that makes me very nervous about
> regressions.
> 
> "Pass it around" == Pass the request ID around without having anything own it.

Oh, RVH needs it for now.  Ok, you can leave it here and we'll move it over when
we move the rest of the navigation stuff to RenderFrameHost.  (Nasko has CLs in
progress for this.)

https://codereview.chromium.org/143183009/diff/920001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/143183009/diff/920001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:53: void AbortTransfer();
Let's remove this.

[mmenke, 2014-02-15 02:15:13]

Thanks for speedy review, and all the feedback!  And there's nothing to
apologize for - I can still can make M34 (Branch is Tuesday evening, I believe).

https://codereview.chromium.org/143183009/diff/920001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.h (right):

https://codereview.chromium.org/143183009/diff/920001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.h:53: void AbortTransfer();
On 2014/02/15 01:25:45, Charlie Reis wrote:
> Let's remove this.

Done, thanks for catching this!

[mmenke, 2014-02-15 02:16:37]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-02-15 02:18:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/143183009/1050002

[commit-bot: I haz the power, 2014-02-15 04:17:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-15 04:17:26]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[mmenke, 2014-02-15 04:30:07]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-02-15 04:30:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/143183009/1050002

[commit-bot: I haz the power, 2014-02-15 08:29:56]

Change committed as 251561