Fix a potential crash by clearing callback_ before running it.

R=tony
BUG=69592

Review URL: http://codereview.chromium.org/6325012

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

4 files changed, 62 insertions, 15 deletions

diff --git a/webkit/glue/image_resource_fetcher.cc b/webkit/glue/image_resource_fetcher.cc
index 1396aa3..b6f6620 100644
--- a/webkit/glue/image_resource_fetcher.cc
+++ b/webkit/glue/image_resource_fetcher.cc
@@ -47,8 +47,12 @@ void ImageResourceFetcher::OnURLFetchComplete(
     // If we get here, it means no image from server or couldn't decode the
     // response as an image. The delegate will see a null image, indicating
     // that an error occurred.
-  callback_->Run(this, bitmap);
-  callback_.reset();
+
+  // Take care to clear callback_ before running the callback as it may lead to
+  // our destruction.
+  scoped_ptr<Callback> callback;
+  callback.swap(callback_);
+  callback->Run(this, bitmap);
 }
 
 }  // namespace webkit_glue
diff --git a/webkit/glue/resource_fetcher.cc b/webkit/glue/resource_fetcher.cc
index 0220ca5..d231f88 100644
--- a/webkit/glue/resource_fetcher.cc
+++ b/webkit/glue/resource_fetcher.cc
@@ -53,6 +53,18 @@ void ResourceFetcher::Start(WebFrame* frame) {
   loader_->loadAsynchronously(request, this);
 }
 
+void ResourceFetcher::RunCallback(const WebURLResponse& response,
+                                  const std::string& data) {
+  if (!callback_.get())
+    return;
+
+  // Take care to clear callback_ before running the callback as it may lead to
+  // our destruction.
+  scoped_ptr<Callback> callback;
+  callback.swap(callback_);
+  callback->Run(response, data);
+}
+
 /////////////////////////////////////////////////////////////////////////////
 // WebURLLoaderClient methods
 
@@ -93,10 +105,7 @@ void ResourceFetcher::didFinishLoading(
   DCHECK(!completed_);
   completed_ = true;
 
-  if (callback_.get()) {
-    callback_->Run(response_, data_);
-    callback_.reset();
-  }
+  RunCallback(response_, data_);
 }
 
 void ResourceFetcher::didFail(WebURLLoader* loader, const WebURLError& error) {
@@ -104,10 +113,7 @@ void ResourceFetcher::didFail(WebURLLoader* loader, const WebURLError& error) {
   completed_ = true;
 
   // Go ahead and tell our delegate that we're done.
-  if (callback_.get()) {
-    callback_->Run(WebURLResponse(), std::string());
-    callback_.reset();
-  }
+  RunCallback(WebURLResponse(), std::string());
 }
 
 /////////////////////////////////////////////////////////////////////////////
diff --git a/webkit/glue/resource_fetcher.h b/webkit/glue/resource_fetcher.h
index 013b326..eb9411b 100644
--- a/webkit/glue/resource_fetcher.h
+++ b/webkit/glue/resource_fetcher.h
@@ -75,9 +75,6 @@ class ResourceFetcher : public WebKit::WebURLLoaderClient {
   // URL we're fetching
   GURL url_;
 
-  // Callback when we're done
-  scoped_ptr<Callback> callback_;
-
   // A copy of the original resource response
   WebKit::WebURLResponse response_;
 
@@ -88,6 +85,12 @@ class ResourceFetcher : public WebKit::WebURLLoaderClient {
   // Start the actual download.
   void Start(WebKit::WebFrame* frame);
 
+  void RunCallback(const WebKit::WebURLResponse& response,
+                   const std::string& data);
+
+  // Callback when we're done
+  scoped_ptr<Callback> callback_;
+
   // Buffer to hold the content from the server.
   std::string data_;
 
diff --git a/webkit/glue/resource_fetcher_unittest.cc b/webkit/glue/resource_fetcher_unittest.cc
index 79bd53e..23834bf 100644
--- a/webkit/glue/resource_fetcher_unittest.cc
+++ b/webkit/glue/resource_fetcher_unittest.cc
@@ -46,8 +46,8 @@ class FetcherDelegate {
     return ::NewCallback(this, &FetcherDelegate::OnURLFetchComplete);
   }
 
-  void OnURLFetchComplete(const WebURLResponse& response,
-                          const std::string& data) {
+  virtual void OnURLFetchComplete(const WebURLResponse& response,
+                                  const std::string& data) {
     response_ = response;
     data_ = data;
     completed_ = true;
@@ -223,4 +223,38 @@ TEST_F(ResourceFetcherTests, ResourceFetcherTimeout) {
   EXPECT_TRUE(delegate->time_elapsed_ms() < kMaxWaitTimeMs);
 }
 
+class EvilFetcherDelegate : public FetcherDelegate {
+ public:
+  void SetFetcher(ResourceFetcher* fetcher) {
+    fetcher_.reset(fetcher);
+  }
+
+  void OnURLFetchComplete(const WebURLResponse& response,
+                          const std::string& data) {
+    // Destroy the ResourceFetcher here.  We are testing that upon returning
+    // to the ResourceFetcher that it does not crash.
+    fetcher_.reset();
+    FetcherDelegate::OnURLFetchComplete(response, data);
+  }
+
+ private:
+  scoped_ptr<ResourceFetcher> fetcher_;
+};
+
+TEST_F(ResourceFetcherTests, ResourceFetcherDeletedInCallback) {
+  ASSERT_TRUE(test_server_.Start());
+
+  WebFrame* frame = test_shell_->webView()->mainFrame();
+
+  // Grab a page that takes at least 1 sec to respond, but set the fetcher to
+  // timeout in 0 sec.
+  GURL url(test_server_.GetURL("slow?1"));
+  scoped_ptr<EvilFetcherDelegate> delegate(new EvilFetcherDelegate);
+  scoped_ptr<ResourceFetcher> fetcher(new ResourceFetcherWithTimeout(
+      url, frame, 0, delegate->NewCallback()));
+  delegate->SetFetcher(fetcher.release());
+
+  delegate->WaitForResponse();
+}
+
 }  // namespace