Allow Offline MHTML save operations to succeed even with missing frames.

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 1a71a9e50db3a2bce64d885faf1b41c85c6ed4ec..cf3f158a03e4323ead03629787da6a09eac9e228 100644
--- a/content/browser/download/mhtml_generation_browsertest.cc
+++ b/content/browser/download/mhtml_generation_browsertest.cc
@@ -15,10 +15,11 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/histogram_tester.h"
 #include "base/threading/thread_restrictions.h"
+#include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
+#include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/frame_messages.h"
 #include "content/public/browser/render_process_host.h"
-#include "content/public/browser/web_contents.h"
 #include "content/public/common/mhtml_generation_params.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
@@ -107,7 +108,7 @@ class MHTMLGenerationTest : public ContentBrowserTest {
   }
 
   void GenerateMHTML(const MHTMLGenerationParams& params, const GURL& url) {
-    NavigateToURL(shell(), url);
+    ASSERT_TRUE(NavigateToURL(shell(), url));
     GenerateMHTMLForCurrentPage(params);
   }
 
@@ -144,7 +145,7 @@ class MHTMLGenerationTest : public ContentBrowserTest {
     // 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);
+    ASSERT_TRUE(NavigateToURL(shell(), url));
     if (!skip_verification_of_original_page) {
       AssertExpectationsAboutCurrentTab(expected_number_of_frames,
                                         expected_substrings,
@@ -162,7 +163,8 @@ class MHTMLGenerationTest : public ContentBrowserTest {
     // 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(), net::FilePathToFileURL(params.file_path));
+    ASSERT_TRUE(
+        NavigateToURL(shell(), net::FilePathToFileURL(params.file_path)));
     AssertExpectationsAboutCurrentTab(expected_number_of_frames,
                                       expected_substrings,
                                       forbidden_substrings_in_saved_page);
@@ -222,6 +224,8 @@ IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTML) {
   path = path.Append(FILE_PATH_LITERAL("test.mht"));
 
   GenerateMHTML(path, embedded_test_server()->GetURL("/simple_page.html"));
+
+  // Checks that no other test failure was detected so far.
   ASSERT_FALSE(HasFailure());
 
   // Make sure the actual generated file has some contents.
@@ -242,6 +246,120 @@ IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTML) {
       static_cast<int>(MhtmlSaveStatus::SUCCESS), 1);
 }
 
+// Removes all children of the root tree node once the first MHTML serialization
+// response (regarding the main frame) is received from the renderer but before
+// that message is actually handled by the browser UI thread.
+class ChildFrameRemoverFilter : public BrowserMessageFilter {
+ public:
+  ChildFrameRemoverFilter(WebContentsImpl* web_contents)
+      : BrowserMessageFilter(FrameMsgStart), web_contents_(web_contents) {
+    DCHECK(web_contents_);
+  }
+
+ protected:
+  ~ChildFrameRemoverFilter() override {}
+
+ private:
+  bool OnMessageReceived(const IPC::Message& message) override {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    if (message.type() == FrameHostMsg_SerializeAsMHTMLResponse::ID) {
+      if (!already_received_response_) {
+        already_received_response_ = true;
+        BrowserThread::PostTask(
+            BrowserThread::UI, FROM_HERE,
+            base::Bind(&ChildFrameRemoverFilter::RemoveAllChildFrames,
+                       base::Unretained(this)));
+      } else {
+        ADD_FAILURE()
+            << "Should not receive another MHTML serialization response";
+      }
+    }
+    return false;
+  }
+
+  void RemoveAllChildFrames() {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    int tree_node_count = 1;
+    for (auto tree_node : web_contents_->GetFrameTree()->Nodes())
+      tree_node_count += tree_node->child_count();
+    // Note: at least 2 children nodes are needed to test all code paths.
+    ASSERT_GE(tree_node_count, 3);
+    FrameTreeNode* root = web_contents_->GetFrameTree()->root();
+    while (root->child_count())
+      root->RemoveChild(root->child_at(0));
+    ASSERT_EQ(0u, root->child_count());
+  }
+
+  WebContentsImpl* web_contents_;
+  bool already_received_response_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(ChildFrameRemoverFilter);
+};
+
+// Tests that if child frames are removed while saving a MHTML page the
+// operation fails by default.
+IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, FailDueToMissingIframe) {
+  ASSERT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL("/site_per_process_main.html")));
+
+  scoped_refptr<BrowserMessageFilter> filter = new ChildFrameRemoverFilter(
+      static_cast<WebContentsImpl*>(shell()->web_contents()));
+  shell()->web_contents()->GetRenderProcessHost()->AddFilter(filter.get());
+
+  base::FilePath path(temp_dir_.GetPath());
+  path = path.Append(FILE_PATH_LITERAL("test.mht"));
+
+  MHTMLGenerationParams params(path);
+
+  // Verifies the default is to not ignore missing iframes.
+  ASSERT_FALSE(params.ignore_missing_frames);
+
+  GenerateMHTMLForCurrentPage(params);
+
+  // Checks that no other test failure was detected so far.
+  ASSERT_FALSE(HasFailure());
+
+  // Verify the size reported by the callback is negative (an error happened).
+  EXPECT_LT(file_size(), 0);
+
+  // Checks that the final status reported to UMA is correct.
+  histogram_tester()->ExpectUniqueSample(
+      "PageSerialization.MhtmlGeneration.FinalSaveStatus",
+      static_cast<int>(MhtmlSaveStatus::FRAME_NO_LONGER_EXISTS), 1);
+}
+
+// Tests that a MHTML save operation does not fail when frames are removed if
+// the caller configures it to ignore missing frames.
+IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, SucceedIgnoringMissingIframe) {
+  ASSERT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL("/site_per_process_main.html")));
+
+  scoped_refptr<BrowserMessageFilter> filter = new ChildFrameRemoverFilter(
+      static_cast<WebContentsImpl*>(shell()->web_contents()));
+  shell()->web_contents()->GetRenderProcessHost()->AddFilter(filter.get());
+
+  base::FilePath path(temp_dir_.GetPath());
+  path = path.Append(FILE_PATH_LITERAL("test.mht"));
+
+  MHTMLGenerationParams params(path);
+
+  // Enables the ignoring of missing iframes.
+  params.ignore_missing_frames = true;
+
+  GenerateMHTMLForCurrentPage(params);
+
+  // Checks that no other test failure was detected so far.
+  ASSERT_FALSE(HasFailure());
+
+  // Verify the size reported by the callback is positive.
+  EXPECT_GT(file_size(), 0);
+
+  // Checks that the final status reported to UMA is correct.
+  histogram_tester()->ExpectUniqueSample(
+      "PageSerialization.MhtmlGeneration.FinalSaveStatus",
+      static_cast<int>(MhtmlSaveStatus::SUCCESS), 1);
+}
+
 class GenerateMHTMLAndExitRendererMessageFilter : public BrowserMessageFilter {
  public:
   GenerateMHTMLAndExitRendererMessageFilter(
@@ -254,6 +372,7 @@ class GenerateMHTMLAndExitRendererMessageFilter : public BrowserMessageFilter {
 
  private:
   bool OnMessageReceived(const IPC::Message& message) override {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
     if (message.type() == FrameHostMsg_SerializeAsMHTMLResponse::ID) {
       // After |return false| below, this IPC message will be handled by the
       // product code as illustrated below.  (1), (2), (3) depict points in time
@@ -317,6 +436,7 @@ class GenerateMHTMLAndExitRendererMessageFilter : public BrowserMessageFilter {
   };
 
   void TaskX() {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE, base::Bind(
             &GenerateMHTMLAndExitRendererMessageFilter::TaskY,
@@ -324,6 +444,7 @@ class GenerateMHTMLAndExitRendererMessageFilter : public BrowserMessageFilter {
   }
 
   void TaskY() {
+    DCHECK_CURRENTLY_ON(BrowserThread::FILE);
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE, base::Bind(
             &GenerateMHTMLAndExitRendererMessageFilter::TaskZ,
@@ -331,6 +452,7 @@ class GenerateMHTMLAndExitRendererMessageFilter : public BrowserMessageFilter {
   }
 
   void TaskZ() {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
     render_process_host_->FastShutdownIfPossible();
   }
 
@@ -341,7 +463,8 @@ class GenerateMHTMLAndExitRendererMessageFilter : public BrowserMessageFilter {
 
 // Regression test for the crash/race from https://crbug.com/612098.
 IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTMLAndExitRenderer) {
-  NavigateToURL(shell(), embedded_test_server()->GetURL("/simple_page.html"));
+  ASSERT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL("/simple_page.html")));
 
   RenderProcessHostImpl* render_process_host =
       static_cast<RenderProcessHostImpl*>(