bluetooth: Post a task when sending GATT events to simulate another thread.

bluetooth: Post a task when sending GATT events to simulate another thread.

On Android, Bluetooth GATT events arrive on a different thread so we post
them on the UI thread when they arrive. There have been a couple of bugs
where multiple events arrive at the same time so when the task on the
UI thread runs, it runs with invalid information.

To simulate this behavior we wrap ThreadUtils in ThreadUtilsWrapper and
in tests always post a task. For example:

SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({1, 2}));
SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({2, 1}));
base::RunLoop().RunUntilIdle();

In this case the first SimulateGattCharacteristicValueChanged() sets
characteristic's value to {1, 2} then posts a task to notify the clients.
Then the second SimulateGattCharacteristicValueChanged() gets called and
sets the characteritic's value to {2, 1} and posts a task to notify the
clients. When the first posted task runs it sees the value is {2, 1} and
notifies of this value. Which is incorrect.

The first of a set of patches to fix race conditions in Android:

 [1] Post a task when sending GATT events. (This patch).
 [2.1] Close and null out BluetoothGatt on the UI Thread http://crrev.com/2702163002
 [2.2] Copy characteristic's value before posting task http://crrev.com/2707823002

BUG=691407
Review-Url: https://codereview.chromium.org/2708513002
Cr-Commit-Position: refs/heads/master@{#455034}
Committed: https://chromium.googlesource.com/chromium/src/+/9409d055c1933a95f40b7fb2ff08fa26a97f4e08

[ortuno, 2017-02-20 00:43:45]

Description was changed from

==========
bluetooth: Post a task when sending GATT events to simulate another thread.

On Android, Bluetooth GATT events arrive on a different thread so we post
them on the UI thread when they arrive. There have been a couple of bugs
where multiple events arrive at the same time so when the task on the
UI thread runs, it runs with invalid information.

To simulate this behavior we wrap ThreadUtils in ThreadUtilsWrapper and
in tests always post a task. For example:

SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({1, 2}));
SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({2, 1}));
base::RunLoop().RunUntilIdle();

In this case the first SimulateGattCharacteristicValueChanged() sets
characteristic's value to {1, 2} then posts a task to notify the clients.
Then the second SimulateGattCharacteristicValueChanged() gets called and
sets the characteritic's value to {2, 1} and posts a task to notify the
clients. When the first posted task runs it sees the value is {2, 1} and
notifies of this value. Which is incorrect.

BUG=691407
==========

to

==========
bluetooth: Post a task when sending GATT events to simulate another thread.

On Android, Bluetooth GATT events arrive on a different thread so we post
them on the UI thread when they arrive. There have been a couple of bugs
where multiple events arrive at the same time so when the task on the
UI thread runs, it runs with invalid information.

To simulate this behavior we wrap ThreadUtils in ThreadUtilsWrapper and
in tests always post a task. For example:

SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({1, 2}));
SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({2, 1}));
base::RunLoop().RunUntilIdle();

In this case the first SimulateGattCharacteristicValueChanged() sets
characteristic's value to {1, 2} then posts a task to notify the clients.
Then the second SimulateGattCharacteristicValueChanged() gets called and
sets the characteritic's value to {2, 1} and posts a task to notify the
clients. When the first posted task runs it sees the value is {2, 1} and
notifies of this value. Which is incorrect.

The first of a set of patches to fix race conditions in Android:

 [1] Post a task when sending GATT events. (This patch).
 [2] Close BluetoothGatt on UI thread.
 [3] Copy characteristic's value before posting task.

BUG=691407
==========

[ortuno, 2017-02-20 05:00:05]

Description was changed from

==========
bluetooth: Post a task when sending GATT events to simulate another thread.

On Android, Bluetooth GATT events arrive on a different thread so we post
them on the UI thread when they arrive. There have been a couple of bugs
where multiple events arrive at the same time so when the task on the
UI thread runs, it runs with invalid information.

To simulate this behavior we wrap ThreadUtils in ThreadUtilsWrapper and
in tests always post a task. For example:

SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({1, 2}));
SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({2, 1}));
base::RunLoop().RunUntilIdle();

In this case the first SimulateGattCharacteristicValueChanged() sets
characteristic's value to {1, 2} then posts a task to notify the clients.
Then the second SimulateGattCharacteristicValueChanged() gets called and
sets the characteritic's value to {2, 1} and posts a task to notify the
clients. When the first posted task runs it sees the value is {2, 1} and
notifies of this value. Which is incorrect.

