Bluetooth: macOS: DidModifyServices can happens while scanning

Bluetooth: macOS: DidModifyServices can happens while scanning

This patch is to fix this scenario:
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverCharacteristics
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidDiscoverCharacteristics

We need to wait until all the pending characteristic discoveries are done until we can start scanning
for descriptors. We need to make sure descriptors are not ready if DidModifyServices notification
is received while scanning for descriptors.


Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is equal to 0.

BUG=690204
Review-Url: https://codereview.chromium.org/2638653002
Cr-Commit-Position: refs/heads/master@{#469457}
Committed: https://chromium.googlesource.com/chromium/src/+/74fe98e203e687c3c96f25128fc55e94981465a7

[jlebel, 2017-01-15 21:12:28]

Description was changed from

==========
Bluetooth: macOS: No double discovery

Adding a boolean discovery_pending_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac, to make sure characteristic and descriptor
discovery is not called while the answer is still pending.
If -[CBPeripheral discoverDescriptorsForCharacteristic:] is called while the
answer is pending, then -[id<CBPeripheralDelegate>
peripheral:didDiscoverDescriptorsForCharacteristic:error:] will be called twice.
The issue is the same for -[CBPeripheral discoverCharacteristics:forService:]
and -[id<CBPeripheralDelegate>
peripheral:didDiscoverCharacteristicsForService:error:].

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

to

==========
Bluetooth: macOS: No double discovery

Adding a boolean discovery_pending_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac, to make sure characteristic and descriptor
discovery is not called while the answer is still pending.
If -[CBPeripheral discoverDescriptorsForCharacteristic:] is called while the
answer is pending, then -[id<CBPeripheralDelegate>
peripheral:didDiscoverDescriptorsForCharacteristic:error:] will be called twice.
The issue is the same for -[CBPeripheral discoverCharacteristics:forService:]
and -[id<CBPeripheralDelegate>
peripheral:didDiscoverCharacteristicsForService:error:].

BUG=681414
==========

[jlebel, 2017-01-15 21:29:30]

Patchset #2 (id:20001) has been deleted

[jlebel, 2017-01-15 21:43:14]

Description was changed from

==========
Bluetooth: macOS: No double discovery

Adding a boolean discovery_pending_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac, to make sure characteristic and descriptor
discovery is not called while the answer is still pending.
If -[CBPeripheral discoverDescriptorsForCharacteristic:] is called while the
answer is pending, then -[id<CBPeripheralDelegate>
peripheral:didDiscoverDescriptorsForCharacteristic:error:] will be called twice.
The issue is the same for -[CBPeripheral discoverCharacteristics:forService:]
and -[id<CBPeripheralDelegate>
peripheral:didDiscoverCharacteristicsForService:error:].

BUG=681414
==========

to

==========
Bluetooth: macOS: No double discovery

Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.

BUG=681414
==========

[jlebel, 2017-01-15 21:57:58]

jlebel@chromium.org changed reviewers:
+ fbeaufort@chromium.org, scheib@chromium.org

[jlebel, 2017-01-15 21:57:58]

Hello Vincent,

Can you review this patch?

Thanks,

[jlebel, 2017-01-15 23:01:26]

Description was changed from

==========
Bluetooth: macOS: No double discovery

Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.

BUG=681414
==========

to

==========
Bluetooth: macOS: Fix for double discovery

Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.

BUG=681414
==========

[scheib, 2017-01-17 18:37:57]

Please add a test to reproduces this.

[ortuno, 2017-01-19 23:54:21]

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

[ortuno, 2017-01-19 23:54:21]

Why is this needed? DiscoverCharacteristics and DiscoveryDescriptors should only
be called once per Service/Characteristic.

[jlebel, 2017-02-08 22:49:15]

Description was changed from

==========
Bluetooth: macOS: Fix for double discovery

Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.

BUG=681414
==========

to

==========
Bluetooth: macOS: DidModifyServices can happens while scanning

This patch is to fix this scenario:
Device: DidModifyServices notification
    Chrome: Scan for primary services
Device: DidDiscoverServices
    Chrome: Scan for characteristics
Device: DidModifyServices notification
    Chrome: Scan for primary services
Device: DidDiscoverCharacteristics
Device: DidDiscoverServices
    Chrome: Scan for characteristics
Device: DidDiscoverCharacteristics

We need to wait until all the pending characteristic discoveries are done until
we can start scanning
for descriptors. We need to make sure descriptors are not ready if
DidModifyServices notification
is received while scanning for descriptors.


Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.

This issue was triggered by the crbug.com/681414 but this patch doesn't solve
it.
==========

[jlebel, 2017-02-08 22:49:51]

Description was changed from

==========
Bluetooth: macOS: DidModifyServices can happens while scanning

This patch is to fix this scenario:
Device: DidModifyServices notification
    Chrome: Scan for primary services
Device: DidDiscoverServices
    Chrome: Scan for characteristics
Device: DidModifyServices notification
    Chrome: Scan for primary services
Device: DidDiscoverCharacteristics
Device: DidDiscoverServices
    Chrome: Scan for characteristics
Device: DidDiscoverCharacteristics

We need to wait until all the pending characteristic discoveries are done until
we can start scanning
for descriptors. We need to make sure descriptors are not ready if
DidModifyServices notification
is received while scanning for descriptors.


Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.

This issue was triggered by the crbug.com/681414 but this patch doesn't solve
it.
==========

to

==========
Bluetooth: macOS: DidModifyServices can happens while scanning

This patch is to fix this scenario:
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverCharacteristics
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidDiscoverCharacteristics

We need to wait until all the pending characteristic discoveries are done until
we can start scanning
for descriptors. We need to make sure descriptors are not ready if
DidModifyServices notification
is received while scanning for descriptors.


Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.

This issue was triggered by the crbug.com/681414 but this patch doesn't solve
it.
==========

[jlebel, 2017-02-08 22:54:45]

Description was changed from

==========
Bluetooth: macOS: DidModifyServices can happens while scanning

This patch is to fix this scenario:
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverCharacteristics
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidDiscoverCharacteristics

We need to wait until all the pending characteristic discoveries are done until
we can start scanning
for descriptors. We need to make sure descriptors are not ready if
DidModifyServices notification
is received while scanning for descriptors.


Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.

This issue was triggered by the crbug.com/681414 but this patch doesn't solve
it.
==========

to

==========
Bluetooth: macOS: DidModifyServices can happens while scanning

This patch is to fix this scenario:
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverCharacteristics
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidDiscoverCharacteristics

We need to wait until all the pending characteristic discoveries are done until
we can start scanning
for descriptors. We need to make sure descriptors are not ready if
DidModifyServices notification
is received while scanning for descriptors.


Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.
==========

[jlebel, 2017-02-08 23:38:26]

Description was changed from

==========
Bluetooth: macOS: DidModifyServices can happens while scanning

This patch is to fix this scenario:
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverCharacteristics
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidDiscoverCharacteristics

We need to wait until all the pending characteristic discoveries are done until
we can start scanning
for descriptors. We need to make sure descriptors are not ready if
DidModifyServices notification
is received while scanning for descriptors.


Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.
==========

to

==========
Bluetooth: macOS: DidModifyServices can happens while scanning

This patch is to fix this scenario:
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverCharacteristics
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidDiscoverCharacteristics

We need to wait until all the pending characteristic discoveries are done until
we can start scanning
for descriptors. We need to make sure descriptors are not ready if
DidModifyServices notification
is received while scanning for descriptors.


Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.

BUG=690204
==========

[ortuno, 2017-02-09 00:38:08]

Do we need tests for this?

[jlebel, 2017-02-09 00:54:34]

On 2017/02/09 00:38:08, ortuno wrote:
> Do we need tests for this?

Tests are nicer. I want to try to implement them. I'm not sure I can yet.

[jlebel, 2017-04-02 22:50:36]

Patchset #3 (id:60001) has been deleted

[jlebel, 2017-04-10 00:24:16]

On 2017/02/09 00:54:34, jlebel wrote:
> On 2017/02/09 00:38:08, ortuno wrote:
> > Do we need tests for this?
> 
> Tests are nicer. I want to try to implement them. I'm not sure I can yet.

Done.

[scheib, 2017-04-10 20:55:54]

Yay for tests! A few comment requests:

https://codereview.chromium.org/2638653002/diff/120001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_service_unittest.cc (right):

https://codereview.chromium.org/2638653002/diff/120001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:287: // Tests to
receive a services changed notification, while waiting for
The comment for TwoPendingServiceDiscoveryRequests helps explain the test more,
please improve these test descriptions and include why they're likely only macOS
relevant.

https://codereview.chromium.org/2638653002/diff/120001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:348: // Simulates to
receive an extra discvoery characteristic notification from
Keep the #if defined mac guard around each test. Also, spelling 'discovery'.

[ortuno, 2017-04-11 00:58:35]

Looking at how complex this is and how hard the code is to follow I can't help
but wonder if we should decouple the GATT Discovery Procedure from the objects.

strawman proposal: Have a BluetoothGattExplorer (Or Discoverer or whatever)
whose sole purpose is to walk down the GATT tree and notify at the end. The
Explorer would listen to all the ServicesDiscovered, CharacteristicsDiscovered,
etc. events and keep track of all the pending discoveries. When it calls
GattDiscoveryComplete() BluetoothDevice can walk down the tree knowing that all
attributes have been discovered.

This would result in BluetoothLowEnergyDeviceMac code being similar to Android's
code where we just call DiscoverServices, wait for the reply and walk down the
Gatt tree creating our objects.

[jlebel, 2017-04-11 20:37:00]

Patchset #6 (id:140001) has been deleted

[jlebel, 2017-04-11 20:44:41]

For the BluetoothGattExplorer class idea: crbug.com/710599

https://codereview.chromium.org/2638653002/diff/120001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_service_unittest.cc (right):

https://codereview.chromium.org/2638653002/diff/120001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:287: // Tests to
receive a services changed notification, while waiting for
On 2017/04/10 20:55:54, scheib wrote:
> The comment for TwoPendingServiceDiscoveryRequests helps explain the test
more,
> please improve these test descriptions and include why they're likely only
macOS
> relevant.

Is my comment clear?

https://codereview.chromium.org/2638653002/diff/120001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:348: // Simulates to
receive an extra discvoery characteristic notification from
On 2017/04/10 20:55:54, scheib wrote:
> Keep the #if defined mac guard around each test. Also, spelling 'discovery'.

Done.

[ortuno, 2017-04-11 22:36:08]

Does this include a test for this crash http://crbug.com/707601?

[jlebel, 2017-04-11 22:38:45]

On 2017/04/11 22:36:08, ortuno wrote:
> Does this include a test for this crash http://crbug.com/707601

Yes. This patch will avoid crashing. But it is how to fix this bug.

[ortuno, 2017-04-11 22:40:00]

On 2017/04/11 at 22:38:45, jlebel wrote:
> On 2017/04/11 22:36:08, ortuno wrote:
> > Does this include a test for this crash http://crbug.com/707601
> 
> Yes. This patch will avoid crashing. But it is how to fix this bug.

I mean does this patch include a test that would make sure
http://crbug.com/707601 is fixed?

[jlebel, 2017-04-11 22:50:08]

On 2017/04/11 22:38:45, jlebel wrote:
> On 2017/04/11 22:36:08, ortuno wrote:
> > Does this include a test for this crash http://crbug.com/707601
> 
> Yes. This patch will avoid crashing. But it is how to fix this bug.

It is NOT how to fix this bug.

But I guess the real origin of this bug is the fact macOS call twice
"didConnectPeripheral" as we found out in crbug.com/681414.
And because of that we have 2 scanning cycle in parallel (to scan for services,
characteristics and descriptors). With that patch and crrev.com/2641133003 the
code will be able to handle starting 2 scanning cycles at the same times.

[jlebel, 2017-04-13 01:12:24]

On 2017/04/11 22:50:08, jlebel wrote:
> On 2017/04/11 22:38:45, jlebel wrote:
> > On 2017/04/11 22:36:08, ortuno wrote:
> > > Does this include a test for this crash http://crbug.com/707601
> > 
> > Yes. This patch will avoid crashing. But it is how to fix this bug.
> 
> It is NOT how to fix this bug.
> 
> But I guess the real origin of this bug is the fact macOS call twice
> "didConnectPeripheral" as we found out in crbug.com/681414.
> And because of that we have 2 scanning cycle in parallel (to scan for
services,
> characteristics and descriptors). With that patch and crrev.com/2641133003 the
> code will be able to handle starting 2 scanning cycles at the same times.

Yes, crbug.com/707601 is covered by
BluetoothRemoteGattCharacteristicTest.SimulateDeviceModificationWhileDiscoveringDescriptors.
I've just added more unit tests (and I had to update the code to make those
tests work).

