Remove ref-counting from FilePathWatcher.

Remove ref-counting from FilePathWatcher.

Keep the PlatformDelegate in a std::unique_ptr instead of a scoped_refptr
in FilePathWatcher. Use weak ptrs when posting callbacks bound to a
PlatformDelegate.

With this change, it is guaranteed that a PlatformDelegate does not
outlive its parent FilePathWatcher. This will allow multiple code
simplifications in FilePathWatcher.

BUG=675631
Review-Url: https://codereview.chromium.org/2596273003
Cr-Commit-Position: refs/heads/master@{#442404}
Committed: https://chromium.googlesource.com/chromium/src/+/0ac2f0f892f6c9998134902dbc0fc3d78d8517a9

[fdoray, 2016-12-23 13:51:11]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 13:51:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2016-12-23 13:59:56]

Description was changed from

==========
Do not allow multiple references to Linux's FilePathWatcherImpl.

Before this CL, both FilePathWatcher and a callback posted from
a Inotify thread could hold a reference to a FilePathWatcherImpl
on Linux. With this CL, the reference in the callback posted from
the Inotify thread is replaced with a weak ptr. This means that
there is always exactly one reference to a given FilePathWatcherImpl
instance and it is owned by a FilePathWatcher.

This is a first step towards getting rid of ref-counting in
FilePathWatcher. It also reduces the number of lines of code
in file_path_watcher_linux.cc.

BUG=675631
==========

to

==========
Keep a single reference to Linux's FilePathWatcherImpl.

Before this CL, both FilePathWatcher and a callback posted from
a Inotify thread could hold a reference to a FilePathWatcherImpl
on Linux. With this CL, the reference in the callback posted from
the Inotify thread is replaced with a weak ptr. This means that
there is always exactly one reference to a given FilePathWatcherImpl
instance and it is owned by a FilePathWatcher.

This is a first step towards getting rid of ref-counting in
FilePathWatcher. It also reduces the number of lines of code
in file_path_watcher_linux.cc.

BUG=675631
==========

[commit-bot: I haz the power, 2016-12-23 14:38:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 14:38:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2016-12-23 15:34:03]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 15:34:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2016-12-23 15:34:45]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org

[fdoray, 2016-12-23 15:34:46]

PTAL

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
File base/files/file_path_watcher_linux.cc (right):

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
base/files/file_path_watcher_linux.cc:328: // cancelled).
Cancel() is only called from ~FilePathWatcher
https://cs.chromium.org/chromium/src/base/files/file_path_watcher.cc?sq=packa...
With this CL, the only reference to |this| is owned by FilePathWatcher. This
means that |this| is destroyed and its weak ptrs are invalidated right after
Cancel() is called.

In separate CLs, I plan to:
- Get rid of ref-counting in file_path_watcher_*, to make it clear that a
FilePathWatcherImpl is owned by a FilePathWatcher (and does not outlive this
FilePathWatcher).
- Move code from FilePathWatcherImpl::Cancel() to
FilePathWatcherImpl::~FilePathWatcherImpl() and remove
FilePathWatcherImpl::Cancel().

[gab, 2016-12-23 16:05:06]

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
File base/files/file_path_watcher_linux.cc (right):

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
base/files/file_path_watcher_linux.cc:325: // This method is invoked on the
Inotify thread. Switch to task_runner() to
DCHECK not on TaskRunner thread?

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
base/files/file_path_watcher_linux.cc:328: // cancelled).
On 2016/12/23 15:34:45, fdoray wrote:
> Cancel() is only called from ~FilePathWatcher
>
https://cs.chromium.org/chromium/src/base/files/file_path_watcher.cc?sq=packa...
> With this CL, the only reference to |this| is owned by FilePathWatcher. This
> means that |this| is destroyed and its weak ptrs are invalidated right after
> Cancel() is called.
> 
> In separate CLs, I plan to:
> - Get rid of ref-counting in file_path_watcher_*, to make it clear that a
> FilePathWatcherImpl is owned by a FilePathWatcher (and does not outlive this
> FilePathWatcher).
> - Move code from FilePathWatcherImpl::Cancel() to
> FilePathWatcherImpl::~FilePathWatcherImpl() and remove
> FilePathWatcherImpl::Cancel(). 

