Rework FileWatcher to avoid race condition upon deletion.

Rework FileWatcher to avoid race condition upon deletion.

BUG=none
TEST=none

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

[Mattias Nissler, 2010-08-06 14:04:38]

This is work in progress and not yet meant for review. I'm off for the weekend
in a few moments, so I'm sending this to you to let you know in which direction
I'm heading at the moment and you can comment whether you agree on the approach
I've taken. What I've done is basically make the delegate RefCountedThreadSafe,
so we can ensure it's not going away to early. That requires changes to
UserStyleSheetWatcher (basically splitting it into two parts). The unit tests
run and there are no memory leaks, so I think it's a possible solution to our
problem. Oh, and it's Linux only at the moment, I'll take a look at win and mac
next week.

[tony, 2010-08-06 20:55:02]

http://codereview.chromium.org/2868114/diff/1/2
File chrome/browser/file_watcher.h (right):

http://codereview.chromium.org/2868114/diff/1/2#newcode30
chrome/browser/file_watcher.h:30: // reference, since that would create a
reference cycle.
It seems like this will be hard to enforce and hard to notice.  I guess the
valgrind bots will catch this type of mistake . . .

http://codereview.chromium.org/2868114/diff/1/5
File chrome/browser/user_style_sheet_watcher.cc (right):

