Thread IO safety: file_util annotated, IO thread blocked from doing IO

Thread IO safety: annotate file_util, and block IO thread from doing IO

- Mark functions in file_util_posix as requiring permission to perform
  disk actions.
- Mark the IO thread as disallowed from performing disk actions.
- Temporarily work around the protections in places where we currently
  have bugs.

BUG=59847, 59849, 60207, 60211
TEST=no dchecks in debug builds

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

[Evan Martin, 2010-10-21 17:24:04]

jam: FYI
will: you might as well review, since we discussed this so much

[willchan no longer on Chromium, 2010-10-21 17:45:54]

I just took a look at your thread_restrictions.cc implementation.  Isn't this
going to crash on shutdown?  LazyInstance will get destroyed by the
AtExitManager, by which point out known browser threads are dead, but there's no
guarantee that a worker pool thread is dead.  So, if the worker thread invokes
one of these file functions after the AtExitManager has run, it looks like we
will crash in debug mode on shutdown.  I am concerned that this will cause flaky
shutdown crashes in the disk cache code (see net/base/file_stream_posix.cc),
since that code posts to the worker thread and does file I/O.

Btw, are you planning on getting to base/platform_file at some point?

[Evan Martin, 2010-10-21 17:51:16]

On 2010/10/21 17:45:54, willchan wrote:
> I just took a look at your thread_restrictions.cc implementation.  Isn't this
> going to crash on shutdown?  LazyInstance will get destroyed by the
> AtExitManager, by which point out known browser threads are dead, but there's
no
> guarantee that a worker pool thread is dead.

I will make it leak on exit, and send you a different CL first.

[Evan Martin, 2010-10-21 18:07:45]

On 2010/10/21 17:45:54, willchan wrote:
> I just took a look at your thread_restrictions.cc implementation.  Isn't this
> going to crash on shutdown?

I sent a separate change for that.  Can you review this one assuming that
ThreadRestrictions works, and I will wait until the other change is good before
committing this?

> Btw, are you planning on getting to base/platform_file at some point?

Yes.  And the UI thread, and Mac, and Windows... starting small

[willchan no longer on Chromium, 2010-10-21 18:45:06]

http://codereview.chromium.org/3872002/diff/12001/13001
File base/file_util_posix.cc (right):

http://codereview.chromium.org/3872002/diff/12001/13001#newcode53
base/file_util_posix.cc:53: if (!realpath(path.value().c_str(), buf))
Are you ignoring this one on purpose?

