Properly honor base::SharedMemory semantics for name="" to mean...

Properly honor base::SharedMemory semantics for name="" to mean
new/private shared memory on POSIX.  Transition base::SharedMemory
implementation to file/mmap() to prevent leaking of wired kernel
resources and allow easier cleanup.  Enable one more shared_memory
unit test for POSIX.  Enable stats_table_unittest.cc for Mac, and
modify it so it cleans up.

[John Grabowski, 2009-01-31 04:00:14]

Amanda: please review for correctness, and that this is a reasonable direction
for Mac.
Evan: please confirm this is a reasonable direction for Linux.
Brettw: please review that previous semantics are honored.

[brettw, 2009-01-31 16:46:17]

http://codereview.chromium.org/19724/diff/91/95
File base/file_util_posix.cc (right):

http://codereview.chromium.org/19724/diff/91/95#newcode277
Line 277: int _CreateAndOpenFdForTemporaryFile(FilePath* path) {
What are you trying to do here with the leading underscore? Local functions
should be static or in a n anon. namespace. Don't use a leading underscore.

http://codereview.chromium.org/19724/diff/91/95#newcode287
Line 287: fsync(fd);
Why did you add this? Seems like it could use a comment. It would be nice to
avoid syncs if at all possible.

http://codereview.chromium.org/19724/diff/91/98
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/19724/diff/91/98#newcode702
Line 702: // create; make sure they are open and exist
Comments should begin with a capital and end with a period.

http://codereview.chromium.org/19724/diff/91/99
File base/shared_memory.h (right):

http://codereview.chromium.org/19724/diff/91/99#newcode134
Line 134: bool FilenameForMemoryName(const std::wstring memname,
Make this string a reference so you don't make a copy.

http://codereview.chromium.org/19724/diff/91/97
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/19724/diff/91/97#newcode77
Line 77: std::wstring mem_filename;
2 space indents

http://codereview.chromium.org/19724/diff/91/100
File chrome/renderer/render_process.cc (right):

http://codereview.chromium.org/19724/diff/91/100#newcode124
Line 124: std::wstring root_name(L"");
Can you just remove this variable?

[John Grabowski, 2009-02-01 05:58:45]

http://codereview.chromium.org/19724/diff/91/95
File base/file_util_posix.cc (right):

http://codereview.chromium.org/19724/diff/91/95#newcode277
Line 277: int _CreateAndOpenFdForTemporaryFile(FilePath* path) {
On 2009/01/31 16:46:17, brettw wrote:
> What are you trying to do here with the leading underscore? Local functions
> should be static or in a n anon. namespace. Don't use a leading underscore.

Sorry; I was thinking in Python.  _ removed.

http://codereview.chromium.org/19724/diff/91/95#newcode287
Line 287: fsync(fd);
On 2009/01/31 16:46:17, brettw wrote:
> Why did you add this? Seems like it could use a comment. It would be nice to
> avoid syncs if at all possible.

I was being conservative.  I have had problems in the past with files not being
fully created before mmapping them.  As an example, if I grow a file by
triggering an implicit zerofill (seek off then end then write one byte), then
mmap the file in multiple processes, the memory won't be shared properly.
In any case, I removed the fsync and all unit tests still pass.

http://codereview.chromium.org/19724/diff/91/98
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/19724/diff/91/98#newcode702
Line 702: // create; make sure they are open and exist
On 2009/01/31 16:46:17, brettw wrote:
> Comments should begin with a capital and end with a period.

Fixed.

http://codereview.chromium.org/19724/diff/91/99
File base/shared_memory.h (right):

http://codereview.chromium.org/19724/diff/91/99#newcode134
Line 134: bool FilenameForMemoryName(const std::wstring memname,
On 2009/01/31 16:46:17, brettw wrote:
> Make this string a reference so you don't make a copy.

Done

http://codereview.chromium.org/19724/diff/91/97
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/19724/diff/91/97#newcode77
Line 77: std::wstring mem_filename;
On 2009/01/31 16:46:17, brettw wrote:
> 2 space indents

Fixed.

http://codereview.chromium.org/19724/diff/91/100
File chrome/renderer/render_process.cc (right):

http://codereview.chromium.org/19724/diff/91/100#newcode124
Line 124: std::wstring root_name(L"");
On 2009/01/31 16:46:17, brettw wrote:
> Can you just remove this variable?

Yes; removed.

[Evan Martin, 2009-02-02 17:54:39]

swapping out myself with agl in reviewers


agl's unix-fu > my unix-fu.

[Amanda Walker, 2009-02-02 18:18:18]

Overall lgtm, though I'd lean towards trying out flock or lockf immediately
instead of a posix semaphore. Cleanup is simpler, and in the longer lockf might
allow us lock sections of the memory block rather than the whole thing.

http://codereview.chromium.org/19724/diff/278/289
File base/base.xcodeproj/project.pbxproj (right):

http://codereview.chromium.org/19724/diff/278/289#newcode1563
Line 1563: 33C591920F33B9950026A42A /* stats_table_unittest.cc in Sources */,
alphabetize build phase files

[agl, 2009-02-02 18:26:53]

In the limit, we need to get rid of the whole idea of passing names around for
tempfiles and shared memory on POSIX systems. However, since we have issues now,
this patch makes sense.

http://codereview.chromium.org/19724/diff/278/281
File base/file_util.h (right):

http://codereview.chromium.org/19724/diff/278/281#newcode272
Line 272: // Create and open a temporary file.  File is opened for read/write.
Needs a comment about mutating @path.

http://codereview.chromium.org/19724/diff/278/282
File base/file_util_posix.cc (right):

http://codereview.chromium.org/19724/diff/278/282#newcode277
Line 277: int CreateAndOpenFdForTemporaryFile(FilePath* path) {
The usual pattern for temp files is to immediately unlink the file from the
directory. If you aren't going to do that, then the comment in the .h file needs
to reflect that.

http://codereview.chromium.org/19724/diff/278/282#newcode279
Line 279: return false;
false coerces to 0, which is a valid file descriptor. This needs to be -1

http://codereview.chromium.org/19724/diff/278/282#newcode281
Line 281: *path = path->Append(kTempFileName);
Something funny is going on here. @path is mutated to include the mkstemp
template at the end, but then we take a copy and mkstemp mutates the copy. The
end result is that @path is mutated, but has the wrong filename in it. Maybe the
copy should be a reference?

http://codereview.chromium.org/19724/diff/278/282#newcode286
Line 286: int fd = mkstemp(buffer);
This variable (@fd) isn't needed.

http://codereview.chromium.org/19724/diff/278/282#newcode292
Line 292: int fd = CreateAndOpenFdForTemporaryFile(path);
If CreateAndOpenFdForTemporaryFile doesn't unlink the file, then this function
doesn't just create a filename - it also creates the file.

http://codereview.chromium.org/19724/diff/278/284
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/19724/diff/278/284#newcode108
Line 108: DCHECK(memname.find_first_of(L"/") == std::string::npos);
One should also check for NUL charactors.

http://codereview.chromium.org/19724/diff/278/284#newcode111
Line 111: if (file_util::GetTempDir(&temp_dir) == false)
Shared memory segments should be created in /dev/shm

http://codereview.chromium.org/19724/diff/278/284#newcode134
Line 134: DCHECK(posix_flags & O_RDWR);
Should be (O_RDWR | O_WRONLY)

http://codereview.chromium.org/19724/diff/278/284#newcode153
Line 153: // Careful: "w+" will delete if it already exists.
s/delete/truncate

http://codereview.chromium.org/19724/diff/278/284#newcode167
Line 167: fp = file_util::OpenFile(mem_filename, mode.c_str());
One cannot safely open tempfiles with OpenFile. One /must/ use one of the
mkstemp family.

http://codereview.chromium.org/19724/diff/278/284#newcode180
Line 180: if (fseek(fp, 0, SEEK_END) != 0)
(Not important, but an fstat can do this in a single system call.)

http://codereview.chromium.org/19724/diff/278/284#newcode188
Line 188: size_t writesize = size-current_size;
Spaces around the -

http://codereview.chromium.org/19724/diff/278/284#newcode206
Line 206: DCHECK(mapped_file_);
0 is a valid file descriptor. This needs to be mapped_file_ >= 0.

[John Grabowski, 2009-02-02 21:28:41]

Verbal promise made to Amanda to follow-up wrt the semaphore transition.
Performance tests warranted (e.g. netbooting as suggested by thomasvl).

New diffs uploaded.
Unit tests (both base_unittests and chrome's "unit_tests") re-ran on OSX.
(Still waiting on Win try server for new changes).

http://codereview.chromium.org/19724/diff/278/289
File base/base.xcodeproj/project.pbxproj (right):

http://codereview.chromium.org/19724/diff/278/289#newcode1563
Line 1563: 33C591920F33B9950026A42A /* stats_table_unittest.cc in Sources */,
On 2009/02/02 18:18:18, Amanda Walker wrote:
> alphabetize build phase files

Moved.

http://codereview.chromium.org/19724/diff/278/281
File base/file_util.h (right):

http://codereview.chromium.org/19724/diff/278/281#newcode272
Line 272: // Create and open a temporary file.  File is opened for read/write.
On 2009/02/02 18:26:53, agl wrote:
> Needs a comment about mutating @path.

Added

http://codereview.chromium.org/19724/diff/278/282
File base/file_util_posix.cc (right):

http://codereview.chromium.org/19724/diff/278/282#newcode277
Line 277: int CreateAndOpenFdForTemporaryFile(FilePath* path) {
On 2009/02/02 18:26:53, agl wrote:
> The usual pattern for temp files is to immediately unlink the file from the
> directory. If you aren't going to do that, then the comment in the .h file
needs
> to reflect that.

The comment is this:
// TODO(erikkay): rename this function and track down all of the callers.
I will clarify it.

http://codereview.chromium.org/19724/diff/278/282#newcode279
Line 279: return false;
On 2009/02/02 18:26:53, agl wrote:
> false coerces to 0, which is a valid file descriptor. This needs to be -1

Fixed

http://codereview.chromium.org/19724/diff/278/282#newcode281
Line 281: *path = path->Append(kTempFileName);
On 2009/02/02 18:26:53, agl wrote:
> Something funny is going on here. @path is mutated to include the mkstemp
> template at the end, but then we take a copy and mkstemp mutates the copy. The
> end result is that @path is mutated, but has the wrong filename in it. Maybe
the
> copy should be a reference?

There is nothing funny here.
1. GetTempDir(): gets the temp directory (e.g. /tmp)
2. kTempFileName is appended (so now it's /tmp/com.google.chrome.XXXX)
3. *path = the return value, since Append() returns a new value and does not
mutate it's object
    (As an alternative, we could have done 
    FilePath baseparth; 
   GetTempDir(basepath);
   *path = basepath->Append(kTempFileName);
   (I've basically done that due to the shmem dir transition, but it doesn't
change the meaning)
4. mkstemp() replaces the Xs in the string.
All the file_util Temp functions take pointers, not refs, for the args they
modify, which is the right thing (C++ style guide).
Perhaps I misunderstand your objection?

http://codereview.chromium.org/19724/diff/278/282#newcode286
Line 286: int fd = mkstemp(buffer);
On 2009/02/02 18:26:53, agl wrote:
> This variable (@fd) isn't needed.

removed

http://codereview.chromium.org/19724/diff/278/282#newcode292
Line 292: int fd = CreateAndOpenFdForTemporaryFile(path);
On 2009/02/02 18:26:53, agl wrote:
> If CreateAndOpenFdForTemporaryFile doesn't unlink the file, then this function
> doesn't just create a filename - it also creates the file.

That is true, and that was the case before I changed the code.
In file_util.h there is this comment:
// TODO(erikkay): rename this function and track down all of the callers.
When I asked him, he was talking about renaming all BlahCreateFileName() calls
to be BlahCreateFile(), since that's what they do.  I will add a comment but
don't want to break previous semantics.

http://codereview.chromium.org/19724/diff/278/284
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/19724/diff/278/284#newcode108
Line 108: DCHECK(memname.find_first_of(L"/") == std::string::npos);
On 2009/02/02 18:26:53, agl wrote:
> One should also check for NUL charactors.

added

http://codereview.chromium.org/19724/diff/278/284#newcode111
Line 111: if (file_util::GetTempDir(&temp_dir) == false)
On 2009/02/02 18:26:53, agl wrote:
> Shared memory segments should be created in /dev/shm

Added (for Linux; there is no /dev/shm on OSX).
Also added a unit test for the new file_util::GetShmemTempDir().

http://codereview.chromium.org/19724/diff/278/284#newcode134
Line 134: DCHECK(posix_flags & O_RDWR);
On 2009/02/02 18:26:53, agl wrote:
> Should be (O_RDWR | O_WRONLY)

modified

http://codereview.chromium.org/19724/diff/278/284#newcode153
Line 153: // Careful: "w+" will delete if it already exists.
On 2009/02/02 18:26:53, agl wrote:
> s/delete/truncate

fixed

http://codereview.chromium.org/19724/diff/278/284#newcode167
Line 167: fp = file_util::OpenFile(mem_filename, mode.c_str());
On 2009/02/02 18:26:53, agl wrote:
> One cannot safely open tempfiles with OpenFile. One /must/ use one of the
> mkstemp family.

Look carefully at the "if" boundaries; this case is when name is not L"", so the
name is specified, so it is not a temp file.  This is the case where memory is
shared by name.  Although we don't plan to do that in Chrome (eventually), it is
definitely used in the unit tests.

http://codereview.chromium.org/19724/diff/278/284#newcode180
Line 180: if (fseek(fp, 0, SEEK_END) != 0)
On 2009/02/02 18:26:53, agl wrote:
> (Not important, but an fstat can do this in a single system call.)

transitioned

http://codereview.chromium.org/19724/diff/278/284#newcode188
Line 188: size_t writesize = size-current_size;
On 2009/02/02 18:26:53, agl wrote:
> Spaces around the -

spaced

http://codereview.chromium.org/19724/diff/278/284#newcode206
Line 206: DCHECK(mapped_file_);
On 2009/02/02 18:26:53, agl wrote:
> 0 is a valid file descriptor. This needs to be mapped_file_ >= 0.

adjusted

[agl, 2009-02-02 21:40:18]

http://codereview.chromium.org/19724/diff/278/282
File base/file_util_posix.cc (right):

http://codereview.chromium.org/19724/diff/278/282#newcode281
Line 281: *path = path->Append(kTempFileName);
On 2009/02/02 21:28:41, John Grabowski wrote:
> Perhaps I misunderstand your objection?

You're correct up to step 4. mkstemp mutates @buffer, which points into
@tmpdir_string, which is a copy of @path->value(). Since @tmpdir_string is a
copy, the mutation isn't reflected in @path.

http://codereview.chromium.org/19724/diff/278/284
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/19724/diff/278/284#newcode167
Line 167: fp = file_util::OpenFile(mem_filename, mode.c_str());
On 2009/02/02 21:28:41, John Grabowski wrote:
> Look carefully at the "if" boundaries; this case is when name is not L"", so
the
> name is specified, so it is not a temp file.

Yep, I was wrong here.

[John Grabowski, 2009-02-02 23:18:22]

http://codereview.chromium.org/19724/diff/278/282
File base/file_util_posix.cc (right):

http://codereview.chromium.org/19724/diff/278/282#newcode281
Line 281: *path = path->Append(kTempFileName);
On 2009/02/02 21:40:18, agl wrote:
> On 2009/02/02 21:28:41, John Grabowski wrote:
> > Perhaps I misunderstand your objection?
> 
> You're correct up to step 4. mkstemp mutates @buffer, which points into
> @tmpdir_string, which is a copy of @path->value(). Since @tmpdir_string is a
> copy, the mutation isn't reflected in @path.
> 

OK; I understand now.
There is no copy.  path->value() returns a reference.
In any case, I've done 2 things:
1. made it explicit by declaring tmpdir_string to be a reference.
2. Beefed up the unit test for this to now make sure tmpfiles don't have the
same name, proving it works (proving the name gets modified properly).

[agl, 2009-02-02 23:34:05]

LGTM

http://codereview.chromium.org/19724/diff/278/282
File base/file_util_posix.cc (right):

http://codereview.chromium.org/19724/diff/278/282#newcode281
Line 281: *path = path->Append(kTempFileName);
On 2009/02/02 23:18:22, John Grabowski wrote:
> 1. made it explicit by declaring tmpdir_string to be a reference.

"Made it explicit" suggests that it's a non-semantic change, but making it a
reference is a very semantically significant change.

I think the code is ok now. Submit as is is fine, although one can avoid a
const_cast by copying kTempFileName to a stack buffer, having mkstemp mutate
that and appending to @path.


AGL