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

chrome/browser/printing/pdf_to_emf_converter.cc

Index: chrome/browser/printing/pdf_to_emf_converter.cc
diff --git a/chrome/browser/printing/pdf_to_emf_converter.cc b/chrome/browser/printing/pdf_to_emf_converter.cc
index df45a924a559b0bc06df798c05555322dc0ce159..8aa80572a311ac6c600a15d2e0684b1cf071c400 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,52 @@ 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();

[scottmg, 2014/09/08 19:06:11]

I'm not sure why it was re-opened readonly before, is there any reason why it
might be bad to provide a writable pdf?

[Vitaly Buka (NO REVIEWS), 2014/09/08 19:19:57]

I see no reason why it's bad, because target process is the only user of that
PDF. It's possible to avoid that but it just makes code more complicated.

Before this change it was opened twice, the first time is inside
base::WriteFile(). To guaranty proper deletion we need to use
FLAG_DELETE_ON_CLOSE on the first open.

 }
 
 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() {};

[scottmg, 2014/09/08 19:06:10]

no ;

[Vitaly Buka (NO REVIEWS), 2014/09/08 19:19:58]

Done.

+
+  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

[scottmg, 2014/09/08 19:06:11]

that seems pretty worrying, do you have any insight on what's not being
destroyed?

PrintedDocument holds a scoped_refptr assigned in SetPage. Is PrintedDocument
not destroyed then?

[Vitaly Buka (NO REVIEWS), 2014/09/08 19:19:57]

Yes PrintedDocument. I have no more details yet, but I don't expect we would
like to merge fix for that into branch.

On 2014/09/08 19:06:11, scottmg wrote:

+    // |files_| must be released as soon as possible to guaranty deletion.

[scottmg, 2014/09/08 19:06:10]

nit; guaranty -> guarantee

[Vitaly Buka (NO REVIEWS), 2014/09/08 19:19:58]

Done.

+    // It's know that that this Emf file is going to be played just once to

[scottmg, 2014/09/08 19:06:10]

nit; "know that that" -> "known that"

[Vitaly Buka (NO REVIEWS), 2014/09/08 19:19:58]

Done.

+    // 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 +166,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 +186,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 +285,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();
   }
 }