Fixed shutdown concurrency issues in FilePathWatcher.

Fixed shutdown concurrency issues in FilePathWatcher.

BUG=None
TEST=FilePathWatcherTest(browser_tests)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79029
(dcommit crashed, closing manually)

[Joao da Silva, 2011-03-15 18:04:39]

FilePathWatcherImpl wasn't cleaning up correctly if its required thread was shut
down, and in some cases also wouldn't free its memory, due to being
DeleteOnFileThread.

Now the _mac and _win FilePathWatcherImpls observe their required threads (UI
and File, respectively) for destruction, and cleanup properly. The _inotify
version doesn't have this issue.

A related issue is the leakage of the impl, when the File thread isn't available
anymore. This is fixed by removing DeleteOnFileThread, which was only require in
_win. The _win version now handles that through listening for destruction of the
File thread for cleanup, and allowing the impl to be released on any thread.

[Bernhard Bauer, 2011-03-15 22:00:07]

We might want to have FileBasedPolicyLoader observe browser shutdown
notifications and stop watching the policy file, in order not to be on the
critical path later in the shutdown sequence.

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
File chrome/browser/file_path_watcher/file_path_watcher.h (right):

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher.h:38: // back for each
change. Returns true on success.
While you're at it, could you add a comment stating that this method should only
be called on the FILE thread?

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
File chrome/browser/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_mac.cc:121: // thread is
shutting down and cleans up, or the Task (that wasn't executed)
I guess the first case refers to WillDestroyCurrentMessageLoop? In that case, it
might be better to put that part of the comment there, or the whole comment at
the beginning of the class.

[Mattias Nissler (ping if slow), 2011-03-16 09:41:14]

First round of comments.

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
File chrome/browser/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_mac.cc:125:
DestroyEventStream();
If I understand correctly, the DestroyEventStream() should be a noop here since
we have either been canceled through the reposted Cancel() task or the message
loop observer. So shouldn't we rather put a DCHECK here that the event stream is
gone and/or canceled_ == true?

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_mac.cc:261: canceled_ = true;
Hm, I think we have a problem here. DestroyEventStream() is also called from
UpdateEventStream(), which might get called multiple times during the lifetime
of FilePathWatcher. However, if you set canceled_ to true here, all following
UpdateEventStream calls will bail out early, which is not what we want.

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_mac.cc:270:
MessageLoop::current()->RemoveDestructionObserver(this);
Same problem as above, this will kill our destruction observer early, even if
we're only reinitializing the watch.

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
File chrome/browser/file_path_watcher/file_path_watcher_win.cc (right):

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_win.cc:83: if
(!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
We are guaranteed to run on the file thread here. UpdateWatch also requires
this. See FilePathWatcher::Watch(). Putting a DCHECK in is probably still a good
idea, even if only for documentation purposes.

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_win.cc:129: DestroyWatch();
Same comment as for Mac: Shouldn't we already have been shut down at this point
and can we DCHECK for that?

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_win.cc:275:
MessageLoop::current()->RemoveDestructionObserver(this);
Oh, so what happens when UpdateWatch is executed multiple times during
operation? Don't kill our destruction observer early then?

[Joao da Silva, 2011-03-18 16:09:17]

This CL has been updated after the changes introduced in
http://codereview.chromium.org/6670081.

The bugs still exist after that change, and this CL solves them. I verified that
the FileBasedPolicyLoader is deleted at the right time during shutdown (as well
as the whole policy stuff through the BrowserPolicyConnector), but what causes
the bug is the reference held by the FilePathWatcherImpl, as the
FileBasedPolicyLoader is its delegate. Observing browser shutdown won't solve
this issue.

Would be nice to break the reference cycle, maybe in a future CL.

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
File chrome/browser/file_path_watcher/file_path_watcher.h (right):

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher.h:38: // back for each
change. Returns true on success.
On 2011/03/15 22:00:07, Bernhard Bauer wrote:
> While you're at it, could you add a comment stating that this method should
only
> be called on the FILE thread?

Done.

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
File chrome/browser/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_mac.cc:121: // thread is
shutting down and cleans up, or the Task (that wasn't executed)
On 2011/03/15 22:00:07, Bernhard Bauer wrote:
> I guess the first case refers to WillDestroyCurrentMessageLoop? In that case,
it
> might be better to put that part of the comment there, or the whole comment at
> the beginning of the class.

Done.

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_mac.cc:125:
DestroyEventStream();
On 2011/03/16 09:41:14, Mattias Nissler wrote:
> If I understand correctly, the DestroyEventStream() should be a noop here
since
> we have either been canceled through the reposted Cancel() task or the message
> loop observer. So shouldn't we rather put a DCHECK here that the event stream
is
> gone and/or canceled_ == true?
This covers the case when Cancel() tries to switch to the UI thread by posting a
task, but that task is never executed (because the MessageLoop is shutting
down).
In that case, either
1) the Task is deleted, and we enter this dtor and have to cancel now (notice
that calling Cancel() from the dtor will only perform any work in this case, and
it's the right thread), or
2) the Task is NOT deleted (cause we're not running under Valgrind or something)
and was not executed. In this case, we Cancel() when the thread is about to
terminate, on WillDestroyCurrentMessageLoop.

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_mac.cc:261: canceled_ = true;
On 2011/03/16 09:41:14, Mattias Nissler wrote:
> Hm, I think we have a problem here. DestroyEventStream() is also called from
> UpdateEventStream(), which might get called multiple times during the lifetime
> of FilePathWatcher. However, if you set canceled_ to true here, all following
> UpdateEventStream calls will bail out early, which is not what we want.
Horrible mistake, thanks for spotting. The try bots agree with you :-)

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
File chrome/browser/file_path_watcher/file_path_watcher_win.cc (right):

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_win.cc:83: if
(!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
On 2011/03/16 09:41:14, Mattias Nissler wrote:
> We are guaranteed to run on the file thread here. UpdateWatch also requires
> this. See FilePathWatcher::Watch(). Putting a DCHECK in is probably still a
good
> idea, even if only for documentation purposes.

Done.

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_win.cc:129: DestroyWatch();
On 2011/03/16 09:41:14, Mattias Nissler wrote:
> Same comment as for Mac: Shouldn't we already have been shut down at this
point
> and can we DCHECK for that?
Same reply as for Mac: this might be required when Cancel() has to post a task
to the file thread, that is never executed during shutdown.

http://codereview.chromium.org/6697020/diff/1/chrome/browser/file_path_watche...
chrome/browser/file_path_watcher/file_path_watcher_win.cc:275:
MessageLoop::current()->RemoveDestructionObserver(this);
On 2011/03/16 09:41:14, Mattias Nissler wrote:
> Oh, so what happens when UpdateWatch is executed multiple times during
> operation? Don't kill our destruction observer early then?
Same mistake as on the Mac version.

[Mattias Nissler (ping if slow), 2011-03-21 16:07:45]

Looks like the right approach in general. However, I still don't understand why
we would need the CancelInternal() in the dtor.

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
File content/common/file_path_watcher/file_path_watcher_inotify.cc (right):

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_inotify.cc:397: // executed
during shutdown, and |this| is just Released.
But shouldn't we then see the WillDestroyCurrentMessageLoop() invocation? If we
call CancelInternal from here, how can we be sure that MessageLoop::current() is
non-NULL?

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:44: // In either case,
DestroyEventStream() is called on the right thread.
Does the second case actually happen? I would expect that before all tasks get
deleted, the WillDestroyCurrentMessageLoop() gets called. So why would we need
code in the dtor?

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:264: //
DCHECK(run_loop_message_loop()->BelongsToCurrentThread());
So just remove this block?

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
File content/common/file_path_watcher/file_path_watcher_win.cc (right):

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_win.cc:25: // In either case,
we are on the right thread when cleaning up.
same question as for the others

[Joao da Silva, 2011-03-22 10:10:08]

Thanks for offline discussions.
Please see comments.

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
File content/common/file_path_watcher/file_path_watcher_inotify.cc (right):

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_inotify.cc:397: // executed
during shutdown, and |this| is just Released.
On 2011/03/21 16:07:45, Mattias Nissler wrote:
> But shouldn't we then see the WillDestroyCurrentMessageLoop() invocation? If
we
> call CancelInternal from here, how can we be sure that MessageLoop::current()
is
> non-NULL?
I saw a code path that actually triggers this, though it's an unlikely event.
First, ~FilePathWatcher() calls Cancel(), and a Task is posted. The Task holds a
reference to |this|, and the FilePathWatcher releases its reference. So the Task
is holding the object. But the message loop of the Task is already shutting
down, and the Task is never executed. And before notifying observers of the
destruction of the message loop, the message loop deletes pending tasks. At 
this point, |this| is destroyed and never cleaned up. It must clean up now,
otherwise the destruction notification will be triggered on a stale pointer.

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:44: // In either case,
DestroyEventStream() is called on the right thread.
On 2011/03/21 16:07:45, Mattias Nissler wrote:
> Does the second case actually happen? I would expect that before all tasks get
> deleted, the WillDestroyCurrentMessageLoop() gets called. So why would we need
> code in the dtor?
It does happen, see previous comment.

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:264: //
DCHECK(run_loop_message_loop()->BelongsToCurrentThread());
On 2011/03/21 16:07:45, Mattias Nissler wrote:
> So just remove this block?

Done.

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
File content/common/file_path_watcher/file_path_watcher_win.cc (right):

http://codereview.chromium.org/6697020/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_win.cc:25: // In either case,
we are on the right thread when cleaning up.
On 2011/03/21 16:07:45, Mattias Nissler wrote:
> same question as for the others
Same reply :-)

[Joao da Silva, 2011-03-22 16:31:07]

Here's a revised version, using a custom Task to Cancel the
FilePathWatcher::PlatformDelegate either when run or deleted.

Manually tested and officially supported by the try bots :-)

Mattias: please have another look, and thanks for the suggestion.

[Mattias Nissler (ping if slow), 2011-03-22 17:23:43]

I like this version. The only thing I'm unsure about is writing the cancelled
flag from the various Cancel() methods on potentially the wrong thread. Is this
safe w.r.t. to races on the flag?

http://codereview.chromium.org/6697020/diff/15001/content/common/file_path_wa...
File content/common/file_path_watcher/file_path_watcher.h (right):

http://codereview.chromium.org/6697020/diff/15001/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher.h:54: class CancelTask:
public Task {
missing space before :

http://codereview.chromium.org/6697020/diff/15001/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher.h:65:
scoped_refptr<PlatformDelegate> delegate_;
DISALLOW_COPY_AND_ASSIGN

[Joao da Silva, 2011-03-22 17:42:33]

Thanks for reviewing, see comments below.

> I like this version. The only thing I'm unsure about is writing the cancelled
> flag from the various Cancel() methods on potentially the wrong thread. Is
this
> safe w.r.t. to races on the flag?

I believe it's safe as it is. set_cancelled() is called on only 2 cases:

1) on Cancel(), when Watch() was never called. This could be on any thread, but
is not a problem. There is nothing to clean up in this case, Cancel() was called
from ~FilePathWatcher(), and after returning from Cancel() the FilePathWatcher
will release its reference on the FilePathWatcherImpl, which will be destructed
and checks the flag, all in the same thread. So this case is OK.

2) Cancel() posts a CancelTask. In this case, the CancelTask will hold a
reference to the FilePathWatcherImpl, preventing it from being destroyed with
the FilePathWatcher. The CancelTask will call CancelOnMessageLoopThread() on the
right thread, which will call set_cancelled(). Once this is done, the task is
also done and is deleted, releasing the reference on the Impl and is_cancelled()
is then called on the destructor. This is again happening on the same thread as
the set_cancelled() call.