[jlebel, 2017-04-13 01:18:02]

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

[commit-bot: I haz the power, 2017-04-13 01:19:34]

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-04-13 01:24:28]

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

[commit-bot: I haz the power, 2017-04-13 01:24:29]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_rel_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_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jlebel, 2017-04-13 07:45:46]

On 2017/04/13 01:12:24, jlebel wrote:
> On 2017/04/11 22:50:08, jlebel wrote:
> > On 2017/04/11 22:38:45, jlebel wrote:
> > > On 2017/04/11 22:36:08, ortuno wrote:
> > > > Does this include a test for this crash http://crbug.com/707601
> > > 
> > > Yes. This patch will avoid crashing. But it is how to fix this bug.
> > 
> > It is NOT how to fix this bug.
> > 
> > But I guess the real origin of this bug is the fact macOS call twice
> > "didConnectPeripheral" as we found out in crbug.com/681414.
> > And because of that we have 2 scanning cycle in parallel (to scan for
> services,
> > characteristics and descriptors). With that patch and crrev.com/2641133003
the
> > code will be able to handle starting 2 scanning cycles at the same times.
> 
> Yes, crbug.com/707601 is covered by
>
BluetoothRemoteGattCharacteristicTest.SimulateDeviceModificationWhileDiscoveringDescriptors.
> I've just added more unit tests (and I had to update the code to make those
> tests work).

