Make DeviceSensorService implement MainLoop::DestructionObserver

The DeviceSensorService shutdown currently occurs after the IO thread shuts
down in //content's browser_main_loop.cc. If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

So, in this patch we let DeviceSensorService observe the thread it is
running on. For Non-Android platform, DeviceSensorService is run in IO thread.
It shuts itself down when IO thread is going to close. For Android platform, the
DeviceSensorService shutdown is only called when running browsertests, with this
modification the shutdown is later (for browsertest) but still safe.

BUG=698664
Review-Url: https://codereview.chromium.org/2730333002
Cr-Commit-Position: refs/heads/master@{#457422}
Committed: https://chromium.googlesource.com/chromium/src/+/0e3e0d1ec792363339551d3e26fce7fdf1e4cc83

[ke.he, 2017-03-06 11:40:35]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 11:40:42]

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, 2017-03-06 11:44:20]

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

[commit-bot: I haz the power, 2017-03-06 11:44:21]

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

[ke.he, 2017-03-07 03:32:30]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 03:33:57]

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, 2017-03-07 03:39:47]

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

[commit-bot: I haz the power, 2017-03-07 03:39:48]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ke.he, 2017-03-07 03:44:43]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 03:45:33]

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, 2017-03-07 05:07:05]

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

[commit-bot: I haz the power, 2017-03-07 05:07:06]

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

[ke.he, 2017-03-07 06:09:36]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 06:10:12]

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, 2017-03-07 07:42:09]

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

[commit-bot: I haz the power, 2017-03-07 07:42:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ke.he, 2017-03-07 08:45:34]

Description was changed from

==========
Make Device Serive be responsible to shutdown DeviceSensorService.

BUG=698664
==========

to

==========
Make Device Service be responsible to shutdown DeviceSensorService.

BUG=698664
==========

[ke.he, 2017-03-07 08:48:07]

Description was changed from

==========
Make Device Service be responsible to shutdown DeviceSensorService.

BUG=698664
==========

to

==========
Make Device Service be responsible to shutdown DeviceSensorService.

The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

In this patch we add a IOMessageLoopObserver which lives longer than
DeviceService and be responsible to do shutdown when it observes IO thread is
closing.

BUG=698664
==========

[ke.he, 2017-03-07 08:48:18]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 08:48:37]

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

[ke.he, 2017-03-07 09:56:40]

ke.he@intel.com changed reviewers:
+ blundell@chromium.org

[ke.he, 2017-03-07 09:56:41]

Hi, Colin,
So strange that it breaks the chrome uninstall on windows. The log shows
"[0306/225125.591:ERROR:uninstall.cc(132)] Failed to delete path: C:\Program
Files (x86)\Chromium\Application\59.0.3034.0\Installer\setup.exe".
I suspect it's because of the instable of trybot? If still fail tomorrow, I'll
merge this CL with "Port DeviceSensor into DeviceService" and try again.

PTAL(Although trybot fails). Thanks.

https://codereview.chromium.org/2730333002/diff/60001/services/device/device_...
File services/device/device_service.cc (right):

https://codereview.chromium.org/2730333002/diff/60001/services/device/device_...
services/device/device_service.cc:94: new
IOMessageLoopObserver(io_task_runner_);
Now the IOMessageLoopObserver is created in DeviceService's constructor. The
singleton DeviceSensorService is created after mojo connection has been
established.
So if no client has ever requested DeviceService to create DeviceSensorService,
IOMessageLoopObserver still tries to shutdown the DeviceSensorService. Although
the shutdown code is safe but I guess we should avoid this happen.
One solution is to move the creation of IOMessageLoopObserver into each
DeviceService::Create(). In the DeviceService::Create() we check if
IOMessageLoopObserver has been created already, if yes, do nothing, if no,
create it.
Any comments please? Thanks!

https://codereview.chromium.org/2730333002/diff/60001/services/device/device_...
services/device/device_service.cc:99: ShutDownPlatformServices();
For Android, still shutdown in UI thread. It is safe even occurs before IO
thread close.

