bluetooth: Shutdown BluetoothAdapter before DBus on ChromeOS.

bluetooth: Shutdown BluetoothAdapter before DBus on ChromeOS.

The BluetoothAdapter is a lazy instance that may live due to references
until the shutdown of the browser. On ChromeOS the adapter and related
classes must release references to the DBusThreadManager before it is
shutdown. This patch adds an explicit call for that shutdown.

BUG=447387
Committed: https://crrev.com/5d49416fd60af9a9b7ee7e6d57835aa28ce9c909
Cr-Commit-Position: refs/heads/master@{#311986}

[scheib, 2015-01-14 04:39:34]

scheib@chromium.org changed reviewers:
+ armansito@chromium.org

[stevenjb, 2015-01-14 16:54:26]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2015-01-14 16:54:27]

Combining an explicit shutdown with a lazy initializer is awkward.  Can we make
initialization of BluetoothAdapter explicit?

[scheib, 2015-01-14 17:27:29]

On 2015/01/14 16:54:27, stevenjb wrote:
> Combining an explicit shutdown with a lazy initializer is awkward.  Can we
make
> initialization of BluetoothAdapter explicit?

We could call Initialize, but it wouldn't have much to do, since we don't want
the BluetoothAdapter running unless there's a user of it.

It's not clear to me why lazy loading is at odds with an explicit call to
shutdown for correct shutdown ordering.

I'm still in favor of the DButThreadManager::Observer pattern to simplify this
ordering requirement, though.

[armansito, 2015-01-14 19:52:36]

On 2015/01/14 17:27:29, scheib wrote:
> On 2015/01/14 16:54:27, stevenjb wrote:
> > Combining an explicit shutdown with a lazy initializer is awkward.  Can we
> make
> > initialization of BluetoothAdapter explicit?
> 
> We could call Initialize, but it wouldn't have much to do, since we don't want
> the BluetoothAdapter running unless there's a user of it.
> 
> It's not clear to me why lazy loading is at odds with an explicit call to
> shutdown for correct shutdown ordering.
>

+1, I think this generally makes sense.
 
> I'm still in favor of the DButThreadManager::Observer pattern to simplify this
> ordering requirement, though.

I agree with this as well, I think the current explicit-shutdown pattern on many
singletons in the correct order is generally awkward and messy. I've frequently
seen code break when with innocuous changes to the shutdown/initialize
sequences. Eitherway, it's too late in the game to change that and I also
understand some of the concerns raised regarding adding an ::Observer.

But in general, adding a Shutdown function within the current lazy
initialization pattern works I think. The BluetoothAdapter class is in fact a
singleton object but it's different from other ChromeOS system singletons in
that it has to work on Mac and Windows as well, so it has to accommodate some
restrictions on those platforms.

[armansito, 2015-01-14 19:52:43]

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter.h:193: virtual void Shutdown();
Again, should this just be a BluetoothAdapterChromeOS-specific function?

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:267: VLOG(1) <<
object_path_.value() << ": Creating RFCOMM service: "
Add check for IsPresent in all public API functions, here and below.

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_factory.h (right):

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_factory.h:34: static void Shutdown();
Should this be ChromeOS only (i.e. within macros)?

[stevenjb, 2015-01-14 20:25:07]

Taking a closer look at how BluetoothAdapter is created and use, I propose the
following:

1. Make these changes Chrome OS only.
2. Rename 'Shutdown' -> 'NotifyDBusThreadManagerShutdown'

Then it will be more clear that this isn't really a "Shutdown" operation for
BluetoothAdapter itself, and it makes it closer to the 'Observer' pattern that
you prefer, only rather than adding the overhead of managing observers to
DBusThreadManager for a single outlier, we call it explicitly in the two places
where we need to.

Sound reasonable?

[armansito, 2015-01-14 20:29:32]