http://codereview.chromium.org/3872002/diff/12001/13001#newcode565
base/file_util_posix.cc:565: if (!getcwd(system_buffer, sizeof(system_buffer)))
{
How about this one?

http://codereview.chromium.org/3872002/diff/12001/13001#newcode575
base/file_util_posix.cc:575: int ret = chdir(path.value().c_str());
Shouldn't this block too?

http://codereview.chromium.org/3872002/diff/12001/13001#newcode745
base/file_util_posix.cc:745: void MemoryMappedFile::CloseHandles() {
This one?

http://codereview.chromium.org/3872002/diff/12001/13002
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/3872002/diff/12001/13002#newcode152
base/shared_memory_posix.cc:152: // and be deleted before they ever make it out
to disk.
Where are we hitting this?  I wonder if this is a
browser-process/renderer-process specific assumption?  It seems unfortunate to
have to allow this at such a low level.

http://codereview.chromium.org/3872002/diff/12001/13004
File base/thread_restrictions.h (right):

http://codereview.chromium.org/3872002/diff/12001/13004#newcode46
base/thread_restrictions.h:46: class ScopedAllowIO {
within a class definition, nested class definitions go first, ahead of function
declarations.

http://codereview.chromium.org/3872002/diff/12001/13011
File chrome/test/out_of_proc_test_runner.cc (right):

http://codereview.chromium.org/3872002/diff/12001/13011#newcode488
chrome/test/out_of_proc_test_runner.cc:488: // Run AtExit handlers once per
iteration.
I don't know enough about this code to say if this is correct.  Gimme a sec to
look deeper.

[willchan no longer on Chromium, 2010-10-21 18:50:53]

http://codereview.chromium.org/3872002/diff/12001/13011
File chrome/test/out_of_proc_test_runner.cc (right):

http://codereview.chromium.org/3872002/diff/12001/13011#newcode488
chrome/test/out_of_proc_test_runner.cc:488: // Run AtExit handlers once per
iteration.
On 2010/10/21 18:45:08, willchan wrote:
> I don't know enough about this code to say if this is correct.  Gimme a sec to
> look deeper.

I looked at this again, why is this necessary?  Are we using
Singleton/LazyInstance in the test launcher code?  The actual unit test code is
run in a separate process, so I'm not sure what this is here for.  Oh, is this
because we use thread locals in ThreadRestrictions?

[Evan Martin, 2010-10-21 20:43:02]

Landed the leak fix, responded to your comments.

http://codereview.chromium.org/3872002/diff/12001/13001
File base/file_util_posix.cc (right):

http://codereview.chromium.org/3872002/diff/12001/13001#newcode53
base/file_util_posix.cc:53: if (!realpath(path.value().c_str(), buf))
On 2010/10/21 18:45:08, willchan wrote:
> Are you ignoring this one on purpose?

Nope, fixed.

http://codereview.chromium.org/3872002/diff/12001/13001#newcode565
base/file_util_posix.cc:565: if (!getcwd(system_buffer, sizeof(system_buffer)))
{
On 2010/10/21 18:45:08, willchan wrote:
> How about this one?

Figured this one is ok, though I'm not sure.  I will try it and see if there are
any surprises.

http://codereview.chromium.org/3872002/diff/12001/13001#newcode575
base/file_util_posix.cc:575: int ret = chdir(path.value().c_str());
On 2010/10/21 18:45:08, willchan wrote:
> Shouldn't this block too?

I had reasoned like getcwd, but it seems more likely this would block, fixed.

http://codereview.chromium.org/3872002/diff/12001/13001#newcode745
base/file_util_posix.cc:745: void MemoryMappedFile::CloseHandles() {
On 2010/10/21 18:45:08, willchan wrote:
> This one?

Had figured you couldn't close without opening, but it could be passed between
threads and it can't hurt to check.

http://codereview.chromium.org/3872002/diff/12001/13002
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/3872002/diff/12001/13002#newcode152
base/shared_memory_posix.cc:152: // and be deleted before they ever make it out
to disk.
On 2010/10/21 18:45:08, willchan wrote:
> Where are we hitting this?  I wonder if this is a
> browser-process/renderer-process specific assumption?  It seems unfortunate to
> have to allow this at such a low level.

IPC messages, which happen on the IO thread, involve creating/destroying shared
memory.  I think it would be very difficult to change the architecture around
it.  Markus and I discussed this a bit and eventually I convinced myself that if
/dev/shm is about as likely to be paged out as your main browser process.

[willchan no longer on Chromium, 2010-10-21 20:57:40]

LGTM

http://codereview.chromium.org/3872002/diff/12001/13001
File base/file_util_posix.cc (right):

http://codereview.chromium.org/3872002/diff/12001/13001#newcode565
base/file_util_posix.cc:565: if (!getcwd(system_buffer, sizeof(system_buffer)))
{
On 2010/10/21 20:43:02, Evan Martin wrote:
> On 2010/10/21 18:45:08, willchan wrote:
> > How about this one?
> 
> Figured this one is ok, though I'm not sure.  I will try it and see if there
are
> any surprises.

From the manpage:
"The getcwd() function copies an absolute pathname of the current working
directory to the array pointed to by buf, which is of length size."
and
"ENOENT The current working directory has been unlinked."

Doesn't this mean it requires hitting disk?

http://codereview.chromium.org/3872002/diff/12001/13002
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/3872002/diff/12001/13002#newcode152
base/shared_memory_posix.cc:152: // and be deleted before they ever make it out
to disk.
On 2010/10/21 20:43:02, Evan Martin wrote:
> On 2010/10/21 18:45:08, willchan wrote:
> > Where are we hitting this?  I wonder if this is a
> > browser-process/renderer-process specific assumption?  It seems unfortunate
to
> > have to allow this at such a low level.
> 
> IPC messages, which happen on the IO thread, involve creating/destroying
shared
> memory.  I think it would be very difficult to change the architecture around
> it.  Markus and I discussed this a bit and eventually I convinced myself that
if
> /dev/shm is about as likely to be paged out as your main browser process.

The /dev/shm assumption seems reasonable, at least afaict, but what about when
|name| is non-empty, so it's not necessarily on /dev/shm?

[Evan Martin, 2010-10-21 21:54:50]

Note new file in this change: chrome/browser/service/service_process_control.cc

http://codereview.chromium.org/3872002/diff/12001/13002
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/3872002/diff/12001/13002#newcode152
base/shared_memory_posix.cc:152: // and be deleted before they ever make it out
to disk.
On 2010/10/21 20:57:41, willchan wrote:
> On 2010/10/21 20:43:02, Evan Martin wrote:
> > On 2010/10/21 18:45:08, willchan wrote:
> > > Where are we hitting this?  I wonder if this is a
> > > browser-process/renderer-process specific assumption?  It seems
unfortunate
> to
> > > have to allow this at such a low level.
> > 
> > IPC messages, which happen on the IO thread, involve creating/destroying
> shared
> > memory.  I think it would be very difficult to change the architecture
around
> > it.  Markus and I discussed this a bit and eventually I convinced myself
that
> if
> > /dev/shm is about as likely to be paged out as your main browser process.
> 
> The /dev/shm assumption seems reasonable, at least afaict, but what about when
> |name| is non-empty, so it's not necessarily on /dev/shm?

name is just the filename, not the path; it's still /dev/shm.
(It's only used for statstable.)

[Evan Martin, 2010-10-21 22:45:52]

Oops, mentioned your earlier LGTM (my only changes since that are to put more
AssertIOAllowed and to mark one more buggy file)

[Evan Martin, 2010-10-21 22:46:04]

On 2010/10/21 22:45:52, Evan Martin wrote:
> Oops, mentioned your earlier LGTM (my only changes since that are to put more

s/mentioned/missed/

> AssertIOAllowed and to mark one more buggy file)