Bug fix: produce unique temp file names when the VFS requests a handle to a n...

Bug fix: produce unique temp file names when the VFS requests a handle to a null file.

BUG=none
TEST=none


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

[michaeln, 2009-09-04 17:34:27]

http://codereview.chromium.org/194021/diff/1/2
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/194021/diff/1/2#newcode392
Line 392: } while (file_util::PathExists(temp_file_name));
This is running on the IO thread. We need to avoid the file system call to
PathExists here.

Do we need to test for existence really? Will the flags being used cause any
pre-existing file to be overwritten? If not, could we forcibly delete the
temp_file_name on the FILE thread prior to attempting to open/create anew.

[dumi, 2009-09-05 00:25:44]

On 2009/09/04 17:34:27, michaeln wrote:
> http://codereview.chromium.org/194021/diff/1/2
> File chrome/browser/renderer_host/database_dispatcher_host.cc (right):
> 
> http://codereview.chromium.org/194021/diff/1/2#newcode392
> Line 392: } while (file_util::PathExists(temp_file_name));
> This is running on the IO thread. We need to avoid the file system call to
> PathExists here.

moved this to the file thread.

> Do we need to test for existence really? Will the flags being used cause any
> pre-existing file to be overwritten? If not, could we forcibly delete the
> temp_file_name on the FILE thread prior to attempting to open/create anew.

It's probably not a good thing to delete/overwrite an existing file, so I think
we should keep the PathExists check here. This would add a performance overhead
only if the user manually adds XYZ.db-temp files to the DB dir (which I can't
see happening), or if we have a bug (in which case it's better to keep the files
around than to delete them and make some app not work).

[darin (slow to review), 2009-09-05 04:37:42]