On 2015/01/14 20:25:07, stevenjb wrote:
> Taking a closer look at how BluetoothAdapter is created and use, I propose the
> following:
> 
> 1. Make these changes Chrome OS only.
> 2. Rename 'Shutdown' -> 'NotifyDBusThreadManagerShutdown'
> 
> Then it will be more clear that this isn't really a "Shutdown" operation for
> BluetoothAdapter itself, and it makes it closer to the 'Observer' pattern that
> you prefer, only rather than adding the overhead of managing observers to
> DBusThreadManager for a single outlier, we call it explicitly in the two
places
> where we need to.
> 
> Sound reasonable?

Both sounds reasonable to me, especially making these changes Chrome OS only
(for now at least), if Vincent agrees as well.

[armansito, 2015-01-14 20:49:04]

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:78: is_shutdown_ = true;
Here I would explicitly set present as false or at least notify all observers
with PresentChanged

[scheib, 2015-01-14 23:07:18]

On 2015/01/14 20:29:32, armansito wrote:
> On 2015/01/14 20:25:07, stevenjb wrote:
> > Taking a closer look at how BluetoothAdapter is created and use, I propose
the
> > following:
> > 
> > 1. Make these changes Chrome OS only.
> > 2. Rename 'Shutdown' -> 'NotifyDBusThreadManagerShutdown'
> > 
> > Sound reasonable?
> 
> Both sounds reasonable to me, especially making these changes Chrome OS only
> (for now at least), if Vincent agrees as well.

Done

[scheib, 2015-01-14 23:07:27]

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter.h:193: virtual void Shutdown();
On 2015/01/14 19:52:43, armansito wrote:
> Again, should this just be a BluetoothAdapterChromeOS-specific function?

Done.

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:78: is_shutdown_ = true;
On 2015/01/14 20:49:04, armansito wrote:
> Here I would explicitly set present as false or at least notify all observers
> with PresentChanged

Done: reworked to call RemoveAdapter().

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:267: VLOG(1) <<
object_path_.value() << ": Creating RFCOMM service: "
On 2015/01/14 19:52:43, armansito wrote:
> Add check for IsPresent in all public API functions, here and below.

Added a check for dbus_is_shutdown_ added.

A test relies on CreateRfcommService working when IsPresent is false.

https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_factory.h (right):

https://codereview.chromium.org/848613003/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_factory.h:34: static void Shutdown();
On 2015/01/14 19:52:43, armansito wrote:
> Should this be ChromeOS only (i.e. within macros)?

Done.

[scheib, 2015-01-14 23:07:57]

Patchset #1 (id:1) has been deleted

[scheib, 2015-01-14 23:08:23]

Patchset #2 (id:40001) has been deleted

[stevenjb, 2015-01-14 23:44:14]

lgtm

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter.h:194: virtual void
OnDBusThreadManagerShutdown();
nit: We could make this a pure virtual and eliminate the need for a
NOTIMPLEMENTED method in the .cc file (and, ensure that it gets implemented on a
chromeos build).

[armansito, 2015-01-14 23:49:47]

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter.cc (right):

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter.cc:31: #endif
Remove

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter.h:195: #endif
I wouldn't even add this method to BluetoothAdapter. You can just directly
static_cast the adapter to BluetoothAdapterChromeOS and call its shutdown method
from BluetoothAdapterFactory.

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_factory.h (right):

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_factory.h:33: // Shutdown the adapter, ending
reliance DBusThreadManager, and resulting in
nit: I would make this a bit more descriptive, maybe something like "Shutdown
the adapter. This will tear down and clean up all objects owned by
BluetoothAdapter. After this call, the BluetoothAdapter will behave as if no
Bluetooth controller exists in the local system".

[scheib, 2015-01-14 23:50:04]

keybuk, it (with a quick blame) appears you introduced
BluetoothSocketChromeOSTest_ListenBeforeAdapterStart which validates that 
BluetoothAdapterChromeOS::CreateRfcommService
can be called when  
BluetoothAdapterChromeOS::IsPresent is false:

https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...

The test is quite intentional, but doesn't really explain why this scenario is
expected and it does seem a bit odd without additional context.

Would you verify that this is behavior we'd like to preserve?

[scheib, 2015-01-14 23:50:22]

