Adds a feature to MHTML serialization that omits subframes and subresources marked no-store.

content/browser/download/mhtml_generation_browsertest.cc

Index: content/browser/download/mhtml_generation_browsertest.cc
diff --git a/content/browser/download/mhtml_generation_browsertest.cc b/content/browser/download/mhtml_generation_browsertest.cc
index 00f3aeb1964ce93c82c864ee176517fb083ec02c..21af9320a554517e7187ef90a3312345952364e4 100644
--- a/content/browser/download/mhtml_generation_browsertest.cc
+++ b/content/browser/download/mhtml_generation_browsertest.cc
@@ -11,6 +11,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/strings/utf_string_conversions.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/mhtml_generation_params.h"
 #include "content/public/test/browser_test_utils.h"
@@ -18,10 +19,12 @@
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
+#include "net/base/filename_util.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/WebKit/public/web/WebFindOptions.h"
 
 using testing::ContainsRegex;
 using testing::HasSubstr;
@@ -29,6 +32,57 @@ using testing::Not;
 
 namespace content {
 
+namespace {
+
+// A dummy WebContentsDelegate which tracks whether CloseContents() has been
+// called. It refuses the actual close but keeps track of whether the renderer
+// requested it.

[dcheng, 2016/05/20 20:25:02]

I think this comment might be out of date?

[dewittj, 2016/05/20 21:22:22]

Done.

+class FindTrackingDelegate : public WebContentsDelegate {
+ public:
+  FindTrackingDelegate(const std::string& search)
+      : search_(search), matches_(-1) {}
+
+  // Returns number of result.

[ncarter (slow), 2016/05/19 23:36:35]

result -> results.

[dewittj, 2016/05/20 16:46:03]

Done.

+  int Wait(WebContents* web_contents) {
+    WebContentsDelegate* old_delegate = web_contents->GetDelegate();
+    web_contents->SetDelegate(this);
+
+    blink::WebFindOptions options;
+    options.matchCase = false;
+
+    web_contents->Find(global_request_id++, base::UTF8ToUTF16(search_),
+                       options);
+    run_loop_.Run();
+
+    web_contents->SetDelegate(old_delegate);
+
+    return matches_;
+  }
+
+  void FindReply(WebContents* web_contents,
+                 int request_id,
+                 int number_of_matches,
+                 const gfx::Rect& selection_rect,
+                 int active_match_ordinal,
+                 bool final_update) override {
+    matches_ = number_of_matches;
+    run_loop_.Quit();
+  }
+
+  static int global_request_id;
+
+ private:
+  std::string search_;
+  int matches_;
+  base::RunLoop run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(FindTrackingDelegate);
+};
+
+// static
+int FindTrackingDelegate::global_request_id = 0;
+}

[dcheng, 2016/05/20 20:25:01]

Nit: newline before and // namespace after }

[dewittj, 2016/05/20 21:22:22]

Done.

+
 class MHTMLGenerationTest : public ContentBrowserTest {
  public:
   MHTMLGenerationTest() : has_mhtml_callback_run_(false), file_size_(0) {}
@@ -65,6 +119,68 @@ class MHTMLGenerationTest : public ContentBrowserTest {
     return file_size;
   }
 
+  void TestOriginalVsSavedPage(
+      const GURL& url,
+      const MHTMLGenerationParams params,
+      int expected_number_of_frames,
+      const std::vector<std::string>& expected_substrings,
+      const std::vector<std::string>& forbidden_substrings_in_saved_page,
+      bool skip_verification_of_original_page = false) {
+    // Navigate to the test page and verify if test expectations
+    // are met (this is mostly a sanity check - a failure to meet
+    // expectations would probably mean that there is a test bug
+    // (i.e. that we got called with wrong expected_foo argument).
+    NavigateToURL(shell(), url);
+    if (!skip_verification_of_original_page) {
+      AssertExpectationsAboutCurrentTab(expected_number_of_frames,
+                                        expected_substrings,
+                                        std::vector<std::string>());
+    }
+
+    GenerateMHTML(params, url);
+    ASSERT_FALSE(HasFailure());
+
+    // Stop the test server (to make sure the locally saved page
+    // is self-contained / won't try to open original resources).
+    ASSERT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete());
+
+    // Open the saved page and verify if test expectations are
+    // met (i.e. if the same expectations are met for "after"
+    // [saved version of the page] as for the "before"
+    // [the original version of the page].
+    NavigateToURL(shell(), GURL(net::FilePathToFileURL(params.file_path)));

[dcheng, 2016/05/20 20:25:02]

This is already a GURL, no need to call GURL()'s conversion ctor.

[dewittj, 2016/05/20 21:22:22]

Done.

+    AssertExpectationsAboutCurrentTab(expected_number_of_frames,
+                                      expected_substrings,
+                                      forbidden_substrings_in_saved_page);
+  }
+
+  void AssertExpectationsAboutCurrentTab(
+      int expected_number_of_frames,
+      const std::vector<std::string>& expected_substrings,
+      const std::vector<std::string>& forbidden_substrings) {
+    int actual_number_of_frames =
+        shell()->web_contents()->GetAllFrames().size();
+    EXPECT_EQ(expected_number_of_frames, actual_number_of_frames);
+
+    for (const auto& expected_substring : expected_substrings) {
+      FindTrackingDelegate delegate(expected_substring);
+      int actual_number_of_matches = delegate.Wait(shell()->web_contents());
+      EXPECT_EQ(1, actual_number_of_matches)
+          << "Verifying that \"" << expected_substring << "\" appears "
+          << "exactly once in the text of web contents of "
+          << shell()->web_contents()->GetURL().spec();
+    }
+
+    for (const auto& forbidden_substring : forbidden_substrings) {
+      FindTrackingDelegate delegate(forbidden_substring);
+      int actual_number_of_matches = delegate.Wait(shell()->web_contents());
+      EXPECT_EQ(0, actual_number_of_matches)
+          << "Verifying that \"" << forbidden_substring << "\" doesn't "
+          << "appear in the text of web contents of "
+          << shell()->web_contents()->GetURL().spec();
+    }
+  }
+
   bool has_mhtml_callback_run() const { return has_mhtml_callback_run_; }
   int64_t file_size() const { return file_size_; }
 
@@ -199,6 +315,101 @@ IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTMLObeyNoStoreMainFrame) {
   EXPECT_THAT(mhtml, Not(HasSubstr("test body")));
 }
 
+IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest,
+                       GenerateMHTMLIgnoreNoStoreSubFrame) {
+  base::FilePath path(temp_dir_.path());
+  path = path.Append(FILE_PATH_LITERAL("test.mht"));
+
+  GURL url(embedded_test_server()->GetURL("/page_with_nostore_iframe.html"));
+
+  // Generate MHTML, specifying the FAIL_FOR_NO_STORE_MAIN_FRAME policy.
+  MHTMLGenerationParams params(path);
+  params.cache_control_policy =
+      content::MHTMLCacheControlPolicy::FAIL_FOR_NO_STORE_MAIN_FRAME;
+
+  GenerateMHTML(params, url);
+  // We expect that there was no error (file size -1 indicates an error.)
+  EXPECT_LT(0, file_size());
+
+  std::string mhtml;
+  ASSERT_TRUE(base::ReadFileToString(path, &mhtml));
+
+  EXPECT_THAT(mhtml, HasSubstr("Main Frame"));
+  // Make sure that no-store subresources exist in this mode.
+  EXPECT_THAT(mhtml, HasSubstr("no-store test body"));
+  EXPECT_THAT(mhtml, ContainsRegex("Content-Location:.*nostore.jpg"));
+}
+
+IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTMLObeyNoStoreSubFrame) {
+  base::FilePath path(temp_dir_.path());
+  path = path.Append(FILE_PATH_LITERAL("test.mht"));
+
+  GURL url(embedded_test_server()->GetURL("/page_with_nostore_iframe.html"));
+
+  // Generate MHTML, specifying the FAIL_FOR_NO_STORE_MAIN_FRAME policy.
+  MHTMLGenerationParams params(path);
+  params.cache_control_policy = content::MHTMLCacheControlPolicy::
+      SKIP_ANY_FRAME_OR_RESOURCE_MARKED_NO_STORE;
+
+  GenerateMHTML(params, url);
+  // We expect that there was no error (file size -1 indicates an error.)
+  EXPECT_LT(0, file_size());
+
+  std::string mhtml;
+  ASSERT_TRUE(base::ReadFileToString(path, &mhtml));
+
+  EXPECT_THAT(mhtml, HasSubstr("Main Frame"));
+  // Make sure the contents are missing.
+  EXPECT_THAT(mhtml, Not(HasSubstr("no-store test body")));
+  // This image comes from a resource marked no-store.
+  EXPECT_THAT(mhtml, Not(ContainsRegex("Content-Location:.*nostore.jpg")));
+}
+
+IN_PROC_BROWSER_TEST_F(
+    MHTMLGenerationTest,
+    ViewedMHTMLContainsNoStoreContentIfNoCacheControlPolicy) {
+  // Generate MHTML, specifying the FAIL_FOR_NO_STORE_MAIN_FRAME policy.
+  base::FilePath path(temp_dir_.path());
+  path = path.Append(FILE_PATH_LITERAL("test.mht"));
+  MHTMLGenerationParams params(path);
+
+  // No special cache control options so we should see both frames.
+  std::vector<std::string> expectations{

[ncarter (slow), 2016/05/19 23:36:35]

std::vector<std::string> expectations = {

(per
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...)

[dewittj, 2016/05/20 16:46:03]

Done.

+      "Main Frame, normal headers.", "Cache-Control: no-store test body",
+  };
+  std::vector<std::string> forbidden;
+  TestOriginalVsSavedPage(
+      embedded_test_server()->GetURL("/page_with_nostore_iframe.html"), params,
+      2 /* expected number of frames */, expectations, forbidden);
+
+  std::string mhtml;
+  ASSERT_TRUE(base::ReadFileToString(params.file_path, &mhtml));
+}
+
+IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest,
+                       ViewedMHTMLDoesNotContainNoStoreContent) {
+  // Generate MHTML, specifying the FAIL_FOR_NO_STORE_MAIN_FRAME policy.
+  base::FilePath path(temp_dir_.path());
+  path = path.Append(FILE_PATH_LITERAL("test.mht"));
+  MHTMLGenerationParams params(path);
+  params.cache_control_policy = content::MHTMLCacheControlPolicy::
+      SKIP_ANY_FRAME_OR_RESOURCE_MARKED_NO_STORE;
+
+  // No special cache control options so we should see both frames.
+  std::vector<std::string> expectations{

[ncarter (slow), 2016/05/19 23:36:35]

std::vector<std::string> expectations = {

[dewittj, 2016/05/20 16:46:03]

Done.

+      "Main Frame, normal headers.",
+  };
+  std::vector<std::string> forbidden{

[ncarter (slow), 2016/05/19 23:36:35]

 std::vector<std::string> forbidden = {

[dewittj, 2016/05/20 16:46:03]

Done.

+      "Cache-Control: no-store test body",
+  };
+  TestOriginalVsSavedPage(
+      embedded_test_server()->GetURL("/page_with_nostore_iframe.html"), params,
+      2 /* expected number of frames */, expectations, forbidden);
+
+  std::string mhtml;
+  ASSERT_TRUE(base::ReadFileToString(params.file_path, &mhtml));
+}
+
 // Test suite that allows testing --site-per-process against cross-site frames.
 // See http://dev.chromium.org/developers/design-documents/site-isolation.
 class MHTMLGenerationSitePerProcessTest : public MHTMLGenerationTest {