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

content/browser/download/mhtml_generation_browsertest.cc

 // Copyright (c) 2012 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 <stdint.h>
 #include <memory>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
 #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"
 #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"
@@ skipping 183 matching lines @@
 
 // Tests that generating a MHTML does create contents.
 // Note that the actual content of the file is not tested, the purpose of this
 // test is to ensure we were successful in creating the MHTML data from the
 // renderer.
 IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTML) {
   base::FilePath path(temp_dir_.GetPath());
   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.
   EXPECT_GT(file_size(), 0);  // Verify the size reported by the callback.
   EXPECT_GT(ReadFileSizeFromDisk(path), 100);  // Verify the actual file size.
 
   {
     base::ThreadRestrictions::ScopedAllowIO allow_io_for_content_verification;
     std::string mhtml;
     ASSERT_TRUE(base::ReadFileToString(path, &mhtml));
     EXPECT_THAT(mhtml,
                 HasSubstr("Content-Transfer-Encoding: quoted-printable"));
   }
 
   // Checks that the final status reported to UMA is correct.
   histogram_tester()->ExpectUniqueSample(
       "PageSerialization.MhtmlGeneration.FinalSaveStatus",
       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(
       RenderProcessHostImpl* render_process_host)
       : BrowserMessageFilter(FrameMsgStart),
         render_process_host_(render_process_host) {}
 
  protected:
   ~GenerateMHTMLAndExitRendererMessageFilter() override {}
 
@@ skipping 389 matching lines @@
 
   // Make sure that URLs of both frames are present
   // (note that these are single-line regexes).
   EXPECT_THAT(
       mhtml,
       ContainsRegex("Content-Location:.*/frame_tree/page_with_one_frame.html"));
   EXPECT_THAT(mhtml, ContainsRegex("Content-Location:.*/title1.html"));
 }
 
 }  // namespace content