This patch starts to get big. I can remove one or two things that can be
reviewed in their own patch, if you want.

[scheib, 2017-04-13 17:54:17]

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

[scheib, 2017-04-17 21:25:48]

On 2017/04/13 07:45:46, jlebel wrote:
> On 2017/04/13 01:12:24, jlebel wrote:
> > On 2017/04/11 22:50:08, jlebel wrote:
> > > On 2017/04/11 22:38:45, jlebel wrote:
> > > > On 2017/04/11 22:36:08, ortuno wrote:
> > > > > Does this include a test for this crash http://crbug.com/707601
> > > > 
> > > > Yes. This patch will avoid crashing. But it is how to fix this bug.
> > > 
> > > It is NOT how to fix this bug.
> > > 
> > > But I guess the real origin of this bug is the fact macOS call twice
> > > "didConnectPeripheral" as we found out in crbug.com/681414.
> > > And because of that we have 2 scanning cycle in parallel (to scan for
> > services,
> > > characteristics and descriptors). With that patch and crrev.com/2641133003
> the
> > > code will be able to handle starting 2 scanning cycles at the same times.
> > 
> > Yes, crbug.com/707601 is covered by
> >
>
BluetoothRemoteGattCharacteristicTest.SimulateDeviceModificationWhileDiscoveringDescriptors.
> > I've just added more unit tests (and I had to update the code to make those
> > tests work).
> 
> This patch starts to get big. I can remove one or two things that can be
> reviewed in their own patch, if you want.