There is also one call to is_cancelled() from UpdateStream(), but this call can
only happen on the "cleanup" thread from case 2 above, so it's not an issue
either.

I hope the long text is not confusing :-) Does it make sense to you?

http://codereview.chromium.org/6697020/diff/15001/content/common/file_path_wa...
File content/common/file_path_watcher/file_path_watcher.h (right):

http://codereview.chromium.org/6697020/diff/15001/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher.h:54: class CancelTask:
public Task {
On 2011/03/22 17:23:44, Mattias Nissler wrote:
> missing space before :

Done.

http://codereview.chromium.org/6697020/diff/15001/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher.h:65:
scoped_refptr<PlatformDelegate> delegate_;
On 2011/03/22 17:23:44, Mattias Nissler wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Mattias Nissler (ping if slow), 2011-03-22 17:48:17]

On 2011/03/22 17:42:33, joaodasilva wrote:
> Thanks for reviewing, see comments below.
> 
> > I like this version. The only thing I'm unsure about is writing the
cancelled
> > flag from the various Cancel() methods on potentially the wrong thread. Is
> this
> > safe w.r.t. to races on the flag?
> 
> I believe it's safe as it is. set_cancelled() is called on only 2 cases:
> 
> 1) on Cancel(), when Watch() was never called. This could be on any thread,
but
> is not a problem. There is nothing to clean up in this case, Cancel() was
called
> from ~FilePathWatcher(), and after returning from Cancel() the FilePathWatcher
> will release its reference on the FilePathWatcherImpl, which will be
destructed
> and checks the flag, all in the same thread. So this case is OK.