[commit-bot: I haz the power, 2017-03-07 10:44:18]

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

[commit-bot: I haz the power, 2017-03-07 10:44:19]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ke.he, 2017-03-07 12:09:10]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 12:09:23]

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, 2017-03-07 14:12:54]

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

[commit-bot: I haz the power, 2017-03-07 14:12:56]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[blundell, 2017-03-07 16:14:36]

Hi Ke He,

This is great, thanks! I don't know why the win trybot is failing either, but I
do have a question: Could we just make DeviceSensorService a DestructionObserver
of the MessageLoop that it gets constructed on? That seems like it could be
simpler code structure-wise. We could then see if the trybot failure still fails
in that code constructure.

[ke.he, 2017-03-08 06:47:17]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 06:47:39]

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

[ke.he, 2017-03-08 08:04:08]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 08:04:43]

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, 2017-03-08 09:05:39]

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

[commit-bot: I haz the power, 2017-03-08 09:05:41]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-03-08 09:19:55]

Hi, Colin, PTAL.

By the way the windows uninstall is because of in-stable of trybot. I dry the
bot again it shows that the win_chromium_rel_ng pass, while win_chromium_x64_ng
still fail.

https://codereview.chromium.org/2730333002/diff/120001/device/sensors/device_...
File device/sensors/device_sensor_service.cc (right):

https://codereview.chromium.org/2730333002/diff/120001/device/sensors/device_...
device/sensors/device_sensor_service.cc:21: #if !defined(OS_ANDROID)
For Non-Android platform, The strongBinding happens in IO thread, so the
DeviceSensorService should in IO thread too. For Android platform, it is in UI
thread.

https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
File services/device/device_service.cc (right):

https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
services/device/device_service.cc:43: TRACE_EVENT0("shutdown",
"DeviceService::Subsystem:SensorService");
For Android platform, it is safe to shutdown in UI thread, no need to care about
whether IO thread is live or not.

[ke.he, 2017-03-08 09:21:27]

Description was changed from

==========
Make Device Service be responsible to shutdown DeviceSensorService.

The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

In this patch we add a IOMessageLoopObserver which lives longer than
DeviceService and be responsible to do shutdown when it observes IO thread is
closing.

BUG=698664
==========

to

==========
Make Device Service be responsible to shutdown DeviceSensorService.

The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

In this patch we let DeviceSensorService observes the IO thread, In Non-Android
platform, DeviceSensorService runs in IO thread, it shutdown itself when
observing IO thread is closing. In Android platform, DeviceSensorService runs
in UI thread, DeviceService do the shutdown operation in its destrution in UI
thread.

BUG=698664
==========

[ke.he, 2017-03-08 09:22:15]

Description was changed from

==========
Make Device Service be responsible to shutdown DeviceSensorService.

The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

In this patch we let DeviceSensorService observes the IO thread, In Non-Android
platform, DeviceSensorService runs in IO thread, it shutdown itself when
observing IO thread is closing. In Android platform, DeviceSensorService runs
in UI thread, DeviceService do the shutdown operation in its destrution in UI
thread.

BUG=698664
==========

to

==========
Make DeviceSensorService observes IOThread to shutdown in right time.

The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

In this patch we let DeviceSensorService observes the IO thread, In Non-Android
platform, DeviceSensorService runs in IO thread, it shutdown itself when
observing IO thread is closing. In Android platform, DeviceSensorService runs
in UI thread, DeviceService do the shutdown operation in its destrution in UI
thread.

BUG=698664
==========

[blundell, 2017-03-08 17:38:18]

Great, thanks! One question below.

https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
File services/device/device_service.cc (right):

https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
services/device/device_service.cc:43: TRACE_EVENT0("shutdown",
"DeviceService::Subsystem:SensorService");
On 2017/03/08 09:19:55, ke.he wrote:
> For Android platform, it is safe to shutdown in UI thread, no need to care
about
> whether IO thread is live or not.

Do you think that we could just use the same mechanism on Android as on other
platforms, i.e., have it observe the destruction of the message loop that it's
running on?

[ke.he, 2017-03-09 02:37:33]

