Fix ImportantFileWriter leaking a .tmp file when passed an invalid path

Fix ImportantFileWriter leaking a .tmp file when passed an invalid path

When passed an invalid path (ex. "bad/../path"), ImportantFileWriter
creates a temp file, sees that it's invalid, then returns without deleting it. 
The file being invalid is an implementation detail of base::File's 
Initialize() method, which sets an error field to 
FILE_ERROR_ACCESS_DENIED if the file's path contains the parent ".." operator. 

This stops ImportantFileWriterTest.FailedWriteWithObserver from leaking
a temp file.

BUG=691388
Review-Url: https://codereview.chromium.org/2711523002
Cr-Commit-Position: refs/heads/master@{#452266}
Committed: https://chromium.googlesource.com/chromium/src/+/05db50c5c72b8368af872b768cb637c294e99f44

[proberge, 2017-02-21 16:17:08]

Description was changed from

==========
Fix ImportantFileWriter leaking a .tmp file when passed an invalid path

When passed an invalid path (ex. "bad/../path"), ImportantFileWriter
creates a temp file, fails to open it, then returns without deleting it.

This stops ImportantFileWriterTest.FailedWriteWithObserver from leaking
a temp file.

BUG=691388
==========

to

==========
Fix ImportantFileWriter leaking a .tmp file when passed an invalid path

When passed an invalid path (ex. "bad/../path"), ImportantFileWriter
creates a temp file, fails to open it, then returns without deleting it.

This stops ImportantFileWriterTest.FailedWriteWithObserver from leaking
a temp file.

BUG=691388
==========

[proberge, 2017-02-21 16:17:08]

proberge@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-02-21 23:52:41]

Dumb question: how do we end up with a temporary file that we can't open but we
can delete?

[proberge, 2017-02-22 14:12:37]

On 2017/02/21 23:52:41, dcheng wrote:
> Dumb question: how do we end up with a temporary file that we can't open but
we
> can delete?

Not a dumb question at all! In the base::File constructor, we call Initialize
(https://cs.chromium.org/chromium/src/base/files/file.cc?l=85) which sets the
file as invalid (error_details_ = FILE_ERROR_ACCESS_DENIED) if the
FilePath::ReferencesParent returns true for the file's path.

In other words, it's "won't open due to security reasons" and not "can't open".

[dcheng, 2017-02-22 18:07:35]

On 2017/02/22 14:12:37, proberge wrote:
> On 2017/02/21 23:52:41, dcheng wrote:
> > Dumb question: how do we end up with a temporary file that we can't open but
> we
> > can delete?
> 
> Not a dumb question at all! In the base::File constructor, we call Initialize
> (https://cs.chromium.org/chromium/src/base/files/file.cc?l=85) which sets the
> file as invalid (error_details_ = FILE_ERROR_ACCESS_DENIED) if the
> FilePath::ReferencesParent returns true for the file's path.
> 
> In other words, it's "won't open due to security reasons" and not "can't
open".

LGTM with that documented in a comment or the CL description. The API is
certainly a bit non-intuitive though...

[proberge, 2017-02-22 21:06:08]

Description was changed from

==========
Fix ImportantFileWriter leaking a .tmp file when passed an invalid path

When passed an invalid path (ex. "bad/../path"), ImportantFileWriter
creates a temp file, fails to open it, then returns without deleting it.

This stops ImportantFileWriterTest.FailedWriteWithObserver from leaking
a temp file.

BUG=691388
==========

to

==========
Fix ImportantFileWriter leaking a .tmp file when passed an invalid path

When passed an invalid path (ex. "bad/../path"), ImportantFileWriter
creates a temp file, sees that it's invalid, then returns without deleting it.
The file being invalid is an implementation detail of base::File's Initialize()
method, which sets an error field to FILE_ERROR_ACCESS_DENIED if the file's path
contains the parent ".." operator. 

This stops ImportantFileWriterTest.FailedWriteWithObserver from leaking
a temp file.

BUG=691388
==========

[proberge, 2017-02-22 21:06:29]

Description was changed from

==========
Fix ImportantFileWriter leaking a .tmp file when passed an invalid path

When passed an invalid path (ex. "bad/../path"), ImportantFileWriter
creates a temp file, sees that it's invalid, then returns without deleting it.
The file being invalid is an implementation detail of base::File's Initialize()
method, which sets an error field to FILE_ERROR_ACCESS_DENIED if the file's path
contains the parent ".." operator. 

This stops ImportantFileWriterTest.FailedWriteWithObserver from leaking
a temp file.

BUG=691388
==========

to

==========
Fix ImportantFileWriter leaking a .tmp file when passed an invalid path

When passed an invalid path (ex. "bad/../path"), ImportantFileWriter
creates a temp file, sees that it's invalid, then returns without deleting it. 
The file being invalid is an implementation detail of base::File's 
Initialize() method, which sets an error field to 
FILE_ERROR_ACCESS_DENIED if the file's path contains the parent ".." operator. 

This stops ImportantFileWriterTest.FailedWriteWithObserver from leaking
a temp file.

BUG=691388
==========

[proberge, 2017-02-22 21:06:52]

The CQ bit was checked by proberge@chromium.org

[commit-bot: I haz the power, 2017-02-22 21:07:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 23:24:10]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487797612197490, "parent_rev":
"fc15ef0491997795eee00a1595727b26628990c7", "commit_rev":
"05db50c5c72b8368af872b768cb637c294e99f44"}

[commit-bot: I haz the power, 2017-02-22 23:25:06]

Description was changed from

==========
Fix ImportantFileWriter leaking a .tmp file when passed an invalid path

When passed an invalid path (ex. "bad/../path"), ImportantFileWriter
creates a temp file, sees that it's invalid, then returns without deleting it. 
The file being invalid is an implementation detail of base::File's 
Initialize() method, which sets an error field to 
FILE_ERROR_ACCESS_DENIED if the file's path contains the parent ".." operator. 

This stops ImportantFileWriterTest.FailedWriteWithObserver from leaking
a temp file.

BUG=691388
==========

to

==========
Fix ImportantFileWriter leaking a .tmp file when passed an invalid path

When passed an invalid path (ex. "bad/../path"), ImportantFileWriter
creates a temp file, sees that it's invalid, then returns without deleting it. 
The file being invalid is an implementation detail of base::File's 
Initialize() method, which sets an error field to 
FILE_ERROR_ACCESS_DENIED if the file's path contains the parent ".." operator. 

This stops ImportantFileWriterTest.FailedWriteWithObserver from leaking
a temp file.

BUG=691388

Review-Url: https://codereview.chromium.org/2711523002
Cr-Commit-Position: refs/heads/master@{#452266}
Committed:
https://chromium.googlesource.com/chromium/src/+/05db50c5c72b8368af872b768cb6...
==========

[commit-bot: I haz the power, 2017-02-22 23:25:07]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/05db50c5c72b8368af872b768cb6...