New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 725349 link

Starred by 1 user
Status: Archived
Owner:
█ sonnysasaka@chromium.org
Last visit > 30 days ago
Closed: Jun 2017
Cc:
josephsih@chromium.org
█ jlklein@chromium.org
█ lesliewatkins@chromium.org
r...@chromium.org
█ sonnysasaka@chromium.org
tbarzic@chromium.org
█ mcchou@chromium.org
khorimoto@chromium.org
jhawkins@chromium.org
█ rjahagir@chromium.org
michae...@chromium.org
█ hennessywill@chromium.org
█ tengs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 672263



Sign in to add a comment

BluetoothAdapter::RegisterAdvertisement always fails with ERROR_ADVERTISEMENT_ALREADY_EXISTS

Project Member Reported by hansberry@chromium.org, May 23 2017 
Chrome Version: (copy from chrome://version)
Chrome: 60.0.3108.0
Platform: ChromeOS 9576.0.0

What steps will reproduce the problem?
(1) Call BluetoothAdapter::RegisterAdvertisement with reasonable arguments. See usage at [1] (this has worked for ~3 months but recently broke).

What is the expected result?
The call to succeed.

What happens instead?
Fails with error ERROR_ADVERTISEMENT_ALREADY_EXISTS. This is bizarre because no other logic (to my knowledge) is using this API. 

Using 'btmgmt advinfo' provides:

Supported flags: connectable general-discoverable limited-discoverable managed-flags tx-power scan-rsp-appearance scan-rsp-local-name 
Max advertising data len: 31
Max scan response data len: 31
Max instances: 5
Instances list with 0 items

Might there have been an upstreaming issue, like there was with  crbug.com/720063 ? 

1) https://cs.chromium.org/chromium/src/chromeos/components/tether/ble_advertiser.cc?l=81

Comment 1 by josephsih@chromium.org, May 23 2017

Cc: josephsih@chromium.org
Owner: sonnysasaka@chromium.org
Hi Sonny, when we uprev to bluez 5.44, we saw some upstream patches about advertising dbus interface. Could you check whether chrome needs to modify as well? To verify this, please check if you could correctly register an advertisement with both "manufacturer data" and "service data".

Specifically, the signatures of manufacturer data and service data have been enforced. The signatures were not enforced before. And hence even if we do not use the correct expected types before, it would not be complained. Now, correct data types must be used. Otherwise, advertisement registration would fail.

Sonny, could you help check the adv registration code in chrome to see if the signatures for manufacturer data and service data are correct?

If there are any problems, please check the following two CLs in bluez.

commit 758dae03725ed7a1138d848aad940a73bbf5d659
core/advertising: Fix not parsing data properly

commit d356a6242be9a1aba2cca871b79618e3b28a5ec2
core/advertising: Fix crash when passing invalid dictionary

In my case, I need to modify our autotest for BLE advertising to accommodate the CLs. Please refer to our autotest patch "bluetooth: fix advertising data format" (https://chromium-review.googlesource.com/#/c/485540/3/client/cros/bluetooth/advertisement.py).

Comment 2 by r...@chromium.org, May 23 2017

Labels: M-60

Comment 3 by hansberry@chromium.org, May 23 2017 

Hi Joseph,

Are you sure that manufacturer data and service data signature enforcement are what would cause this issue? Wouldn't that return a INVALID_ADVERTISEMENT_ERROR_CODE as opposed to a ERROR_ADVERTISEMENT_ALREADY_EXISTS error?

Comment 4 by r...@chromium.org, May 23 2017

Status: Assigned (was: Untriaged)
It is possible that the error gets handled incorrectly.

Comment 5 by sonnysasaka@chromium.org, May 24 2017 

Yes, I confirmed this. I am writing a CL to fix this.

Comment 6 by hansberry@chromium.org, May 26 2017 

Hi Sonny,

Do you have a rough estimate for when you expect to have a fix for this issue? My stakeholders are curious when we'll be able to dogfood our feature.

Thanks!

Comment 7 by sonnysasaka@chromium.org, May 26 2017 

Hi Ryan, the CL has just been out for review: https://codereview.chromium.org/2907913002/

Comment 8 by hansberry@chromium.org, May 26 2017 

Thanks Sonny! :)

Comment 9 by sonnysasaka@chromium.org, May 26 2017

Cc: rjahagir@chromium.org hennessywill@chromium.org sonnysasaka@chromium.org tbarzic@chromium.org michae...@chromium.org
 Issue 720063  has been merged into this issue.

Comment 10 by hansberry@chromium.org, May 26 2017 

Wanted to add that patching in https://codereview.chromium.org/2907913002/ completely fixes our issue.
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/02bf4db5faf5a62c0b373e1ea66109bea4ef4b6a

commit 02bf4db5faf5a62c0b373e1ea66109bea4ef4b6a
Author: sonnysasaka <sonnysasaka@chromium.org>
Date: Thu Jun 01 20:59:23 2017

Fix ManufacturerData and ServiceData DBus data type to match with bluez.

Bluez recent changes corresponding to this CL:
https://chromium.googlesource.com/chromiumos/third_party/bluez/+/758dae03725ed7a1138d848aad940a73bbf5d659
https://chromium.googlesource.com/chromiumos/third_party/bluez/+/d356a6242be9a1aba2cca871b79618e3b28a5ec2

BUG= 725349 
TEST=Called chrome.bluetoothLowEnergy.registerAdvertisement with
advertisement data containing both manufacturerData and serviceData, the
operation succeeded.

Review-Url: https://codereview.chromium.org/2907913002
Cr-Commit-Position: refs/heads/master@{#476433}

[modify] https://crrev.com/02bf4db5faf5a62c0b373e1ea66109bea4ef4b6a/device/bluetooth/dbus/bluetooth_le_advertisement_service_provider.cc

Comment 12 by sonnysasaka@chromium.org, Jun 1 2017

Status: Fixed (was: Assigned)

Comment 13 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)
►

Sign in to add a comment