https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
File services/device/device_service.cc (right):

https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
services/device/device_service.cc:43: TRACE_EVENT0("shutdown",
"DeviceService::Subsystem:SensorService");
For Android platform, DeviceSensorService runs in UI thread, it needs to get an
"io_task_runner" to add itself into IO MessageLoop's observer list.
DeviceSensorService is process singleton, it is created in
"DeviceSensorService::GetInstance()" when "DeviceSensorHost::StartPolling()" is
first called. so we need to pass the "io_task_runner" to DeviceSensorHost in
"RenderProcessHostImpl::AddUIThreadInterface(...)".

I'm not sure whether above make the code too complex, that's why I shutdown
DeviceSensorService-android here.

Please help to decide that should I add the "io_task_runner" in DeviceSensorHost
and DeviceSensorService? Thanks!

[blundell, 2017-03-09 16:06:58]

On 2017/03/09 02:37:33, ke.he wrote:
>
https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
> File services/device/device_service.cc (right):
> 
>
https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
> services/device/device_service.cc:43: TRACE_EVENT0("shutdown",
> "DeviceService::Subsystem:SensorService");
> For Android platform, DeviceSensorService runs in UI thread, it needs to get
an
> "io_task_runner" to add itself into IO MessageLoop's observer list.
> DeviceSensorService is process singleton, it is created in
> "DeviceSensorService::GetInstance()" when "DeviceSensorHost::StartPolling()"
is
> first called. so we need to pass the "io_task_runner" to DeviceSensorHost in
> "RenderProcessHostImpl::AddUIThreadInterface(...)".
> 
> I'm not sure whether above make the code too complex, that's why I shutdown
> DeviceSensorService-android here.
> 
> Please help to decide that should I add the "io_task_runner" in
DeviceSensorHost
> and DeviceSensorService? Thanks!

Hi Ke He,

What I'm proposing is that on Android, the DeviceSensorService listen for
destruction of the *UI* thread. That is, the DeviceSensorService would always
shut down keyed off the destruction of the MessageLoop *on which it's running.*
Do you think that would work?

Thanks,

Colin

[ke.he, 2017-03-09 21:09:18]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 21:09: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, 2017-03-09 21:56:04]

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

[commit-bot: I haz the power, 2017-03-09 21:56:05]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-03-13 08:39:44]

On 2017/03/09 16:06:58, blundell (OOO until March 13) wrote:
> On 2017/03/09 02:37:33, ke.he wrote:
> >
>
https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
> > File services/device/device_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
> > services/device/device_service.cc:43: TRACE_EVENT0("shutdown",
> > "DeviceService::Subsystem:SensorService");
> > For Android platform, DeviceSensorService runs in UI thread, it needs to get
> an
> > "io_task_runner" to add itself into IO MessageLoop's observer list.
> > DeviceSensorService is process singleton, it is created in
> > "DeviceSensorService::GetInstance()" when "DeviceSensorHost::StartPolling()"
> is
> > first called. so we need to pass the "io_task_runner" to DeviceSensorHost in
> > "RenderProcessHostImpl::AddUIThreadInterface(...)".
> > 
> > I'm not sure whether above make the code too complex, that's why I shutdown
> > DeviceSensorService-android here.
> > 
> > Please help to decide that should I add the "io_task_runner" in
> DeviceSensorHost
> > and DeviceSensorService? Thanks!
> 
> Hi Ke He,
> 
> What I'm proposing is that on Android, the DeviceSensorService listen for
> destruction of the *UI* thread. That is, the DeviceSensorService would always
> shut down keyed off the destruction of the MessageLoop *on which it's
running.*
> Do you think that would work?
> 
> Thanks,
> 
> Colin

Hi, Colin,
I found the BrowserMainLoop::ShutdownThreadsAndCleanUp() has never been called
in Android platform. See src/content/app/android/content_main.cc, it calls
ContentMainRunner::Initialize() and Run(), but no ShutDown(). It means that
there is no shutdown operation for DeviceSensorService from the first beginning
in Android platform. There are some useful operations in
BrowserMainLoop::ShutdownThreadsAndCleanUp() such as closing threads, reset
mojos... I didn't find where the android implementation do those operations
elsewhere. Is it a bug or I'm wrong?

