Leak DeviceMonitorLinux on shutdown.

Leak DeviceMonitorLinux on shutdown.

In preparation for the migration of many Chrome threads to
TaskScheduler, we want to get rid of MessageLoop destruction
observers.

This CL removes the code that deletes DeviceMonitorLinux from
a destruction observer and uses a LazyInstance::Leaky to manage
the lifetime of DeviceMonitorLinux.

This CL also helps reducing the amount of work done during
shutdown
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/AMB2Y7Sqj48/QltA07wUCQAJ

BUG=650318
Committed: https://crrev.com/7a9ea7caa267b3779d7984a6834e2d35b723cefb
Cr-Commit-Position: refs/heads/master@{#426518}

[fdoray, 2016-10-19 18:16:46]

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

[commit-bot: I haz the power, 2016-10-19 18:17:17]

Dry run: 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, 2016-10-19 18:54:33]

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

[commit-bot: I haz the power, 2016-10-19 18:54:34]

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

[fdoray, 2016-10-20 13:53:33]

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

[commit-bot: I haz the power, 2016-10-20 13:53:45]

Dry run: 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, 2016-10-20 14:44:47]

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

[commit-bot: I haz the power, 2016-10-20 14:44:47]

Dry run: This issue passed the CQ dry run.

[fdoray, 2016-10-20 15:33:51]

Description was changed from

==========
Leak DeviceMonitorLinux on shutdown.

BUG=
==========

to

==========
Leak DeviceMonitorLinux on shutdown.

In preparation for the migration of many Chrome threads to
TaskScheduler, we want to get rid of MessageLoop destruction
observers.

This CL removes the code that deletes DeviceMonitorLinux from
a destruction observer and uses a LazyInstance::Leaky to manage
the lifetime of DeviceMonitorLinux.

This CL also helps reducing the amount of work done during
shutdown
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/AMB2Y7Sqj48/QltA0...

BUG=650318
==========

[fdoray, 2016-10-20 15:34:27]

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

[fdoray, 2016-10-20 15:34:27]

PTAL

[Ken Rockot(use gerrit already), 2016-10-20 16:18:03]

LGTM.

So what's the timeline for getting rid of MessageLoop DestructionObserver?

We have generic Mojo bindings objects which rely on this destruction event for a
meaningful signal, and in many cases this signal triggers a deallocation.

These objects are used on common browser threads as well as arbitrary other
threads which may most certainly be torn down before shutdown, and therefore
leaky behavior is unacceptable. What's the plan for satisfying these
constraints?

Will we be able to subscribe to some notification that scheduler is going to
stop scheduling tasks in a given sequence?

[fdoray, 2016-10-20 17:02:41]

On 2016/10/20 16:18:03, Ken Rockot wrote:
> LGTM.
> 
> So what's the timeline for getting rid of MessageLoop DestructionObserver?
> 
> We have generic Mojo bindings objects which rely on this destruction event for
a
> meaningful signal, and in many cases this signal triggers a deallocation.
> 
> These objects are used on common browser threads as well as arbitrary other
> threads which may most certainly be torn down before shutdown, and therefore
> leaky behavior is unacceptable. What's the plan for satisfying these
> constraints?

TaskScheduler does not target the IO thread. If your Mojo bindings objects
register destruction observers exclusively on the IO thread, nothing has to
change in the near future. If you Mojo bindings objects register destruction
observers on other threads, please send us a link to the relevant code and we'll
make sure that TaskScheduler accommodates your needs.
 
> Will we be able to subscribe to some notification that scheduler is going to
> stop scheduling tasks in a given sequence?

We don't plan to support TaskScheduler shutdown observers. In production and in
browser tests code, Chrome shutdown should be observed instead of TaskScheduler
shutdown. In other tests, uninitialization should be done by overriding
TearDown(). We want TaskScheduler to accommodate the needs of every developer.
Therefore, if you have a link to a test that absolutely need to observe
TaskScheduler shutdown, please share it with us.

[fdoray, 2016-10-20 17:03:22]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-10-20 17:03:56]

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

[Ken Rockot(use gerrit already), 2016-10-20 17:09:48]

