Fix webkit URLRequestJob subtypes to handle Kill() correctly.

webkit/appcache/appcache_url_request_job.cc

 // Copyright (c) 2010 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 <vector>
 
 #include "webkit/appcache/appcache_url_request_job.h"
 
+#include "base/compiler_specific.h"
 #include "base/message_loop.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_util.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_status.h"
 
 namespace appcache {
 
 AppCacheURLRequestJob::AppCacheURLRequestJob(
     net::URLRequest* request, AppCacheStorage* storage)
     : URLRequestJob(request), storage_(storage),
       has_been_started_(false), has_been_killed_(false),
       delivery_type_(AWAITING_DELIVERY_ORDERS),
       cache_id_(kNoCacheId), is_fallback_(false),
       cache_entry_not_found_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(read_callback_(
-          this, &AppCacheURLRequestJob::OnReadComplete)) {
+          this, &AppCacheURLRequestJob::OnReadComplete)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
   DCHECK(storage_);
 }
 
 AppCacheURLRequestJob::~AppCacheURLRequestJob() {
   if (storage_)
     storage_->CancelDelegateCallbacks(this);
 }
 
 void AppCacheURLRequestJob::DeliverAppCachedResponse(
     const GURL& manifest_url, int64 cache_id, const AppCacheEntry& entry,
@@ skipping 19 matching lines @@
   DCHECK(!has_delivery_orders());
   delivery_type_ = ERROR_DELIVERY;
   storage_ = NULL;  // not needed
   MaybeBeginDelivery();
 }
 
 void AppCacheURLRequestJob::MaybeBeginDelivery() {
   if (has_been_started() && has_delivery_orders()) {
     // Start asynchronously so that all error reporting and data
     // callbacks happen as they would for network requests.
-    MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(
-        this, &AppCacheURLRequestJob::BeginDelivery));
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        method_factory_.NewRunnableMethod(
+            &AppCacheURLRequestJob::BeginDelivery));
   }
 }
 
 void AppCacheURLRequestJob::BeginDelivery() {
   DCHECK(has_delivery_orders() && has_been_started());
 
   if (has_been_killed())

[michaeln, 2010/12/06 22:51:38]

I think the changes to this job subclass aren't needed. The other classes look
good, but the mods to this class could be removed from the CL. Even with the
method_factory and the call to revokeAll(), we still need this test for
has_been_killed(), so the changes give us two different ways of doing the same
thing (halting progress after Kill). I'd vote to leave it as is with just one
way of not progressing after kill has been called. WDYT?

[willchan no longer on Chromium, 2010/12/07 00:52:38]

Hm, I see.  That's unfortunate that we still need has_been_killed().  But I'd
still like to keep my change if you don't mind (do you think it'll confuse the
code much?).  The reason is because I'm trying to stop refcounting
URLRequestJob, and NewRunnableMethod() is calling AddRef() and Release() on
|this|.  The method_factory prevents that.

PS: In the future, I may try to refactor the code to eliminate the necessity for
has_been_killed(), since I want to simply delete the object instead of calling
Kill() on it.  I will need to read this object more carefully in the future
since it appears appcache code calls Kill() itself.

     return;
 
   switch (delivery_type_) {
     case NETWORK_DELIVERY:
       // To fallthru to the network, we restart the request which will
       // cause a new job to be created to retrieve the resource from the
       // network. Our caller is responsible for arranging to not re-intercept
       // the same request.
       NotifyRestartRequired();
       break;
@@ skipping 114 matching lines @@
 
 void AppCacheURLRequestJob::Kill() {
   if (!has_been_killed_) {
     has_been_killed_ = true;
     reader_.reset();
     if (storage_) {
       storage_->CancelDelegateCallbacks(this);
       storage_ = NULL;
     }
     URLRequestJob::Kill();
+    method_factory_.RevokeAll();
   }
 }
 
 net::LoadState AppCacheURLRequestJob::GetLoadState() const {
   if (!has_been_started())
     return net::LOAD_STATE_IDLE;
   if (!has_delivery_orders())
     return net::LOAD_STATE_WAITING_FOR_CACHE;
   if (delivery_type_ != APPCACHED_DELIVERY)
     return net::LOAD_STATE_IDLE;
@@ skipping 48 matching lines @@
     return;
   }
 
   // If multiple ranges are requested, we play dumb and
   // return the entire response with 200 OK.
   if (ranges.size() == 1U)
     range_requested_ = ranges[0];
 }
 
 }  // namespace appcache
-