The first of a set of patches to fix race conditions in Android:

 [1] Post a task when sending GATT events. (This patch).
 [2] Close BluetoothGatt on UI thread.
 [3] Copy characteristic's value before posting task.

BUG=691407
==========

to

==========
bluetooth: Post a task when sending GATT events to simulate another thread.

On Android, Bluetooth GATT events arrive on a different thread so we post
them on the UI thread when they arrive. There have been a couple of bugs
where multiple events arrive at the same time so when the task on the
UI thread runs, it runs with invalid information.

To simulate this behavior we wrap ThreadUtils in ThreadUtilsWrapper and
in tests always post a task. For example:

SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({1, 2}));
SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({2, 1}));
base::RunLoop().RunUntilIdle();

In this case the first SimulateGattCharacteristicValueChanged() sets
characteristic's value to {1, 2} then posts a task to notify the clients.
Then the second SimulateGattCharacteristicValueChanged() gets called and
sets the characteritic's value to {2, 1} and posts a task to notify the
clients. When the first posted task runs it sees the value is {2, 1} and
notifies of this value. Which is incorrect.

The first of a set of patches to fix race conditions in Android:

 [1] Post a task when sending GATT events. (This patch).
 [2.1] Close and null out BluetoothGatt on the UI Thread
http://crrev.com/2702163002
 [2.2] Copy characteristic's value before posting task.

BUG=691407
==========

[ortuno, 2017-02-20 05:55:34]

Description was changed from

==========
bluetooth: Post a task when sending GATT events to simulate another thread.

On Android, Bluetooth GATT events arrive on a different thread so we post
them on the UI thread when they arrive. There have been a couple of bugs
where multiple events arrive at the same time so when the task on the
UI thread runs, it runs with invalid information.

To simulate this behavior we wrap ThreadUtils in ThreadUtilsWrapper and
in tests always post a task. For example:

SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({1, 2}));
SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({2, 1}));
base::RunLoop().RunUntilIdle();

In this case the first SimulateGattCharacteristicValueChanged() sets
characteristic's value to {1, 2} then posts a task to notify the clients.
Then the second SimulateGattCharacteristicValueChanged() gets called and
sets the characteritic's value to {2, 1} and posts a task to notify the
clients. When the first posted task runs it sees the value is {2, 1} and
notifies of this value. Which is incorrect.

The first of a set of patches to fix race conditions in Android:

 [1] Post a task when sending GATT events. (This patch).
 [2.1] Close and null out BluetoothGatt on the UI Thread
http://crrev.com/2702163002
 [2.2] Copy characteristic's value before posting task.

BUG=691407
==========

to

==========
bluetooth: Post a task when sending GATT events to simulate another thread.

On Android, Bluetooth GATT events arrive on a different thread so we post
them on the UI thread when they arrive. There have been a couple of bugs
where multiple events arrive at the same time so when the task on the
UI thread runs, it runs with invalid information.

To simulate this behavior we wrap ThreadUtils in ThreadUtilsWrapper and
in tests always post a task. For example:

SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({1, 2}));
SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({2, 1}));
base::RunLoop().RunUntilIdle();

In this case the first SimulateGattCharacteristicValueChanged() sets
characteristic's value to {1, 2} then posts a task to notify the clients.
Then the second SimulateGattCharacteristicValueChanged() gets called and
sets the characteritic's value to {2, 1} and posts a task to notify the
clients. When the first posted task runs it sees the value is {2, 1} and
notifies of this value. Which is incorrect.

The first of a set of patches to fix race conditions in Android:

 [1] Post a task when sending GATT events. (This patch).
 [2.1] Close and null out BluetoothGatt on the UI Thread
http://crrev.com/2702163002
 [2.2] Copy characteristic's value before posting task
http://crrev.com/2707823002

BUG=691407
==========

[ortuno, 2017-02-20 05:55:59]

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

[commit-bot: I haz the power, 2017-02-20 05:56:10]

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-02-20 06:58:42]

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

[commit-bot: I haz the power, 2017-02-20 06:58:43]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-02-21 10:44:25]

ortuno@chromium.org changed reviewers:
+ jyasskin@chromium.org

[ortuno, 2017-02-21 10:44:25]

jyasskin: PTAL

[ortuno, 2017-02-27 23:06:26]