Back to this CL, if we let DeviceSensorService to listen the UI thread, I'm not
sure whether the UI-destrution event could be sent out (maybe after
ChromeActivity.java::onDestroy()). I'm still reading codes, sorry for moving
slowly in this CL:(

[blundell, 2017-03-13 15:47:46]

On 2017/03/13 08:39:44, ke.he wrote:
> On 2017/03/09 16:06:58, blundell (OOO until March 13) wrote:
> > On 2017/03/09 02:37:33, ke.he wrote:
> > >
> >
>
https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
> > > File services/device/device_service.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2730333002/diff/120001/services/device/device...
> > > services/device/device_service.cc:43: TRACE_EVENT0("shutdown",
> > > "DeviceService::Subsystem:SensorService");
> > > For Android platform, DeviceSensorService runs in UI thread, it needs to
get
> > an
> > > "io_task_runner" to add itself into IO MessageLoop's observer list.
> > > DeviceSensorService is process singleton, it is created in
> > > "DeviceSensorService::GetInstance()" when
"DeviceSensorHost::StartPolling()"
> > is
> > > first called. so we need to pass the "io_task_runner" to DeviceSensorHost
in
> > > "RenderProcessHostImpl::AddUIThreadInterface(...)".
> > > 
> > > I'm not sure whether above make the code too complex, that's why I
shutdown
> > > DeviceSensorService-android here.
> > > 
> > > Please help to decide that should I add the "io_task_runner" in
> > DeviceSensorHost
> > > and DeviceSensorService? Thanks!
> > 
> > Hi Ke He,
> > 
> > What I'm proposing is that on Android, the DeviceSensorService listen for
> > destruction of the *UI* thread. That is, the DeviceSensorService would
always
> > shut down keyed off the destruction of the MessageLoop *on which it's
> running.*
> > Do you think that would work?
> > 
> > Thanks,
> > 
> > Colin
> 
> Hi, Colin,
> I found the BrowserMainLoop::ShutdownThreadsAndCleanUp() has never been called
> in Android platform. See src/content/app/android/content_main.cc, it calls
> ContentMainRunner::Initialize() and Run(), but no ShutDown(). It means that
> there is no shutdown operation for DeviceSensorService from the first
beginning
> in Android platform. There are some useful operations in
> BrowserMainLoop::ShutdownThreadsAndCleanUp() such as closing threads, reset
> mojos... I didn't find where the android implementation do those operations
> elsewhere. Is it a bug or I'm wrong?
> 
> Back to this CL, if we let DeviceSensorService to listen the UI thread, I'm
not
> sure whether the UI-destrution event could be sent out (maybe after
> ChromeActivity.java::onDestroy()). I'm still reading codes, sorry for moving
> slowly in this CL:(

Hi Ke He,

This is a very good observation! You are correct; the clean shutdown process is
simply not used on Android except in tests
(https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t...).
Hence, the only way that our changes here would matter for Android is if we
cause tests to fail. I suggest that you simply use the DestructionObserver
approach on all platforms, run it through the trybots, and see if they're green
on Android. My hunch is that it will be absolutely fine. It should cause the
DeviceSensorService to shut down on Android in tests here:
https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t...,
which is slightly later than was happening before. I don't think that that
change should matter in any case since Mojo itself is shut down earlier.

Assuming that it's green, add a condensed version of what we've discussed here
to the CL description to explain how this change does and does not impact
Android.

[ke.he, 2017-03-14 09:49:02]

Description was changed from

==========
Make DeviceSensorService observes IOThread to shutdown in right time.

The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

In this patch we let DeviceSensorService observes the IO thread, In Non-Android
platform, DeviceSensorService runs in IO thread, it shutdown itself when
observing IO thread is closing. In Android platform, DeviceSensorService runs
in UI thread, DeviceService do the shutdown operation in its destrution in UI
thread.

BUG=698664
==========

to

==========
Make DeviceSensorService observes IOThread to shutdown in right time.

The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

So, in this patch we let DeviceSensorService observes the thread it is
running on. For Non-Android platform, DeviceSensorService is run in IO thread,
It shutdown itself when IO thread is going to close. For Android platform, The
DeviceSensorService shutdown is only called when running browsertest, with this
modification the shutdown is later(for browsertest) but still safe.

BUG=698664
==========

[ke.he, 2017-03-14 09:50:16]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 09:50:29]

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

[ke.he, 2017-03-14 09:53:21]

Description was changed from

==========
Make DeviceSensorService observes IOThread to shutdown in right time.

The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

So, in this patch we let DeviceSensorService observes the thread it is
running on. For Non-Android platform, DeviceSensorService is run in IO thread,
It shutdown itself when IO thread is going to close. For Android platform, The
DeviceSensorService shutdown is only called when running browsertest, with this
modification the shutdown is later(for browsertest) but still safe.

BUG=698664
==========

to

==========
Make DeviceSensorService observes IOThread to shutdown in right time.

The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

So, in this patch we let DeviceSensorService observes the thread it is
running on. For Non-Android platform, DeviceSensorService is run in IO thread,
It shutdown itself when IO thread is going to close. For Android platform, The
DeviceSensorService shutdown is only called when running browsertest, with this
modification the shutdown is later(for browsertest) but still safe.

BUG=698664
==========

[ke.he, 2017-03-14 09:54:09]

Description was changed from

==========
Make DeviceSensorService observes IOThread to shutdown in right time.

The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

So, in this patch we let DeviceSensorService observes the thread it is
running on. For Non-Android platform, DeviceSensorService is run in IO thread,
It shutdown itself when IO thread is going to close. For Android platform, The
DeviceSensorService shutdown is only called when running browsertest, with this
modification the shutdown is later(for browsertest) but still safe.

BUG=698664
==========

to

==========
The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

So, in this patch we let DeviceSensorService observes the thread it is
running on. For Non-Android platform, DeviceSensorService is run in IO thread,
It shutdown itself when IO thread is going to close. For Android platform, The
DeviceSensorService shutdown is only called when running browsertest, with this
modification the shutdown is later(for browsertest) but still safe.

BUG=698664
==========

[commit-bot: I haz the power, 2017-03-14 10:36:32]

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

[commit-bot: I haz the power, 2017-03-14 10:36:33]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-03-14 13:49:14]

