Delete temporarily dir for PDF to EMF conversion after EMF files closed.

printing::Emf keeps files locked so base::ScopedTempDir can't delete them and leaks files.

Failures or crashes still may leak files. I ignore them to have change simple enough for merging into beta.

BUG=411681
NOTRY=true
R=scottmg@chromium.org, thestig@chromium.org

Review URL: https://codereview.chromium.org/547203002

Cr-Commit-Position: refs/heads/master@{#293848}

6 files changed, 88 insertions, 32 deletions

diff --git a/chrome/browser/printing/pdf_to_emf_converter.cc b/chrome/browser/printing/pdf_to_emf_converter.cc
index df45a92..ad99e3a 100644
--- a/chrome/browser/printing/pdf_to_emf_converter.cc
+++ b/chrome/browser/printing/pdf_to_emf_converter.cc
@@ -16,6 +16,7 @@
 #include "content/public/browser/child_process_data.h"
 #include "content/public/browser/utility_process_host.h"
 #include "content/public/browser/utility_process_host_client.h"
+#include "printing/emf_win.h"
 #include "printing/page_range.h"
 #include "printing/pdf_render_settings.h"
 
@@ -25,14 +26,12 @@ namespace {
 
 using content::BrowserThread;
 
-class FileHandlers {
+class FileHandlers
+    : public base::RefCountedThreadSafe<FileHandlers,
+                                        BrowserThread::DeleteOnFileThread> {
  public:
   FileHandlers() {}
 
-  ~FileHandlers() {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  }
-
   void Init(base::RefCountedMemory* data);
   bool IsValid();
 
@@ -40,6 +39,11 @@ class FileHandlers {
     return temp_dir_.path().AppendASCII("output.emf");
   }
 
+  base::FilePath GetEmfPagePath(int page_number) const {
+    return GetEmfPath().InsertBeforeExtensionASCII(
+        base::StringPrintf(".%d", page_number));
+  }
+
   base::FilePath GetPdfPath() const {
     return temp_dir_.path().AppendASCII("input.pdf");
   }
@@ -56,6 +60,11 @@ class FileHandlers {
   }
 
  private:
+  friend struct BrowserThread::DeleteOnThread<BrowserThread::FILE>;
+  friend class base::DeleteHelper<FileHandlers>;
+
+  ~FileHandlers() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); }
+
   base::ScopedTempDir temp_dir_;
   base::File pdf_file_;
 };
@@ -67,20 +76,51 @@ void FileHandlers::Init(base::RefCountedMemory* data) {
     return;
   }
 
+  pdf_file_.Initialize(GetPdfPath(),
+                       base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE |
+                           base::File::FLAG_READ |
+                           base::File::FLAG_DELETE_ON_CLOSE);
   if (static_cast<int>(data->size()) !=
-      base::WriteFile(GetPdfPath(), data->front_as<char>(), data->size())) {
+      pdf_file_.WriteAtCurrentPos(data->front_as<char>(), data->size())) {
+    pdf_file_.Close();
     return;
   }
-
-  // Reopen in read only mode.
-  pdf_file_.Initialize(GetPdfPath(),
-                       base::File::FLAG_OPEN | base::File::FLAG_READ);
+  pdf_file_.Seek(base::File::FROM_BEGIN, 0);
+  pdf_file_.Flush();
 }
 
 bool FileHandlers::IsValid() {
   return pdf_file_.IsValid();
 }
 
+// Modification of Emf to keep references to |FileHandlers|.
+// |FileHandlers| must be deleted after the last metafile is closed because
+// Emf holds files locked.
+// Ideally we want to use FLAG_DELETE_ON_CLOSE, but it requires large changes.
+// It's going to be done for crbug.com/408184
+class TempEmf : public Emf {
+ public:
+  explicit TempEmf(const scoped_refptr<FileHandlers>& files) : files_(files) {}
+  virtual ~TempEmf() {}
+
+  virtual bool SafePlayback(HDC hdc) const OVERRIDE {
+    bool result = Emf::SafePlayback(hdc);
+    TempEmf* this_mutable = const_cast<TempEmf*>(this);
+    // TODO(vitalybuka): Fix destruction of metafiles. For some reasons
+    // instances of Emf are not deleted. crbug.com/411683
+    // |files_| must be released as soon as possible to guarantee deletion.
+    // It's know that this Emf file is going to be played just once to
+    // a printer. So just release files here.
+    this_mutable->Close();
+    this_mutable->files_ = NULL;
+    return result;
+  }
+
+ private:
+  scoped_refptr<FileHandlers> files_;
+  DISALLOW_COPY_AND_ASSIGN(TempEmf);
+};
+
 // Converts PDF into EMF.
 // Class uses 3 threads: UI, IO and FILE.
 // Internal workflow is following:
