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

When the DomDistillerViewerSource got a request for chrome-distiller://foobar/
it would crash if there was an empty path argument. This CL adds a safety check
for the length of the path, to ensure to not take a substring of an empty
string. 

In addition, this adds regression tests for the crash it fixes. It also fixes
the rest of the disabled browser tests for the DomDistillerViewerSource. 

BUG=356866

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

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

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_) {
@@ -161,83 +107,87 @@ class DomDistillerViewerSourceBrowserTest : public InProcessBrowserTest {
       EXPECT_CALL(*distiller_factory_, CreateDistillerImpl())
           .WillOnce(testing::Return(distiller));
     }
+    if (expect_distiller_page_) {
+      MockDistillerPage* distiller_page = new MockDistillerPage();
+      EXPECT_CALL(*distiller_page_factory_, CreateDistillerPageImpl())
+          .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 bool expect_distiller_page_;
   static MockDistillerFactory* distiller_factory_;
 };
 
 FakeDB::EntryMap* DomDistillerViewerSourceBrowserTest::database_model_;
 bool DomDistillerViewerSourceBrowserTest::expect_distillation_ = false;
+bool DomDistillerViewerSourceBrowserTest::expect_distiller_page_ = 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;
+  expect_distiller_page_ = 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);
+  expect_distiller_page_ = false;
+  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;
+  expect_distiller_page_ = 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,31 +195,36 @@ 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;
+  expect_distiller_page_ = 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;
+  expect_distiller_page_ = 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;
+  expect_distiller_page_ = false;
+  const GURL url(std::string(chrome::kDomDistillerScheme) + "://bogus/foobar");
+  ViewSingleDistilledPage(url, "text/html");
+}
 
 IN_PROC_BROWSER_TEST_F(DomDistillerViewerSourceBrowserTest,
                        MultiPageArticle) {
   expect_distillation_ = false;
+  expect_distiller_page_ = true;
   dom_distiller::DomDistillerServiceFactory::GetInstance()
       ->SetTestingFactoryAndUse(browser()->profile(), &Build);
 
@@ -285,7 +240,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(

components/dom_distiller/content/dom_distiller_viewer_source.cc

@@ -273,8 +273,12 @@ void DomDistillerViewerSource::StartDataRequest(
           content::RenderFrameHost::FromID(render_process_id,
                                            render_frame_id));
   DCHECK(web_contents);
-  RequestViewerHandle* request_viewer_handle =
-      new RequestViewerHandle(web_contents, scheme_, path.substr(1), callback);
+  // An empty |path| is invalid, but guard against it. If not empty, assume
+  // |path| starts with '?', which is stripped away.
+  const std::string path_after_query_separator =
+      path.size() > 0 ? path.substr(1) : "";
+  RequestViewerHandle* request_viewer_handle = new RequestViewerHandle(
+      web_contents, scheme_, path_after_query_separator, callback);
   scoped_ptr<ViewerHandle> viewer_handle = viewer::CreateViewRequest(
       dom_distiller_service_, path, request_viewer_handle);