ortuno@chromium.org changed reviewers:
+ scheib@chromium.org
- jyasskin@chromium.org

[ortuno, 2017-02-27 23:06:27]

scheib: PTAL

[scheib, 2017-03-04 18:47:05]

lgtm

https://codereview.chromium.org/2708513002/diff/1/device/bluetooth/test/andro...
File
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java
(right):

https://codereview.chromium.org/2708513002/diff/1/device/bluetooth/test/andro...
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:94:
return;
Is this ever expected to happen? I didn't see a flow where it does, but may have
overlooked. Use an assert if not expected.(they work now, yay,
https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java....
)

[ortuno, 2017-03-07 03:40:52]

Thanks!

https://codereview.chromium.org/2708513002/diff/1/device/bluetooth/test/andro...
File
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java
(right):

https://codereview.chromium.org/2708513002/diff/1/device/bluetooth/test/andro...
device/bluetooth/test/android/java/src/org/chromium/device/bluetooth/Fakes.java:94:
return;
On 2017/03/04 at 18:47:05, scheib wrote:
> Is this ever expected to happen? I didn't see a flow where it does, but may
have overlooked. Use an assert if not expected.(they work now, yay,
https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java....
)

I think it's left over code from some intermediate step. I could add an assert
but I think the test failing in the next line should be enough.

[ortuno, 2017-03-07 03:41:09]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2017-03-07 03:41:09]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/2708513002/#ps20001
(title: "Remove unnecessary if statement")

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

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:06:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488858069136650,
"parent_rev": "bfc0b58db4179e622fe480ef22d483fa1897cef1", "commit_rev":
"9409d055c1933a95f40b7fb2ff08fa26a97f4e08"}

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

Description was changed from

==========
bluetooth: Post a task when sending GATT events to simulate another thread.

On Android, Bluetooth GATT events arrive on a different thread so we post
them on the UI thread when they arrive. There have been a couple of bugs
where multiple events arrive at the same time so when the task on the
UI thread runs, it runs with invalid information.

To simulate this behavior we wrap ThreadUtils in ThreadUtilsWrapper and
in tests always post a task. For example:

SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({1, 2}));
SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({2, 1}));
base::RunLoop().RunUntilIdle();

In this case the first SimulateGattCharacteristicValueChanged() sets
characteristic's value to {1, 2} then posts a task to notify the clients.
Then the second SimulateGattCharacteristicValueChanged() gets called and
sets the characteritic's value to {2, 1} and posts a task to notify the
clients. When the first posted task runs it sees the value is {2, 1} and
notifies of this value. Which is incorrect.

The first of a set of patches to fix race conditions in Android:

 [1] Post a task when sending GATT events. (This patch).
 [2.1] Close and null out BluetoothGatt on the UI Thread
http://crrev.com/2702163002
 [2.2] Copy characteristic's value before posting task
http://crrev.com/2707823002

BUG=691407
==========

to

==========
bluetooth: Post a task when sending GATT events to simulate another thread.

On Android, Bluetooth GATT events arrive on a different thread so we post
them on the UI thread when they arrive. There have been a couple of bugs
where multiple events arrive at the same time so when the task on the
UI thread runs, it runs with invalid information.

To simulate this behavior we wrap ThreadUtils in ThreadUtilsWrapper and
in tests always post a task. For example:

SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({1, 2}));
SimulateGattCharacteristicValueChanged(std::vector<uint8_t>({2, 1}));
base::RunLoop().RunUntilIdle();

In this case the first SimulateGattCharacteristicValueChanged() sets
characteristic's value to {1, 2} then posts a task to notify the clients.
Then the second SimulateGattCharacteristicValueChanged() gets called and
sets the characteritic's value to {2, 1} and posts a task to notify the
clients. When the first posted task runs it sees the value is {2, 1} and
notifies of this value. Which is incorrect.

The first of a set of patches to fix race conditions in Android:

 [1] Post a task when sending GATT events. (This patch).
 [2.1] Close and null out BluetoothGatt on the UI Thread
http://crrev.com/2702163002
 [2.2] Copy characteristic's value before posting task
http://crrev.com/2707823002

BUG=691407

Review-Url: https://codereview.chromium.org/2708513002
Cr-Commit-Position: refs/heads/master@{#455034}
Committed:
https://chromium.googlesource.com/chromium/src/+/9409d055c1933a95f40b7fb2ff08...
==========

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/9409d055c1933a95f40b7fb2ff08...