Fix cancellation of a pair of URLRequestJob subclasses

net/url_request/url_request_job.cc

Index: net/url_request/url_request_job.cc
diff --git a/net/url_request/url_request_job.cc b/net/url_request/url_request_job.cc
index 14c5c8f00adb08a0eb5d8bf2909d6187c6c7f18a..8d0b23a01f3a20e48a7fb33cc65198f57923952d 100644
--- a/net/url_request/url_request_job.cc
+++ b/net/url_request/url_request_job.cc
@@ -378,7 +378,9 @@ void URLRequestJob::NotifyHeadersComplete() {
   if (has_handled_response_)
     return;

[mmenke, 2015/08/03 21:01:20]

These three checks seem aimed at papering over buggy subclasses that don't
respect Kill().  That goes for pretty much all of the !request_ checks in this
file, actually.

Thinking we may want to turn them into CHECKs, see what triggers them, and see
if we can fix the code, rather than have these all over the place.  Or we could
make those checks more hacky, but more robust - either detach the request on
cancellation (Which seems likely to break consumers), or add in cancellation
checks whever we check request is not NULL, which also seems pretty bad.

[davidben, 2015/08/05 18:56:39]

Clearly we should unrefcount URLRequestJob. :-) That won't take forever.
(Actually, perhaps we should queue that for the next fixit?)

(I don't have enough of this class swapped in to be able to usefully comment
CHECKs and cancels and stuff.)

[mmenke, 2015/08/05 20:00:24]

I agree with removing refcounting, but that wouldn't help with a lot of the
issues here - we keep the URLRequestJob on cancel.  We'd have to change that,
too, to fix a lot of these.

 
-  DCHECK(!request_->status().is_io_pending());
+  // This should not be called on error, and the job type should have cleared
+  // IO_PENDING state before calling this method.
+  DCHECK(request_->status().is_success());
 
   // Initialize to the current time, and let the subclass optionally override
   // the time stamps if it has that information.  The default request_time is
@@ -761,8 +763,12 @@ const URLRequestStatus URLRequestJob::GetStatus() {
 }
 
 void URLRequestJob::SetStatus(const URLRequestStatus &status) {
-  if (request_)
+  if (request_) {
+    // A URLRequestJob should not replace a failed or cancelled status.
+    DCHECK(request_->status().is_success() ||
+           request_->status().is_io_pending());

[mmenke, 2015/08/03 21:01:20]

Want to change the added DCHECKs to CHECKs (Particularly this one) in a followup
CL, and see what we turn up.  Would, of course, revert that change at some point
before it hits stable.

[davidben, 2015/08/05 18:56:39]

SGTM.

     request_->set_status(status);
+  }
 }
 
 void URLRequestJob::SetProxyServer(const HostPortPair& proxy_server) {