Make the distiller cancellable.

Makes the distiller cancellable, it is now safe to delete the distiller
even when distillation is in progress. On deleting the distiller any
callbacks posted by ongoing tasks are now cancelled.

BUG=340431
TEST=Included.

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

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

12 files changed, 133 insertions, 44 deletions

diff --git a/components/dom_distiller/android/component_jni_registrar.h b/components/dom_distiller/android/component_jni_registrar.h
index ace0035..ce24fb1 100644
--- a/components/dom_distiller/android/component_jni_registrar.h
+++ b/components/dom_distiller/android/component_jni_registrar.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_DOM_DISTILLER_ANDROID_COMPONENT_JNI_REGISTRAR_H
-#define COMPONENTS_DOM_DISTILLER_ANDROID_COMPONENT_JNI_REGISTRAR_H
+#ifndef COMPONENTS_DOM_DISTILLER_ANDROID_COMPONENT_JNI_REGISTRAR_H_
+#define COMPONENTS_DOM_DISTILLER_ANDROID_COMPONENT_JNI_REGISTRAR_H_
 
 #include <jni.h>
 
@@ -18,4 +18,4 @@ bool RegisterDomDistiller(JNIEnv* env);
 
 }  // namespace dom_distiller
 
-#endif  // COMPONENTS_DOM_DISTILLER_ANDROID_COMPONENT_JNI_REGISTRAR_H
+#endif  // COMPONENTS_DOM_DISTILLER_ANDROID_COMPONENT_JNI_REGISTRAR_H_
diff --git a/components/dom_distiller/content/distiller_page_web_contents.cc b/components/dom_distiller/content/distiller_page_web_contents.cc
index 649593c..2333d03 100644
--- a/components/dom_distiller/content/distiller_page_web_contents.cc
+++ b/components/dom_distiller/content/distiller_page_web_contents.cc
@@ -19,17 +19,16 @@
 namespace dom_distiller {
 
 scoped_ptr<DistillerPage> DistillerPageWebContentsFactory::CreateDistillerPage(
-    DistillerPage::Delegate* delegate) const {
+    const base::WeakPtr<DistillerPage::Delegate>& delegate) const {
   DCHECK(browser_context_);
   return scoped_ptr<DistillerPage>(
       new DistillerPageWebContents(delegate, browser_context_));
 }
 
 DistillerPageWebContents::DistillerPageWebContents(
-    DistillerPage::Delegate* delegate,
+    const base::WeakPtr<Delegate>& delegate,
     content::BrowserContext* browser_context)
-  : DistillerPage(delegate),
-    browser_context_(browser_context) {}
+    : DistillerPage(delegate), browser_context_(browser_context) {}
 
 DistillerPageWebContents::~DistillerPageWebContents() {
 }
diff --git a/components/dom_distiller/content/distiller_page_web_contents.h b/components/dom_distiller/content/distiller_page_web_contents.h
index 97bd2e0..8d8bf3e 100644
--- a/components/dom_distiller/content/distiller_page_web_contents.h
+++ b/components/dom_distiller/content/distiller_page_web_contents.h
@@ -31,7 +31,7 @@ class DistillerPageWebContentsFactory : public DistillerPageFactory {
   virtual ~DistillerPageWebContentsFactory() {}
 
   virtual scoped_ptr<DistillerPage> CreateDistillerPage(
-      DistillerPage::Delegate* delegate) const OVERRIDE;
+      const base::WeakPtr<DistillerPage::Delegate>& delegate) const OVERRIDE;
 
  private:
   content::BrowserContext* browser_context_;
@@ -41,7 +41,7 @@ class DistillerPageWebContentsFactory : public DistillerPageFactory {
 class DistillerPageWebContents : public DistillerPage,
                                  public content::WebContentsObserver {
  public:
-  DistillerPageWebContents(DistillerPage::Delegate* delegate,
+  DistillerPageWebContents(const base::WeakPtr<Delegate>& delegate,
                            content::BrowserContext* browser_context);
   virtual ~DistillerPageWebContents();
 
diff --git a/components/dom_distiller/content/distiller_page_web_contents_browsertest.cc b/components/dom_distiller/content/distiller_page_web_contents_browsertest.cc
index c90bd96..4c72eb7 100644
--- a/components/dom_distiller/content/distiller_page_web_contents_browsertest.cc
+++ b/components/dom_distiller/content/distiller_page_web_contents_browsertest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/memory/weak_ptr.h"
 #include "base/run_loop.h"
 #include "base/values.h"
 #include "components/dom_distiller/content/distiller_page_web_contents.h"
@@ -62,8 +63,9 @@ class DistillerPageWebContentsTest
 
 IN_PROC_BROWSER_TEST_F(DistillerPageWebContentsTest, LoadPage) {
   ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+  base::WeakPtrFactory<DistillerPage::Delegate> weak_factory(this);
   DistillerPageWebContents distiller_page(
-      this, shell()->web_contents()->GetBrowserContext());
+      weak_factory.GetWeakPtr(), shell()->web_contents()->GetBrowserContext());
   distiller_page_ = &distiller_page;
   distiller_page.Init();
   base::RunLoop run_loop;
diff --git a/components/dom_distiller/core/distiller.cc b/components/dom_distiller/core/distiller.cc
index 243b2796..55558f9 100644
--- a/components/dom_distiller/core/distiller.cc
+++ b/components/dom_distiller/core/distiller.cc
@@ -50,11 +50,12 @@ DistillerImpl::DistillerImpl(
     const DistillerPageFactory& distiller_page_factory,
     const DistillerURLFetcherFactory& distiller_url_fetcher_factory)
     : distiller_url_fetcher_factory_(distiller_url_fetcher_factory),
-      max_pages_in_article_(kMaxPagesInArticle) {
+      max_pages_in_article_(kMaxPagesInArticle),
+      weak_factory_(this) {
   page_distiller_.reset(new PageDistiller(distiller_page_factory));
 }
 
-DistillerImpl::~DistillerImpl() { DCHECK(AreAllPagesFinished()); }
+DistillerImpl::~DistillerImpl() {}
 
 void DistillerImpl::Init() {
   DCHECK(AreAllPagesFinished());
@@ -123,7 +124,7 @@ void DistillerImpl::DistillNextPage() {
     page_distiller_->DistillPage(
         url,
         base::Bind(&DistillerImpl::OnPageDistillationFinished,
-                   base::Unretained(this),
+                   weak_factory_.GetWeakPtr(),
                    page_num,
                    url));
   }
@@ -186,7 +187,7 @@ void DistillerImpl::FetchImage(int page_num,
 
   fetcher->FetchURL(item,
                     base::Bind(&DistillerImpl::OnFetchImageDone,
-                               base::Unretained(this),
+                               weak_factory_.GetWeakPtr(),
                                page_num,
                                base::Unretained(fetcher),
                                image_id));
diff --git a/components/dom_distiller/core/distiller.h b/components/dom_distiller/core/distiller.h
index 082dcad..a61825e 100644
--- a/components/dom_distiller/core/distiller.h
+++ b/components/dom_distiller/core/distiller.h
@@ -13,6 +13,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
+#include "base/memory/weak_ptr.h"
 #include "components/dom_distiller/core/article_distillation_update.h"
 #include "components/dom_distiller/core/distiller_url_fetcher.h"
 #include "components/dom_distiller/core/page_distiller.h"
@@ -179,6 +180,8 @@ class DistillerImpl : public Distiller {
 
   size_t max_pages_in_article_;
 
+  base::WeakPtrFactory<DistillerImpl> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(DistillerImpl);
 };
 
diff --git a/components/dom_distiller/core/distiller_page.cc b/components/dom_distiller/core/distiller_page.cc
index 3f4bffe..7f188ea 100644
--- a/components/dom_distiller/core/distiller_page.cc
+++ b/components/dom_distiller/core/distiller_page.cc
@@ -12,8 +12,9 @@ namespace dom_distiller {
 DistillerPageFactory::~DistillerPageFactory() {}
 
 DistillerPage::DistillerPage(
-    DistillerPage::Delegate* delegate)
+    const base::WeakPtr<DistillerPage::Delegate>& delegate)
     : state_(NO_CONTEXT), delegate_(delegate) {
+  DCHECK(delegate_.get());
 }
 
 DistillerPage::~DistillerPage() {
@@ -42,16 +43,14 @@ void DistillerPage::ExecuteJavaScript(const std::string& script) {
 void DistillerPage::OnLoadURLDone() {
   DCHECK_EQ(LOADING_PAGE, state_);
   state_ = PAGE_AVAILABLE;
-  if (!delegate_)
-    return;
+  DCHECK(delegate_.get());
   delegate_->OnLoadURLDone();
 }
 
 void DistillerPage::OnLoadURLFailed() {
   state_ = PAGELOAD_FAILED;
   scoped_ptr<base::Value> empty(base::Value::CreateNullValue());
-  if (!delegate_)
-    return;
+  DCHECK(delegate_.get());
   delegate_->OnExecuteJavaScriptDone(GURL(), empty.get());
 }
 
@@ -59,8 +58,7 @@ void DistillerPage::OnExecuteJavaScriptDone(const GURL& page_url,
                                             const base::Value* value) {
   DCHECK_EQ(EXECUTING_JAVASCRIPT, state_);
   state_ = PAGE_AVAILABLE;
-  if (!delegate_)
-    return;
+  DCHECK(delegate_.get());
   delegate_->OnExecuteJavaScriptDone(page_url, value);
 }
 
diff --git a/components/dom_distiller/core/distiller_page.h b/components/dom_distiller/core/distiller_page.h
index 4e22e8d..94ad51e 100644
--- a/components/dom_distiller/core/distiller_page.h
+++ b/components/dom_distiller/core/distiller_page.h
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/values.h"
 #include "url/gurl.h"
 
@@ -26,7 +27,8 @@ class DistillerPage {
                                          const base::Value* value) {}
   };
 
-  explicit DistillerPage(Delegate* delegate);
+  // Specifies the Delegate that owns this distiller page.
+  explicit DistillerPage(const base::WeakPtr<Delegate>& delegate);
 
   virtual ~DistillerPage();
 
@@ -86,7 +88,8 @@ class DistillerPage {
   State state_;
 
  private:
-  Delegate* delegate_;
+  // The pointer to the delegate that owns this distiller page.
+  base::WeakPtr<Delegate> delegate_;
   DISALLOW_COPY_AND_ASSIGN(DistillerPage);
 };
 
@@ -96,7 +99,7 @@ class DistillerPageFactory {
   virtual ~DistillerPageFactory();
 
   virtual scoped_ptr<DistillerPage> CreateDistillerPage(
-      DistillerPage::Delegate* delegate) const = 0;
+      const base::WeakPtr<DistillerPage::Delegate>& delegate) const = 0;
 };
 
 }  // namespace dom_distiller
diff --git a/components/dom_distiller/core/distiller_unittest.cc b/components/dom_distiller/core/distiller_unittest.cc
index 24647a42..471f91f 100644
--- a/components/dom_distiller/core/distiller_unittest.cc
+++ b/components/dom_distiller/core/distiller_unittest.cc
@@ -183,45 +183,62 @@ namespace dom_distiller {
 
 class TestDistillerURLFetcher : public DistillerURLFetcher {
  public:
-  TestDistillerURLFetcher() : DistillerURLFetcher(NULL) {
+  explicit TestDistillerURLFetcher(bool delay_fetch)
+      : DistillerURLFetcher(NULL), delay_fetch_(delay_fetch) {
     responses_[kImageURLs[0]] = string(kImageData[0]);
     responses_[kImageURLs[1]] = string(kImageData[1]);
   }
 
-  void CallCallback(string url, const URLFetcherCallback& callback) {
-    callback.Run(responses_[url]);
-  }
-
   virtual void FetchURL(const string& url,
                         const URLFetcherCallback& callback) OVERRIDE {
+    DCHECK(callback_.is_null());
+    url_ = url;
+    callback_ = callback;
+    if (!delay_fetch_) {
+      PostCallbackTask();
+    }
+  }
+
+  void PostCallbackTask() {
     ASSERT_TRUE(base::MessageLoop::current());
     base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(&TestDistillerURLFetcher::CallCallback,
-                   base::Unretained(this),
-                   url,
-                   callback));
+        FROM_HERE, base::Bind(callback_, responses_[url_]));
   }
 
+ private:
   std::map<string, string> responses_;
+  string url_;
+  URLFetcherCallback callback_;
+  bool delay_fetch_;
 };
 
 class TestDistillerURLFetcherFactory : public DistillerURLFetcherFactory {
  public:
   TestDistillerURLFetcherFactory() : DistillerURLFetcherFactory(NULL) {}
+
   virtual ~TestDistillerURLFetcherFactory() {}
   virtual DistillerURLFetcher* CreateDistillerURLFetcher() const OVERRIDE {
-    return new TestDistillerURLFetcher();
+    return new TestDistillerURLFetcher(false);
   }
 };
 
+class MockDistillerURLFetcherFactory : public DistillerURLFetcherFactory {
+ public:
+  explicit MockDistillerURLFetcherFactory()
+      : DistillerURLFetcherFactory(NULL) {}
+  virtual ~MockDistillerURLFetcherFactory() {}
+
+  MOCK_CONST_METHOD0(CreateDistillerURLFetcher, DistillerURLFetcher*());
+};
+
 class MockDistillerPage : public DistillerPage {
  public:
   MOCK_METHOD0(InitImpl, void());
   MOCK_METHOD1(LoadURLImpl, void(const GURL& gurl));
   MOCK_METHOD1(ExecuteJavaScriptImpl, void(const string& script));
 
-  explicit MockDistillerPage(DistillerPage::Delegate* delegate)
+  explicit MockDistillerPage(
+      const base::WeakPtr<DistillerPage::Delegate>& delegate)
       : DistillerPage(delegate) {}
 };
 
@@ -229,10 +246,10 @@ class MockDistillerPageFactory : public DistillerPageFactory {
  public:
   MOCK_CONST_METHOD1(
       CreateDistillerPageMock,
-      DistillerPage*(DistillerPage::Delegate* delegate));
+      DistillerPage*(const base::WeakPtr<DistillerPage::Delegate>& delegate));
 
   virtual scoped_ptr<DistillerPage> CreateDistillerPage(
-      DistillerPage::Delegate* delegate) const OVERRIDE {
+      const base::WeakPtr<DistillerPage::Delegate>& delegate) const OVERRIDE {
     return scoped_ptr<DistillerPage>(CreateDistillerPageMock(delegate));
   }
 };
@@ -269,7 +286,7 @@ ACTION_P3(DistillerPageOnExecuteJavaScriptDone, distiller_page, url, list) {
 }
 
 ACTION_P2(CreateMockDistillerPage, list, kurl) {
-  DistillerPage::Delegate* delegate = arg0;
+  const base::WeakPtr<DistillerPage::Delegate>& delegate = arg0;
   MockDistillerPage* distiller_page = new MockDistillerPage(delegate);
   EXPECT_CALL(*distiller_page, InitImpl());
   EXPECT_CALL(*distiller_page, LoadURLImpl(kurl))
@@ -280,11 +297,25 @@ ACTION_P2(CreateMockDistillerPage, list, kurl) {
   return distiller_page;
 }
 
+ACTION_P2(CreateMockDistillerPageWithPendingJSCallback,
+          distiller_page_ptr,
+          kurl) {
+  const base::WeakPtr<DistillerPage::Delegate>& delegate = arg0;
+  MockDistillerPage* distiller_page = new MockDistillerPage(delegate);
+  *distiller_page_ptr = distiller_page;
+  EXPECT_CALL(*distiller_page, InitImpl());
+  EXPECT_CALL(*distiller_page, LoadURLImpl(kurl))
+      .WillOnce(testing::InvokeWithoutArgs(distiller_page,
+                                           &DistillerPage::OnLoadURLDone));
+  EXPECT_CALL(*distiller_page, ExecuteJavaScriptImpl(_));
+  return distiller_page;
+}
+
 ACTION_P3(CreateMockDistillerPages,
           distiller_data,
           pages_size,
           start_page_num) {
-  DistillerPage::Delegate* delegate = arg0;
+  const base::WeakPtr<DistillerPage::Delegate>& delegate = arg0;
   MockDistillerPage* distiller_page = new MockDistillerPage(delegate);
   EXPECT_CALL(*distiller_page, InitImpl());
   {
@@ -585,4 +616,50 @@ TEST_F(DistillerTest, DeletingArticleDoesNotInterfereWithUpdates) {
       distiller_data.get(), kNumPages, in_sequence_updates_, start_page_num);
 }
 
+TEST_F(DistillerTest, CancelWithDelayedImageFetchCallback) {
+  base::MessageLoopForUI loop;
+  vector<int> image_indices;
+  image_indices.push_back(0);
+  scoped_ptr<base::ListValue> distilled_value =
+      CreateDistilledValueReturnedFromJS(kTitle, kContent, image_indices, "");
+  EXPECT_CALL(page_factory_, CreateDistillerPageMock(_))
+      .WillOnce(CreateMockDistillerPage(distilled_value.get(), GURL(kURL)));
+
+  TestDistillerURLFetcher* delayed_fetcher = new TestDistillerURLFetcher(true);
+  MockDistillerURLFetcherFactory url_fetcher_factory;
+  EXPECT_CALL(url_fetcher_factory, CreateDistillerURLFetcher())
+      .WillOnce(Return(delayed_fetcher));
+  distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory));
+  distiller_->Init();
+  DistillPage(kURL);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Post callback from the url fetcher and then delete the distiller.
+  delayed_fetcher->PostCallbackTask();
+  distiller_.reset();
+
+  base::MessageLoop::current()->RunUntilIdle();
+}
+
+TEST_F(DistillerTest, CancelWithDelayedJSCallback) {
+  base::MessageLoopForUI loop;
+  scoped_ptr<base::ListValue> distilled_value =
+      CreateDistilledValueReturnedFromJS(kTitle, kContent, vector<int>(), "");
+  MockDistillerPage* distiller_page = NULL;
+  EXPECT_CALL(page_factory_, CreateDistillerPageMock(_))
+      .WillOnce(CreateMockDistillerPageWithPendingJSCallback(&distiller_page,
+                                                             GURL(kURL)));
+  distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory_));
+  distiller_->Init();
+  DistillPage(kURL);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  ASSERT_TRUE(distiller_page);
+  // Post the task to execute javascript and then delete the distiller.
+  distiller_page->OnExecuteJavaScriptDone(GURL(kURL), distilled_value.get());
+  distiller_.reset();
+
+  base::MessageLoop::current()->RunUntilIdle();
+}
+
 }  // namespace dom_distiller
diff --git a/components/dom_distiller/core/page_distiller.cc b/components/dom_distiller/core/page_distiller.cc
index 7912b0f..9116b47 100644
--- a/components/dom_distiller/core/page_distiller.cc
+++ b/components/dom_distiller/core/page_distiller.cc
@@ -24,7 +24,10 @@ DistilledPageInfo::DistilledPageInfo() {}
 DistilledPageInfo::~DistilledPageInfo() {}
 
 PageDistiller::PageDistiller(const DistillerPageFactory& distiller_page_factory)
-    : distiller_page_(distiller_page_factory.CreateDistillerPage(this).Pass()) {
+    : weak_factory_(this) {
+  distiller_page_ =
+      distiller_page_factory.CreateDistillerPage(weak_factory_.GetWeakPtr())
+          .Pass();
 }
 
 PageDistiller::~PageDistiller() {}
diff --git a/components/dom_distiller/core/page_distiller.h b/components/dom_distiller/core/page_distiller.h
index bbe3f40..a1bc042 100644
--- a/components/dom_distiller/core/page_distiller.h
+++ b/components/dom_distiller/core/page_distiller.h
@@ -10,6 +10,7 @@
 
 #include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/values.h"
 #include "components/dom_distiller/core/distiller_page.h"
 #include "url/gurl.h"
@@ -63,6 +64,8 @@ class PageDistiller : public DistillerPage::Delegate {
   scoped_ptr<DistillerPage> distiller_page_;
   PageDistillerCallback page_distiller_callback_;
 
+  base::WeakPtrFactory<PageDistiller> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(PageDistiller);
 };
 
diff --git a/components/dom_distiller/core/url_utils_android.h b/components/dom_distiller/core/url_utils_android.h
index 46b3598..bedb1a1 100644
--- a/components/dom_distiller/core/url_utils_android.h
+++ b/components/dom_distiller/core/url_utils_android.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_DOM_DISTILLER_CORE_URL_UTILS_ANDROID_H
-#define COMPONENTS_DOM_DISTILLER_CORE_URL_UTILS_ANDROID_H
+#ifndef COMPONENTS_DOM_DISTILLER_CORE_URL_UTILS_ANDROID_H_
+#define COMPONENTS_DOM_DISTILLER_CORE_URL_UTILS_ANDROID_H_
 
 #include <jni.h>
 
@@ -24,4 +24,4 @@ bool RegisterUrlUtils(JNIEnv* env);
 
 }  // namespace dom_distiller
 
-#endif  // COMPONENTS_DOM_DISTILLER_CORE_URL_UTILS_ANDROID_H
+#endif  // COMPONENTS_DOM_DISTILLER_CORE_URL_UTILS_ANDROID_H_