Ah cool now I see why you'd made all those changes. I think we should bring the
refcount change to this CL though as mixing WeakPtr and refcounting semantics is
weird.

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
base/files/file_path_watcher_linux.cc:331: weak_factory_.GetWeakPtr(),
fired_watch, child, created,
It's not safe to vend WeakPtr from one sequence and deref on another. You'll
need to store a WeakPtr as member in constructor I think (as you were doing
previously but I hadn't realized what was going on)

[commit-bot: I haz the power, 2016-12-23 16:21:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 16:21:17]

Dry run: This issue passed the CQ dry run.

[fdoray, 2016-12-23 21:06:22]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 21:06:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2016-12-23 21:11:53]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 21:12:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2016-12-23 21:15:41]

Description was changed from

==========
Keep a single reference to Linux's FilePathWatcherImpl.

Before this CL, both FilePathWatcher and a callback posted from
a Inotify thread could hold a reference to a FilePathWatcherImpl
on Linux. With this CL, the reference in the callback posted from
the Inotify thread is replaced with a weak ptr. This means that
there is always exactly one reference to a given FilePathWatcherImpl
instance and it is owned by a FilePathWatcher.

This is a first step towards getting rid of ref-counting in
FilePathWatcher. It also reduces the number of lines of code
in file_path_watcher_linux.cc.

BUG=675631
==========

to

==========
Remove ref-counting from FilePathWatcher.

Keep the PlatformDelegate in a std::unique_ptr instead of a scoped_refptr
in FilePathWatcher. Use weak ptrs when posting callbacks bound to a
PlatformDelegate.

With this change, it is guaranteed that a PlatformDelegate does not
outlive its parent FilePathWatcher. This will allow multiple code
simplifications in FilePathWatcher.

BUG=675631
==========

[fdoray, 2016-12-23 21:16:07]

PTAnL

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
File base/files/file_path_watcher_linux.cc (right):

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
base/files/file_path_watcher_linux.cc:325: // This method is invoked on the
Inotify thread. Switch to task_runner() to
On 2016/12/23 16:05:06, gab (HOHOHO) wrote:
> DCHECK not on TaskRunner thread?

Done.

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
base/files/file_path_watcher_linux.cc:328: // cancelled).
On 2016/12/23 16:05:06, gab (HOHOHO) wrote:
> On 2016/12/23 15:34:45, fdoray wrote:
> > Cancel() is only called from ~FilePathWatcher
> >
>
https://cs.chromium.org/chromium/src/base/files/file_path_watcher.cc?sq=packa...
> > With this CL, the only reference to |this| is owned by FilePathWatcher. This
> > means that |this| is destroyed and its weak ptrs are invalidated right after
> > Cancel() is called.
> > 
> > In separate CLs, I plan to:
> > - Get rid of ref-counting in file_path_watcher_*, to make it clear that a
> > FilePathWatcherImpl is owned by a FilePathWatcher (and does not outlive this
> > FilePathWatcher).
> > - Move code from FilePathWatcherImpl::Cancel() to
> > FilePathWatcherImpl::~FilePathWatcherImpl() and remove
> > FilePathWatcherImpl::Cancel(). 
> 
> Ah cool now I see why you'd made all those changes. I think we should bring
the
> refcount change to this CL though as mixing WeakPtr and refcounting semantics
is
> weird.

Done.

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
base/files/file_path_watcher_linux.cc:331: weak_factory_.GetWeakPtr(),
fired_watch, child, created,
On 2016/12/23 16:05:06, gab (HOHOHO) wrote:
> It's not safe to vend WeakPtr from one sequence and deref on another. You'll
> need to store a WeakPtr as member in constructor I think (as you were doing
> previously but I hadn't realized what was going on)

Actually, the WeakPtrs of a given WeakPtrFactory are bound to the first sequence
on which one of them is dereferenced. There is nothing unsafe about getting a
WeakPtr from the factory on another sequence (as long as we know that the
factory won't be deleted during the call - which is the case because
OnFilePathChanged is called within the scope of InotifyReader::lock_ and
~FilePathWatcherImpl removes |this| from InotifyReader::watchers_ within the
scope of InotifyReader::lock_).

[commit-bot: I haz the power, 2016-12-23 21:20:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 21:20:52]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2017-01-05 21:22:54]

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
File base/files/file_path_watcher_linux.cc (right):

