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

content/browser/download/mhtml_generation_manager.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 "content/browser/download/mhtml_generation_manager.h"
 
 #include <map>
 #include <queue>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/files/file.h"
 #include "base/guid.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #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"
 #include "content/browser/bad_message.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/common/frame_messages.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
@@ skipping 37 matching lines @@
 
   // Indicates if more calls to SendToNextRenderFrame are needed.
   bool IsDone() const {
     bool waiting_for_response_from_renderer =
         frame_tree_node_id_of_busy_frame_ !=
         FrameTreeNode::kFrameTreeNodeInvalidId;
     bool no_more_requests_to_send = pending_frame_tree_node_ids_.empty();
     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,
                            base::TerminationStatus status,
                            int exit_code) override;
   void RenderProcessHostDestroyed(RenderProcessHost* host) override;
 
   void MarkAsFinished();
 
   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
   // |frame_tree_node_to_content_id_| map, but with the keys translated from
   // frame_tree_node_id into a |site_instance|-specific routing_id.
   std::map<int, std::string> CreateFrameRoutingIdToContentId(
       SiteInstance* site_instance);
 
   // Id used to map renderer responses to jobs.
   // See also MHTMLGenerationManager::id_to_job_ map.
@@ skipping 20 matching lines @@
   int frame_tree_node_id_of_busy_frame_;
 
   // The handle to the file the MHTML is saved to for the browser process.
   base::File browser_file_;
 
   // Map from frames to content ids (see WebFrameSerializer::generateMHTMLParts
   // for more details about what "content ids" are and how they are used).
   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_;
   std::string salt_;
 
   // The callback to call once generation is complete.
   const GenerateMHTMLCallback callback_;
 
   // Whether the job is finished (set to true only for the short duration of
   // time between MHTMLGenerationManager::JobFinished is called and the job is
@@ skipping 63 matching lines @@
 
   FrameMsg_SerializeAsMHTML_Params ipc_params;
   ipc_params.job_id = job_id_;
   ipc_params.mhtml_boundary_marker = mhtml_boundary_marker_;
   ipc_params.mhtml_binary_encoding = params_.use_binary_encoding;
   ipc_params.mhtml_cache_control_policy = params_.cache_control_policy;
   ipc_params.mhtml_popup_overlay_removal = params_.remove_popup_overlay;
 
   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.
     return MhtmlSaveStatus::FRAME_NO_LONGER_EXISTS;
   RenderFrameHost* rfh = ftn->current_frame_host();
 
   // Get notified if the target of the IPC message dies between responding.
   observed_renderer_process_host_.RemoveAll();
   observed_renderer_process_host_.Add(rfh->GetProcess());
 