Sorry for delay, ortuno and I discussed and I'll be doing an additional review
pass on this.

[scheib, 2017-04-18 04:43:21]

I think this looks good code logic wise. I ask below for comments to help make
the tests easier to read. I also wonder if all the Mac specific test fixture
methods should be prefixed or suffixed with 'Mac' so that it's clear in the test
code when we're manipulating the mac fixture specific bits? E.g. 
SimulateMacDidDiscover...?
MacSimulateDidDiscover...?
SimulateDidDiscover...Mac?

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_unittest.cc (right):

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:476:
SimulateGattServicesDiscovered(
Why does the above use SimulateDidDiscoverServices, and down here
SimulateGattServicesDiscovered? Explain in test code/comments.

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:515: // Unexpected system call to
explain here why another didDiscoverServices is unexpected to happen, because we
only expect it to be called in response to discoverservices. Note earlier where
we expect that our code has made the call to discoverservices. Make it very
obvious how this test works and what the expected result is.

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:258: // This should never
happens, just in case it happens with a device,
"never happen"

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:336: // This should never
happens, just in case it happens with a device,
ditto

[scheib, 2017-04-21 17:49:11]

I see https://crbug.com/713922 suggesting the full
device>service>characteristic>descriptor tree be built in mocks before starting
test code. That's reasonable and would likely simplify some of this test code.

Before we even get to that though, I took another look and it's not clear to my
why SimulateGattServicesDiscovered can't include the AddServicesToDevice and
SimulateDidDiscoverServices logic as well?

Similarly SimulateGattCharacteristic including AddCharacteristicToService &
SimulateDidDiscoverCharacteristics, etc?

[jlebel, 2017-04-29 22:08:04]

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

[jlebel, 2017-04-29 22:08:07]

I'm not sure to understand:
"Before we even get to that though, I took another look and it's not clear to my
why SimulateGattServicesDiscovered can't include the AddServicesToDevice and
SimulateDidDiscoverServices logic as well?"

It is the case right now. The bug is saying that
SimulateGattServicesDiscovered() is doing too much at once. It is not possible
to configure the device with services, characteristics and descriptors before to
make it available to WebBluetooth code.

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_unittest.cc (right):

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:476:
SimulateGattServicesDiscovered(
On 2017/04/18 04:43:21, scheib wrote:
> Why does the above use SimulateDidDiscoverServices, and down here
> SimulateGattServicesDiscovered? Explain in test code/comments.

Done.

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:515: // Unexpected system call to
On 2017/04/18 04:43:21, scheib wrote:
> explain here why another didDiscoverServices is unexpected to happen, because
we
> only expect it to be called in response to discoverservices. Note earlier
where
> we expect that our code has made the call to discoverservices. Make it very
> obvious how this test works and what the expected result is.

Done.

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:258: // This should never
happens, just in case it happens with a device,
On 2017/04/18 04:43:21, scheib wrote:
> "never happen"

Done.

https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:336: // This should never
happens, just in case it happens with a device,
On 2017/04/18 04:43:21, scheib wrote:
> ditto

Done.

[commit-bot: I haz the power, 2017-04-29 22:08:11]

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-04-29 23:40:45]

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

[commit-bot: I haz the power, 2017-04-29 23:40:47]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-04-30 23:32:58]