https://codereview.chromium.org/2596273003/diff/40001/base/files/file_path_wa...
base/files/file_path_watcher_linux.cc:331: weak_factory_.GetWeakPtr(),
fired_watch, child, created,
On 2016/12/23 21:16:07, fdoray wrote:
> On 2016/12/23 16:05:06, gab (HOHOHO) wrote:
> > It's not safe to vend WeakPtr from one sequence and deref on another. You'll
> > need to store a WeakPtr as member in constructor I think (as you were doing
> > previously but I hadn't realized what was going on)
> 
> Actually, the WeakPtrs of a given WeakPtrFactory are bound to the first
sequence
> on which one of them is dereferenced. There is nothing unsafe about getting a
> WeakPtr from the factory on another sequence (as long as we know that the
> factory won't be deleted during the call - which is the case because
> OnFilePathChanged is called within the scope of InotifyReader::lock_ and
> ~FilePathWatcherImpl removes |this| from InotifyReader::watchers_ within the
> scope of InotifyReader::lock_).

Good point, and it's implicit that FilePathWatcherImpl must not be deleted while
a member method call is in progress so it works, don't mind me :)!

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
base/files/file_path_watcher_fsevents.cc:78: DCHECK(!task_runner() ||
task_runner()->RunsTasksOnCurrentThread());
Is this a new requirement? Where is it documented?

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
base/files/file_path_watcher_fsevents.cc:164: dispatch_async(watcher->queue_, ^{
Is the async dispatch required here? i.e. it was required before to avoid
running from within the callback itself but this won't be the case from the
posted task, right?

PS: I don't understand ObjC :), can you have someone who does take a look as
well?

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
File base/files/file_path_watcher_kqueue.cc (right):

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
base/files/file_path_watcher_kqueue.cc:272:
Bind(&FilePathWatcherKQueue::OnKQueueReadable, Unretained(this)));
Can you add a comment explaining why it's okay to Unretained here?

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
File base/files/file_path_watcher_stub.cc (right):

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
base/files/file_path_watcher_stub.cc:30:
DISALLOW_COPY_AND_ASSIGN(FilePathWatcherImpl);
Probably need an explicit public default constructor because of this?

[fdoray, 2017-01-05 23:03:24]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 23:04:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fdoray, 2017-01-05 23:04:10]

fdoray@chromium.org changed reviewers:
+ mark@chromium.org

[fdoray, 2017-01-05 23:04:11]

gab@: PTAnL

mark@: Please review changes in base/files/file_path_watcher_fsevents.cc

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
base/files/file_path_watcher_fsevents.cc:78: DCHECK(!task_runner() ||
task_runner()->RunsTasksOnCurrentThread());
On 2017/01/05 21:22:53, gab wrote:
> Is this a new requirement? Where is it documented?

The DCHECK below is not new.

The DCHECK above documents an existing requirement:

A FilePathWatcherFSEvents can only be owned by a FilePathWatcherMac which can
only be owned by a FilePathWatcher. That means that the destructor of
FilePathWatcherFSEvents can only be invoked (indirectly) by the destructor of
FilePathWatcher. task_runner() is set in Watch(). From
https://cs.chromium.org/chromium/src/base/files/file_path_watcher.h?l=30:
"[FilePathWatcher] must be destroyed on the sequence that invokes Watch()."

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
base/files/file_path_watcher_fsevents.cc:164: dispatch_async(watcher->queue_, ^{
On 2017/01/05 21:22:53, gab wrote:
> Is the async dispatch required here? i.e. it was required before to avoid
> running from within the callback itself but this won't be the case from the
> posted task, right?
> 
> PS: I don't understand ObjC :), can you have someone who does take a look as
> well?

Yes it's required. UpdateEventStream() must run on the same GCD sequence as
StartEventStream/DestroyEventStream/FSEventsCallback. I tried to clarify the
comment. Added mark@ to review this.

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
File base/files/file_path_watcher_kqueue.cc (right):

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
base/files/file_path_watcher_kqueue.cc:272:
Bind(&FilePathWatcherKQueue::OnKQueueReadable, Unretained(this)));
On 2017/01/05 21:22:53, gab wrote:
> Can you add a comment explaining why it's okay to Unretained here?

Done.

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
File base/files/file_path_watcher_stub.cc (right):

https://codereview.chromium.org/2596273003/diff/100001/base/files/file_path_w...
base/files/file_path_watcher_stub.cc:30:
DISALLOW_COPY_AND_ASSIGN(FilePathWatcherImpl);
On 2017/01/05 21:22:53, gab wrote:
> Probably need an explicit public default constructor because of this?

Done.

[commit-bot: I haz the power, 2017-01-05 23:16:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 23:16:31]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[gab, 2017-01-09 17:21:39]

lgtm (and I'm happy with the ObjC code on second pass as well w/ your replies),
thanks!

https://codereview.chromium.org/2596273003/diff/120001/base/files/file_path_w...
File base/files/file_path_watcher_mac.cc (right):

https://codereview.chromium.org/2596273003/diff/120001/base/files/file_path_w...
base/files/file_path_watcher_mac.cc:49: std::unique_ptr<PlatformDelegate> impl_;
DISALLOW_...

[fdoray, 2017-01-09 21:30:21]

https://codereview.chromium.org/2596273003/diff/120001/base/files/file_path_w...
File base/files/file_path_watcher_mac.cc (right):

https://codereview.chromium.org/2596273003/diff/120001/base/files/file_path_w...
base/files/file_path_watcher_mac.cc:49: std::unique_ptr<PlatformDelegate> impl_;
On 2017/01/09 17:21:39, gab wrote:
> DISALLOW_...

Done.

[fdoray, 2017-01-09 21:30:25]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-01-09 21:30:25]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2596273003/#ps140001
(title: "fix build errors and add DISALLOW_COPY_AND_ASSIGN")

[commit-bot: I haz the power, 2017-01-09 21:31:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 21:42:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 21:42:14]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mark Mentovai, 2017-01-09 21:47:14]

LGTM

[fdoray, 2017-01-09 21:54:07]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-01-09 21:54:08]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2596273003/#ps160001
(title: "fix mac build error")