http://codereview.chromium.org/194021/diff/4001/4002
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/194021/diff/4001/4002#newcode67
Line 67: static FilePath GetUniqueTempFileName() {
please use one of the methods from base/file_util for creating a temporary file.
file_util::CreateTemporaryFileInDir is probably suitable here.

[dumi, 2009-09-08 18:16:49]

> please use one of the methods from base/file_util for creating a temporary
file.
> file_util::CreateTemporaryFileInDir is probably suitable here.

this function doesn't really work for us, because it doesn't allow us to provide
the flags we want, such as PLATFORM_FILE_DELETE_ON_CLOSE.

[darin (slow to review), 2009-09-08 18:20:22]

On 2009/09/08 18:16:49, dumi wrote:
> > please use one of the methods from base/file_util for creating a temporary
> file.
> > file_util::CreateTemporaryFileInDir is probably suitable here.
> 
> this function doesn't really work for us, because it doesn't allow us to
provide
> the flags we want, such as PLATFORM_FILE_DELETE_ON_CLOSE.

Then, I recommend extending it to support a flags parameter.  My point is that
we should keep our code for creating temporary files in one place.

[dumi, 2009-09-09 18:42:43]

I used 'bool file_util::CreateTemporaryFileInDir()' to get a temp file name.
Please take another look.

Adam: please double-check the POSIX implementation for this method.

[agl, 2009-09-09 18:52:32]

http://codereview.chromium.org/194021/diff/6/8
File base/file_util_posix.cc (right):

http://codereview.chromium.org/194021/diff/6/8#newcode419
Line 419: FILE* file = CreateAndOpenTemporaryFileInDir(dir, temp_file);
I'm a little confused here. This function never returns the FILE* - so it's
leaking a handle. Also, it deletes the file, so the file cannot be accessed by
anyone else anywhere. Surely you need to return the FILE*?

[dumi, 2009-09-09 19:25:59]

> I'm a little confused here. This function never returns the FILE* - so it's
> leaking a handle. Also, it deletes the file, so the file cannot be accessed by
> anyone else anywhere. Surely you need to return the FILE*?

Fixed (I think).

[darin (slow to review), 2009-09-10 18:32:59]

http://codereview.chromium.org/194021/diff/10/3005
File base/file_util_posix.cc (right):

http://codereview.chromium.org/194021/diff/10/3005#newcode418
Line 418: int fd = CreateAndOpenFdForTemporaryFile(dir, temp_file);
mkstemp is documented as returning -1 on error.  perhaps you should compare
against -1 instead?  i'm pretty sure that all negative values are invalid, but
that's not what the docs for mkstemp say.

http://codereview.chromium.org/194021/diff/10/3005#newcode419
Line 419: return ((fd >= 0) && !close(fd) && Delete(*temp_file, false));
why are you unlinking the file here?  the windows implementation doesn't do
that.

http://codereview.chromium.org/194021/diff/10/3004
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/194021/diff/10/3004#newcode180
Line 180: { base::kInvalidPlatformFileValue };
i think this would be more readable if you just did member variable assignments.

#if defined(OS_WIN)
        res_params.file_handle = base::kInvalidPlatformFileValue;
#else
...
#endif

http://codereview.chromium.org/194021/diff/10/3004#newcode186
Line 186: NewRunnableFunction(SendMessage, sender,
nit: it is very common place in chrome code to indent w/ 4 white spaces when
breaking a long line.

http://codereview.chromium.org/194021/diff/10/3004#newcode196
Line 196: base::CreatePlatformFile(file_name.ToWStringHack(), flags, NULL);
nit: indent by 4 spaces

[dumi, 2009-09-10 21:50:34]

http://codereview.chromium.org/194021/diff/10/3005
File base/file_util_posix.cc (right):

http://codereview.chromium.org/194021/diff/10/3005#newcode418
Line 418: int fd = CreateAndOpenFdForTemporaryFile(dir, temp_file);
On 2009/09/10 18:32:59, darin wrote:
> mkstemp is documented as returning -1 on error.  perhaps you should compare
> against -1 instead?  i'm pretty sure that all negative values are invalid, but
> that's not what the docs for mkstemp say.

even though the docs say that mkstemp returns -1 on error, i think i'd rather
keep checking that the return value is not < 0. that seems to be more consistent
with the 'FILE* CreateAndOpenTemporaryFileInDir()' function and with most linux
code that deals with FDs that i've seen. adam, do you have a preference here?

http://codereview.chromium.org/194021/diff/10/3005#newcode419
Line 419: return ((fd >= 0) && !close(fd) && Delete(*temp_file, false));
On 2009/09/10 18:32:59, darin wrote:
> why are you unlinking the file here?  the windows implementation doesn't do
> that.

fixed. not unlinking is the correct/consistent behavior, but i wasn't sure if
sqlite would be fine with reopening an existing (empty) file instead of creating
a new one. it looks like it is.

http://codereview.chromium.org/194021/diff/10/3004
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/194021/diff/10/3004#newcode180
Line 180: { base::kInvalidPlatformFileValue };
On 2009/09/10 18:32:59, darin wrote:
> i think this would be more readable if you just did member variable
assignments.
> 
> #if defined(OS_WIN)
>         res_params.file_handle = base::kInvalidPlatformFileValue;
> #else
> ...
> #endif

i refactored this code and deleted this block. however, i made this change in
the other 2 places with identical code.

http://codereview.chromium.org/194021/diff/10/3004#newcode186
Line 186: NewRunnableFunction(SendMessage, sender,
On 2009/09/10 18:32:59, darin wrote:
> nit: it is very common place in chrome code to indent w/ 4 white spaces when
> breaking a long line.

i removed these lines, but i changed the indentation everywhere else in this
file.

http://codereview.chromium.org/194021/diff/10/3004#newcode196
Line 196: base::CreatePlatformFile(file_name.ToWStringHack(), flags, NULL);
On 2009/09/10 18:32:59, darin wrote:
> nit: indent by 4 spaces

done.

[darin (slow to review), 2009-09-11 00:11:19]

LGTM w/ nits addressed:

http://codereview.chromium.org/194021/diff/10002/10003
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/194021/diff/10002/10003#newcode70
Line 70: if ((params.file_name == params.db_dir) &&
nit: you could drop some of these parens.  at least the ones around the !(...)
instances seem like they hurt readability more than they help.

http://codereview.chromium.org/194021/diff/10002/10003#newcode177
Line 177: if (!file_util::CreateTemporaryFileInDir(params.db_dir, &file_name)) {
it is nice if upon failure, the out parameters of a method are not modified. 
this way you don't have to write code like you are doing here at every call
site.

it's ok to break such a change out into a separate CL.

http://codereview.chromium.org/194021/diff/10002/10003#newcode413
Line 413: base::FileDescriptor(base::kInvalidPlatformFileValue, true);
nit: 4 space indent for line continuation

[dumi, 2009-09-11 00:49:47]

http://codereview.chromium.org/194021/diff/10002/10003
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/194021/diff/10002/10003#newcode70
Line 70: if ((params.file_name == params.db_dir) &&
On 2009/09/11 00:11:19, darin wrote:
> nit: you could drop some of these parens.  at least the ones around the !(...)
> instances seem like they hurt readability more than they help.

done.

http://codereview.chromium.org/194021/diff/10002/10003#newcode177
Line 177: if (!file_util::CreateTemporaryFileInDir(params.db_dir, &file_name)) {
On 2009/09/11 00:11:19, darin wrote:
> it is nice if upon failure, the out parameters of a method are not modified. 
> this way you don't have to write code like you are doing here at every call
> site.
> 
> it's ok to break such a change out into a separate CL.

will try to change this in a new CL.

http://codereview.chromium.org/194021/diff/10002/10003#newcode413
Line 413: base::FileDescriptor(base::kInvalidPlatformFileValue, true);
On 2009/09/11 00:11:19, darin wrote:
> nit: 4 space indent for line continuation

done.

[michaeln, 2009-09-11 00:51:50]

LGTM!