Move the writing of the MHTML footer to the browser process.

content/browser/download/mhtml_generation_manager.cc

Index: content/browser/download/mhtml_generation_manager.cc
diff --git a/content/browser/download/mhtml_generation_manager.cc b/content/browser/download/mhtml_generation_manager.cc
index 6bcea32cfa26142cb38e8e30f3695e83802fd1ce..50c70523ecd3240c137620746232821e52605f27 100644
--- a/content/browser/download/mhtml_generation_manager.cc
+++ b/content/browser/download/mhtml_generation_manager.cc
@@ -16,6 +16,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/scoped_observer.h"
 #include "base/stl_util.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
@@ -74,8 +75,10 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver {
   }
 
   // Close the file on the file thread and respond back on the UI thread with
-  // file size.
-  void CloseFile(base::Callback<void(int64_t file_size)> callback);
+  // status and file size.
+  void WriteFooterAndCloseFile(
+      base::Callback<void(std::tuple<MhtmlSaveStatus, int64_t>)> callback,
+      MhtmlSaveStatus save_status);
 
   // RenderProcessHostObserver:
   void RenderProcessExited(RenderProcessHost* host,
@@ -88,7 +91,15 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver {
   void ReportRendererMainThreadTime(base::TimeDelta renderer_main_thread_time);
 
  private:
-  static int64_t CloseFileOnFileThread(base::File file);
+  // Writes the MHTML footer to the file and closes it.
+  //
+  // The exact same boundary marker must to used for all "boundaries" -- header,

[nasko, 2017/03/08 19:29:25]

nit: "must be used"

[carlosk, 2017/03/08 21:45:54]

Fixed that here and the mirror occurrences of this comment in MHTMLArchiver.
Also slightly rephrased the comment.

+  // components and footer -- that belong to the same MHTML document (see also
+  // rfc1341, section 7.2.1, "boundary" description).
+  static std::tuple<MhtmlSaveStatus, int64_t>
+  WriteFooterAndCloseFileOnFileThread(MhtmlSaveStatus save_status,
+                                      std::string boundary,
+                                      base::File file);
   void AddFrame(RenderFrameHost* render_frame_host);
 
   // Creates a new map with values (content ids) the same as in
@@ -129,7 +140,7 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver {
   std::map<int, std::string> frame_tree_node_to_content_id_;
 
   // MIME multipart boundary to use in the MHTML doc.
-  std::string mhtml_boundary_marker_;
+  const std::string mhtml_boundary_marker_;
 
   // Digests of URIs of already generated MHTML parts.
   std::set<std::string> digests_of_already_serialized_uris_;
@@ -213,7 +224,6 @@ MhtmlSaveStatus MHTMLGenerationManager::Job::SendToNextRenderFrame() {
 
   int frame_tree_node_id = pending_frame_tree_node_ids_.front();
   pending_frame_tree_node_ids_.pop();
-  ipc_params.is_last_frame = pending_frame_tree_node_ids_.empty();
 
   FrameTreeNode* ftn = FrameTreeNode::GloballyFindByID(frame_tree_node_id);
   if (!ftn)  // The contents went away.
@@ -260,6 +270,11 @@ void MHTMLGenerationManager::Job::MarkAsFinished() {
     return;
 
   is_finished_ = true;
+
+  // Stopping RenderProcessExited notifications is needed to avoid calling
+  // JobFinished twice.  See also https://crbug.com/612098.
+  observed_renderer_process_host_.RemoveAll();

[nasko, 2017/03/08 19:29:25]

Why did this piece of code move?

[carlosk, 2017/03/08 21:45:54]

I moved it so to bundle together all the actual logic in this method
(non-metrics related). This (important) piece of code was less likely to be
noticed where it was before (it happened to me).

[nasko, 2017/03/08 23:19:31]

Keeping at least the TRACE macro towards the beginning of the method, before any
code has executed seems more consistent with the rest of the codebase though.

[carlosk, 2017/03/08 23:37:32]

I would agree with you for regular trace events. But this one is a) an instant
event and b) was created to mark the very end of the job. So having it after
that call is in fact ever so slightly more correct. :)

+
   TRACE_EVENT_NESTABLE_ASYNC_INSTANT0("page-serialization", "JobFinished",
                                       this);
 
@@ -289,10 +304,6 @@ void MHTMLGenerationManager::Job::MarkAsFinished() {
         "PageSerialization.MhtmlGeneration.RendererMainThreadTime.SlowestFrame",
         longest_renderer_main_thread_time_);
   }
-
-  // Stopping RenderProcessExited notifications is needed to avoid calling
-  // JobFinished twice.  See also https://crbug.com/612098.
-  observed_renderer_process_host_.RemoveAll();
 }
 
 void MHTMLGenerationManager::Job::ReportRendererMainThreadTime(
@@ -321,19 +332,28 @@ void MHTMLGenerationManager::Job::RenderProcessHostDestroyed(
   observed_renderer_process_host_.Remove(host);
 }
 
-void MHTMLGenerationManager::Job::CloseFile(
-    base::Callback<void(int64_t)> callback) {
+void MHTMLGenerationManager::Job::WriteFooterAndCloseFile(
+    base::Callback<void(std::tuple<MhtmlSaveStatus, int64_t>)> callback,
+    MhtmlSaveStatus save_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(!mhtml_boundary_marker_.empty());
 
   if (!browser_file_.IsValid()) {
-    callback.Run(-1);
+    if (save_status == MhtmlSaveStatus::SUCCESS)
+      save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR;

[nasko, 2017/03/08 19:29:25]

Without any comment, this looks very strange and wrong. If the file is invalid,
why is the status even checked?

[carlosk, 2017/03/08 21:45:54]

This is to not hide an earlier error in case one exists. I forgot to add a
comment to clarify that but it has now been added.

+    callback.Run(std::make_tuple(save_status, -1));
     return;
   }
 
+  // If no previous error occurred the boundary should be sent.
   BrowserThread::PostTaskAndReplyWithResult(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&MHTMLGenerationManager::Job::CloseFileOnFileThread,
-                 base::Passed(std::move(browser_file_))),
+      base::Bind(
+          &MHTMLGenerationManager::Job::WriteFooterAndCloseFileOnFileThread,

[nasko, 2017/03/08 19:29:25]

This name looks very long. Since marker is always written, why not keep it as a
close file naming? The new design is that writing the marker is part of closing
the file.

[carlosk, 2017/03/08 21:45:54]

Done. Reverted back to old naming and kept the updated comments.

+          save_status,
+          (save_status == MhtmlSaveStatus::SUCCESS ? mhtml_boundary_marker_
+                                                   : std::string()),
+          base::Passed(std::move(browser_file_))),
       callback);
 }
 
@@ -378,12 +398,35 @@ MhtmlSaveStatus MHTMLGenerationManager::Job::OnSerializeAsMHTMLResponse(
 }
 
 // static
-int64_t MHTMLGenerationManager::Job::CloseFileOnFileThread(base::File file) {
+std::tuple<MhtmlSaveStatus, int64_t>
+MHTMLGenerationManager::Job::WriteFooterAndCloseFileOnFileThread(
+    MhtmlSaveStatus save_status,
+    std::string boundary,
+    base::File file) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  DCHECK(file.IsValid());
-  int64_t file_size = file.GetLength();
-  file.Close();
-  return file_size;
+
+  // If no previous error occurred the boundary should have been provided.
+  if (save_status == MhtmlSaveStatus::SUCCESS) {
+    TRACE_EVENT0("page-serialization",
+                 "MHTMLGenerationManager::Job MHTML footer writing");
+    DCHECK(!boundary.empty());
+    std::string footer = base::StringPrintf("--%s--\r\n", boundary.c_str());
+    DCHECK(base::IsStringASCII(footer));
+    if (file.WriteAtCurrentPos(footer.data(), footer.size()) < 0)
+      save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR;
+  }
+
+  // If the file is still valid try to close it. Only update the status if that
+  // won't hide an earlier error.
+  int64_t file_size = -1;
+  if (file.IsValid()) {
+    file_size = file.GetLength();
+    file.Close();
+  } else if (save_status == MhtmlSaveStatus::SUCCESS) {
+    save_status = MhtmlSaveStatus::FILE_CLOSING_ERROR;
+  }
+
+  return std::make_tuple(save_status, file_size);
 }
 
 MHTMLGenerationManager* MHTMLGenerationManager::GetInstance() {
@@ -434,16 +477,20 @@ void MHTMLGenerationManager::OnSerializeAsMHTMLResponse(
                                   job);
   job->ReportRendererMainThreadTime(renderer_main_thread_time);
 
+  // If the renderer succeeded notify the Job and update the status.
   if (save_status == MhtmlSaveStatus::SUCCESS) {
     save_status = job->OnSerializeAsMHTMLResponse(
         digests_of_uris_of_serialized_resources);
   }
 
+  // If there was a failure (either from the renderer or from the job) then
+  // terminate the job and return.
   if (save_status != MhtmlSaveStatus::SUCCESS) {
     JobFinished(job, save_status);
     return;
   }
 
+  // Otherwise report completion if the job is done.
   if (job->IsDone())
     JobFinished(job, MhtmlSaveStatus::SUCCESS);
 }
@@ -493,24 +540,23 @@ void MHTMLGenerationManager::JobFinished(Job* job,
                                          MhtmlSaveStatus save_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(job);
-
   job->MarkAsFinished();
-  job->CloseFile(
+  job->WriteFooterAndCloseFile(
       base::Bind(&MHTMLGenerationManager::OnFileClosed,
                  base::Unretained(this),  // Safe b/c |this| is a singleton.
-                 job->id(), save_status));
+                 job->id()),
+      save_status);
 }
 
-void MHTMLGenerationManager::OnFileClosed(int job_id,
-                                          MhtmlSaveStatus save_status,
-                                          int64_t file_size) {
+void MHTMLGenerationManager::OnFileClosed(
+    int job_id,
+    std::tuple<MhtmlSaveStatus, int64_t> save_status_size) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  // Checks if an error happened while closing the file.
-  if (save_status == MhtmlSaveStatus::SUCCESS && file_size < 0)
-    save_status = MhtmlSaveStatus::FILE_CLOSING_ERROR;
+  MhtmlSaveStatus save_status = std::get<0>(save_status_size);
+  int64_t file_size = std::get<1>(save_status_size);
 
   Job* job = FindJob(job_id);
+  DCHECK(job);
   TRACE_EVENT_NESTABLE_ASYNC_END2(
       "page-serialization", "SavingMhtmlJob", job, "job save status",
       GetMhtmlSaveStatusLabel(save_status), "file size", file_size);