[commit-bot: I haz the power, 2017-01-09 21:55:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 23:38:46]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1483998847576740,
"parent_rev": "722a6795c0472558994b50ec6669284eaf0e57ee", "commit_rev":
"0ac2f0f892f6c9998134902dbc0fc3d78d8517a9"}

[commit-bot: I haz the power, 2017-01-09 23:39:14]

Description was changed from

==========
Remove ref-counting from FilePathWatcher.

Keep the PlatformDelegate in a std::unique_ptr instead of a scoped_refptr
in FilePathWatcher. Use weak ptrs when posting callbacks bound to a
PlatformDelegate.

With this change, it is guaranteed that a PlatformDelegate does not
outlive its parent FilePathWatcher. This will allow multiple code
simplifications in FilePathWatcher.

BUG=675631
==========

to

==========
Remove ref-counting from FilePathWatcher.

Keep the PlatformDelegate in a std::unique_ptr instead of a scoped_refptr
in FilePathWatcher. Use weak ptrs when posting callbacks bound to a
PlatformDelegate.

With this change, it is guaranteed that a PlatformDelegate does not
outlive its parent FilePathWatcher. This will allow multiple code
simplifications in FilePathWatcher.

BUG=675631

Review-Url: https://codereview.chromium.org/2596273003
Cr-Commit-Position: refs/heads/master@{#442404}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ac2f0f892f6c9998134902dbc0f...
==========

[commit-bot: I haz the power, 2017-01-09 23:39:15]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/0ac2f0f892f6c9998134902dbc0f...