On 2017/04/29 at 22:08:07, jlebel wrote:
> I'm not sure to understand:
> "Before we even get to that though, I took another look and it's not clear to
my why SimulateGattServicesDiscovered can't include the AddServicesToDevice and
SimulateDidDiscoverServices logic as well?"
> 
> It is the case right now. The bug is saying that
SimulateGattServicesDiscovered() is doing too much at once. It is not possible
to configure the device with services, characteristics and descriptors before to
make it available to WebBluetooth code.
> 

I think he means why do we need two calls: AddServicesToDevices and then
SimulateDidDiscoverServices i.e. why can' SimulateDidDiscoverServices do all the
work?

>
https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
> File device/bluetooth/bluetooth_device_unittest.cc (right):
> 
>
https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
> device/bluetooth/bluetooth_device_unittest.cc:476:
SimulateGattServicesDiscovered(
> On 2017/04/18 04:43:21, scheib wrote:
> > Why does the above use SimulateDidDiscoverServices, and down here
> > SimulateGattServicesDiscovered? Explain in test code/comments.
> 
> Done.
> 
>
https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
> device/bluetooth/bluetooth_device_unittest.cc:515: // Unexpected system call
to
> On 2017/04/18 04:43:21, scheib wrote:
> > explain here why another didDiscoverServices is unexpected to happen,
because we
> > only expect it to be called in response to discoverservices. Note earlier
where
> > we expect that our code has made the call to discoverservices. Make it very
> > obvious how this test works and what the expected result is.
> 
> Done.
> 
>
https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
> File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):
> 
>
https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
> device/bluetooth/bluetooth_low_energy_device_mac.mm:258: // This should never
happens, just in case it happens with a device,
> On 2017/04/18 04:43:21, scheib wrote:
> > "never happen"
> 
> Done.
> 
>
https://codereview.chromium.org/2638653002/diff/180001/device/bluetooth/bluet...
> device/bluetooth/bluetooth_low_energy_device_mac.mm:336: // This should never
happens, just in case it happens with a device,
> On 2017/04/18 04:43:21, scheib wrote:
> > ditto
> 
> Done.

[jlebel, 2017-05-01 13:32:28]

SimulateGattServicesDiscovered() does the same thing than:
AddServicesToDevice()
SimulateDidDiscoverServicesMac()
SimulateDidDiscoverCharacteristicsMac()
SimulateDidDiscoverDescriptorsMac()

