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..037499c4118321456ae32d34d224aa6da4a424db 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"
@@ -73,9 +74,12 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver {
     return !waiting_for_response_from_renderer && no_more_requests_to_send;
   }
 
-  // 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);
+  // Write the MHTML footer and close the file on the file thread and respond
+  // back on the UI thread with the updated status and file size (which will be
+  // negative in case of errors).
+  void CloseFile(
+      base::Callback<void(std::tuple<MhtmlSaveStatus, int64_t>)> callback,

[dcheng, 2017/03/09 01:56:51]

Pass by const ref. Also, I'm not a content owner, but I would prefer a struct
with explicitly named parameters rather than using a tuple.

[carlosk, 2017/03/09 22:03:35]

I did change to const ref. But these are just two integer values that I would
normally pass as copies if they were regular parameters.

And as this is just a class private exchange it seemed like an overkill to
create a strut just to be able to return two parameters at once. If this was
public or needed more than 2 values I would have switched to a struc (kind of my
rule-of-thumb).

[dcheng, 2017/03/09 23:16:58]

I would still prefer to just declare a simple local struct, for maintainability
in the future. It's more robust to code changes in the future accidentally
breaking things (granted, it's unlikely at the moment, since the parameters are
different types, but it's a low-cost thing to do)

+      MhtmlSaveStatus save_status);
 
   // RenderProcessHostObserver:
   void RenderProcessExited(RenderProcessHost* host,
@@ -88,7 +92,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.
+  //
+  // Note: The same |boundary| marker must be used for all "boundaries" -- in
+  // the header, parts and footer -- that belong to the same MHTML document (see
+  // also rfc1341, section 7.2.1, "boundary" description).
+  static std::tuple<MhtmlSaveStatus, int64_t> CloseFileOnFileThread(
+      MhtmlSaveStatus save_status,
+      std::string boundary,

[dcheng, 2017/03/09 01:56:51]

const std::string&

[carlosk, 2017/03/09 22:03:36]

In this specific case the copy was intended. Between the time this call is
scheduled and it is executed the Job itself can be destroyed what would create
an invalid reference. That's also why I go the extra mile to send an empty
string in case the boundary will not be used.

[dcheng, 2017/03/09 22:08:46]

The default callback binding is always by copy, even if the parameter is a const
ref parameter:
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Pass...
So you don't need to explicitly copy here, since it will be copied into
callback's storage.

(This is different from how some other callback systems work)

[carlosk, 2017/03/09 23:07:29]

Understood and done. Thanks for the pointer.

+      base::File file);
   void AddFrame(RenderFrameHost* render_frame_host);
 
   // Creates a new map with values (content ids) the same as in
@@ -129,7 +141,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 +225,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 +271,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();
+
   TRACE_EVENT_NESTABLE_ASYNC_INSTANT0("page-serialization", "JobFinished",
                                       this);
 
@@ -289,10 +305,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(
@@ -322,18 +334,27 @@ void MHTMLGenerationManager::Job::RenderProcessHostDestroyed(
 }
 
 void MHTMLGenerationManager::Job::CloseFile(
-    base::Callback<void(int64_t)> callback) {
+    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);
+    // Only update the status if that won't hide an earlier error.
+    if (save_status == MhtmlSaveStatus::SUCCESS)
+      save_status = MhtmlSaveStatus::FILE_WRITTING_ERROR;
+    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::CloseFileOnFileThread, save_status,
+          (save_status == MhtmlSaveStatus::SUCCESS ? mhtml_boundary_marker_
+                                                   : std::string()),
+          base::Passed(std::move(browser_file_))),

[dcheng, 2017/03/09 01:56:51]

Nit: base::Passed(&browser_file_)

[carlosk, 2017/03/09 22:03:35]

Done.

       callback);
 }
 
@@ -378,12 +399,34 @@ MhtmlSaveStatus MHTMLGenerationManager::Job::OnSerializeAsMHTMLResponse(
 }
 
 // static
-int64_t MHTMLGenerationManager::Job::CloseFileOnFileThread(base::File file) {
+std::tuple<MhtmlSaveStatus, int64_t>
+MHTMLGenerationManager::Job::CloseFileOnFileThread(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(
       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);