@@ -125,7 +165,7 @@ class PdfToEmfUtilityProcessHostClient
       double scale_factor);
   void OnFilesReadyOnUIThread();
 
-  scoped_ptr<FileHandlers, BrowserThread::DeleteOnFileThread> files_;
+  scoped_refptr<FileHandlers> files_;
   printing::PdfRenderSettings settings_;
   PdfToEmfConverter::ResultCallback callback_;
   base::WeakPtr<content::UtilityProcessHost> utility_process_host_;
@@ -145,14 +185,12 @@ void PdfToEmfUtilityProcessHostClient::Convert(
     const PdfToEmfConverter::ResultCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   callback_ = callback;
-  CHECK(!files_);
-  files_.reset(new FileHandlers());
+  CHECK(!files_.get());
+  files_ = new FileHandlers();
   BrowserThread::PostTaskAndReply(
       BrowserThread::FILE,
       FROM_HERE,
-      base::Bind(&FileHandlers::Init,
-                 base::Unretained(files_.get()),
-                 make_scoped_refptr(data)),
+      base::Bind(&FileHandlers::Init, files_, make_scoped_refptr(data)),
       base::Bind(&PdfToEmfUtilityProcessHostClient::OnFilesReadyOnUIThread,
                  this));
 }
@@ -246,19 +284,21 @@ void PdfToEmfUtilityProcessHostClient::RunCallbackOnUIThread(
     const std::vector<printing::PageRange>& page_ranges,
     double scale_factor) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  std::vector<base::FilePath> page_filenames;
+  ScopedVector<Metafile> pages;
   std::vector<printing::PageRange>::const_iterator iter;
   for (iter = page_ranges.begin(); iter != page_ranges.end(); ++iter) {
     for (int page_number = iter->from; page_number <= iter->to; ++page_number) {
-      page_filenames.push_back(files_->GetEmfPath().InsertBeforeExtensionASCII(
-          base::StringPrintf(".%d", page_number)));
+      scoped_ptr<TempEmf> metafile(new TempEmf(files_));
+      if (!metafile->InitFromFile(files_->GetEmfPagePath(page_number))) {
+        NOTREACHED() << "Invalid metafile";
+        metafile.reset();
+      }
+      pages.push_back(metafile.release());
     }
   }
+  files_ = NULL;
   if (!callback_.is_null()) {
-    BrowserThread::PostTask(
-        BrowserThread::UI,
-        FROM_HERE,
-        base::Bind(callback_, scale_factor, page_filenames));
+    callback_.Run(scale_factor, &pages);
     callback_.Reset();
   }
 }
diff --git a/chrome/browser/printing/pdf_to_emf_converter.h b/chrome/browser/printing/pdf_to_emf_converter.h
index 86afb05..0ad5a1a 100644
--- a/chrome/browser/printing/pdf_to_emf_converter.h
+++ b/chrome/browser/printing/pdf_to_emf_converter.h
@@ -7,6 +7,7 @@
 
 #include "base/callback.h"
 #include "base/memory/ref_counted_memory.h"
