Move MHTML file writing out of the renderer main thread.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index b15731cc3614963ced8c7dd7a64cbe1dd3390b5f..97e9ca209b395fe6318ebee8b6a11171cc6595df 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/auto_reset.h"
+#include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/debug/alias.h"
 #include "base/debug/asan_invalid_access.h"
@@ -28,6 +29,7 @@
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task_runner_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
@@ -250,6 +252,8 @@
 #include "media/mojo/clients/mojo_decoder_factory.h"  // nogncheck
 #endif
 
+using base::Time;
+using base::TimeDelta;
 using blink::WebCachePolicy;
 using blink::WebContentDecryptionModule;
 using blink::WebContextMenuData;
@@ -293,6 +297,7 @@ using blink::WebServiceWorkerProvider;
 using blink::WebSettings;
 using blink::WebStorageQuotaCallbacks;
 using blink::WebString;
+using blink::WebThreadSafeData;
 using blink::WebURL;
 using blink::WebURLError;
 using blink::WebURLRequest;
@@ -300,8 +305,6 @@ using blink::WebURLResponse;
 using blink::WebUserGestureIndicator;
 using blink::WebVector;
 using blink::WebView;
-using base::Time;
-using base::TimeDelta;
 
 #if defined(OS_ANDROID)
 using blink::WebFloatPoint;