For some tests, I need to do simulate incorrect behavior between those steps. So
I can't use SimulateGattServicesDiscovered() and I need to do it step by step.

[scheib, 2017-05-01 17:20:34]

lgtm

https://codereview.chromium.org/2638653002/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_unittest.cc (right):

https://codereview.chromium.org/2638653002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:514: // Fist system call to
'First'

https://codereview.chromium.org/2638653002/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/2638653002/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:131: void
AddServicesToDevice(BluetoothDevice* device,
Please add the Mac suffix to these as well.

[jlebel, 2017-05-01 21:17:00]

Patchset #9 (id:220001) has been deleted

[jlebel, 2017-05-01 21:27:14]

https://codereview.chromium.org/2638653002/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_unittest.cc (right):

https://codereview.chromium.org/2638653002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_unittest.cc:514: // Fist system call to
On 2017/05/01 17:20:33, scheib wrote:
> 'First'

Done.

https://codereview.chromium.org/2638653002/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/2638653002/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:131: void
AddServicesToDevice(BluetoothDevice* device,
On 2017/05/01 17:20:33, scheib wrote:
> Please add the Mac suffix to these as well.

Ok, I was thinking that those methods could be used for crbug.com/713922

[scheib, 2017-05-01 21:44:22]

https://codereview.chromium.org/2638653002/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/2638653002/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:131: void
AddServicesToDevice(BluetoothDevice* device,
On 2017/05/01 21:27:14, jlebel wrote:
> On 2017/05/01 17:20:33, scheib wrote:
> > Please add the Mac suffix to these as well.
> 
> Ok, I was thinking that those methods could be used for crbug.com/713922

Ok, but let's rename them if/when they are moved to cross platform test fixture.

[jlebel, 2017-05-04 17:18:29]

Patchset #10 (id:260001) has been deleted

[jlebel, 2017-05-04 17:19:03]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2017-05-04 17:19:03]

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/2638653002/#ps240001
(title: "First and Mac suffix")

[commit-bot: I haz the power, 2017-05-04 17:19:36]

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-05-04 17:23:19]

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

[commit-bot: I haz the power, 2017-05-04 17:23:21]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  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_compile_dbg_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_...)

[jlebel, 2017-05-04 19:27:14]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2017-05-04 19:27:15]

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/2638653002/#ps280001
(title: "Merge")

[commit-bot: I haz the power, 2017-05-04 19:27:49]

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-05-04 20:45:20]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1493926034625490,
"parent_rev": "63120f6b07e2315545b80732bb94aa1930ae58cb", "commit_rev":
"74fe98e203e687c3c96f25128fc55e94981465a7"}

[commit-bot: I haz the power, 2017-05-04 20:45:31]

Description was changed from

==========
Bluetooth: macOS: DidModifyServices can happens while scanning

This patch is to fix this scenario:
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverCharacteristics
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidDiscoverCharacteristics

We need to wait until all the pending characteristic discoveries are done until
we can start scanning
for descriptors. We need to make sure descriptors are not ready if
DidModifyServices notification
is received while scanning for descriptors.


Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.

BUG=690204
==========

to

==========
Bluetooth: macOS: DidModifyServices can happens while scanning

This patch is to fix this scenario:
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidModifyServices notification
    - Chrome: Scan for primary services
* Device: DidDiscoverCharacteristics
* Device: DidDiscoverServices
    - Chrome: Scan for characteristics
* Device: DidDiscoverCharacteristics

We need to wait until all the pending characteristic discoveries are done until
we can start scanning
for descriptors. We need to make sure descriptors are not ready if
DidModifyServices notification
is received while scanning for descriptors.


Adding discovery_pending_count_ in BluetoothRemoteGattServiceMac and
BluetoothRemoteGattCharacteristicMac.
|is_discovery_complete_| can be set to true when discovery_pending_count_ is
equal to 0.

BUG=690204

Review-Url: https://codereview.chromium.org/2638653002
Cr-Commit-Position: refs/heads/master@{#469457}
Committed:
https://chromium.googlesource.com/chromium/src/+/74fe98e203e687c3c96f25128fc5...
==========

[commit-bot: I haz the power, 2017-05-04 20:45:33]

Committed patchset #10 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/74fe98e203e687c3c96f25128fc5...