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..7396bec0f86181c3936dd53040a44b0220bc7a7d 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"
@@ -222,6 +223,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 +245,116 @@ 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) {}

[Łukasz Anforowicz, 2016/11/28 22:15:37]

nit: DCHECK(web_contents_) ?

[carlosk, 2016/11/29 02:11:20]

Done.

+
+ protected:
+  ~ChildFrameRemoverFilter() override {}
+
+ private:
+  bool OnMessageReceived(const IPC::Message& message) override {

[Łukasz Anforowicz, 2016/11/28 22:15:37]

nit: DCHECK_CURRENTLY_ON(BrowserThread::IO) ?

[carlosk, 2016/11/29 02:11:20]

Done.

+    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() {

[Łukasz Anforowicz, 2016/11/28 22:15:37]

nit: DCHECK_CURRENTLY_ON(BrowserThread::UI) ?

(this is not very important, but I think it does help with readability - with
the DCHECK it is immediately obvious which method is called on which thread
[without the DCHECK it is also pretty straightforward but requires extra
thinking]).

[carlosk, 2016/11/29 02:11:20]

Agreed and done. Also added these checks to the other filter class.

+    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(root->child_count() - 1));

[Łukasz Anforowicz, 2016/11/28 22:15:37]

Is it okay to instead call root->RemoveChild(0)?  This would simplify the code a
little bit, while also removing all child frames (although in a different,
reversed order).  If the order of removal is important, then can you please add
a comment saying this + explaining why this is important?

[carlosk, 2016/11/29 02:11:20]

This was just my ODD of always removing vector elements from its end. :)

But done anyway (with just a tiny bit of pain).

[Łukasz Anforowicz, 2016/11/29 19:18:13]

Thanks.

I see now that the simpler code might be O(N^2) because it needs to shift the
remaining elements with each removal.  Hmmm... maybe this is fine (test code
with only a few elements) but I think I now understand why you initially did
this in a different way.

+    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) {
+  NavigateToURL(shell(),

[Łukasz Anforowicz, 2016/11/28 22:15:37]

nit: ASSERT_TRUE(NavigateToURL(...)

(this will also make alexmos@ happy I think - see
https://chromium.googlesource.com/chromium/src/+/4df0d565e3cd9258f0a5fc4517bf...
:-)

[carlosk, 2016/11/29 02:11:20]

Done for all calls of this function.

+                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) {
+  NavigateToURL(shell(),

[Łukasz Anforowicz, 2016/11/28 22:15:37]

nit: ASSERT_TRUE(NavigateToURL(...)

[carlosk, 2016/11/29 02:11:20]

Done.

+                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(