@@ skipping 26 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   MHTMLGenerationManager::GetInstance()->RenderProcessExited(this);
 }
 
 void MHTMLGenerationManager::Job::MarkAsFinished() {
   DCHECK(!is_finished_);
   if (is_finished_)
     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);
 
   // End of job timing reports.
   if (!wait_on_renderer_start_time_.is_null()) {
     base::TimeDelta renderer_wait_time =
         base::TimeTicks::Now() - wait_on_renderer_start_time_;
     UMA_HISTOGRAM_TIMES(
         "PageSerialization.MhtmlGeneration.BrowserWaitForRendererTime."
         "SingleFrame",
         renderer_wait_time);
     all_renderers_wait_time_ += renderer_wait_time;
   }
   if (!all_renderers_wait_time_.is_zero()) {
     UMA_HISTOGRAM_TIMES(
         "PageSerialization.MhtmlGeneration.BrowserWaitForRendererTime."
         "FrameTree",
         all_renderers_wait_time_);
   }
   if (!all_renderers_main_thread_time_.is_zero()) {
     UMA_HISTOGRAM_TIMES(
         "PageSerialization.MhtmlGeneration.RendererMainThreadTime.FrameTree",
         all_renderers_main_thread_time_);
   }
   if (!longest_renderer_main_thread_time_.is_zero()) {
     UMA_HISTOGRAM_TIMES(
         "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(
     base::TimeDelta renderer_main_thread_time) {
   DCHECK(renderer_main_thread_time > base::TimeDelta());
   if (renderer_main_thread_time > base::TimeDelta())
     all_renderers_main_thread_time_ += renderer_main_thread_time;
   if (renderer_main_thread_time > longest_renderer_main_thread_time_)
     longest_renderer_main_thread_time_ = renderer_main_thread_time;
 }
 
 void MHTMLGenerationManager::Job::AddFrame(RenderFrameHost* render_frame_host) {
   auto* rfhi = static_cast<RenderFrameHostImpl*>(render_frame_host);
   int frame_tree_node_id = rfhi->frame_tree_node()->frame_tree_node_id();
   pending_frame_tree_node_ids_.push(frame_tree_node_id);
 
   std::string guid = base::GenerateGUID();
   std::string content_id = base::StringPrintf("<frame-%d-%s@mhtml.blink>",
                                               frame_tree_node_id, guid.c_str());
   frame_tree_node_to_content_id_[frame_tree_node_id] = content_id;
 }
 
 void MHTMLGenerationManager::Job::RenderProcessHostDestroyed(
     RenderProcessHost* host) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   observed_renderer_process_host_.Remove(host);
 }
 
 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);
 }
 
 bool MHTMLGenerationManager::Job::IsMessageFromFrameExpected(
     RenderFrameHostImpl* sender) {
   int sender_id = sender->frame_tree_node()->frame_tree_node_id();
   if (sender_id != frame_tree_node_id_of_busy_frame_)
     return false;
 
   // We only expect one message per frame - let's make sure subsequent messages
@@ skipping 24 matching lines @@
       digests_of_uris_of_serialized_resources.end());
 
   // Report success if all frames have been processed.
   if (pending_frame_tree_node_ids_.empty())
     return MhtmlSaveStatus::SUCCESS;
 
   return SendToNextRenderFrame();
 }
 
 // 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() {
   return base::Singleton<MHTMLGenerationManager>::get();
 }
 
 MHTMLGenerationManager::MHTMLGenerationManager() : next_job_id_(0) {}
 
 MHTMLGenerationManager::~MHTMLGenerationManager() {
 }
@@ skipping 30 matching lines @@
     NOTREACHED();
     ReceivedBadMessage(sender->GetProcess(),
                        bad_message::DWNLD_INVALID_SERIALIZE_AS_MHTML_RESPONSE);
     return;
   }
 
   TRACE_EVENT_NESTABLE_ASYNC_END0("page-serialization", "WaitingOnRenderer",
                                   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);
 }
 
 // static
 base::File MHTMLGenerationManager::CreateFile(const base::FilePath& file_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
   // SECURITY NOTE: A file descriptor to the file created below will be passed
   // to multiple renderer processes which (in out-of-process iframes mode) can
@@ skipping 29 matching lines @@
   MhtmlSaveStatus save_status = job->SendToNextRenderFrame();
   if (save_status != MhtmlSaveStatus::SUCCESS) {
     JobFinished(job, save_status);
   }
 }
 
 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);
   UMA_HISTOGRAM_TIMES("PageSerialization.MhtmlGeneration.FullPageSavingTime",
                       base::TimeTicks::Now() - job->creation_time());
   UMA_HISTOGRAM_ENUMERATION("PageSerialization.MhtmlGeneration.FinalSaveStatus",
                             static_cast<int>(save_status),
                             static_cast<int>(MhtmlSaveStatus::LAST));
   job->callback().Run(save_status == MhtmlSaveStatus::SUCCESS ? file_size : -1);
   id_to_job_.erase(job_id);
@@ skipping 21 matching lines @@
   return iter->second.get();
 }
 
 void MHTMLGenerationManager::RenderProcessExited(Job* job) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(job);
   JobFinished(job, MhtmlSaveStatus::RENDER_PROCESS_EXITED);
 }
 
 }  // namespace content