Add some tests for ThrottlingResourceHandler.

content/browser/loader/throttling_resource_handler.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 "content/browser/loader/throttling_resource_handler.h"
 
 #include <utility>
 
 #include "content/browser/loader/resource_controller.h"
-#include "content/browser/loader/resource_request_info_impl.h"
 #include "content/public/common/resource_response.h"
 #include "net/url_request/url_request.h"
 
 namespace content {
 
 ThrottlingResourceHandler::ThrottlingResourceHandler(
     std::unique_ptr<ResourceHandler> next_handler,
     net::URLRequest* request,
     ScopedVector<ResourceThrottle> throttles)
     : LayeredResourceHandler(request, std::move(next_handler)),
@@ skipping 95 matching lines @@
   cancelled_by_resource_throttle_ = true;
   controller()->CancelAndIgnore();
 }
 
 void ThrottlingResourceHandler::CancelWithError(int error_code) {
   cancelled_by_resource_throttle_ = true;
   controller()->CancelWithError(error_code);
 }
 
 void ThrottlingResourceHandler::Resume() {
-  DCHECK(!cancelled_by_resource_throttle_);
+  // Throttles expect to be able to cancel requests out-of-band, so just do
+  // nothing if one request resumes after another cancels. Can't even recognize
+  // out-of-band cancels and for synchronous teardown, since don't know if the
+  // currently active throttle called Cancel() or if it was another one.

[Randy Smith (Not in Mondays), 2016/12/20 22:16:41]

I'd really like to understand this use case better--as per my comment on the
other CL, the NavigationResourceThrottle code looks to me as if it only cancels
asynchronously, rather than OOB.  Can you give me a pointer to a case where this
happens that I can reproduce and poke at in a debugger?  (The fact that this
used to be a DCHECK() and is now a conditional suggests to me it's not easy to
do.)

[mmenke, 2017/01/04 22:02:29]

I was wrong about NavigationResourceThrottle (It's the NavigationResourceHandler
that does it out of band, so isn't a problem here).  *However* it looks like
SafeBrowsingResourceThrottle does out-of-band checks on Android (When
database_manager_->ChecksAreAlwaysAsync() is true).

When the result comes it, it cancels the request if it's a prefetch, and shows a
blocking page otherwise.  So that's a pretty rare case.  However,
ResourceThrottle cancels aren't too common, so I could well see other ones that
do that.  While, longer term, I'm fine with banning the practice, if we do that,
we should crash when the condition is violated.  DCHECKs on rare races just
aren't good enough (Note that to trigger, the DCHECK would have needed the
SafeBrowsingThrottle to non-blockingly asynchonously check a prefetch resource
in OnWillStartRequest, the request to advance to
WillProcessResponse/WillRedirect, then either the SafeBrowsingThrottle or
another ResourceThrottle cancel the request, then either while posting the
teardown task, or after the RedirectToFileHandler deferred destruction, another
resource throttle to cancel the request.

So yea...It's not common.  Even if other throttles do this more often than the
SafeBrowsing one, it's still an unlikely condition to hit, but as-is, code's not
correct.

You suggested I planned to add OutOfBandCancel to ResourceThrottle:  I'm
actually not sure if we should do that.  It should be a brain-dead API that
external consumers can use.  I think it may be better to just have the
ThrottlingResourceHandler handle OOB cancels (Possibly by just waiting for them
to be in-band cancels, or be asynchronously doing in-band cancels).  I'd also be
fine with forbitting it, as long as we turned it into a crash.

[mmenke, 2017/01/05 18:50:00]

Sorry, I was wrong.

For the problematic case, have to have the SafeBrowsingThrottle cancel out of
band and then another ResourceController resume.  Another problematic case is if
one throttle cancels out of band and then we get a OnResponseStarted call.  The
reason I didn't fix that is we won't get a crash (At least in this RT) in that
case when we switch to the new API.  In the OOB-cancel and then resume case, we
will get one, if we were deferred during the OOB cancel.

[Randy Smith (Not in Mondays), 2017/01/11 19:57:15]

Oh, what a mess.  Ok.

* For purposes of this CL, the current plan (just allow it on the normal Cancel
interface) is ok.

* Long term, my inclination is to forbid it and make things that need to do out
of band cancel take some other path (e.g. record the information they need to
tell the RDHI to cancel the request).  This makes the systems that need to
support the corner case take solid responsibility for it.

* Your argument about CHECK vs. DCHECK makes sense to me.

+  if (cancelled_by_resource_throttle_)
+    return;
 
   DeferredStage last_deferred_stage = deferred_stage_;
   deferred_stage_ = DEFERRED_NONE;
   // Clear information about the throttle that delayed the request.
   request()->LogUnblocked();
   switch (last_deferred_stage) {
     case DEFERRED_NONE:
       NOTREACHED();
       break;
     case DEFERRED_START:
@@ skipping 50 matching lines @@
   } else if (!defer) {
     controller()->Resume();
   }
 }
 
 void ThrottlingResourceHandler::OnRequestDeferred(int throttle_index) {
   request()->LogBlockedBy(throttles_[throttle_index]->GetNameForLogging());
 }
 
 }  // namespace content