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

chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc

 // Copyright 2014 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 <string.h>
 
 #include "base/command_line.h"
 #include "base/guid.h"
 #include "base/path_service.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 54 matching lines @@
   entry.set_entry_id(entry_id);
   if (!page_url.empty()) {
     ArticleEntryPage* page = entry.add_pages();
     page->set_url(page_url);
   }
   return entry;
 }
 
 }  // 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() {}
   virtual ~DomDistillerViewerSourceBrowserTest() {}
 
   virtual void SetUpOnMainThread() OVERRIDE {
     database_model_ = new FakeDB::EntryMap;
   }
 
   virtual void CleanUpOnMainThread() OVERRIDE { delete database_model_; }
 
   virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
     command_line->AppendSwitch(switches::kEnableDomDistiller);
   }
 
   static KeyedService* Build(content::BrowserContext* context) {
     FakeDB* fake_db = new FakeDB(database_model_);
     distiller_factory_ = new MockDistillerFactory();
     MockDistillerPageFactory* distiller_page_factory_ =
         new MockDistillerPageFactory();
     DomDistillerContextKeyedService* service =
         new DomDistillerContextKeyedService(
             scoped_ptr<DomDistillerStoreInterface>(
                 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_) {
       // There will only be destillation of an article if the database contains
       // the article.
       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_;
   static MockDistillerFactory* distiller_factory_;
 };
 
 FakeDB::EntryMap* DomDistillerViewerSourceBrowserTest::database_model_;
 bool DomDistillerViewerSourceBrowserTest::expect_distillation_ = false;
 MockDistillerFactory* DomDistillerViewerSourceBrowserTest::distiller_factory_ =
     NULL;
 
 // 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
 // are enabled when the CSS resource is loaded. This CSS might be bundle with
 // 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);
-
-  // 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());
+  ViewSingleDistilledPage(url, "text/css");
 }
 
+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) {
   expect_distillation_ = false;
   dom_distiller::DomDistillerServiceFactory::GetInstance()
       ->SetTestingFactoryAndUse(browser()->profile(), &Build);
 
   scoped_refptr<content::MessageLoopRunner> distillation_done_runner =
       new content::MessageLoopRunner;
 
   FakeDistiller* distiller = new FakeDistiller(
       false,
       distillation_done_runner->QuitClosure());
   EXPECT_CALL(*distiller_factory_, CreateDistillerImpl())
       .WillOnce(testing::Return(distiller));
 
   // 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(
       chrome::kDomDistillerScheme, GURL("http://urlthatlooksvalid.com")));
   chrome::NavigateParams params(browser(), url, content::PAGE_TRANSITION_TYPED);
   chrome::Navigate(&params);
   distillation_done_runner->Run();
 
   // Fake a multi-page response from distiller.
 
@@ skipping 59 matching lines @@
   EXPECT_TRUE(content::ExecuteScriptAndExtractString(
       contents, kGetLoadIndicatorClassName, &result));
   EXPECT_EQ("hidden", result);
   EXPECT_TRUE(content::ExecuteScriptAndExtractString(
       contents, kGetContent , &result));
   EXPECT_THAT(result, HasSubstr("Page 1 content"));
   EXPECT_THAT(result, HasSubstr("Page 2 content"));
 }
 
 }  // namespace dom_distiller