On 2016/10/20 at 17:02:41, fdoray wrote:
> On 2016/10/20 16:18:03, Ken Rockot wrote:
> > LGTM.
> > 
> > So what's the timeline for getting rid of MessageLoop DestructionObserver?
> > 
> > We have generic Mojo bindings objects which rely on this destruction event
for a
> > meaningful signal, and in many cases this signal triggers a deallocation.
> > 
> > These objects are used on common browser threads as well as arbitrary other
> > threads which may most certainly be torn down before shutdown, and therefore
> > leaky behavior is unacceptable. What's the plan for satisfying these
> > constraints?
> 
> TaskScheduler does not target the IO thread. If your Mojo bindings objects
register destruction observers exclusively on the IO thread, nothing has to
change in the near future. If you Mojo bindings objects register destruction
observers on other threads, please send us a link to the relevant code and we'll
make sure that TaskScheduler accommodates your needs.

These are not just testing concerns. These objects are usable on any thread;
today they're used on the UI thread, IO thread, FILE thread, along with other
non-browser threads. They're used on transient threads which die at a time other
than shutdown.

The generic culprit is our mojo::Watcher object
(https://cs.chromium.org/chromium/src/mojo/public/cpp/system/watcher.h) which is
ultimately used to receive notifications about a pipe's state. This is
ultimately used to implement mojo::Binding<T> and mojo::StrongBinding<T>, and
message loop destruction triggers the connection error handler set on these
objects.

Some mojo::Binding<T> users deallocate on connection error.
mojo::StrongBinding<T> always deallocates on connection error.

>  
> > Will we be able to subscribe to some notification that scheduler is going to
> > stop scheduling tasks in a given sequence?
> 
> We don't plan to support TaskScheduler shutdown observers. In production and
in browser tests code, Chrome shutdown should be observed instead of
TaskScheduler shutdown. In other tests, uninitialization should be done by
overriding TearDown(). We want TaskScheduler to accommodate the needs of every
developer. Therefore, if you have a link to a test that absolutely need to
observe TaskScheduler shutdown, please share it with us.

Can I ask why we wouldn't expose this event? It seems unnecessary to conflate
scheduler lifetime with process lifetime just because that's the common case for
browser threads.

[commit-bot: I haz the power, 2016-10-20 17:22:53]

Description was changed from

==========
Leak DeviceMonitorLinux on shutdown.

In preparation for the migration of many Chrome threads to
TaskScheduler, we want to get rid of MessageLoop destruction
observers.

This CL removes the code that deletes DeviceMonitorLinux from
a destruction observer and uses a LazyInstance::Leaky to manage
the lifetime of DeviceMonitorLinux.

This CL also helps reducing the amount of work done during
shutdown
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/AMB2Y7Sqj48/QltA0...

BUG=650318
==========

to

==========
Leak DeviceMonitorLinux on shutdown.

In preparation for the migration of many Chrome threads to
TaskScheduler, we want to get rid of MessageLoop destruction
observers.

This CL removes the code that deletes DeviceMonitorLinux from
a destruction observer and uses a LazyInstance::Leaky to manage
the lifetime of DeviceMonitorLinux.

This CL also helps reducing the amount of work done during
shutdown
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/AMB2Y7Sqj48/QltA0...

BUG=650318
==========

[commit-bot: I haz the power, 2016-10-20 17:22:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 13:19:44]

Description was changed from

==========
Leak DeviceMonitorLinux on shutdown.

In preparation for the migration of many Chrome threads to
TaskScheduler, we want to get rid of MessageLoop destruction
observers.

This CL removes the code that deletes DeviceMonitorLinux from
a destruction observer and uses a LazyInstance::Leaky to manage
the lifetime of DeviceMonitorLinux.

This CL also helps reducing the amount of work done during
shutdown
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/AMB2Y7Sqj48/QltA0...

BUG=650318
==========

to

==========
Leak DeviceMonitorLinux on shutdown.

In preparation for the migration of many Chrome threads to
TaskScheduler, we want to get rid of MessageLoop destruction
observers.

This CL removes the code that deletes DeviceMonitorLinux from
a destruction observer and uses a LazyInstance::Leaky to manage
the lifetime of DeviceMonitorLinux.

This CL also helps reducing the amount of work done during
shutdown
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/AMB2Y7Sqj48/QltA0...

BUG=650318

Committed: https://crrev.com/7a9ea7caa267b3779d7984a6834e2d35b723cefb
Cr-Commit-Position: refs/heads/master@{#426518}
==========

[commit-bot: I haz the power, 2016-10-21 13:19:45]

Patchset 2 (id:??) landed as
https://crrev.com/7a9ea7caa267b3779d7984a6834e2d35b723cefb
Cr-Commit-Position: refs/heads/master@{#426518}