I was actually wondering about concurrent calls to Watch() that would race
against the Cancel(), but this cannot happen since Watch() and Cancel() must be
called on the same thread.

> 
> 2) Cancel() posts a CancelTask. In this case, the CancelTask will hold a
> reference to the FilePathWatcherImpl, preventing it from being destroyed with
> the FilePathWatcher. The CancelTask will call CancelOnMessageLoopThread() on
the
> right thread, which will call set_cancelled(). Once this is done, the task is
> also done and is deleted, releasing the reference on the Impl and
is_cancelled()
> is then called on the destructor. This is again happening on the same thread
as
> the set_cancelled() call.

Correct.

> 
> There is also one call to is_cancelled() from UpdateStream(), but this call
can
> only happen on the "cleanup" thread from case 2 above, so it's not an issue
> either.
> 
> I hope the long text is not confusing :-) Does it make sense to you?

Yes, thanks.

> 
>
http://codereview.chromium.org/6697020/diff/15001/content/common/file_path_wa...
> File content/common/file_path_watcher/file_path_watcher.h (right):
> 
>
http://codereview.chromium.org/6697020/diff/15001/content/common/file_path_wa...
> content/common/file_path_watcher/file_path_watcher.h:54: class CancelTask:
> public Task {
> On 2011/03/22 17:23:44, Mattias Nissler wrote:
> > missing space before :
> 
> Done.
> 
>
http://codereview.chromium.org/6697020/diff/15001/content/common/file_path_wa...
> content/common/file_path_watcher/file_path_watcher.h:65:
> scoped_refptr<PlatformDelegate> delegate_;
> On 2011/03/22 17:23:44, Mattias Nissler wrote:
> > DISALLOW_COPY_AND_ASSIGN
> 
> Done.

And finally: LGTM!