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

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

Committed: https://crrev.com/e12ae0b0a1e9d18c585e63c37b51be7cef404772
Cr-Commit-Position: refs/heads/master@{#293848}

[Vitaly Buka (NO REVIEWS), 2014-09-07 09:06:48]

vitalybuka@chromium.org changed reviewers:
+ scottmg@chromium.org, thestig@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-09-08 01:08:38]

Patchset #3 (id:40001) has been deleted

[Vitaly Buka (NO REVIEWS), 2014-09-08 01:15:23]

Patchset #4 (id:80001) has been deleted

[Vitaly Buka (NO REVIEWS), 2014-09-08 01:15:28]

Patchset #3 (id:60001) has been deleted

[scottmg, 2014-09-08 18:18:35]

It seems a bit odd to be deleting wildcard directories here. Is it not possible
to have printing::Emf unlock at the right point so that ScopedTempDir can work?

[Vitaly Buka (NO REVIEWS), 2014-09-08 18:26:15]

On 2014/09/08 18:18:35, scottmg wrote:
> It seems a bit odd to be deleting wildcard directories here. Is it not
possible
> to have printing::Emf unlock at the right point so that ScopedTempDir can
work?

Deleting wildcard is not necessary on clean Chrome install with unlocking code.

However we already have this bug in Chrome 37 and wasting users space on disk.
So wildcards are there to cleanup that mess.

[Vitaly Buka (NO REVIEWS), 2014-09-08 18:27:49]

On 2014/09/08 18:26:15, Vitaly Buka wrote:
> On 2014/09/08 18:18:35, scottmg wrote:
> > It seems a bit odd to be deleting wildcard directories here. Is it not
> possible
> > to have printing::Emf unlock at the right point so that ScopedTempDir can
> work?
> 
> Deleting wildcard is not necessary on clean Chrome install with unlocking
code.
> 
> However we already have this bug in Chrome 37 and wasting users space on disk.
> So wildcards are there to cleanup that mess.

Wildcards are pretty specific and in %temp% dir, so I don't expect any harm from
that.

[scottmg, 2014-09-08 18:29:48]

On 2014/09/08 18:26:15, Vitaly Buka wrote:
> On 2014/09/08 18:18:35, scottmg wrote:
> > It seems a bit odd to be deleting wildcard directories here. Is it not
> possible
> > to have printing::Emf unlock at the right point so that ScopedTempDir can
> work?
> 
> Deleting wildcard is not necessary on clean Chrome install with unlocking
code.
> 
> However we already have this bug in Chrome 37 and wasting users space on disk.
> So wildcards are there to cleanup that mess.

I see. What do you think about splitting the deletion code from the bug fix
then? I don't think we should ship the temp cleanup other than in 37 then right?

[Vitaly Buka (NO REVIEWS), 2014-09-08 18:32:12]

On 2014/09/08 18:29:48, scottmg wrote:
> On 2014/09/08 18:26:15, Vitaly Buka wrote:
> > On 2014/09/08 18:18:35, scottmg wrote:
> > > It seems a bit odd to be deleting wildcard directories here. Is it not
> > possible
> > > to have printing::Emf unlock at the right point so that ScopedTempDir can
> > work?
> > 
> > Deleting wildcard is not necessary on clean Chrome install with unlocking
> code.
> > 
> > However we already have this bug in Chrome 37 and wasting users space on
disk.
> > So wildcards are there to cleanup that mess.
> 
> I see. What do you think about splitting the deletion code from the bug fix
> then? I don't think we should ship the temp cleanup other than in 37 then
right?

Do you think we need push cleanup into 37?
I thought to push both into 38. And for 39 I have patch in progress for
crbug.com/408184, which makes all this irrelevant.

[Lei Zhang, 2014-09-08 18:37:28]

How big are these files? Can we just let Windows Disk Cleanup delete these for
us when the user gets around to it? All Windows temp dirs I have seen tend to
get cluttered over time and require Disk Cleanup anyway.

[scottmg, 2014-09-08 18:38:27]

On 2014/09/08 18:37:28, Lei Zhang wrote:
> How big are these files? Can we just let Windows Disk Cleanup delete these for
> us when the user gets around to it? All Windows temp dirs I have seen tend to
> get cluttered over time and require Disk Cleanup anyway.

That would be my inclination too. It's not great, but I don't like deleting
things manually from temp that we don't know we own.

[scottmg, 2014-09-08 18:38:58]