Colin, PTAL, Thanks.

[blundell, 2017-03-14 17:20:14]

Description was changed from

==========
The DeviceSensorService shutdown originally occurs after the IO thread shuts
down in //content's browser_main_loop.cc, If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

So, in this patch we let DeviceSensorService observes the thread it is
running on. For Non-Android platform, DeviceSensorService is run in IO thread,
It shutdown itself when IO thread is going to close. For Android platform, The
DeviceSensorService shutdown is only called when running browsertest, with this
modification the shutdown is later(for browsertest) but still safe.

BUG=698664
==========

to

==========
The DeviceSensorService shutdown currently occurs after the IO thread shuts
down in //content's browser_main_loop.cc. If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

So, in this patch we let DeviceSensorService observe the thread it is
running on. For Non-Android platform, DeviceSensorService is run in IO thread.
It shuts itself down when IO thread is going to close. For Android platform, the
DeviceSensorService shutdown is only called when running browsertests, with this
modification the shutdown is later (for browsertest) but still safe.

BUG=698664
==========

[blundell, 2017-03-14 17:20:38]

blundell@chromium.org changed reviewers:
+ timvolodine@chromium.org

[blundell, 2017-03-14 17:20:39]

lgtm

+Tim

[ke.he, 2017-03-15 06:18:55]

ke.he@intel.com changed reviewers:
+ jam@chromium.org

[ke.he, 2017-03-15 06:18:56]

Also add Jam@ since we changed browser_main_loop.cc.

[jam, 2017-03-15 17:07:18]

lgtm

[timvolodine, 2017-03-15 17:22:34]

On 2017/03/15 17:07:18, jam wrote:
> lgtm

