Fix cancellation of a pair of URLRequestJob subclasses

net/url_request/url_request_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_job.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 358 matching lines @@
     return;
 
   request_->NotifyBeforeNetworkStart(defer);
 }
 
 void URLRequestJob::NotifyHeadersComplete() {
   if (!request_ || !request_->has_delegate())
     return;  // The request was destroyed, so there is no more work to do.
 
   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
   // set by URLRequest before it calls our Start method.
   request_->response_info_.response_time = base::Time::Now();
   GetResponseInfo(&request_->response_info_);
 
   // When notifying the delegate, the delegate can release the request
   // (and thus release 'this').  After calling to the delgate, we must
   // check the request pointer to see if it still exists, and return
@@ skipping 362 matching lines @@
 
 const URLRequestStatus URLRequestJob::GetStatus() {
   if (request_)
     return request_->status();
   // If the request is gone, we must be cancelled.
   return URLRequestStatus(URLRequestStatus::CANCELED,
                           ERR_ABORTED);
 }
 
 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) {
   request_->proxy_server_ = proxy_server;
 }
 
 bool URLRequestJob::ReadRawDataForFilter(int* bytes_read) {
   bool rv = false;
 
   DCHECK(bytes_read);
@@ skipping 123 matching lines @@
   // Alter the referrer if redirecting cross-origin (especially HTTP->HTTPS).
   redirect_info.new_referrer =
       ComputeReferrerForRedirect(request_->referrer_policy(),
                                  request_->referrer(),
                                  redirect_info.new_url).spec();
 
   return redirect_info;
 }
 
 }  // namespace net