+#include "base/memory/scoped_vector.h"
 
 namespace base {
 class FilePath;
@@ -18,11 +19,14 @@ class PdfRenderSettings;
 
 namespace printing {
 
+class Metafile;
+
 class PdfToEmfConverter {
  public:
   // Callback for when the PDF is converted to an EMF.
-  typedef base::Callback<void(double /*scale_factor*/,
-                              const std::vector<base::FilePath>& /*emf_files*/)>
+  // Takes ownership of metafiles.
+  typedef base::Callback<
+      void(double /*scale_factor*/, ScopedVector<Metafile>* /*emf_files*/)>
       ResultCallback;
   virtual ~PdfToEmfConverter() {}
 
diff --git a/chrome/browser/printing/print_view_manager_base.cc b/chrome/browser/printing/print_view_manager_base.cc
index 26de64b..45f34df 100644
--- a/chrome/browser/printing/print_view_manager_base.cc
+++ b/chrome/browser/printing/print_view_manager_base.cc
@@ -130,7 +130,7 @@ void PrintViewManagerBase::OnDidGetDocumentCookie(int cookie) {
 void PrintViewManagerBase::OnPdfToEmfConverted(
     const PrintHostMsg_DidPrintPage_Params& params,
     double scale_factor,
-    const std::vector<base::FilePath>& emf_files) {
+    ScopedVector<Metafile>* emf_files) {
   if (!print_job_.get())
     return;
 
@@ -138,20 +138,23 @@ void PrintViewManagerBase::OnPdfToEmfConverted(
   if (!document)
     return;
 
-  for (size_t i = 0; i < emf_files.size(); ++i) {
-    scoped_ptr<printing::Emf> metafile(new printing::Emf);
-    if (!metafile->InitFromFile(emf_files[i])) {
-      NOTREACHED() << "Invalid metafile";
+  for (size_t i = 0; i < emf_files->size(); ++i) {
+    if (!(*emf_files)[i]) {
       web_contents()->Stop();
       return;
     }
+  }
+
+  for (size_t i = 0; i < emf_files->size(); ++i) {
     // Update the rendered document. It will send notifications to the listener.
     document->SetPage(i,
-                      metafile.release(),
+                      (*emf_files)[i],
                       scale_factor,
                       params.page_size,
                       params.content_area);
   }
+  // document->SetPage took ownership of all EMFs.
+  emf_files->weak_clear();
 
   ShouldQuitFromInnerMessageLoop();
 }
diff --git a/chrome/browser/printing/print_view_manager_base.h b/chrome/browser/printing/print_view_manager_base.h
index 1c84f2b..0a8c5e5 100644
--- a/chrome/browser/printing/print_view_manager_base.h
+++ b/chrome/browser/printing/print_view_manager_base.h
@@ -23,6 +23,7 @@ class RenderViewHost;
 namespace printing {
 
 class JobEventDetails;
+class Metafile;
 class PdfToEmfConverter;
 class PrintJob;
 class PrintJobWorkerOwner;
@@ -137,7 +138,7 @@ class PrintViewManagerBase : public content::NotificationObserver,
   // Called on completion of converting the pdf to emf.
   void OnPdfToEmfConverted(const PrintHostMsg_DidPrintPage_Params& params,
                            double scale_factor,
-                           const std::vector<base::FilePath>& emf_file);
+                           ScopedVector<Metafile>* emf_files);
 #endif  // OS_WIN
 
   content::NotificationRegistrar registrar_;
diff --git a/printing/emf_win.cc b/printing/emf_win.cc
index d9472f9..b6c9b4f 100644
--- a/printing/emf_win.cc
+++ b/printing/emf_win.cc
@@ -169,9 +169,14 @@ Emf::Emf() : emf_(NULL), hdc_(NULL), page_count_(0) {
 }
 
 Emf::~Emf() {
+  Close();
+}
+
+void Emf::Close() {
   DCHECK(!hdc_);
   if (emf_)
     DeleteEnhMetaFile(emf_);
+  emf_ = NULL;
 }
 
 bool Emf::InitToFile(const base::FilePath& metafile_path) {
diff --git a/printing/emf_win.h b/printing/emf_win.h
index fa27409..e31e204 100644
--- a/printing/emf_win.h
+++ b/printing/emf_win.h
@@ -42,6 +42,9 @@ class PRINTING_EXPORT Emf : public Metafile {
   Emf();
   virtual ~Emf();
 
+  // Closes metafile.
+  void Close();
+
   // Generates a new metafile that will record every GDI command, and will
   // be saved to |metafile_path|.
   virtual bool InitToFile(const base::FilePath& metafile_path);