Reland 232312 (https://codereview.chromium.org/33363003/), with fix.

This was reverted in 232369 (And relanded and reverted again after that).

The problem was that, per spec, the "data" parameter returned to the
callback is valid until destroyed, but
ResourceFetcherTests.ResourceFetcherDeletedInCallback destoyed the fetcher
before reading the data parameter.  The CL also unintentionally introduced
a change where partially received content was returned on timeout.  I've
reverted to the old behavior.

Original CL description:
Cleanup ResourceFetcher in preparation to add to public content API,
in preparation to move error page logic from content to chrome:

* Merge ResourceFetcher and ResourceFetcherWithTimeout.
* Remove unnecessary member variables.
* Remove Cancel() (All consumers were deleting and cancelling at the
same time, anyways, and deleting already cancels).
* Make definition order match declaration order.
* Fix outdated comments.

TBR=darin@chromium.org
BUG=308232
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232312

Review URL: https://codereview.chromium.org/56013003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@235051 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 71 insertions, 109 deletions

diff --git a/content/renderer/fetchers/alt_error_page_resource_fetcher.cc b/content/renderer/fetchers/alt_error_page_resource_fetcher.cc
index 56cb6b8..d9b3d07 100644
--- a/content/renderer/fetchers/alt_error_page_resource_fetcher.cc
+++ b/content/renderer/fetchers/alt_error_page_resource_fetcher.cc
@@ -29,19 +29,16 @@ AltErrorPageResourceFetcher::AltErrorPageResourceFetcher(
       callback_(callback),
       original_request_(original_request),
       original_error_(original_error) {
-  fetcher_.reset(new ResourceFetcherWithTimeout(
-      url, frame, WebURLRequest::TargetIsMainFrame, kDownloadTimeoutSec,
+  fetcher_.reset(new ResourceFetcher(
+      url, frame, WebURLRequest::TargetIsMainFrame,
       base::Bind(&AltErrorPageResourceFetcher::OnURLFetchComplete,
                  base::Unretained(this))));
+  fetcher_->SetTimeout(base::TimeDelta::FromSeconds(kDownloadTimeoutSec));
 }
 
 AltErrorPageResourceFetcher::~AltErrorPageResourceFetcher() {
 }
 
-void AltErrorPageResourceFetcher::Cancel() {
-  fetcher_->Cancel();
-}
-
 void AltErrorPageResourceFetcher::OnURLFetchComplete(
     const WebURLResponse& response,
     const std::string& data) {
diff --git a/content/renderer/fetchers/alt_error_page_resource_fetcher.h b/content/renderer/fetchers/alt_error_page_resource_fetcher.h
index 7fcf34c..5f23f28 100644
--- a/content/renderer/fetchers/alt_error_page_resource_fetcher.h
+++ b/content/renderer/fetchers/alt_error_page_resource_fetcher.h
@@ -17,7 +17,7 @@ class WebURLResponse;
 }
 
 namespace content {
-class ResourceFetcherWithTimeout;
+class ResourceFetcher;
 
 // Used for downloading alternate dns error pages. Once downloading is done
 // (or fails), the webview delegate is notified.
@@ -39,15 +39,12 @@ class AltErrorPageResourceFetcher {
       const Callback& callback);
   ~AltErrorPageResourceFetcher();
 
-  // Stop any pending loads.
-  void Cancel();
-
  private:
   void OnURLFetchComplete(const blink::WebURLResponse& response,
                           const std::string& data);
 
   // Does the actual fetching.
-  scoped_ptr<ResourceFetcherWithTimeout> fetcher_;
+  scoped_ptr<ResourceFetcher> fetcher_;
 
   blink::WebFrame* frame_;
   Callback callback_;
diff --git a/content/renderer/fetchers/image_resource_fetcher.cc b/content/renderer/fetchers/image_resource_fetcher.cc
index fe6d1cd..78ef8b4 100644
--- a/content/renderer/fetchers/image_resource_fetcher.cc
+++ b/content/renderer/fetchers/image_resource_fetcher.cc
@@ -39,8 +39,6 @@ ImageResourceFetcher::ImageResourceFetcher(
 }
 
 ImageResourceFetcher::~ImageResourceFetcher() {
-  if (!fetcher_->completed())
-    fetcher_->Cancel();
 }
 
 void ImageResourceFetcher::OnURLFetchComplete(
diff --git a/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc b/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
index f910406..2d072fd 100644
--- a/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
+++ b/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
@@ -34,8 +34,6 @@ MultiResolutionImageResourceFetcher::MultiResolutionImageResourceFetcher(
 }
 
 MultiResolutionImageResourceFetcher::~MultiResolutionImageResourceFetcher() {
-  if (!fetcher_->completed())
-    fetcher_->Cancel();
 }
 
 void MultiResolutionImageResourceFetcher::OnURLFetchComplete(
diff --git a/content/renderer/fetchers/resource_fetcher.cc b/content/renderer/fetchers/resource_fetcher.cc
index ccdda07..435a3f8 100644
--- a/content/renderer/fetchers/resource_fetcher.cc
+++ b/content/renderer/fetchers/resource_fetcher.cc
@@ -26,14 +26,12 @@ namespace content {
 ResourceFetcher::ResourceFetcher(const GURL& url, WebFrame* frame,
                                  WebURLRequest::TargetType target_type,
                                  const Callback& callback)
-    : url_(url),
-      target_type_(target_type),
-      completed_(false),
+    : completed_(false),
       callback_(callback) {
   // Can't do anything without a frame.  However, delegate can be NULL (so we
   // can do a http request and ignore the results).
   DCHECK(frame);
-  Start(frame);
+  Start(url, frame, target_type);
 }
 
 ResourceFetcher::~ResourceFetcher() {
@@ -41,16 +39,17 @@ ResourceFetcher::~ResourceFetcher() {
     loader_->cancel();
 }
 
-void ResourceFetcher::Cancel() {
-  if (!completed_) {
-    loader_->cancel();
-    completed_ = true;
-  }
+void ResourceFetcher::SetTimeout(base::TimeDelta timeout) {
+  DCHECK(loader_);
+  DCHECK(!completed_);
+  timeout_timer_.Start(FROM_HERE, timeout, this,
+                       &ResourceFetcher::TimeoutFired);
 }
 
-void ResourceFetcher::Start(WebFrame* frame) {
-  WebURLRequest request(url_);
-  request.setTargetType(target_type_);
+void ResourceFetcher::Start(const GURL& url, WebFrame* frame,
+                            WebURLRequest::TargetType target_type) {
+  WebURLRequest request(url);
+  request.setTargetType(target_type);
   request.setFirstPartyForCookies(frame->document().firstPartyForCookies());
   frame->dispatchWillSendRequest(request);
 
@@ -60,6 +59,8 @@ void ResourceFetcher::Start(WebFrame* frame) {
 
 void ResourceFetcher::RunCallback(const WebURLResponse& response,
                                   const std::string& data) {
+  completed_ = true;
+  timeout_timer_.Stop();
   if (callback_.is_null())
     return;
 
@@ -69,6 +70,12 @@ void ResourceFetcher::RunCallback(const WebURLResponse& response,
   callback.Run(response, data);
 }
 
+void ResourceFetcher::TimeoutFired() {
+  DCHECK(!completed_);
+  loader_->cancel();
+  RunCallback(WebURLResponse(), std::string());
+}
+
 /////////////////////////////////////////////////////////////////////////////
 // WebURLLoaderClient methods
 
@@ -108,38 +115,15 @@ void ResourceFetcher::didReceiveCachedMetadata(
 void ResourceFetcher::didFinishLoading(
     WebURLLoader* loader, double finishTime) {
   DCHECK(!completed_);
-  completed_ = true;
 
   RunCallback(response_, data_);
 }
 
 void ResourceFetcher::didFail(WebURLLoader* loader, const WebURLError& error) {
   DCHECK(!completed_);
-  completed_ = true;
 
   // Go ahead and tell our delegate that we're done.
   RunCallback(WebURLResponse(), std::string());
 }
 
-/////////////////////////////////////////////////////////////////////////////
-// A resource fetcher with a timeout
-
-ResourceFetcherWithTimeout::ResourceFetcherWithTimeout(
-    const GURL& url, WebFrame* frame, WebURLRequest::TargetType target_type,
-    int timeout_secs, const Callback& callback)
-    : ResourceFetcher(url, frame, target_type, callback) {
-  timeout_timer_.Start(FROM_HERE, TimeDelta::FromSeconds(timeout_secs), this,
-                       &ResourceFetcherWithTimeout::TimeoutFired);
-}
-
-ResourceFetcherWithTimeout::~ResourceFetcherWithTimeout() {
-}
-
-void ResourceFetcherWithTimeout::TimeoutFired() {
-  if (!completed_) {
-    loader_->cancel();
-    didFail(NULL, WebURLError());
-  }
-}
-
 }  // namespace content
diff --git a/content/renderer/fetchers/resource_fetcher.h b/content/renderer/fetchers/resource_fetcher.h
index 038dc55..674aa92 100644
--- a/content/renderer/fetchers/resource_fetcher.h
+++ b/content/renderer/fetchers/resource_fetcher.h
@@ -2,12 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
-// A wrapper around ResourceHandle and ResourceHandleClient that simplifies
-// the download of an HTTP object.  The interface is modeled after URLFetcher
-// in the /chrome/browser.
+// A wrapper that simplifies the download of an HTTP object.  The interface is
+// modeled after URLFetcher in /net/url_request/.
 //
-// ResourceFetcher::Delegate::OnURLFetchComplete will be called async after
-// the ResourceFetcher object is created.
+// The callback passed to the constructor will be called async after the
+// resource is retrieved.
 
 #ifndef CONTENT_RENDERER_FETCHERS_RESOURCE_FETCHER_H_
 #define CONTENT_RENDERER_FETCHERS_RESOURCE_FETCHER_H_
@@ -18,12 +17,12 @@
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "content/common/content_export.h"
 #include "third_party/WebKit/public/platform/WebURLLoaderClient.h"
 #include "third_party/WebKit/public/platform/WebURLRequest.h"
 #include "third_party/WebKit/public/platform/WebURLResponse.h"
-#include "url/gurl.h"
 
 class GURL;
 
@@ -40,23 +39,36 @@ class CONTENT_EXPORT ResourceFetcher
  public:
   // This will be called when the URL has been fetched, successfully or not.
   // If there is a failure, response and data will both be empty.  |response|
-  // and |data| are both valid until the URLFetcher instance is destroyed.
-  typedef base::Callback<void(const blink::WebURLResponse&,
-                              const std::string&)> Callback;
+  // and |data| are both valid until the ResourceFetcher instance is destroyed.
+  typedef base::Callback<void(const blink::WebURLResponse& response,
+                              const std::string& data)> Callback;
 
-  // We need a frame to make requests.
+  // A frame is needed to make requests.
   ResourceFetcher(
       const GURL& url, blink::WebFrame* frame,
       blink::WebURLRequest::TargetType target_type,
       const Callback& callback);
+
+  // Deleting a ResourceFetcher will safely cancel the request if it has not yet
+  // completed.
   virtual ~ResourceFetcher();
 
-  // Stop the request and don't call the callback.
-  void Cancel();
+  // Sets a timeout for the request.  Request must not yet have completed
+  // or timed out when called.
+  void SetTimeout(base::TimeDelta timeout);
+
+ private:
+  // Start the actual download.
+  void Start(const GURL& url, blink::WebFrame* frame,
+             blink::WebURLRequest::TargetType target_type);
+
+  void RunCallback(const blink::WebURLResponse& response,
+                   const std::string& data);
 
-  bool completed() const { return completed_; }
+  // Callback for timer that limits how long we wait for the server.  If this
+  // timer fires and the request hasn't completed, we kill the request.
+  void TimeoutFired();
 
- protected:
   // WebURLLoaderClient methods:
   virtual void willSendRequest(
       blink::WebURLLoader* loader, blink::WebURLRequest& new_request,
@@ -79,55 +91,25 @@ class CONTENT_EXPORT ResourceFetcher
 
   scoped_ptr<blink::WebURLLoader> loader_;
 
-  // URL we're fetching
-  GURL url_;
-
-  // Target type
-  blink::WebURLRequest::TargetType target_type_;
-
-  // A copy of the original resource response
-  blink::WebURLResponse response_;
-
-  // Set to true once the request is compelte.
+  // Set to true once the request is complete.
   bool completed_;
 
- private:
-  // Start the actual download.
-  void Start(blink::WebFrame* frame);
+  // Buffer to hold the content from the server.
+  std::string data_;
 
-  void RunCallback(const blink::WebURLResponse& response,
-                   const std::string& data);
+  // A copy of the original resource response.
+  blink::WebURLResponse response_;
 
-  // Callback when we're done
+  // Callback when we're done.
   Callback callback_;
 
-  // Buffer to hold the content from the server.
-  std::string data_;
-
   // Buffer to hold metadata from the cache.
   std::string metadata_;
-};
-
-/////////////////////////////////////////////////////////////////////////////
-// A resource fetcher with a timeout
-class CONTENT_EXPORT ResourceFetcherWithTimeout
-    : NON_EXPORTED_BASE(public ResourceFetcher) {
- public:
-  ResourceFetcherWithTimeout(const GURL& url,
-                             blink::WebFrame* frame,
-                             blink::WebURLRequest::TargetType target_type,
-                             int timeout_secs,
-                             const Callback& callback);
-  virtual ~ResourceFetcherWithTimeout();
 
- private:
-  // Callback for timer that limits how long we wait for the alternate error
-  // page server.  If this timer fires and the request hasn't completed, we
-  // kill the request.
-  void TimeoutFired();
+  // Limit how long to wait for the server.
+  base::OneShotTimer<ResourceFetcher> timeout_timer_;
 
-  // Limit how long we wait for the alternate error page server.
-  base::OneShotTimer<ResourceFetcherWithTimeout> timeout_timer_;
+  DISALLOW_COPY_AND_ASSIGN(ResourceFetcher);
 };
 
 }  // namespace content
diff --git a/content/renderer/resource_fetcher_browsertest.cc b/content/renderer/resource_fetcher_browsertest.cc
index b03ad45..8397da0 100644
--- a/content/renderer/resource_fetcher_browsertest.cc
+++ b/content/renderer/resource_fetcher_browsertest.cc
@@ -8,6 +8,7 @@
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
+#include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.h"
@@ -108,10 +109,13 @@ class EvilFetcherDelegate : public FetcherDelegate {
 
   virtual void OnURLFetchComplete(const WebURLResponse& response,
                                   const std::string& data) OVERRIDE {
+    FetcherDelegate::OnURLFetchComplete(response, data);
+
     // Destroy the ResourceFetcher here.  We are testing that upon returning
-    // to the ResourceFetcher that it does not crash.
+    // to the ResourceFetcher that it does not crash.  This must be done after
+    // calling FetcherDelegate::OnURLFetchComplete, since deleting the fetcher
+    // invalidates |response| and |data|.
     fetcher_.reset();
-    FetcherDelegate::OnURLFetchComplete(response, data);
   }
 
  private:
@@ -186,9 +190,10 @@ class ResourceFetcherTests : public ContentBrowserTest {
     WebFrame* frame = GetRenderView()->GetWebView()->mainFrame();
 
     scoped_ptr<FetcherDelegate> delegate(new FetcherDelegate);
-    scoped_ptr<ResourceFetcher> fetcher(new ResourceFetcherWithTimeout(
+    scoped_ptr<ResourceFetcher> fetcher(new ResourceFetcher(
         url, frame, WebURLRequest::TargetIsMainFrame,
-        0, delegate->NewCallback()));
+        delegate->NewCallback()));
+    fetcher->SetTimeout(base::TimeDelta());
 
     delegate->WaitForResponse();
 
@@ -204,9 +209,10 @@ class ResourceFetcherTests : public ContentBrowserTest {
     WebFrame* frame = GetRenderView()->GetWebView()->mainFrame();
 
     scoped_ptr<EvilFetcherDelegate> delegate(new EvilFetcherDelegate);
-    scoped_ptr<ResourceFetcher> fetcher(new ResourceFetcherWithTimeout(
+    scoped_ptr<ResourceFetcher> fetcher(new ResourceFetcher(
         url, frame, WebURLRequest::TargetIsMainFrame,
-        0, delegate->NewCallback()));
+        delegate->NewCallback()));
+    fetcher->SetTimeout(base::TimeDelta());
     delegate->SetFetcher(fetcher.release());
 
     delegate->WaitForResponse();