(Especially since as you say it's entirely going away soon anyway)


On Mon, Sep 8, 2014 at 11:38 AM, <scottmg@chromium.org> wrote:

> On 2014/09/08 18:37:28, Lei Zhang wrote:
>
>> How big are these files? Can we just let Windows Disk Cleanup delete
>> these for
>> us when the user gets around to it? All Windows temp dirs I have seen
>> tend to
>> get cluttered over time and require Disk Cleanup anyway.
>>
>
> That would be my inclination too. It's not great, but I don't like deleting
> things manually from temp that we don't know we own.
>
> https://codereview.chromium.org/547203002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Vitaly Buka (NO REVIEWS), 2014-09-08 18:43:02]

On 2014/09/08 18:37:28, Lei Zhang wrote:
> How big are these files? Can we just let Windows Disk Cleanup delete these for
> us when the user gets around to it? All Windows temp dirs I have seen tend to
> get cluttered over time and require Disk Cleanup anyway.

regular docs are couple of megs.
images may take tens of megs per page.

[Vitaly Buka (NO REVIEWS), 2014-09-08 18:46:34]

On 2014/09/08 18:43:02, Vitaly Buka wrote:
> On 2014/09/08 18:37:28, Lei Zhang wrote:
> > How big are these files? Can we just let Windows Disk Cleanup delete these
for
> > us when the user gets around to it? All Windows temp dirs I have seen tend
to
> > get cluttered over time and require Disk Cleanup anyway.
> 
> regular docs are couple of megs.
> images may take tens of megs per page.

PTAL. Removed scheduled file cleanup.

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

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:89: pdf_file_.Flush();
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?

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:105: virtual ~TempEmf() {};
no ;

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:111: // instances of Emf are not
deleted. crbug.com/411683
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?

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:112: // |files_| must be
released as soon as possible to guaranty deletion.
nit; guaranty -> guarantee

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:113: // It's know that that this
Emf file is going to be played just once to
nit; "know that that" -> "known that"

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:156: emf_files->weak_clear();
Add a comment here or above SetPage that ownership of Metafile* is taken by
SetPage.

[scottmg, 2014-09-08 19:15:09]

I think PrintViewManagerBase holds PrintJob holds the PrintDocument which holds
the PrintedPage. But I don't know the lifetime of PrintViewManagerBase.

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

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:89: pdf_file_.Flush();
On 2014/09/08 19:06:11, scottmg wrote:
> 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?

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.

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:105: virtual ~TempEmf() {};
On 2014/09/08 19:06:10, scottmg wrote:
> no ;

Done.

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:111: // instances of Emf are not
deleted. crbug.com/411683
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:
> 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?

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:112: // |files_| must be
released as soon as possible to guaranty deletion.
On 2014/09/08 19:06:10, scottmg wrote:
> nit; guaranty -> guarantee

Done.

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:113: // It's know that that this
Emf file is going to be played just once to
On 2014/09/08 19:06:10, scottmg wrote:
> nit; "know that that" -> "known that"

Done.

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/547203002/diff/120001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:156: emf_files->weak_clear();
On 2014/09/08 19:06:11, scottmg wrote:
> Add a comment here or above SetPage that ownership of Metafile* is taken by
> SetPage.

Done.

[scottmg, 2014-09-08 19:25:01]

ok, lgtm if Lei is happy.

[Lei Zhang, 2014-09-08 19:59:09]

lgtm

[Vitaly Buka (NO REVIEWS), 2014-09-08 21:19:56]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-09-08 21:24:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/547203002/140001

[Vitaly Buka (NO REVIEWS), 2014-09-09 04:21:35]

The CQ bit was unchecked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-09-09 04:22:11]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-09-09 04:29:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/547203002/140001

[Vitaly Buka (NO REVIEWS), 2014-09-09 04:36:23]

Committed patchset #5 to pending queue manually as 1c0b573 (presubmit
successful).

[commit-bot: I haz the power, 2014-09-09 04:46:16]

Committed patchset #5 (id:140001) as 1c0b5733cc0ee21518630a5778bb0d28baa27722

[Vitaly Buka (NO REVIEWS), 2014-09-10 00:04:42]

printing entire
http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/pdf_reference_1...
wasting 1.5Gb, so maybe we should consider wildcard cleanup?

[commit-bot: I haz the power, 2014-09-10 03:50:21]

Patchset 5 (id:??) landed as
https://crrev.com/e12ae0b0a1e9d18c585e63c37b51be7cef404772
Cr-Commit-Position: refs/heads/master@{#293848}