@@ -726,18 +729,17 @@ class LinkRewritingDelegate : public WebFrameSerializer::LinkRewritingDelegate {
 // 1. Bases shouldSkipResource and getContentID responses on contents of
 //    FrameMsg_SerializeAsMHTML_Params.
 // 2. Stores digests of urls of serialized resources (i.e. urls reported via
-//    shouldSkipResource) into |digests_of_uris_of_serialized_resources| passed
+//    shouldSkipResource) into |serialized_resources_uri_digests| passed
 //    to the constructor.
 class MHTMLPartsGenerationDelegate
     : public WebFrameSerializer::MHTMLPartsGenerationDelegate {
  public:
   MHTMLPartsGenerationDelegate(
       const FrameMsg_SerializeAsMHTML_Params& params,
-      std::set<std::string>* digests_of_uris_of_serialized_resources)
+      std::set<std::string>* serialized_resources_uri_digests)
       : params_(params),
-        digests_of_uris_of_serialized_resources_(
-            digests_of_uris_of_serialized_resources) {
-    DCHECK(digests_of_uris_of_serialized_resources_);
+        serialized_resources_uri_digests_(serialized_resources_uri_digests) {
+    DCHECK(serialized_resources_uri_digests_);
   }
 
   bool shouldSkipResource(const WebURL& url) override {
@@ -749,7 +751,7 @@ class MHTMLPartsGenerationDelegate
       return true;
 
     // Let's record |url| as being serialized for the *current* frame.
-    auto pair = digests_of_uris_of_serialized_resources_->insert(digest);
+    auto pair = serialized_resources_uri_digests_->insert(digest);
     bool insertion_took_place = pair.second;
     DCHECK(insertion_took_place);  // Blink should dedupe within a frame.
 
@@ -775,11 +777,41 @@ class MHTMLPartsGenerationDelegate
 
  private:
   const FrameMsg_SerializeAsMHTML_Params& params_;
-  std::set<std::string>* digests_of_uris_of_serialized_resources_;
+  std::set<std::string>* serialized_resources_uri_digests_;
 
   DISALLOW_COPY_AND_ASSIGN(MHTMLPartsGenerationDelegate);
 };
 
+// Writes to file the serialized and encoded MHTML data from WebThreadSafeData
+// instances.
+bool WriteMHTMLToDisk(bool success,

[nasko, 2016/10/05 18:56:54]

This should go in the anonymous namespace at the top of the file.

[carlosk, 2016/10/05 20:23:37]

This is already in that namespace, whose scope ends in line 873.

I anyway moved it down so to be right before the if-define scoped section
begins.

+                      std::vector<WebThreadSafeData> mhtml_contents,
+                      base::File file) {
+  TRACE_EVENT_BEGIN0("page-serialization",
+                     "WriteMHTMLToDisk (RenderFrameImpl)");

[nasko, 2016/10/05 18:56:54]

Since WriteMHTMLToDisk is already RenderFrameImpl internal, why do we need to
include that in the string? It will just add unneeded detail making trace files
bigger.

[carlosk, 2016/10/05 20:23:37]

IMO when I look at trace reports this kinds of thing helps me understand "where"
events happen. This is basically to cope with this function not being named
RenderFrameImpl::WriteMHTMLToDisk. WDYT?

As for the concern in regards of trace file size, this trace is infrequent
enough to be insignificant in that sense.

[nasko, 2016/10/05 20:41:10]

If it is about context, then let's just have it be the method
"RenderFrameImpl::WriteMHTMLToDisk" :). If someone renames the method, they can
change the string too.

[carlosk, 2016/10/05 21:18:01]

As explained offline traces are generally named after the method they trace and
as this is in an anonymous namespace that name wouldn't match and might cause
confusion. Leaving as is.

+  DCHECK(!RenderThread::Get()) << "Should not run in the main renderer thread";
+  if (success) {
+    SCOPED_UMA_HISTOGRAM_TIMER(
+        "PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame");
+    for (const WebThreadSafeData& data : mhtml_contents) {
+      if (!data.isEmpty() &&
+          file.WriteAtCurrentPos(data.data(), data.size()) < 0) {
+        success = false;
+        break;
+      }
+    }
+    // Explicitly close the file here to include any flush operations in the UMA
+    // metric. If this block is not executed close will be called upon |file|
+    // destruction at the end of this method's scope, which is still
+    // before any IPC is sent.
+    file.Close();
+  }
+
+  TRACE_EVENT_END1("page-serialization", "WriteMHTMLToDisk (RenderFrameImpl)",
+                   "success", success);
+  return success;
+}
+
 bool IsHttpPost(const blink::WebURLRequest& request) {
   return request.httpMethod().utf8() == "POST";
 }
@@ -5194,11 +5226,12 @@ void RenderFrameImpl::OnSerializeAsMHTML(
       WebString::fromUTF8(params.mhtml_boundary_marker);
   DCHECK(!mhtml_boundary.isEmpty());
 
-  // Three WebData instances for header, parts and footer.
-  WebData mhtml_contents[3];
-  std::set<std::string> digests_of_uris_of_serialized_resources;
-  MHTMLPartsGenerationDelegate delegate(
-      params, &digests_of_uris_of_serialized_resources);
+  // Holds WebThreadSafeData instances for some or all of header, contents and
+  // footer.
+  std::vector<WebThreadSafeData> mhtml_contents;
+  std::set<std::string> serialized_resources_uri_digests;
+  MHTMLPartsGenerationDelegate delegate(params,
+                                        &serialized_resources_uri_digests);
 
   bool success = true;
 
@@ -5209,9 +5242,9 @@ void RenderFrameImpl::OnSerializeAsMHTML(
     // |data| can be empty if the main frame should be skipped.  If the main
     // frame is skipped, then the whole archive is bad, so bail to the error
     // condition.
-    mhtml_contents[0] = WebFrameSerializer::generateMHTMLHeader(
-        mhtml_boundary, GetWebFrame(), &delegate);
-    success = !mhtml_contents[0].isEmpty();
+    mhtml_contents.emplace_back(WebFrameSerializer::generateMHTMLHeader(
+        mhtml_boundary, GetWebFrame(), &delegate));
+    success = !mhtml_contents.back().isEmpty();
   }
 
   // Generate MHTML parts.  Note that if this is not the main frame, then even
@@ -5221,42 +5254,48 @@ void RenderFrameImpl::OnSerializeAsMHTML(
     TRACE_EVENT0("page-serialization",
                  "RenderFrameImpl::OnSerializeAsMHTML parts serialization");
     // |data| can be empty if the frame should be skipped, but this is OK.
-    mhtml_contents[1] = WebFrameSerializer::generateMHTMLParts(
-        mhtml_boundary, GetWebFrame(), &delegate);
+    mhtml_contents.emplace_back(WebFrameSerializer::generateMHTMLParts(
+        mhtml_boundary, GetWebFrame(), &delegate));
   }
 
   // Generate MHTML footer if needed.
   if (success && params.is_last_frame) {
     TRACE_EVENT0("page-serialization",
                  "RenderFrameImpl::OnSerializeAsMHTML footer");
-    mhtml_contents[2] = WebFrameSerializer::generateMHTMLFooter(mhtml_boundary);
-  }
-
-  // Writes all serialized data to file.
-  // TODO(jcivelli): write the chunks in deferred tasks to give a chance to
-  //                 the message loop to process other events.
-  if (success) {
-    TRACE_EVENT0("page-serialization",
-                 "RenderFrameImpl::OnSerializeAsMHTML writing to file");
-    SCOPED_UMA_HISTOGRAM_TIMER(
-        "PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame");
-    for (const WebData& data : mhtml_contents) {
-      if (file.WriteAtCurrentPos(data.data(), data.size()) < 0) {
-        success = false;
-        break;
-      }
-    }
+    mhtml_contents.emplace_back(
+        WebFrameSerializer::generateMHTMLFooter(mhtml_boundary));
   }
 
-  // Cleanup and notify the browser process about completion.
-  file.Close();  // Need to flush file contents before sending IPC response.
+  // Note: we assume RenderFrameImpl::OnWriteMHTMLToDiskComplete and the rest of
+  // this function will be fast enough to not need to be accounted for in this
+  // metric.
   base::TimeDelta main_thread_use_time = base::TimeTicks::Now() - start_time;
-  Send(new FrameHostMsg_SerializeAsMHTMLResponse(
-      routing_id_, params.job_id, success,
-      digests_of_uris_of_serialized_resources, main_thread_use_time));
   UMA_HISTOGRAM_TIMES(
       "PageSerialization.MhtmlGeneration.RendererMainThreadTime.SingleFrame",
       main_thread_use_time);
+
+  base::PostTaskAndReplyWithResult(
+      RenderThreadImpl::current()->GetFileThreadTaskRunner().get(), FROM_HERE,
+      base::Bind(&WriteMHTMLToDisk, success, base::Passed(&mhtml_contents),
+                 base::Passed(&file)),
+      base::Bind(&RenderFrameImpl::OnWriteMHTMLToDiskComplete,
+                 weak_factory_.GetWeakPtr(), params.job_id,
+                 base::Passed(&serialized_resources_uri_digests),
+                 main_thread_use_time));
+}
+
+void RenderFrameImpl::OnWriteMHTMLToDiskComplete(

[nasko, 2016/10/05 18:56:54]

This should be ordered the same way it is ordered in the header file.
Alternatively, move the declaration in the header.

[carlosk, 2016/10/05 20:23:37]

Done, 2nd option.

I was split between following the same order from the header and keeping things
in context in the .cpp. In this case it seemed better to follow the latter.

From your comment I understand it is OK to mix a method that is not directly and
IPC handler into the "IPC message handlers" section. Am I understanding
correctly?

[nasko, 2016/10/05 20:41:10]

Let's not put non-IPC methods in the middle of IPC handlers.

[carlosk, 2016/10/05 21:18:00]

Done.

+    int job_id,
+    std::set<std::string> serialized_resources_uri_digests,
+    base::TimeDelta main_thread_use_time,
+    bool success) {
+  DCHECK(RenderThread::Get()) << "Must run in the main renderer thread";
+  // Notify the browser process about completion.
+  // Note: this method must be short enough to not need to be accounted for in

[nasko, 2016/10/05 18:56:54]

nit: "must be short enough" implies something that needs to be ensured by code
touching this method. There is nothing one can do here for Send(). Probably
reword it to say that Send is likely small enough to not need accounting. I like
your other comment that has "assume", which was more clear.

[carlosk, 2016/10/05 20:23:37]

Done.

+  // PageSerialization.MhtmlGeneration.RendererMainThreadTime.SingleFrame.
+  Send(new FrameHostMsg_SerializeAsMHTMLResponse(
+      routing_id_, job_id, success, serialized_resources_uri_digests,
+      main_thread_use_time));
 }
 
 void RenderFrameImpl::OnFind(int request_id,