From e12ae0b0a1e9d18c585e63c37b51be7cef404772 Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Mon, 8 Sep 2014 21:33:46 -0700 Subject: 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} --- chrome/browser/printing/pdf_to_emf_converter.cc | 86 ++++++++++++++++------ chrome/browser/printing/pdf_to_emf_converter.h | 8 +- chrome/browser/printing/print_view_manager_base.cc | 15 ++-- chrome/browser/printing/print_view_manager_base.h | 3 +- printing/emf_win.cc | 5 ++ printing/emf_win.h | 3 + 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 { 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; + friend class base::DeleteHelper; + + ~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(data->size()) != - base::WriteFile(GetPdfPath(), data->front_as(), data->size())) { + pdf_file_.WriteAtCurrentPos(data->front_as(), 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& files) : files_(files) {} + virtual ~TempEmf() {} + + virtual bool SafePlayback(HDC hdc) const OVERRIDE { + bool result = Emf::SafePlayback(hdc); + TempEmf* this_mutable = const_cast(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 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 files_; + scoped_refptr files_; printing::PdfRenderSettings settings_; PdfToEmfConverter::ResultCallback callback_; base::WeakPtr 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& page_ranges, double scale_factor) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - std::vector page_filenames; + ScopedVector pages; std::vector::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 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& /*emf_files*/)> + // Takes ownership of metafiles. + typedef base::Callback< + void(double /*scale_factor*/, ScopedVector* /*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& emf_files) { + ScopedVector* 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 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& emf_file); + ScopedVector* 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); -- cgit v1.1