scheib@chromium.org changed reviewers:
+ keybuk@chromium.org

[scheib, 2015-01-14 23:50:23]

keybuk, it (with a quick blame) appears you introduced
BluetoothSocketChromeOSTest_ListenBeforeAdapterStart which validates that 
BluetoothAdapterChromeOS::CreateRfcommService
can be called when  
BluetoothAdapterChromeOS::IsPresent is false:

https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...

The test is quite intentional, but doesn't really explain why this scenario is
expected and it does seem a bit odd without additional context.

Would you verify that this is behavior we'd like to preserve?

[keybuk, 2015-01-14 23:56:59]

Because you can create services at any time, and adapters can come and go.

The socket class takes care of watching the adapter come and go, and
arranging the registration or deregistration accordingly - ie.
BluetoothSocketChromeOS is already checking IsPresent() and using
::Observer
On Wed Jan 14 2015 at 3:50:25 PM <scheib@chromium.org> wrote:

> keybuk, it (with a quick blame) appears you introduced
> BluetoothSocketChromeOSTest_ListenBeforeAdapterStart which validates that
> BluetoothAdapterChromeOS::CreateRfcommService
> can be called when
> BluetoothAdapterChromeOS::IsPresent is false:
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/device/bluetooth/bluetooth_socket_chromeos_unittest.cc&sq=package:
> chromium&type=cs&q=BluetoothSocketChromeOSTest_
> ListenBeforeAdapterStart&l=412
>
> The test is quite intentional, but doesn't really explain why this scenario
> is
> expected and it does seem a bit odd without additional context.
>
> Would you verify that this is behavior we'd like to preserve?
>
> https://codereview.chromium.org/848613003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scheib, 2015-01-15 00:03:40]

keybuk, nevermind, armansito and I are content with reading the code to see how
this works until the adapter comes online.

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter.cc (right):

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter.cc:31: #endif
On 2015/01/14 23:49:47, armansito wrote:
> Remove

Done.

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter.h:194: virtual void
OnDBusThreadManagerShutdown();
On 2015/01/14 23:44:14, stevenjb wrote:
> nit: We could make this a pure virtual and eliminate the need for a
> NOTIMPLEMENTED method in the .cc file (and, ensure that it gets implemented on
a
> chromeos build).
> 

Acknowledged.

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter.h:195: #endif
On 2015/01/14 23:49:47, armansito wrote:
> I wouldn't even add this method to BluetoothAdapter. You can just directly
> static_cast the adapter to BluetoothAdapterChromeOS and call its shutdown
method
> from BluetoothAdapterFactory.

Done.

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_factory.h (right):

https://codereview.chromium.org/848613003/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_factory.h:33: // Shutdown the adapter, ending
reliance DBusThreadManager, and resulting in
On 2015/01/14 23:49:47, armansito wrote:
> nit: I would make this a bit more descriptive, maybe something like "Shutdown
> the adapter. This will tear down and clean up all objects owned by
> BluetoothAdapter. After this call, the BluetoothAdapter will behave as if no
> Bluetooth controller exists in the local system".

Done.

[scheib, 2015-01-15 00:05:23]

scheib@chromium.org changed reviewers:
- keybuk@chromium.org

[scheib, 2015-01-15 00:07:51]

scheib@chromium.org changed reviewers:
+ jam@chromium.org

[scheib, 2015-01-15 00:07:52]

jam: OWNERS for 
content/shell/DEPS
content/shell/browser/shell_views.cc

[armansito, 2015-01-15 00:09:03]

Can you please add a unit test for this to bluetooth_chromeos_unittest.cc?

[scheib, 2015-01-15 18:28:59]

Patchset #3 (id:80001) has been deleted

[scheib, 2015-01-15 18:30:24]

How much unit testing do you think we need. I've added a basic one. Would you
like calls coming from DBUS clients as well? That seems a bit overkill to me
given that this is specifically for a DBUS shutdown scenario.

[jam, 2015-01-15 23:24:13]

https://codereview.chromium.org/848613003/diff/120001/content/shell/browser/s...
File content/shell/browser/shell_views.cc (right):

