bluetooth: Add Web Bluetooth blacklist checks to requestDevice.

bluetooth: Add Web Bluetooth blacklist checks to requestDevice.

Disallow access to blacklisted service UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

BUG=584398
Committed: https://crrev.com/0f3c66db2769c5773d9701e576e66bc8cf467cda
Cr-Commit-Position: refs/heads/master@{#375372}

[scheib, 2016-02-04 22:26:05]

Description was changed from

==========
bluetooth: Initial BluetoothBlacklist integration into Web Bluetooth.

Disallow access to blacklisted UUIDs, as per Web Bluetooth specification.
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

This follow up patch uses the newwly introduced BluetoothBlacklist to
restruct Web Bluetooth operations.

BUG=570121
==========

to

==========
bluetooth: Initial BluetoothBlacklist integration into Web Bluetooth.

Disallow access to blacklisted UUIDs, as per Web Bluetooth specification.
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

This follow up patch uses the newwly introduced BluetoothBlacklist to
restruct Web Bluetooth operations.

BUG=584398
==========

[Jeffrey Yasskin, 2016-02-05 17:10:52]

Description was changed from

==========
bluetooth: Initial BluetoothBlacklist integration into Web Bluetooth.

Disallow access to blacklisted UUIDs, as per Web Bluetooth specification.
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

This follow up patch uses the newwly introduced BluetoothBlacklist to
restruct Web Bluetooth operations.

BUG=584398
==========

to

==========
bluetooth: Initial BluetoothBlacklist integration into Web Bluetooth.

Disallow access to blacklisted UUIDs, as per Web Bluetooth specification.
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

This follow up patch uses the newly introduced BluetoothBlacklist to
restrict Web Bluetooth operations.

BUG=584398
==========

[scheib, 2016-02-12 04:51:19]

Patchset #1 (id:1) has been deleted

[scheib, 2016-02-12 04:51:27]

Patchset #1 (id:20001) has been deleted

[scheib, 2016-02-12 04:51:42]

Patchset #1 (id:40001) has been deleted

[scheib, 2016-02-12 04:51:51]

Patchset #1 (id:60001) has been deleted

[scheib, 2016-02-12 05:08:05]

Description was changed from

==========
bluetooth: Initial BluetoothBlacklist integration into Web Bluetooth.

Disallow access to blacklisted UUIDs, as per Web Bluetooth specification.
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

This follow up patch uses the newly introduced BluetoothBlacklist to
restrict Web Bluetooth operations.

BUG=584398
==========

to

==========
bluetooth: Add Web Bluetooth blacklist checks to requestDevice.

Disallow access to blacklisted service UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

BUG=584398
==========

[scheib, 2016-02-12 05:08:06]

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

[scheib, 2016-02-12 05:08:28]

Patchset #1 (id:80001) has been deleted

[ortuno, 2016-02-12 16:41:47]

https://codereview.chromium.org/1666003002/diff/90001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1666003002/diff/90001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:996: // Use local
optional_services_blacklist_filtered in this method.
Unsure what this comment means or why it's in the middle of the method's
parameters. I think you forgot to delete it?

https://codereview.chromium.org/1666003002/diff/90001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1019: Send(new
BluetoothMsg_RequestDeviceError(
You need to record the outcome with RecordRequestDeviceOutcome.

https://codereview.chromium.org/1666003002/diff/90001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1026:
BluetoothBlacklist::Get().RemoveExcludedUuids(
Didn't know LazyInstance::Get() returned a reference; I thought we usually
returned pointers.

https://codereview.chromium.org/1666003002/diff/90001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1666003002/diff/90001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:208:
// |HeartRateAndHIDAdapter|
Can you add this device to the testing API doc or open an issue to add it?

https://webbluetoothcg.github.io/web-bluetooth/tests/

https://codereview.chromium.org/1666003002/diff/90001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/requestDevice-blacklist.html
(right):

https://codereview.chromium.org/1666003002/diff/90001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice-blacklist.html:42: 
Remove new lines.

https://codereview.chromium.org/1666003002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/1666003002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:65:
MAP_ERROR(RequestDeviceWithBlacklistedUUID, SecurityError, "requestDevice()
called with a filter containing a blacklisted UUID.");
Should we add a link to the spec that explains why the service is blacklisted?
Here is a link to the spec and a shortened version that we can include:

https://webbluetoothcg.github.io/web-bluetooth/#attacks-on-devices

https://goo.gl/4NeimX

[scheib, 2016-02-12 18:25:32]

Thanks,

https://codereview.chromium.org/1666003002/diff/90001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1666003002/diff/90001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:996: // Use local
optional_services_blacklist_filtered in this method.
On 2016/02/12 16:41:47, ortuno wrote:
> Unsure what this comment means or why it's in the middle of the method's
> parameters. I think you forgot to delete it?

See if this new approach is clearer.

https://codereview.chromium.org/1666003002/diff/90001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1019: Send(new
BluetoothMsg_RequestDeviceError(
On 2016/02/12 16:41:47, ortuno wrote:
> You need to record the outcome with RecordRequestDeviceOutcome.

Done.

https://codereview.chromium.org/1666003002/diff/90001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1026:
BluetoothBlacklist::Get().RemoveExcludedUuids(
On 2016/02/12 16:41:47, ortuno wrote:
> Didn't know LazyInstance::Get() returned a reference; I thought we usually
> returned pointers.

One benefit of lazyinstance is that it reserves storage space at link time,
instead of allocating on the heap. By creating the object, not just a pointer to
one that is allocated later, we save a one more heap fragmenting permanent
allocation.

https://codereview.chromium.org/1666003002/diff/90001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1666003002/diff/90001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:208:
// |HeartRateAndHIDAdapter|
On 2016/02/12 16:41:47, ortuno wrote:
> Can you add this device to the testing API doc or open an issue to add it?
> 
> https://webbluetoothcg.github.io/web-bluetooth/tests/

issue filed:
https://github.com/WebBluetoothCG/web-bluetooth/issues/214
I'll wait until the patch series is done to have a complete list of what fakes
are needed.

https://codereview.chromium.org/1666003002/diff/90001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/requestDevice-blacklist.html
(right):

https://codereview.chromium.org/1666003002/diff/90001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice-blacklist.html:42: 
On 2016/02/12 16:41:47, ortuno wrote:
> Remove new lines.

Done.

https://codereview.chromium.org/1666003002/diff/90001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/1666003002/diff/90001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:65:
MAP_ERROR(RequestDeviceWithBlacklistedUUID, SecurityError, "requestDevice()
called with a filter containing a blacklisted UUID.");
On 2016/02/12 16:41:47, ortuno wrote:
> Should we add a link to the spec that explains why the service is blacklisted?
> Here is a link to the spec and a shortened version that we can include:
> 
> https://webbluetoothcg.github.io/web-bluetooth/#attacks-on-devices
> 
> https://goo.gl/4NeimX

Done.

[ortuno, 2016-02-12 18:34:59]

https://codereview.chromium.org/1666003002/diff/110001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1666003002/diff/110001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:997:
optional_services_before_being_filtered_by_blacklist) {
I'm not sure how to handle the naming here. Should this new name trickle all the
way up to WebBluetooth::requestDevice? It's confusing if the name changes
halfway in the chain. In any case you at least want the header to match this
name.

What about optional_services_blacklist_unfiltered or just
optional_services_unfiltered?

[scheib, 2016-02-12 21:00:52]

https://codereview.chromium.org/1666003002/diff/110001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1666003002/diff/110001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:997:
optional_services_before_being_filtered_by_blacklist) {
On 2016/02/12 18:34:59, ortuno wrote:
> I'm not sure how to handle the naming here. Should this new name trickle all
the
> way up to WebBluetooth::requestDevice? It's confusing if the name changes
> halfway in the chain. In any case you at least want the header to match this
> name.
> 
> What about optional_services_blacklist_unfiltered or just
> optional_services_unfiltered?

After in person discussion decided to keep name 'optional_services' consistent
and to add checks when services are accessed later.

[scheib, 2016-02-12 21:00:59]

Patchset #3 (id:130001) has been deleted

[ortuno, 2016-02-12 21:02:54]

lgtm

[ortuno, 2016-02-12 21:02:57]

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

[commit-bot: I haz the power, 2016-02-12 21:04:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1666003002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1666003002/150001

[commit-bot: I haz the power, 2016-02-12 21:14:40]

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

[commit-bot: I haz the power, 2016-02-12 21:14:41]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[scheib, 2016-02-12 21:25:52]

scheib@chromium.org changed reviewers:
+ isherman@chromium.org

[scheib, 2016-02-12 21:26:16]

isherman, histograms.xml review please.

[Ilya Sherman, 2016-02-12 22:47:19]

histograms.xml lgtm

[scheib, 2016-02-13 05:08:45]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-02-13 05:08:55]

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

[ortuno, 2016-02-13 05:44:53]

https://codereview.chromium.org/1666003002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/1666003002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:65:
MAP_ERROR(RequestDeviceWithBlacklistedUUID, SecurityError, "requestDevice()
called with a filter containing a blacklisted UUID. https://goo.gl/4NeimX");
You forgot to add the URL to the test.

[commit-bot: I haz the power, 2016-02-13 06:28:00]

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

[commit-bot: I haz the power, 2016-02-13 06:28:01]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[scheib, 2016-02-13 22:44:25]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-02-13 22:44:33]

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

[scheib, 2016-02-13 22:52:45]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-02-13 22:52:45]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1666003002/#ps170001
(title: "fix test expectation with updated error string")

[commit-bot: I haz the power, 2016-02-13 22:52:52]

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

[commit-bot: I haz the power, 2016-02-14 00:18:52]

Description was changed from

==========
bluetooth: Add Web Bluetooth blacklist checks to requestDevice.

Disallow access to blacklisted service UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

BUG=584398
==========

to

==========
bluetooth: Add Web Bluetooth blacklist checks to requestDevice.

Disallow access to blacklisted service UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

BUG=584398
==========

[commit-bot: I haz the power, 2016-02-14 00:18:53]

Committed patchset #4 (id:170001)

[commit-bot: I haz the power, 2016-02-16 22:48:00]

Description was changed from

==========
bluetooth: Add Web Bluetooth blacklist checks to requestDevice.

Disallow access to blacklisted service UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

BUG=584398
==========

to

==========
bluetooth: Add Web Bluetooth blacklist checks to requestDevice.

Disallow access to blacklisted service UUIDs, as per Web Bluetooth
specification:
https://webbluetoothcg.github.io/web-bluetooth/#the-gatt-blacklist

BUG=584398

Committed: https://crrev.com/0f3c66db2769c5773d9701e576e66bc8cf467cda
Cr-Commit-Position: refs/heads/master@{#375372}
==========

[commit-bot: I haz the power, 2016-02-16 22:48:01]

Patchset 4 (id:??) landed as
https://crrev.com/0f3c66db2769c5773d9701e576e66bc8cf467cda
Cr-Commit-Position: refs/heads/master@{#375372}