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

chrome/browser/printing/pdf_to_emf_converter.cc

 // Copyright 2014 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 "chrome/browser/printing/pdf_to_emf_converter.h"
 
 #include "base/bind_helpers.h"
 #include "base/cancelable_callback.h"
 #include "base/files/file.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "chrome/common/chrome_utility_messages.h"
 #include "chrome/common/chrome_utility_printing_messages.h"
 #include "content/public/browser/browser_thread.h"
 #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"
 
 namespace printing {
 
 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();
 
   base::FilePath GetEmfPath() const {
     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");
   }
 
   IPC::PlatformFileForTransit GetPdfForProcess(base::ProcessHandle process) {
     DCHECK(pdf_file_.IsValid());
     IPC::PlatformFileForTransit transit =
         IPC::TakeFileHandleForProcess(pdf_file_.Pass(), process);
     return transit;
   }
 
   const base::FilePath& GetBasePath() const {
     return temp_dir_.path();
   }
 
  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_;
 };
 
 void FileHandlers::Init(base::RefCountedMemory* data) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   if (!temp_dir_.CreateUniqueTempDir()) {
     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:
 // 1. Create instance on the UI thread. (files_, settings_,)
 // 2. Create file on the FILE thread.
 // 3. Start utility process and start conversion on the IO thread.
 // 4. Run result callback on the UI thread.
 // 5. Instance is destroyed from any thread that has the last reference.
 // 6. FileHandlers destroyed on the FILE thread.
 //    This step posts |FileHandlers| to be destroyed on the FILE thread.
@@ skipping 24 matching lines @@
   void RunCallback(const std::vector<printing::PageRange>& page_ranges,
                    double scale_factor);
 
   void StartProcessOnIOThread();
 
   void RunCallbackOnUIThread(
       const std::vector<printing::PageRange>& page_ranges,
       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_;
 
   DISALLOW_COPY_AND_ASSIGN(PdfToEmfUtilityProcessHostClient);
 };
 
 PdfToEmfUtilityProcessHostClient::PdfToEmfUtilityProcessHostClient(
     const printing::PdfRenderSettings& settings)
     : settings_(settings) {}
 
 PdfToEmfUtilityProcessHostClient::~PdfToEmfUtilityProcessHostClient() {
 }
 
 void PdfToEmfUtilityProcessHostClient::Convert(
     base::RefCountedMemory* data,
     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));
 }
 
 void PdfToEmfUtilityProcessHostClient::OnProcessCrashed(int exit_code) {
   OnFailed();
 }
 
 bool PdfToEmfUtilityProcessHostClient::OnMessageReceived(
   const IPC::Message& message) {
@@ skipping 73 matching lines @@
       base::Bind(&PdfToEmfUtilityProcessHostClient::RunCallbackOnUIThread,
                  this,
                  page_ranges,
                  scale_factor));
 }
 
 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();
   }
 }
 
 class PdfToEmfConverterImpl : public PdfToEmfConverter {
  public:
   PdfToEmfConverterImpl();
 
   virtual ~PdfToEmfConverterImpl();
 
@@ skipping 26 matching lines @@
 }
 
 }  // namespace
 
 // static
 scoped_ptr<PdfToEmfConverter> PdfToEmfConverter::CreateDefault() {
   return scoped_ptr<PdfToEmfConverter>(new PdfToEmfConverterImpl());
 }
 
 }  // namespace printing