ke.he@ : just to double check I assume the message loop destruction will happen
before the DeviceSensorService is destroyed (which is a singleton)?

[ke.he, 2017-03-16 01:33:35]

On 2017/03/15 17:22:34, timvolodine wrote:
> On 2017/03/15 17:07:18, jam wrote:
> > lgtm
> 
> ke.he@ : just to double check I assume the message loop destruction will
happen
> before the DeviceSensorService is destroyed (which is a singleton)?

Hi, Tim,
Yes, MessageLoop happens before the destruction of DeviceSensorService
singleton.
In base/memory/singleton.h, it says "single instance ... will be created on
first use and will be destroyed at normal process exit". So I think
DeviceSensorService lives long enough to receive the message loop destruction
event, it should be safe here.

[blundell, 2017-03-16 08:28:59]

On 2017/03/16 01:33:35, ke.he wrote:
> On 2017/03/15 17:22:34, timvolodine wrote:
> > On 2017/03/15 17:07:18, jam wrote:
> > > lgtm
> > 
> > ke.he@ : just to double check I assume the message loop destruction will
> happen
> > before the DeviceSensorService is destroyed (which is a singleton)?
> 
> Hi, Tim,
> Yes, MessageLoop happens before the destruction of DeviceSensorService
> singleton.
> In base/memory/singleton.h, it says "single instance ... will be created on
> first use and will be destroyed at normal process exit". So I think
> DeviceSensorService lives long enough to receive the message loop destruction
> event, it should be safe here.

+1

[timvolodine, 2017-03-16 13:00:50]

On 2017/03/16 08:28:59, blundell wrote:
> On 2017/03/16 01:33:35, ke.he wrote:
> > On 2017/03/15 17:22:34, timvolodine wrote:
> > > On 2017/03/15 17:07:18, jam wrote:
> > > > lgtm
> > > 
> > > ke.he@ : just to double check I assume the message loop destruction will
> > happen
> > > before the DeviceSensorService is destroyed (which is a singleton)?
> > 
> > Hi, Tim,
> > Yes, MessageLoop happens before the destruction of DeviceSensorService
> > singleton.
> > In base/memory/singleton.h, it says "single instance ... will be created on
> > first use and will be destroyed at normal process exit". So I think
> > DeviceSensorService lives long enough to receive the message loop
destruction
> > event, it should be safe here.
> 
> +1

thanks! lgtm )

[ke.he, 2017-03-16 13:06:54]

The CQ bit was checked by ke.he@intel.com

[commit-bot: I haz the power, 2017-03-16 13:07:03]

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-03-16 13:54:21]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489669614620010,
"parent_rev": "b50fc797e1226819ae917c11566fc213442c3bde", "commit_rev":
"0e3e0d1ec792363339551d3e26fce7fdf1e4cc83"}

[commit-bot: I haz the power, 2017-03-16 13:55:03]

Description was changed from

==========
The DeviceSensorService shutdown currently occurs after the IO thread shuts
down in //content's browser_main_loop.cc. If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

So, in this patch we let DeviceSensorService observe the thread it is
running on. For Non-Android platform, DeviceSensorService is run in IO thread.
It shuts itself down when IO thread is going to close. For Android platform, the
DeviceSensorService shutdown is only called when running browsertests, with this
modification the shutdown is later (for browsertest) but still safe.

BUG=698664
==========

to

==========
The DeviceSensorService shutdown currently occurs after the IO thread shuts
down in //content's browser_main_loop.cc. If we let the Device Sensor be hosted
by DeviceService, we cannot find a right time to do the DeviceSensorService
shutdown operation even as late as in destructor of DeviceService.

So, in this patch we let DeviceSensorService observe the thread it is
running on. For Non-Android platform, DeviceSensorService is run in IO thread.
It shuts itself down when IO thread is going to close. For Android platform, the
DeviceSensorService shutdown is only called when running browsertests, with this
modification the shutdown is later (for browsertest) but still safe.

BUG=698664

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

[commit-bot: I haz the power, 2017-03-16 13:55:04]

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