Call BluetoothGatt#close() after disconnecting

Call BluetoothGatt#close() after disconnecting

This fixes the problem where connections are never properly removed,
causing the BluetoothAdapter to eventually not be able to connect at
all.

BUG=576819
Committed: https://crrev.com/97116835b2cd97b9565020bd09f6c78469694ba3
Cr-Commit-Position: refs/heads/master@{#372986}

[tommyt, 2016-01-22 12:26:56]

Description was changed from

==========
Call BluetoothGatt#close() after disconnecting

This fixes the problem where connections are never properly removed,
causing the BluetoothAdapter to eventually not be able to connect at
all.

BUG=576819
==========

to

==========
Call BluetoothGatt#close() after disconnecting

This fixes the problem where connections are never properly removed,
causing the BluetoothAdapter to eventually not be able to connect at
all.

BUG=576819
==========

[tommyt, 2016-01-22 12:26:56]

tommyt@opera.com changed reviewers:
+ ortuno@chromium.org, scheib@chromium.org

[tommyt, 2016-01-22 12:32:34]

I've made an attempt at fixing this problem. First I tried simply replacing the
disconnect() with a close(). This caused some problems on subsequent connects.
Mainly "GATT operation failed for unknown reason" when trying to write to a
characteristic.

Calling disconnect(), immediately followed by close(), did not work much better,
but calling close() after receiving the connection state changed callback in
response to the disconnect() seems to work well. I have not been able to find
any problems with this approach, and it does keep the list of connections from
filling up.

[scheib, 2016-01-27 00:30:43]

Sorry for review delay -- please ping if no response in 24 hours, this fell
through cracks. Also, typically pick a primary reviewer. If you think ortuno
needs to review, add ortuno and then ortuno should L G T M and due to time zone
concerns add me as OWNER for an owner review. 

Tests.. hmm.. depending on answers elsewhere I'm on the fence for what we need
to do. We certainly could:
- test close is called upon SimulateGattDisconnection.

If closing upon destruction, may want to test that too. But that would need
something similar to RememberCharacteristicForSubsequentAction.

https://codereview.chromium.org/1618273002/diff/20001/device/bluetooth/androi...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java
(right):

https://codereview.chromium.org/1618273002/diff/20001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:62:
disconnectGatt();
close() after disconnectGatt() as well?

https://codereview.chromium.org/1618273002/diff/20001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:152:
mBluetoothGatt.close();
Please add comments here explaining why this is called at this time (vs
replacing disconnect, or immediately after disconnect).

https://codereview.chromium.org/1618273002/diff/40001/device/bluetooth/androi...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java
(right):

https://codereview.chromium.org/1618273002/diff/40001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:61:
private void onBluetoothDeviceAndroidDestruction() {
Should close be called in onBluetoothDeviceAndroidDestruction? You mentioned
issues if close is called right after disconnect, would it be a problem at this
location do you think?

https://codereview.chromium.org/1618273002/diff/40001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:124:
mBluetoothGatt =
If we already have a mBluetoothGatt this code will replace it and we won't call
close() on it. Should close be called now, or should mBluetoothGatt.connect() be
called instead of creating a new one?

[tommyt, 2016-01-28 12:48:22]

https://codereview.chromium.org/1618273002/diff/40001/device/bluetooth/androi...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java
(right):

https://codereview.chromium.org/1618273002/diff/40001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:61:
private void onBluetoothDeviceAndroidDestruction() {
On 2016/01/27 00:30:43, scheib wrote:
> Should close be called in onBluetoothDeviceAndroidDestruction? You mentioned
> issues if close is called right after disconnect, would it be a problem at
this
> location do you think?

Having spent some time today looking at the Android code that handles close
(bta_gattc_deregister in system/bt/bta/gatt/bta_gattc_act.c), it looks like it
should be enough to call only close. The platform code does look like it first
disconnects properly, before deregistering. Why exactly it doesn't work to
replace disconnect with close in disconnectGatt, I'm not sure. It could be that
it works fine from the platform's point of view, but that some state is not
properly tidied up in one of the numerous layers between
ChromeBluetoothDevice.java and bta_gattc_act.c.

I've tested replacing disconnectGatt with if (mBluetoothGatt != null)
mBluetoothGatt.close() at this location, and that feels correct and seems to
work well enough. I might go for that in my next patchset, but I'll do some more
testing first.

https://codereview.chromium.org/1618273002/diff/40001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:124:
mBluetoothGatt =
On 2016/01/27 00:30:43, scheib wrote:
> If we already have a mBluetoothGatt this code will replace it and we won't
call
> close() on it. Should close be called now, or should mBluetoothGatt.connect()
be
> called instead of creating a new one?

If we wanted to do mBluetoothGatt.connect() instead of creating a new gatt
object here, we'd have to make sure the existing gatt object is not one that has
already been closed.

I vote for doing mBluetoothGatt.close() and creating a new gatt object.

[ortuno, 2016-01-28 16:19:16]

https://codereview.chromium.org/1618273002/diff/40001/device/bluetooth/androi...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java
(right):

https://codereview.chromium.org/1618273002/diff/40001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:124:
mBluetoothGatt =
On 2016/01/28 at 12:48:22, tommyt wrote:
> On 2016/01/27 00:30:43, scheib wrote:
> > If we already have a mBluetoothGatt this code will replace it and we won't
call
> > close() on it. Should close be called now, or should
mBluetoothGatt.connect() be
> > called instead of creating a new one?

Could you give an example of when this could happen? We should only be calling
this if no connection exists.

> 
> If we wanted to do mBluetoothGatt.connect() instead of creating a new gatt
object here, we'd have to make sure the existing gatt object is not one that has
already been closed.
> 
> I vote for doing mBluetoothGatt.close() and creating a new gatt object.

+1 for mBluetoothGatt.close(). Calling mBluetoothGatt.connect() auto-connects,
which could lead to different behavior than the first connection.

[scheib, 2016-01-29 04:50:39]

https://codereview.chromium.org/1618273002/diff/40001/device/bluetooth/androi...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java
(right):

https://codereview.chromium.org/1618273002/diff/40001/device/bluetooth/androi...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothDevice.java:61:
private void onBluetoothDeviceAndroidDestruction() {
On 2016/01/28 12:48:22, tommyt wrote:
> On 2016/01/27 00:30:43, scheib wrote:
> > Should close be called in onBluetoothDeviceAndroidDestruction? You mentioned
> > issues if close is called right after disconnect, would it be a problem at
> this
> > location do you think?
> 
> Having spent some time today looking at the Android code that handles close
> (bta_gattc_deregister in system/bt/bta/gatt/bta_gattc_act.c), it looks like it
> should be enough to call only close. The platform code does look like it first
> disconnects properly, before deregistering. Why exactly it doesn't work to
> replace disconnect with close in disconnectGatt, I'm not sure. It could be
that
> it works fine from the platform's point of view, but that some state is not
> properly tidied up in one of the numerous layers between
> ChromeBluetoothDevice.java and bta_gattc_act.c.
> 
> I've tested replacing disconnectGatt with if (mBluetoothGatt != null)
> mBluetoothGatt.close() at this location, and that feels correct and seems to
> work well enough. I might go for that in my next patchset, but I'll do some
more
> testing first.

Ok, just to be clear, I think we should get that patch set on this review and
land what we think will work best in one shot (just in case you were proposing
we land this first and then follow up with another patch).

[tommyt, 2016-01-29 15:02:12]

A new patchset is up. This time, the code should ensure that no matter what
happens, all BluetoothGatt objects will be closed after use.

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device_unittest.cc (left):

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_unittest.cc:188: EXPECT_EQ(1,
gatt_disconnection_attempts_);
The destruction of the device no longer calls disconnect function. Instead it
calls close(), which does ensure that connections are disconnected.

[ortuno, 2016-01-29 16:23:53]

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device_unittest.cc (left):

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_unittest.cc:188: EXPECT_EQ(1,
gatt_disconnection_attempts_);
On 2016/01/29 at 15:02:12, tommyt wrote:
> The destruction of the device no longer calls disconnect function. Instead it
calls close(), which does ensure that connections are disconnected.

@scheib: Should we add another member to BluetoothTest to keep track of calls to
close()? Seems like something we should test but it's an implementation detail
specific to Android.

[scheib, 2016-01-29 21:50:58]

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device_unittest.cc (left):

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_unittest.cc:188: EXPECT_EQ(1,
gatt_disconnection_attempts_);
On 2016/01/29 16:23:53, ortuno wrote:
> On 2016/01/29 at 15:02:12, tommyt wrote:
> > The destruction of the device no longer calls disconnect function. Instead
it
> calls close(), which does ensure that connections are disconnected.
> 
> @scheib: Should we add another member to BluetoothTest to keep track of calls
to
> close()? Seems like something we should test but it's an implementation detail
> specific to Android.

Yes we should test this, and I think we can do it in just the Android testing
code. See fakes comment.

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/test/a...
File
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java
(right):

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/test/a...
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:328:
public void close() {}
close implies disconnect, add:
nativeOnFakeBluetoothGattDisconnect(mDevice.mAdapter.mNativeBluetoothTestAndroid);


Please add an 'open gatt connections' counter to the BluetoothTestAndroid
fixture (or perhaps just the Fakes) and verify in BluetoothTestAndroid::Teardown
that all gatt connections are closed.

[tommyt, 2016-02-01 14:37:13]

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device_unittest.cc (left):

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_unittest.cc:188: EXPECT_EQ(1,
gatt_disconnection_attempts_);
On 2016/01/29 21:50:58, scheib wrote:
> On 2016/01/29 16:23:53, ortuno wrote:
> > On 2016/01/29 at 15:02:12, tommyt wrote:
> > > The destruction of the device no longer calls disconnect function. Instead
> it
> > calls close(), which does ensure that connections are disconnected.
> > 
> > @scheib: Should we add another member to BluetoothTest to keep track of
calls
> to
> > close()? Seems like something we should test but it's an implementation
detail
> > specific to Android.
> 
> Yes we should test this, and I think we can do it in just the Android testing
> code. See fakes comment.

I've resurrected this code and added some test code that increases the
disconnection attempts variable on close as well as on disconnect. I've also
added code that tests that all connections are properly closed.

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/test/a...
File
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java
(right):

https://codereview.chromium.org/1618273002/diff/60001/device/bluetooth/test/a...
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:328:
public void close() {}
On 2016/01/29 21:50:58, scheib wrote:
> close implies disconnect, add:
>
nativeOnFakeBluetoothGattDisconnect(mDevice.mAdapter.mNativeBluetoothTestAndroid);
> 
> 
> Please add an 'open gatt connections' counter to the BluetoothTestAndroid
> fixture (or perhaps just the Fakes) and verify in
BluetoothTestAndroid::Teardown
> that all gatt connections are closed.

Done.

[scheib, 2016-02-01 18:18:49]

https://codereview.chromium.org/1618273002/diff/80001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_android.cc (right):

https://codereview.chromium.org/1618273002/diff/80001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_android.cc:45: }
Also call BluetoothTest::TearDown()

[tommyt, 2016-02-02 09:48:30]

https://codereview.chromium.org/1618273002/diff/80001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_android.cc (right):

https://codereview.chromium.org/1618273002/diff/80001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_android.cc:45: }
On 2016/02/01 18:18:48, scheib wrote:
> Also call BluetoothTest::TearDown()

I even read the comment in bluetooth_test.h that says that this must be done,
but I still forgot it. Oh, well, I have another patchset up that adds this call

[scheib, 2016-02-02 17:57:50]

LGTM

[scheib, 2016-02-02 17:57:55]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-02-02 17:58:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1618273002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1618273002/100001

[commit-bot: I haz the power, 2016-02-02 18:10:22]

Description was changed from

==========
Call BluetoothGatt#close() after disconnecting

This fixes the problem where connections are never properly removed,
causing the BluetoothAdapter to eventually not be able to connect at
all.

BUG=576819
==========

to

==========
Call BluetoothGatt#close() after disconnecting

This fixes the problem where connections are never properly removed,
causing the BluetoothAdapter to eventually not be able to connect at
all.

BUG=576819
==========

[commit-bot: I haz the power, 2016-02-02 18:10:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-02 18:11:36]

Description was changed from

==========
Call BluetoothGatt#close() after disconnecting

This fixes the problem where connections are never properly removed,
causing the BluetoothAdapter to eventually not be able to connect at
all.

BUG=576819
==========

to

==========
Call BluetoothGatt#close() after disconnecting

This fixes the problem where connections are never properly removed,
causing the BluetoothAdapter to eventually not be able to connect at
all.

BUG=576819

Committed: https://crrev.com/97116835b2cd97b9565020bd09f6c78469694ba3
Cr-Commit-Position: refs/heads/master@{#372986}
==========

[commit-bot: I haz the power, 2016-02-02 18:11:36]

Patchset 6 (id:??) landed as
https://crrev.com/97116835b2cd97b9565020bd09f6c78469694ba3
Cr-Commit-Position: refs/heads/master@{#372986}