Fail when saving page as MHTML provides information about the cause.

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 0be80421848d9e92fbf576b7268370ac301841d6..923d2695f372e823c31a2ad72cf56f51bf421331 100644
--- a/content/browser/download/mhtml_generation_manager.cc
+++ b/content/browser/download/mhtml_generation_manager.cc
@@ -56,14 +56,13 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver {
 
   // Handler for FrameHostMsg_SerializeAsMHTMLResponse (a notification from the
   // renderer that the MHTML generation for previous frame has finished).
-  // Returns |true| upon success; |false| otherwise.
-  bool OnSerializeAsMHTMLResponse(
+  // Returns MhtmlSaveStatus::SUCCESS or a specific error status.
+  MhtmlSaveStatus OnSerializeAsMHTMLResponse(
       const std::set<std::string>& digests_of_uris_of_serialized_resources);
 
   // Sends IPC to the renderer, asking for MHTML generation of the next frame.
-  //
-  // Returns true if the message was sent successfully; false otherwise.
-  bool SendToNextRenderFrame();
+  // Returns MhtmlSaveStatus::SUCCESS or a specific error status.
+  MhtmlSaveStatus SendToNextRenderFrame();
 
   // Indicates if more calls to SendToNextRenderFrame are needed.
   bool IsDone() const {
@@ -201,7 +200,7 @@ MHTMLGenerationManager::Job::CreateFrameRoutingIdToContentId(
   return result;
 }
 
-bool MHTMLGenerationManager::Job::SendToNextRenderFrame() {
+MhtmlSaveStatus MHTMLGenerationManager::Job::SendToNextRenderFrame() {
   DCHECK(browser_file_.IsValid());
   DCHECK(!pending_frame_tree_node_ids_.empty());
 
@@ -217,7 +216,7 @@ bool MHTMLGenerationManager::Job::SendToNextRenderFrame() {
 
   FrameTreeNode* ftn = FrameTreeNode::GloballyFindByID(frame_tree_node_id);
   if (!ftn)  // The contents went away.
-    return false;
+    return MhtmlSaveStatus::FRAME_NO_LONGER_EXIST;
   RenderFrameHost* rfh = ftn->current_frame_host();
 
   // Get notified if the target of the IPC message dies between responding.
@@ -243,7 +242,7 @@ bool MHTMLGenerationManager::Job::SendToNextRenderFrame() {
                                     frame_tree_node_id);
   DCHECK(wait_on_renderer_start_time_.is_null());
   wait_on_renderer_start_time_ = base::TimeTicks::Now();
-  return true;
+  return MhtmlSaveStatus::SUCCESS;
 }
 
 void MHTMLGenerationManager::Job::RenderProcessExited(
@@ -350,7 +349,7 @@ bool MHTMLGenerationManager::Job::IsMessageFromFrameExpected(
   return true;
 }
 
-bool MHTMLGenerationManager::Job::OnSerializeAsMHTMLResponse(
+MhtmlSaveStatus MHTMLGenerationManager::Job::OnSerializeAsMHTMLResponse(
     const std::set<std::string>& digests_of_uris_of_serialized_resources) {
   DCHECK(!wait_on_renderer_start_time_.is_null());
   base::TimeDelta renderer_wait_time =
@@ -370,8 +369,9 @@ bool MHTMLGenerationManager::Job::OnSerializeAsMHTMLResponse(
       digests_of_uris_of_serialized_resources.begin(),
       digests_of_uris_of_serialized_resources.end());
 
+  // Report success if all frames have been processed.
   if (pending_frame_tree_node_ids_.empty())
-    return true;  // Report success - all frames have been processed.
+    return MhtmlSaveStatus::SUCCESS;
 
   return SendToNextRenderFrame();
 }
@@ -416,7 +416,7 @@ void MHTMLGenerationManager::SaveMHTML(WebContents* web_contents,
 void MHTMLGenerationManager::OnSerializeAsMHTMLResponse(
     RenderFrameHostImpl* sender,
     int job_id,
-    bool mhtml_generation_in_renderer_succeeded,
+    MhtmlSaveStatus save_status,
     const std::set<std::string>& digests_of_uris_of_serialized_resources,
     base::TimeDelta renderer_main_thread_time) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -433,19 +433,18 @@ void MHTMLGenerationManager::OnSerializeAsMHTMLResponse(
                                   job);
   job->ReportRendererMainThreadTime(renderer_main_thread_time);
 
-  if (!mhtml_generation_in_renderer_succeeded) {
-    JobFinished(job, JobStatus::FAILURE);
-    return;
+  if (save_status == MhtmlSaveStatus::SUCCESS) {
+    save_status = job->OnSerializeAsMHTMLResponse(
+        digests_of_uris_of_serialized_resources);
   }
 
-  if (!job->OnSerializeAsMHTMLResponse(
-          digests_of_uris_of_serialized_resources)) {
-    JobFinished(job, JobStatus::FAILURE);
+  if (save_status != MhtmlSaveStatus::SUCCESS) {
+    JobFinished(job, save_status);
     return;
   }
 
   if (job->IsDone())
-    JobFinished(job, JobStatus::SUCCESS);
+    JobFinished(job, MhtmlSaveStatus::SUCCESS);
 }
 
 // static
@@ -477,18 +476,20 @@ void MHTMLGenerationManager::OnFileAvailable(int job_id,
 
   if (!browser_file.IsValid()) {
     LOG(ERROR) << "Failed to create file";
-    JobFinished(job, JobStatus::FAILURE);
+    JobFinished(job, MhtmlSaveStatus::FILE_CREATION_ERROR);
     return;
   }
 
   job->set_browser_file(std::move(browser_file));
 
-  if (!job->SendToNextRenderFrame()) {
-    JobFinished(job, JobStatus::FAILURE);
+  MhtmlSaveStatus save_status = job->SendToNextRenderFrame();
+  if (save_status != MhtmlSaveStatus::SUCCESS) {
+    JobFinished(job, save_status);
   }
 }
 
-void MHTMLGenerationManager::JobFinished(Job* job, JobStatus job_status) {
+void MHTMLGenerationManager::JobFinished(Job* job,
+                                         MhtmlSaveStatus save_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(job);
 
@@ -496,22 +497,31 @@ void MHTMLGenerationManager::JobFinished(Job* job, JobStatus job_status) {
   job->CloseFile(
       base::Bind(&MHTMLGenerationManager::OnFileClosed,
                  base::Unretained(this),  // Safe b/c |this| is a singleton.
-                 job->id(), job_status));
+                 job->id(), save_status));
 }
 
 void MHTMLGenerationManager::OnFileClosed(int job_id,
-                                          JobStatus job_status,
+                                          MhtmlSaveStatus save_status,
                                           int64_t file_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;

[Łukasz Anforowicz, 2016/11/22 19:22:13]

Interesting, I didn't know that base::File::GetLength can return a negative
value to indicate a failure.

[carlosk, 2016/11/22 23:26:23]

It can't. But that's the "error code" response when CloseFile invokes this
method as a callback. See MHTMLGenerationManager::Job::CloseFile.

+
   Job* job = FindJob(job_id);
   TRACE_EVENT_NESTABLE_ASYNC_END2(
-      "page-serialization", "SavingMhtmlJob", job, "job result",
-      job_status == JobStatus::SUCCESS ? "success" : "failure", "file size",
-      file_size);
+      "page-serialization", "SavingMhtmlJob", job, "job save status",
+      save_status == MhtmlSaveStatus::SUCCESS
+          ? "success"
+          : base::StringPrintf("failure (%d)", static_cast<int>(save_status)),
+      "file size", file_size);
   UMA_HISTOGRAM_TIMES("PageSerialization.MhtmlGeneration.FullPageSavingTime",
                       base::TimeTicks::Now() - job->creation_time());
-  job->callback().Run(job_status == JobStatus::SUCCESS ? file_size : -1);
+  UMA_HISTOGRAM_ENUMERATION("PageSerialization.MhtmlGeneration.FinalSaveStatus",
+                            static_cast<int>(save_status),
+                            static_cast<int>(MhtmlSaveStatus::STATUS_COUNT));
+  job->callback().Run(save_status == MhtmlSaveStatus::SUCCESS ? file_size : -1);
   id_to_job_.erase(job_id);
 }
 
@@ -540,7 +550,7 @@ MHTMLGenerationManager::Job* MHTMLGenerationManager::FindJob(int job_id) {
 void MHTMLGenerationManager::RenderProcessExited(Job* job) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(job);
-  JobFinished(job, JobStatus::FAILURE);
+  JobFinished(job, MhtmlSaveStatus::RENDER_PROCESS_EXITED);
 }
 
 }  // namespace content