[CrOS Tether] Do not register a new Bluetooth advertisement until any previous identical advertisem…

[CrOS Tether] Do not register a new Bluetooth advertisement until any previous identical advertisement has been unregistered.

This fixes the following issue:
  (1) BleAdvertiser starts advertising to device A.
  (2) BleAdvertiser stops advertising to device A. The advertisement starts its asynchyronous unregistration flow.
  (3) BleAdvertiser starts advertising to device A again, but the previous advertisement has not yet been fully unregistered.

After this fix, BleAdvertiser will wait for the previous advertisement to be unregistered in step (3), ensuring that we do not run into the ERROR_ADVERTISEMENT_ALREADY_EXISTS error.

BUG=672263, 739883
Review-Url: https://codereview.chromium.org/2972263002
Cr-Commit-Position: refs/heads/master@{#486047}
Committed: https://chromium.googlesource.com/chromium/src/+/3b45b11eb7575d7e35ff53431dd675e83f428772

[Kyle Horimoto, 2017-07-07 18:24:14]

khorimoto@chromium.org changed reviewers:
+ hansberry@chromium.org

[Ryan Hansberry, 2017-07-11 21:27:42]

lgtm with nits.

https://codereview.chromium.org/2972263002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/ble_advertiser.cc (right):

https://codereview.chromium.org/2972263002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_advertiser.cc:48:
base::Bind(&IndividualAdvertisement::OnAdvertisementUnregisterFailure,
I don't believe that this method is ever called with the code as-is because
IndividualAdvertisement is destroyed by the time this can be called back. So, I
propose we also inject an 'on_advertisement_unregistered_error_callback' to be
placed here (with a corresponding error callback in BleAdvertiser) -- that will
make the code here more consistent, and provides us logs in the bizarre case
that unregistration does fail. Thoughts?

https://codereview.chromium.org/2972263002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_advertiser.cc:255: auto it =
device_id_to_advertisement_map_.find(associated_device_id);
nit: it's now easy to incorrectly conflate what
active_advertisement_device_ids_set_ and device_id_to_advertisement_map_ refer
to (they both seem to be talking about BluetoothAdvertisements when in reality
the first is referring to BluetoothAdvertisements and the latter to
IndividualAdvertisements). Do you mind renaming device_id_to_advertisement_map_
to device_id_to_individual_advertisement_map_?

https://codereview.chromium.org/2972263002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/ble_advertiser.h (right):

https://codereview.chromium.org/2972263002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_advertiser.h:57: // If that is the case, the
newly-created IndividualAdvertisement must wait
please explicitly mention OnPreviousAdvertisementUnregistered()

[rkc, 2017-07-11 21:36:35]

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

[rkc, 2017-07-11 21:36:36]

I thought we were dropping this CL?

[Kyle Horimoto, 2017-07-11 21:44:54]

On 2017/07/11 21:36:36, rkc wrote:
> I thought we were dropping this CL?

We decided to keep this CL simply due to the "maximum of 5 advertisements"
issue. It is a rare edge case, but if we do end up calling
RegisterAdvertisement() multiple times before previous advertisement were
successfully unregistered, we could end up hitting that hard limit of the max
number of advertisements and run into the ERROR_ADVERTISEMENT_ALREADY_EXISTS.
Does that sound okay to you?

[Sonny Sasaka, 2017-07-11 21:55:52]

On 2017/07/11 21:44:54, Kyle Horimoto wrote:
> On 2017/07/11 21:36:36, rkc wrote:
> > I thought we were dropping this CL?
> 
> We decided to keep this CL simply due to the "maximum of 5 advertisements"
> issue. It is a rare edge case, but if we do end up calling
> RegisterAdvertisement() multiple times before previous advertisement were
> successfully unregistered, we could end up hitting that hard limit of the max
> number of advertisements and run into the ERROR_ADVERTISEMENT_ALREADY_EXISTS.
> Does that sound okay to you?

I agree that this would prevent the rare case where the maximum is already
reached, and unregister-then-register could make the max adv exceeded. So making
sure to wait until the previous adv is unregistered before registering a new one
is safer.

[Kyle Horimoto, 2017-07-12 02:22:04]

hansberry@: I know you already LGTM'ed, but PTAL as the change was bit
complicated.

https://codereview.chromium.org/2972263002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/ble_advertiser.cc (right):

https://codereview.chromium.org/2972263002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_advertiser.cc:48:
base::Bind(&IndividualAdvertisement::OnAdvertisementUnregisterFailure,
On 2017/07/11 21:27:41, Ryan Hansberry wrote:
> I don't believe that this method is ever called with the code as-is because
> IndividualAdvertisement is destroyed by the time this can be called back. So,
I
> propose we also inject an 'on_advertisement_unregistered_error_callback' to be
> placed here (with a corresponding error callback in BleAdvertiser) -- that
will
> make the code here more consistent, and provides us logs in the bizarre case
> that unregistration does fail. Thoughts?

Done.

https://codereview.chromium.org/2972263002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_advertiser.cc:255: auto it =
device_id_to_advertisement_map_.find(associated_device_id);
On 2017/07/11 21:27:41, Ryan Hansberry wrote:
> nit: it's now easy to incorrectly conflate what
> active_advertisement_device_ids_set_ and device_id_to_advertisement_map_ refer
> to (they both seem to be talking about BluetoothAdvertisements when in reality
> the first is referring to BluetoothAdvertisements and the latter to
> IndividualAdvertisements). Do you mind renaming
device_id_to_advertisement_map_
> to device_id_to_individual_advertisement_map_?

Done.

https://codereview.chromium.org/2972263002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/ble_advertiser.h (right):

https://codereview.chromium.org/2972263002/diff/1/chromeos/components/tether/...
chromeos/components/tether/ble_advertiser.h:57: // If that is the case, the
newly-created IndividualAdvertisement must wait
On 2017/07/11 21:27:41, Ryan Hansberry wrote:
> please explicitly mention OnPreviousAdvertisementUnregistered()

Done.

[Ryan Hansberry, 2017-07-12 16:27:10]

https://codereview.chromium.org/2972263002/diff/20001/chromeos/components/tet...
File chromeos/components/tether/ble_advertiser.h (right):

https://codereview.chromium.org/2972263002/diff/20001/chromeos/components/tet...
chromeos/components/tether/ble_advertiser.h:125: void
OnAdvertisementUnregistered(const std::string& associated_device_id);
one more nit: make the language of the successful and failed unregistration
methods match. one suggestion: rename 'OnAdvertisementUnregistered' to
'OnUnregisterAdvertisementSuccess'

[Kyle Horimoto, 2017-07-12 17:40:30]

https://codereview.chromium.org/2972263002/diff/20001/chromeos/components/tet...
File chromeos/components/tether/ble_advertiser.h (right):

https://codereview.chromium.org/2972263002/diff/20001/chromeos/components/tet...
chromeos/components/tether/ble_advertiser.h:125: void
OnAdvertisementUnregistered(const std::string& associated_device_id);
On 2017/07/12 16:27:10, Ryan Hansberry wrote:
> one more nit: make the language of the successful and failed unregistration
> methods match. one suggestion: rename 'OnAdvertisementUnregistered' to
> 'OnUnregisterAdvertisementSuccess'

Done.

[Kyle Horimoto, 2017-07-12 17:40:39]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-07-12 17:40:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
hansberry@chromium.org
Link to the patchset: https://codereview.chromium.org/2972263002/#ps40001
(title: "hansberry@ comment.")

[commit-bot: I haz the power, 2017-07-12 17:40:51]

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-07-12 19:08:40]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499881239185420,
"parent_rev": "0a87a53947ecc189c37fec3da875533204826c6d", "commit_rev":
"3b45b11eb7575d7e35ff53431dd675e83f428772"}

[commit-bot: I haz the power, 2017-07-12 19:08:58]

Description was changed from

==========
[CrOS Tether] Do not register a new Bluetooth advertisement until any previous
identical advertisement has been unregistered.

This fixes the following issue:
  (1) BleAdvertiser starts advertising to device A.
  (2) BleAdvertiser stops advertising to device A. The advertisement starts its
asynchyronous unregistration flow.
  (3) BleAdvertiser starts advertising to device A again, but the previous
advertisement has not yet been fully unregistered.

After this fix, BleAdvertiser will wait for the previous advertisement to be
unregistered in step (3), ensuring that we do not run into the
ERROR_ADVERTISEMENT_ALREADY_EXISTS error.

BUG=672263,739883
==========

to

==========
[CrOS Tether] Do not register a new Bluetooth advertisement until any previous
identical advertisement has been unregistered.

This fixes the following issue:
  (1) BleAdvertiser starts advertising to device A.
  (2) BleAdvertiser stops advertising to device A. The advertisement starts its
asynchyronous unregistration flow.
  (3) BleAdvertiser starts advertising to device A again, but the previous
advertisement has not yet been fully unregistered.

After this fix, BleAdvertiser will wait for the previous advertisement to be
unregistered in step (3), ensuring that we do not run into the
ERROR_ADVERTISEMENT_ALREADY_EXISTS error.

BUG=672263,739883

Review-Url: https://codereview.chromium.org/2972263002
Cr-Commit-Position: refs/heads/master@{#486047}
Committed:
https://chromium.googlesource.com/chromium/src/+/3b45b11eb7575d7e35ff53431dd6...
==========

[commit-bot: I haz the power, 2017-07-12 19:08:59]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3b45b11eb7575d7e35ff53431dd6...