ImportantFileWriter

ImportantFileWriter

Introducing a class for writing important files, preventing their corruption during writing.

Switched PrefService to use it. Other classes will be switched in future changesets.

TEST=This may affect things using preferences. Make sure that changes in preferences don't get lost, and that you don't get excessive disk activity when changing preferences.

http://crbug.com/10618

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=14717

[Paweł Hajdan Jr., 2009-04-20 15:49:39]

I'm attaching just the class and test for now to see if it makes sense. After
this initial review is finished, I plan to refactor rest of code to use the new
class, and submit everything for review.

Darin, please especially check if I didn't misuse timer or message loop here.

[darin (slow to review), 2009-04-20 16:20:19]

http://codereview.chromium.org/83001/diff/1/3
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/83001/diff/1/3#newcode49
Line 49: if (file_util::Move(tmp_file, path_)) {
hmm... this seems wrong.  if the first attempt failed, a second attempt a
split-microsecond later will likely also fail.

[darin (slow to review), 2009-04-20 16:20:21]

This reminds me of Mozilla's SafeFileOutputStream class :-)

[Paweł Hajdan Jr., 2009-04-20 16:24:39]

On 2009/04/20 16:20:21, darin wrote:
> This reminds me of Mozilla's SafeFileOutputStream class :-)

It also seems wrong to me, and I'll gladly remove it. I included it only because
that's what current code does (in pref_service and other places).

If there are no major issues with this part, I'd refactor the code that should
now use this class and update this review issue.

[Evan Martin, 2009-04-20 17:49:22]

+adam who has talked about doing this before
+dean because it seems like his sort of thing

The code looks good to me.  I mostly have comment questions.

http://codereview.chromium.org/83001/diff/1/3
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/83001/diff/1/3#newcode19
Line 19: const int kDefaultCommitIntervalMs = 10000;
Should this be a TimeDelta?

http://codereview.chromium.org/83001/diff/1/4
File chrome/common/important_file_writer.h (right):

http://codereview.chromium.org/83001/diff/1/4#newcode17
Line 17: // application crash).
Could you elaborate on this?  (Imagine someone who wasn't aware of the
corruption problem opened this header and wanted to know what the module was
for.)  Maybe briefly mention the commit interval as well.

http://codereview.chromium.org/83001/diff/1/4#newcode24
Line 24: // You have to call this ctor, WriteNow, and ScheduleWrite on the same
thread.
Would it be enough to say "all public functions on the same thread"?

http://codereview.chromium.org/83001/diff/1/4#newcode62
Line 62: int commit_interval_ms_;
Should this be a TimeDelta?

http://codereview.chromium.org/83001/diff/1/5
File chrome/common/important_file_writer_unittest.cc (right):

http://codereview.chromium.org/83001/diff/1/5#newcode85
Line 85: }
Maybe you should have a test for the error condition -- like if the target file
isn't writable or is a directory, make sure you don't leave temporary files
around.  Maybe that's hard to do in a cross-platform way... I guess making it a
directory would work.

[Evan Martin, 2009-04-20 17:49:51]

Oh, and be sure to remove the [WIP] before you commit.

[agl, 2009-04-20 17:59:47]

NACK based on security issues.

http://codereview.chromium.org/83001/diff/1/3
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/83001/diff/1/3#newcode31
Line 31: const FilePath tmp_file(path_.InsertBeforeExtension(" (temporary)"));
spaces in filenames are Windows brain damage!

Also, if the directory can be written by an attacker, this can be used to
clobber an arbitrary file via a symlink. It needs to be a secure tempfile
creation as always.

http://codereview.chromium.org/83001/diff/1/3#newcode33
Line 33: int bytes_written = file_util::WriteFile(tmp_file, data_.c_str(),
data_.data(), not c_str() since it shouldn't need to be NUL termianted.

(Note about secure temp files again)

[Paweł Hajdan Jr., 2009-04-23 20:00:43]

I think that most issues with ImportantFileWriter should now be fixed.

+brettw

I switched PrefService to use ImportantFileWriter to make sure that its
interface is usable in real cases. There is one change I'm not sure about: after
this patch serialization of JSON data doesn't happen on file thread, but on
thread ImportantFileWriter has been created on. It can be made so that the
serialization happens on file thread, but I didn't want to complicate
ImportantFileWriter if that's not necessary. What's your opinion? I'm afraid
that it may have a performance impact, and that serializing on a separate thread
would be better...

[brettw, 2009-04-23 21:27:56]

This is only used for preferences right, now, right?

If we were to serialize on a background thread, we would have to make a copy of
the entire preferences structure or lock it somehow. These things seem just as
slow as serialization as long as we do a decent job of it.

So I think serializing on the main thread should be OK. We do it now anyway and
it hasn't been a big problem. We can do something fancier if we have more
complex files to write.

[Paweł Hajdan Jr., 2009-04-24 18:08:11]

Patch updated.

Here are things I'll definitely wait for before committing:

agl: Please re-check security now.

brettw: Please also look at my Windows changes in file_util_win.cc. They were
necessary to pass PrefServiceTest.Basic because now file write is done using
fwrite and not ::WriteFile.

Here are things I understand are ok now, but feel free to comment or say "don't
commit too soon":

Evan: issues you pointed out (not making static const TimeDelta to avoid static
global objects; everything else made TimeDelta, comment added, etc).

brettw / darin: Serialization on UI thread. Sorry for confusing you. Indeed,
data was serialized on UI thread before, and it shouldn't be an issue.

[agl, 2009-04-24 18:12:51]

important_file_writer.cc: LGTM

[brettw, 2009-04-24 20:32:56]

http://codereview.chromium.org/83001/diff/5028/5031
File base/file_util_posix.cc (right):

http://codereview.chromium.org/83001/diff/5028/5031#newcode377
Line 377: int fd = CreateAndOpenFdForTemporaryFile(directory, path);
Can the rest of this be replaced with a call to CreateAndOpenTemporaryFileInDir?

[Paweł Hajdan Jr., 2009-04-27 08:18:36]

On 2009/04/24 20:32:56, brettw wrote:
> http://codereview.chromium.org/83001/diff/5028/5031
> File base/file_util_posix.cc (right):
> 
> http://codereview.chromium.org/83001/diff/5028/5031#newcode377
> Line 377: int fd = CreateAndOpenFdForTemporaryFile(directory, path);
> Can the rest of this be replaced with a call to
CreateAndOpenTemporaryFileInDir?

Done. Do you think it's ok now? If so, I'd commit it.

[brettw, 2009-04-28 02:06:58]

LGTM

[stoyan, 2009-05-05 18:59:41]

On Windows ::ReplaceFile() is preferred solution over ::MoveFile()