[dom_distiller] Fix crash for invalid chrome-distiller:// requests

chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc

Index: chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
diff --git a/chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc b/chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
index ae6e19e7aa3ea47381fea5ed5fa3a5c7b26a61de..44d75f61e158f7508960ecc497e1ee5fb3d19eff 100644
--- a/chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
+++ b/chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
@@ -72,57 +72,6 @@ ArticleEntry CreateEntry(std::string entry_id, std::string page_url) {
 
 }  // namespace
 
-// WebContents observer that stores reference to the current |RenderViewHost|.
-class LoadSuccessObserver : public content::WebContentsObserver {
- public:
-  explicit LoadSuccessObserver(content::WebContents* contents)
-      : content::WebContentsObserver(contents),
-        validated_url_(GURL()),
-        finished_load_(false),
-        load_failed_(false),
-        web_contents_(contents),
-        render_view_host_(NULL) {}
-
-  virtual void DidFinishLoad(int64 frame_id,
-                             const GURL& validated_url,
-                             bool is_main_frame,
-                             content::RenderViewHost* render_view_host)
-      OVERRIDE {
-    validated_url_ = validated_url;
-    finished_load_ = true;
-    render_view_host_ = render_view_host;
-  }
-
-  virtual void DidFailProvisionalLoad(int64 frame_id,
-                                      const base::string16& frame_unique_name,
-                                      bool is_main_frame,
-                                      const GURL& validated_url,
-                                      int error_code,
-                                      const base::string16& error_description,
-                                      content::RenderViewHost* render_view_host)
-      OVERRIDE {
-    load_failed_ = true;
-  }
-
-  const GURL& validated_url() const { return validated_url_; }
-  bool finished_load() const { return finished_load_; }
-  bool load_failed() const { return load_failed_; }
-  content::WebContents* web_contents() const { return web_contents_; }
-
-  const content::RenderViewHost* render_view_host() const {
-    return render_view_host_;
-  }
-
- private:
-  GURL validated_url_;
-  bool finished_load_;
-  bool load_failed_;
-  content::WebContents* web_contents_;
-  content::RenderViewHost* render_view_host_;
-
-  DISALLOW_COPY_AND_ASSIGN(LoadSuccessObserver);
-};
-
 class DomDistillerViewerSourceBrowserTest : public InProcessBrowserTest {
  public:
   DomDistillerViewerSourceBrowserTest() {}
@@ -149,9 +98,6 @@ class DomDistillerViewerSourceBrowserTest : public InProcessBrowserTest {
                 CreateStoreWithFakeDB(fake_db, FakeDB::EntryMap())),
             scoped_ptr<DistillerFactory>(distiller_factory_),
             scoped_ptr<DistillerPageFactory>(distiller_page_factory_));
-    MockDistillerPage* distiller_page = new MockDistillerPage();
-    EXPECT_CALL(*distiller_page_factory_, CreateDistillerPageImpl())
-        .WillOnce(testing::Return(distiller_page));
     fake_db->InitCallback(true);
     fake_db->LoadCallback(true);
     if (expect_distillation_) {
@@ -160,11 +106,15 @@ class DomDistillerViewerSourceBrowserTest : public InProcessBrowserTest {
       FakeDistiller* distiller = new FakeDistiller(true);
       EXPECT_CALL(*distiller_factory_, CreateDistillerImpl())
           .WillOnce(testing::Return(distiller));
+      MockDistillerPage* distiller_page = new MockDistillerPage();
+      EXPECT_CALL(*distiller_page_factory_, CreateDistillerPageImpl())

[Yaron, 2014/05/16 17:07:42]

I think this is needed even in the expect_distillation_ false case. At least for
test I added, I thought this ends up getting called manually. Did this break
other tests?

[nyquist, 2014/05/16 18:46:44]

Done.

+          .WillOnce(testing::Return(distiller_page));
     }
     return service;
   }
 
-  void ViewSingleDistilledPage(const GURL& url);
+  void ViewSingleDistilledPage(const GURL& url,
+                               const std::string& expected_mime_type);
   // Database entries.
   static FakeDB::EntryMap* database_model_;
   static bool expect_distillation_;
@@ -178,66 +128,59 @@ MockDistillerFactory* DomDistillerViewerSourceBrowserTest::distiller_factory_ =
 
 // The DomDistillerViewerSource renders untrusted content, so ensure no bindings
 // are enabled when the article exists in the database.
-// Flakiness: crbug.com/356866
 IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest,
-                       DISABLED_NoWebUIBindingsArticleExists) {
+                       NoWebUIBindingsArticleExists) {
   // Ensure there is one item in the database, which will trigger distillation.
   const ArticleEntry entry = CreateEntry("DISTILLED", "http://example.com/1");
   AddEntry(entry, database_model_);
   expect_distillation_ = true;
   const GURL url = url_utils::GetDistillerViewUrlFromEntryId(
       chrome::kDomDistillerScheme, entry.entry_id());
-  ViewSingleDistilledPage(url);
+  ViewSingleDistilledPage(url, "text/html");
 }
 
 // The DomDistillerViewerSource renders untrusted content, so ensure no bindings
 // are enabled when the article is not found.
-// Flakiness: crbug.com/356866
 IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest,