http://codereview.chromium.org/2868114/diff/1/5#newcode64
chrome/browser/user_style_sheet_watcher.cc:64: NewRunnableMethod(this,
&UserStyleSheetLoader::SetStyleSheet,
What happens if this gets called while UserStyleSheetLoader is already in the
destructor?  Won't this result in the same bug as before?

Maybe it's ok if we make sure to release the FileWatcher pointer before
releasing the UserStyleSheetLoader pointer, but I think it would still be
possible that FileWatcher is around when we're running ~UserStyleSheetLoader.

[Mattias Nissler, 2010-08-09 09:29:47]

http://codereview.chromium.org/2868114/diff/1/2
File chrome/browser/file_watcher.h (right):

http://codereview.chromium.org/2868114/diff/1/2#newcode30
chrome/browser/file_watcher.h:30: // reference, since that would create a
reference cycle.
On 2010/08/06 20:55:02, tony wrote:
> It seems like this will be hard to enforce and hard to notice.  I guess the
> valgrind bots will catch this type of mistake . . .

That's true. I'll just drop the comment.

http://codereview.chromium.org/2868114/diff/1/5
File chrome/browser/user_style_sheet_watcher.cc (right):

http://codereview.chromium.org/2868114/diff/1/5#newcode64
chrome/browser/user_style_sheet_watcher.cc:64: NewRunnableMethod(this,
&UserStyleSheetLoader::SetStyleSheet,
On 2010/08/06 20:55:02, tony wrote:
> What happens if this gets called while UserStyleSheetLoader is already in the
> destructor?  Won't this result in the same bug as before?

That cannot happen. FileWatcher holds a reference to FileWatcherImpl, which in
turn holds a reference to the callback delegate, which is UserStyleSheetLoader.
When deleting FileWatcher, pending tasks hold their own reference to
UserStyleSheetLoader and will only drop them after the task is done.

> 
> Maybe it's ok if we make sure to release the FileWatcher pointer before
> releasing the UserStyleSheetLoader pointer, but I think it would still be
> possible that FileWatcher is around when we're running ~UserStyleSheetLoader.

Hm, as per my argument above, that shouldn't be possible. Can you elaborate on
the scenario you have in mind?

[tony, 2010-08-09 17:17:21]

I'm convinced this will work.  Let me know when the change is ready for a full
review.

http://codereview.chromium.org/2868114/diff/1/2
File chrome/browser/file_watcher.h (right):

http://codereview.chromium.org/2868114/diff/1/2#newcode30
chrome/browser/file_watcher.h:30: // reference, since that would create a
reference cycle.
On 2010/08/09 09:29:47, mnissler wrote:
> On 2010/08/06 20:55:02, tony wrote:
> > It seems like this will be hard to enforce and hard to notice.  I guess the
> > valgrind bots will catch this type of mistake . . .
> 
> That's true. I'll just drop the comment.

Oh, I think it's worth it to keep the comment (if for no other reason than to
remind me), it just seems likely to regress.

http://codereview.chromium.org/2868114/diff/1/5
File chrome/browser/user_style_sheet_watcher.cc (right):

http://codereview.chromium.org/2868114/diff/1/5#newcode64
chrome/browser/user_style_sheet_watcher.cc:64: NewRunnableMethod(this,
&UserStyleSheetLoader::SetStyleSheet,
On 2010/08/09 09:29:47, mnissler wrote:
> Hm, as per my argument above, that shouldn't be possible. Can you elaborate on
> the scenario you have in mind?

You're right, that can't happen.  I am convinced that this works.  Can you draw
an ascii diagram at the top of this file describing the classes and ownership? 
Thanks!

[Mattias Nissler, 2010-08-09 18:31:27]

I've uploaded an updated version that also adjusts the mac and windows versions
of FileWatcherImpl. UserStyleSheetWatcher now has the problem of initializing
the FileWatcher from the wrong thread, which I'm going to fix tomorrow (and also
add the diagram). Apart from that, everything is in shape, so if you don't mind,
you can start reviewing now.

I've dropped the unit test I added for reproducing the race condition, since it
only reproduces the problem on Linux. For win and mac it's probably not possible
to come up with a unit test without fiddling with MessageLoop internals (so we
can delete FileWatcher immediately before the callback task runs). Moreover,
with the new code, the test would check whether a RefCountedThreadSafe we
operate on had a zero reference count. I think this is already covered in the
DCHECKS within RefCountedThreadSafe, so I thought it's not worth to add the
test.

On 2010/08/09 17:17:21, tony wrote:
> I'm convinced this will work.  Let me know when the change is ready for a full
> review.
> 
> http://codereview.chromium.org/2868114/diff/1/2
> File chrome/browser/file_watcher.h (right):
> 
> http://codereview.chromium.org/2868114/diff/1/2#newcode30
> chrome/browser/file_watcher.h:30: // reference, since that would create a
> reference cycle.
> On 2010/08/09 09:29:47, mnissler wrote:
> > On 2010/08/06 20:55:02, tony wrote:
> > > It seems like this will be hard to enforce and hard to notice.  I guess
the
> > > valgrind bots will catch this type of mistake . . .
> > 
> > That's true. I'll just drop the comment.
> 
> Oh, I think it's worth it to keep the comment (if for no other reason than to
> remind me), it just seems likely to regress.
> 
> http://codereview.chromium.org/2868114/diff/1/5
> File chrome/browser/user_style_sheet_watcher.cc (right):
> 
> http://codereview.chromium.org/2868114/diff/1/5#newcode64
> chrome/browser/user_style_sheet_watcher.cc:64: NewRunnableMethod(this,
> &UserStyleSheetLoader::SetStyleSheet,
> On 2010/08/09 09:29:47, mnissler wrote:
> > Hm, as per my argument above, that shouldn't be possible. Can you elaborate
on
> > the scenario you have in mind?
> 
> You're right, that can't happen.  I am convinced that this works.  Can you
draw
> an ascii diagram at the top of this file describing the classes and ownership?

> Thanks!

[tony, 2010-08-09 18:57:46]

In general, looks good.

If the test you added is consistent on Linux, maybe you can add it and put it
behind an #if defined(OS_LINUX)?

http://codereview.chromium.org/2868114/diff/7001/8007
File chrome/browser/user_style_sheet_watcher.h (right):

http://codereview.chromium.org/2868114/diff/7001/8007#newcode21
chrome/browser/user_style_sheet_watcher.h:21: class UserStyleSheetLoader :
public FileWatcher::Delegate {
Is it possible for us to forward declare put this in
user_style_sheet_watcher.cc?  I think you'd have to move user_style_sheet() and
the destructor into the .cc file too.

[Mattias Nissler, 2010-08-10 11:09:50]

Note I've readded the test and put a comment that indicates it doesn't actually
test anything on Mac and Windows.

http://codereview.chromium.org/2868114/diff/7001/8007
File chrome/browser/user_style_sheet_watcher.h (right):

http://codereview.chromium.org/2868114/diff/7001/8007#newcode21
chrome/browser/user_style_sheet_watcher.h:21: class UserStyleSheetLoader :
public FileWatcher::Delegate {
On 2010/08/09 18:57:46, tony wrote:
> Is it possible for us to forward declare put this in
> user_style_sheet_watcher.cc?  I think you'd have to move user_style_sheet()
and
> the destructor into the .cc file too.

Done.

[tony, 2010-08-10 17:14:35]

LGTM, thanks a lot for doing this!