Prevent URLRequestRedirectJob from doing async execution

net/url_request/url_request_redirect_job.cc

Index: net/url_request/url_request_redirect_job.cc
diff --git a/net/url_request/url_request_redirect_job.cc b/net/url_request/url_request_redirect_job.cc
index b127220552e2975126e72b366cba95f49d42d33b..407473f4ae2699cbc59e675562f4c89a050dc282 100644
--- a/net/url_request/url_request_redirect_job.cc
+++ b/net/url_request/url_request_redirect_job.cc
@@ -81,6 +81,10 @@ int URLRequestRedirectJob::GetResponseCode() const {
 URLRequestRedirectJob::~URLRequestRedirectJob() {}
 
 void URLRequestRedirectJob::StartAsync() {
+  // By this moment request might be already canceled and this job killed
+  if (!request_ || !request_->status().is_success())
+    return;

[mmenke, 2015/07/13 21:14:17]

Also, we should have a unit test for this.  I don't think this should be too
bad, but please feel free to ask me for help, our network testing infrastructure
is a bit ugly, and you shouldn't waste your time learning it unless you're going
to be doing more work on the network code (Which, to be clear, is certainly
always welcome, just not sure that's what you'll be spending your time on in the
future).

My suggestion to to take a look at the test InterceptPost302RedirectGet in
url_request_unittest.cc, and base your test on that.

Just remove the stuff we don't need (No need to be a post, etc), cancel the
request just after starting it, and check that the request fails at the end with
whatever error we passed it.

[mmenke, 2015/07/13 21:14:17]

Instead of this, I think would be a little more consistent with other
implementations is to add an override method URLRequestRedirectJob::Kill() which
calls  weak_factory_.InvalidateWeakPtrs() (And then URLRequestJob::Kill()).

[alexanderk, 2015/07/16 09:48:56]

Well, it's kind of complicated to develop this unit test, at least I still
cannot reproduce the same crash. Meanwhile I found another crash with:

TEST_F(URLRequestTestHTTP, RedirectCancelTests) {
  TestDelegate d;
  {
    //GURL url = GURL("http://www.paypal.com/");
    GURL url = GURL("http://www.facebook.com/");
    scoped_ptr<URLRequest> r(
        default_context_.CreateRequest(url, DEFAULT_PRIORITY, &d));
    r->set_method("GET");
    r->Start();
    EXPECT_TRUE(r->is_pending());
    r->Cancel();
    base::RunLoop().Run();
    EXPECT_EQ(URLRequestStatus::CANCELED, r->status().status());
    EXPECT_EQ(ERR_ABORTED, r->status().error());
  }
}

[mmenke, 2015/07/16 15:26:14]

Thanks for catching this!  This is a bug in the DCHECK that's triggering.  It's
just the logging that's wrong, so not a "real" crasher in release build, but
definitely should be fixed.  I try and get a fix out today.

May need to get that fix in before you can reasonably test your fix, not sure.

[mmenke, 2015/07/16 17:58:32]

Turns out this is actually a symptom of the bug you're fixing. 
URLRequestJob::NotifyHeadersComplete is being called when the request status is
already cancelled, which only happens because of the bug.

+
   receive_headers_end_ = base::TimeTicks::Now();
   response_time_ = base::Time::Now();