-                       DISABLED_NoWebUIBindingsArticleNotFound) {
+                       NoWebUIBindingsArticleNotFound) {
   // The article does not exist, so assume no distillation will happen.
   expect_distillation_ = false;
-  const GURL url(std::string(chrome::kDomDistillerScheme) + "://" +
-                 base::GenerateGUID() + "/");
-  ViewSingleDistilledPage(url);
+  const GURL url = url_utils::GetDistillerViewUrlFromEntryId(
+      chrome::kDomDistillerScheme, "DOES_NOT_EXIST");
+  ViewSingleDistilledPage(url, "text/html");
 }
 
 // The DomDistillerViewerSource renders untrusted content, so ensure no bindings
 // are enabled when requesting to view an arbitrary URL.
-// Flakiness: crbug.com/356866
 IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest,
-                       DISABLED_NoWebUIBindingsViewUrl) {
+                       NoWebUIBindingsViewUrl) {
   // We should expect distillation for any valid URL.
   expect_distillation_ = true;
   GURL view_url("http://www.example.com/1");
   const GURL url = url_utils::GetDistillerViewUrlFromUrl(
       chrome::kDomDistillerScheme, view_url);
-  ViewSingleDistilledPage(url);
+  ViewSingleDistilledPage(url, "text/html");
 }
 
 void DomDistillerViewerSourceBrowserTest::ViewSingleDistilledPage(
-    const GURL& url) {
+    const GURL& url,
+    const std::string& expected_mime_type) {
   // Ensure the correct factory is used for the DomDistillerService.
   dom_distiller::DomDistillerServiceFactory::GetInstance()
       ->SetTestingFactoryAndUse(browser()->profile(), &Build);
 
-  // Setup observer to inspect the RenderViewHost after committed navigation.
-  content::WebContents* contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  LoadSuccessObserver observer(contents);
-
   // Navigate to a URL which the source should respond to.
   ui_test_utils::NavigateToURL(browser(), url);
 
-  // A navigation should have succeeded to the correct URL.
-  ASSERT_FALSE(observer.load_failed());
-  ASSERT_TRUE(observer.finished_load());
-  ASSERT_EQ(url, observer.validated_url());
-  // Ensure no bindings.
-  const content::RenderViewHost* render_view_host = observer.render_view_host();
-  ASSERT_EQ(0, render_view_host->GetEnabledBindings());
-  // The MIME-type should always be text/html for the distilled articles.
-  EXPECT_EQ("text/html", observer.web_contents()->GetContentsMimeType());
+  // Ensure no bindings for the loaded |url|.
+  content::WebContents* contents_after_nav =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents_after_nav != NULL);
+  EXPECT_EQ(url, contents_after_nav->GetLastCommittedURL());
+  const content::RenderViewHost* render_view_host =
+      contents_after_nav->GetRenderViewHost();
+  EXPECT_EQ(0, render_view_host->GetEnabledBindings());
+  EXPECT_EQ(expected_mime_type, contents_after_nav->GetContentsMimeType());
 }
 
 // The DomDistillerViewerSource renders untrusted content, so ensure no bindings
@@ -245,27 +188,28 @@ void DomDistillerViewerSourceBrowserTest::ViewSingleDistilledPage(
 // Chrome or provided by an extension.
 IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest,
                        NoWebUIBindingsDisplayCSS) {
-  // Setup observer to inspect the RenderViewHost after committed navigation.
-  content::WebContents* contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  LoadSuccessObserver observer(contents);
-
+  expect_distillation_ = false;
   // Navigate to a URL which the source should respond to with CSS.
   std::string url_without_scheme = std::string("://foobar/") + kViewerCssPath;
   GURL url(chrome::kDomDistillerScheme + url_without_scheme);
-  ui_test_utils::NavigateToURL(browser(), url);
+  ViewSingleDistilledPage(url, "text/css");
+}
 
-  // A navigation should have succeeded to the correct URL.
-  ASSERT_FALSE(observer.load_failed());
-  ASSERT_TRUE(observer.finished_load());
-  ASSERT_EQ(url, observer.validated_url());
-  // Ensure no bindings.
-  const content::RenderViewHost* render_view_host = observer.render_view_host();
-  ASSERT_EQ(0, render_view_host->GetEnabledBindings());
-  // The MIME-type should always be text/css for the CSS resources.
-  EXPECT_EQ("text/css", observer.web_contents()->GetContentsMimeType());
+IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest,
+                       EmptyURLShouldNotCrash) {
+  // This is a bogus URL, so no distillation will happen.
+  expect_distillation_ = false;
+  const GURL url(std::string(chrome::kDomDistillerScheme) + "://bogus/");
+  ViewSingleDistilledPage(url, "text/html");
 }
 
+IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest,
+                       InvalidURLShouldNotCrash) {
+  // This is a bogus URL, so no distillation will happen.
+  expect_distillation_ = false;
+  const GURL url(std::string(chrome::kDomDistillerScheme) + "://bogus/foobar");
+  ViewSingleDistilledPage(url, "text/html");
+}
 
 IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest,
                        MultiPageArticle) {
@@ -285,7 +229,6 @@ IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest,
   // Setup observer to inspect the RenderViewHost after committed navigation.
   content::WebContents* contents =
       browser()->tab_strip_model()->GetActiveWebContents();
-  LoadSuccessObserver observer(contents);
 
   // Navigate to a URL and wait for the distiller to flush contents to the page.
   GURL url(dom_distiller::url_utils::GetDistillerViewUrlFromUrl(