https://codereview.chromium.org/848613003/diff/120001/content/shell/browser/s...
content/shell/browser/shell_views.cc:444:
device::BluetoothAdapterFactory::OnDBusThreadManagerShutdown();
it seems a bit odd that content shell has to do this. content shell doesn't even
use bluetooth anywhere given the new DEPS that you had to add for this

[scheib, 2015-01-16 17:41:53]

https://codereview.chromium.org/848613003/diff/120001/content/shell/browser/s...
File content/shell/browser/shell_views.cc (right):

https://codereview.chromium.org/848613003/diff/120001/content/shell/browser/s...
content/shell/browser/shell_views.cc:444:
device::BluetoothAdapterFactory::OnDBusThreadManagerShutdown();
On 2015/01/15 23:24:12, jam wrote:
> it seems a bit odd that content shell has to do this. content shell doesn't
even
> use bluetooth anywhere given the new DEPS that you had to add for this

Content shell will use BT soon,
https://codereview.chromium.org/787953004/
I'll move the content_shell change to that patch.

[scheib, 2015-01-16 21:12:30]

scheib@chromium.org changed reviewers:
- jam@chromium.org

[scheib, 2015-01-16 21:33:47]

Patchset #5 (id:140001) has been deleted

[scheib, 2015-01-16 21:35:32]

Patchset #5 (id:160001) has been deleted

[scheib, 2015-01-16 21:49:25]

I've gone back to virtual dispatch, static casting fails with the
MockBluetoothAdapter.

[armansito, 2015-01-16 21:57:19]

https://codereview.chromium.org/848613003/diff/180001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/848613003/diff/180001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:200: void OnDBusThreadManagerShutdown();
If we're going to have this in BluetoothAdapter, then just call it Shutdown,
with no mention for Chrome OS or D-Bus. It doesn't have to do anything for the
other platforms, just say that this shutdowns and invalidates a
BluetoothAdapter.

https://codereview.chromium.org/848613003/diff/180001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_chromeos_unittest.cc (right):

https://codereview.chromium.org/848613003/diff/180001/device/bluetooth/blueto...
device/bluetooth/bluetooth_chromeos_unittest.cc:3357:
adapter_.get())->object_path());
Probably also make a calls to the asynchronous methods here, e.g.
StartDiscoverySession should fail.

[scheib, 2015-01-16 22:27:26]

Patchset #6 (id:200001) has been deleted

[scheib, 2015-01-16 22:28:27]

https://codereview.chromium.org/848613003/diff/180001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/848613003/diff/180001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:200: void OnDBusThreadManagerShutdown();
On 2015/01/16 21:57:18, armansito wrote:
> If we're going to have this in BluetoothAdapter, then just call it Shutdown,
> with no mention for Chrome OS or D-Bus. It doesn't have to do anything for the
> other platforms, just say that this shutdowns and invalidates a
> BluetoothAdapter.

Done.

https://codereview.chromium.org/848613003/diff/180001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_chromeos_unittest.cc (right):

https://codereview.chromium.org/848613003/diff/180001/device/bluetooth/blueto...
device/bluetooth/bluetooth_chromeos_unittest.cc:3357:
adapter_.get())->object_path());
On 2015/01/16 21:57:18, armansito wrote:
> Probably also make a calls to the asynchronous methods here, e.g.
> StartDiscoverySession should fail.

Done.

[armansito, 2015-01-16 22:46:52]

lgtm

[scheib, 2015-01-16 23:14:52]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2015-01-16 23:15:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/848613003/220001

[commit-bot: I haz the power, 2015-01-16 23:15:48]

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

[commit-bot: I haz the power, 2015-01-16 23:15:50]

CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true
in order for the CQ to process them

[scheib, 2015-01-16 23:22:17]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2015-01-16 23:23:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/848613003/240001

[commit-bot: I haz the power, 2015-01-17 00:18:43]

Committed patchset #7 (id:240001)

[commit-bot: I haz the power, 2015-01-17 00:19:49]

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