Prevent URLRequestRedirectJob from doing async execution

net/url_request/url_request_redirect_job.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/url_request/url_request_redirect_job.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
@@ skipping 63 matching lines @@
 int URLRequestRedirectJob::GetResponseCode() const {
   // Should only be called after the URLRequest has been notified there's header
   // information.
   DCHECK(fake_headers_.get());
   return response_code_;
 }
 
 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();
 
   std::string header_string =
       base::StringPrintf("HTTP/1.1 %i Internal Redirect\n"
                              "Location: %s\n"
                              "Non-Authoritative-Reason: %s",
                          response_code_,
                          redirect_destination_.spec().c_str(),
                          redirect_reason_.c_str());
@@ skipping 25 matching lines @@
           base::Unretained(fake_headers_.get())));
 
   // TODO(mmenke):  Consider calling the NetworkDelegate with the headers here.
   // There's some weirdness about how to handle the case in which the delegate
   // tries to modify the redirect location, in terms of how IsSafeRedirect
   // should behave, and whether the fragment should be copied.
   URLRequestJob::NotifyHeadersComplete();
 }
 
 }  // namespace net