Avoid crashing if MHTMLGenerationManager::JobFinished is called twice.

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 60feca041a6491796bd906c67325edb11d6016e3..a5b14d8cc86cf0558bf5b735a35abfd44097d7f9 100644
--- a/content/browser/download/mhtml_generation_manager.cc
+++ b/content/browser/download/mhtml_generation_manager.cc
@@ -75,6 +75,9 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver {
                            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);
@@ -119,6 +122,11 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver {
   // 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_;
@@ -136,6 +144,7 @@ MHTMLGenerationManager::Job::Job(int job_id,
       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(
@@ -388,10 +397,16 @@ 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,