Avoid crashing if MHTMLGenerationManager::JobFinished is called twice.

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>
 
@@ skipping 57 matching lines @@
   // 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);
 
   // RenderProcessHostObserver:
   void RenderProcessExited(RenderProcessHost* host,
                            base::TerminationStatus status,
                            int exit_code) override;
   void RenderProcessHostDestroyed(RenderProcessHost* host) override;
 
+  bool is_finished() const { return is_finished_; }
+  void MarkAsFinished() { is_finished_ = true; }
+
  private:
   static int64_t CloseFileOnFileThread(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);
 
@@ skipping 24 matching lines @@
   // MIME multipart boundary to use in the MHTML doc.
   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
+  // destroyed by MHTMLGenerationManager::OnFileClosed).
+  bool is_finished_;
+
   // RAII helper for registering this Job as a RenderProcessHost observer.
   ScopedObserver<RenderProcessHost, MHTMLGenerationManager::Job>
       observed_renderer_process_host_;
 
   DISALLOW_COPY_AND_ASSIGN(Job);
 };
 
 MHTMLGenerationManager::Job::Job(int job_id,
                                  WebContents* web_contents,
                                  const MHTMLGenerationParams& params,
                                  const GenerateMHTMLCallback& callback)
     : job_id_(job_id),
       params_(params),
       frame_tree_node_id_of_busy_frame_(FrameTreeNode::kFrameTreeNodeInvalidId),
       mhtml_boundary_marker_(net::GenerateMimeMultipartBoundary()),
       salt_(base::GenerateGUID()),
       callback_(callback),
+      is_finished_(false),
       observed_renderer_process_host_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   web_contents->ForEachFrame(base::Bind(
       &MHTMLGenerationManager::Job::AddFrame,
       base::Unretained(this)));  // Safe because ForEachFrame is synchronous.
 
   // Main frame needs to be processed first.
   DCHECK(!pending_frame_tree_node_ids_.empty());
   DCHECK(FrameTreeNode::GloballyFindByID(pending_frame_tree_node_ids_.front())
              ->parent() == nullptr);
@@ skipping 232 matching lines @@
 
   if (!job->SendToNextRenderFrame()) {
     JobFinished(job, JobStatus::FAILURE);
   }
 }
 
 void MHTMLGenerationManager::JobFinished(Job* job, JobStatus job_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(job);
 
-  job->CloseFile(
-      base::Bind(&MHTMLGenerationManager::OnFileClosed,
-                 base::Unretained(this),  // Safe b/c |this| is a singleton.
-                 job->id(), job_status));
+  // Make sure we gracefully handle the case when JobFinished is called twice
+  // (i.e. once from OnSerializeAsMHTMLResponse and while the file is being
+  // closed also called a second time from RenderProcessExited).
+  if (!job->is_finished()) {  // Is this the first time JobFinished is called?
+    job->MarkAsFinished();

[dewittj, 2016/07/20 16:37:48]

May want to clear |job|'s |observed_renderer_process_host_| as well, to prevent
more spurious callbacks.

[Randy Smith (Not in Mondays), 2016/07/20 16:46:58]

This is a good point, but wouldn't this solve this problem in a different
(possibly cleaner) way?  If you stop observing things when the job is signaled
as finished on the UI thread, you remove the possibility of getting further
spurious notifications and keep the flow simple.  

(I have no objection to adding the is_finished_ flag and using it in DCHECKs or
CHECKs to enforce the behavior, but I'm pulled towards a model that naturally
guarantees a single call to JobFinished because of the state machine rather than
one that handles idempotency.)

[Łukasz Anforowicz, 2016/07/20 21:10:07]

Done.  Good point.
Interesting.  This requires ensuring that there is no other way to call
JobFinished twice.  Before this CL a malicious-or-buggy renderer could send 2
SerializeAsMHTMLResponse(mhtml_generation_in_renderer_succeeded=false) quickly
one after the other, and cause similar crash (with no involvement from
RenderProcessExited).

Let me ensure that |frame_tree_node_id_of_busy_frame_|-related verification is
done also in |mhtml_generation_in_renderer_succeeded=false| case.  I believe
this should mean that JobFinished is only called once per job.

Done?

[Randy Smith (Not in Mondays), 2016/07/20 22:47:49]

Sure, that sounds good.  Thanks for thinking the issue through further than I
had.  But that sounds exactly right--there's one place the message can come from
in the success path (the single frame with the message outstanding to it) one it
can come from in the error path, and each path guarantees that the other won't
be able to send a message following.  

+    job->CloseFile(
+        base::Bind(&MHTMLGenerationManager::OnFileClosed,
+                   base::Unretained(this),  // Safe b/c |this| is a singleton.
+                   job->id(), job_status));
+  }
 }
 
 void MHTMLGenerationManager::OnFileClosed(int job_id,
                                           JobStatus job_status,
                                           int64_t file_size) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   Job* job = FindJob(job_id);
   job->callback().Run(job_status == JobStatus::SUCCESS ? file_size : -1);
   id_to_job_.erase(job_id);
@@ skipping 21 matching lines @@
   return iter->second;
 }
 
 void MHTMLGenerationManager::RenderProcessExited(Job* job) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(job);
   JobFinished(job, JobStatus::FAILURE);
 }
 
 }  // namespace content