bluetooth: Move GetCharacteristic(s) over to Mojo

content/browser/bluetooth/web_bluetooth_service_impl.cc

Index: content/browser/bluetooth/web_bluetooth_service_impl.cc
diff --git a/content/browser/bluetooth/web_bluetooth_service_impl.cc b/content/browser/bluetooth/web_bluetooth_service_impl.cc
index 7f6228c080efc28a4702ce315697cb94102eaf8c..a1d49aefb84b9b8c5ef5968ad40d37e3c7aa2ae7 100644
--- a/content/browser/bluetooth/web_bluetooth_service_impl.cc
+++ b/content/browser/bluetooth/web_bluetooth_service_impl.cc
@@ -2,6 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// ID Not In Map Note:
+// A service, characteristic, or descriptor ID not in the corresponding
+// BluetoothDispatcherHost map [service_to_device_, characteristic_to_service_,
+// descriptor_to_characteristic_] implies a hostile renderer because a renderer

[Jeffrey Yasskin, 2016/04/22 01:16:38]

This may not be true after all: if we revoke access to an attribute concurrently
with the renderer making a request about that attribute, then we should just
deny the request and not kill the renderer. No need to fix that while you're
migrating to Mojo, but we should fix it eventually.

[ortuno, 2016/04/25 15:29:45]

Yup. Opened issue so we don't forget: http://crbug.com/605962

+// obtains the corresponding ID from this class and it will be added to the map
+// at that time.
+
 #include "content/browser/bluetooth/web_bluetooth_service_impl.h"
 
 #include "base/thread_task_runner_handle.h"
@@ -58,9 +65,25 @@ blink::mojom::WebBluetoothError TranslateGATTErrorAndRecord(
   return blink::mojom::WebBluetoothError::GATT_UNTRANSLATED_ERROR_CODE;
 }
 
-}  // namespace
+// TODO(ortuno): This should really be a BluetoothDevice method.
+// Replace when implemented. http://crbug.com/552022
+std::vector<device::BluetoothGattCharacteristic*> GetCharacteristicsByUUID(
+    device::BluetoothGattService* service,
+    const std::string& characteristic_uuid) {
+  std::vector<device::BluetoothGattCharacteristic*> characteristics;
+  VLOG(1) << "Looking for characteristic: " << characteristic_uuid;
+  for (device::BluetoothGattCharacteristic* characteristic :
+       service->GetCharacteristics()) {
+    VLOG(1) << "Characteristic in cache: "
+            << characteristic->GetUUID().canonical_value();
+    if (characteristic->GetUUID().canonical_value() == characteristic_uuid) {
+      characteristics.push_back(characteristic);
+    }
+  }
+  return characteristics;
+}
 
-using CacheQueryResult = BluetoothDispatcherHost::CacheQueryResult;
+}  // namespace
 
 WebBluetoothServiceImpl::WebBluetoothServiceImpl(
     RenderFrameHost* render_frame_host,
@@ -90,7 +113,6 @@ void WebBluetoothServiceImpl::DidFinishNavigation(
   if (navigation_handle->HasCommitted() &&
       navigation_handle->GetRenderFrameHost() == render_frame_host_ &&
       !navigation_handle->IsSamePage()) {
-    // After navigation we need to clear the frame's state.
     ClearState();
   }
 }
@@ -107,12 +129,9 @@ void WebBluetoothServiceImpl::GattCharacteristicValueChanged(
     device::BluetoothAdapter* adapter,
     device::BluetoothGattCharacteristic* characteristic,
     const std::vector<uint8_t>& value) {
-  // TODO(ortuno): Only send characteristic value changed events for
-  // characteristics that we've returned in the past. We can't yet do
-  // this because WebBluetoothServiceImpl doesn't have direct access
-  // to the list of returned characteristics.
-  // https://crbug.com/508771
-  if (BluetoothBlacklist::Get().IsExcluded(characteristic->GetUUID())) {
+  // Don't notify of characteristics that we haven't returned.
+  if (!ContainsKey(characteristic_to_service_,
+                   characteristic->GetIdentifier())) {
     return;
   }
 
@@ -143,6 +162,101 @@ void WebBluetoothServiceImpl::SetClient(
   client_.Bind(std::move(client));
 }
 
+void WebBluetoothServiceImpl::RemoteServiceGetCharacteristics(
+    const mojo::String& service_instance_id,
+    bool single_characteristic,
+    const mojo::String& characteristics_uuid,
+    const RemoteServiceGetCharacteristicsCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  RecordWebBluetoothFunctionCall(
+      single_characteristic
+          ? UMAWebBluetoothFunction::SERVICE_GET_CHARACTERISTIC
+          : UMAWebBluetoothFunction::SERVICE_GET_CHARACTERISTICS);
+  RecordGetCharacteristicsCharacteristic(single_characteristic,
+                                         characteristics_uuid);
+
+  if (!characteristics_uuid.is_null() &&
+      BluetoothBlacklist::Get().IsExcluded(
+          device::BluetoothUUID(characteristics_uuid))) {
+    RecordGetCharacteristicsOutcome(single_characteristic,
+                                    UMAGetCharacteristicOutcome::BLACKLISTED);
+    callback.Run(
+        blink::mojom::WebBluetoothError::BLACKLISTED_CHARACTERISTIC_UUID,
+        nullptr /* characteristics */);
+    return;
+  }
+
+  const CacheQueryResult query_result =
+      GetBluetoothDispatcherHost()->QueryCacheForService(GetOrigin(),
+                                                         service_instance_id);
+
+  if (query_result.outcome == CacheQueryOutcome::BAD_RENDERER) {
+    binding_.Close();
+    return;
+  }
+
+  if (query_result.outcome != CacheQueryOutcome::SUCCESS) {
+    RecordGetCharacteristicsOutcome(single_characteristic,
+                                    query_result.outcome);
+    callback.Run(query_result.GetWebError(), nullptr /* characteristics */);
+    return;
+  }
+
+  std::vector<device::BluetoothGattCharacteristic*> characteristics =
+      characteristics_uuid.is_null()
+          ? query_result.service->GetCharacteristics()
+          : GetCharacteristicsByUUID(query_result.service,
+                                     characteristics_uuid);
+
+  mojo::Array<blink::mojom::WebBluetoothRemoteGATTCharacteristicPtr>
+      response_characteristics;
+  for (device::BluetoothGattCharacteristic* characteristic : characteristics) {
+    if (BluetoothBlacklist::Get().IsExcluded(characteristic->GetUUID())) {
+      continue;
+    }
+    std::string characteristic_instance_id = characteristic->GetIdentifier();
+    auto insert_result = characteristic_to_service_.insert(
+        std::make_pair(characteristic_instance_id, service_instance_id));
+    // If value is already in map, DCHECK it's valid.
+    if (!insert_result.second)
+      DCHECK(insert_result.first->second == service_instance_id);
+
+    blink::mojom::WebBluetoothRemoteGATTCharacteristicPtr characteristic_ptr =
+        blink::mojom::WebBluetoothRemoteGATTCharacteristic::New();
+    characteristic_ptr->instance_id =
+        mojo::String::From(characteristic_instance_id);
+    characteristic_ptr->uuid =
+        characteristics_uuid.is_null()
+            ? mojo::String::From(characteristic->GetUUID().canonical_value())
+            : nullptr;

[Jeffrey Yasskin, 2016/04/22 01:16:38]

The characteristic has a UUID even if the user isn't filtering by UUID. We
should include it unconditionally, right?

[ortuno, 2016/04/25 15:29:45]

If the user is filtering by UUID then the renderer knows the UUID already so
there is no need to send it over. This optimization existed in Dispatcher Host
before and I tried to keep it here. It definitely makes the code harder to read
(here and on the renderer) and I don't expect users to be calling
GetCharacteristic a lot so I wouldn't be opposed to removing it.

[Jeffrey Yasskin, 2016/04/25 17:26:10]

Yeah, we might DCHECK that they're the same on the renderer side, but I like
having simpler code. Sending an extra 36 bytes isn't likely to be significant,
and we could optimize UUIDs in general if it is.

[ortuno, 2016/04/25 20:37:41]

Done.

+    characteristic_ptr->properties =
+        static_cast<uint32_t>(characteristic->GetProperties());
+    response_characteristics.push_back(std::move(characteristic_ptr));
+
+    if (single_characteristic) {
+      break;
+    }
+  }
+
+  if (!response_characteristics.empty()) {
+    RecordGetCharacteristicsOutcome(single_characteristic,
+                                    UMAGetCharacteristicOutcome::SUCCESS);
+    callback.Run(blink::mojom::WebBluetoothError::SUCCESS,
+                 std::move(response_characteristics));
+    return;
+  }
+  RecordGetCharacteristicsOutcome(
+      single_characteristic,
+      characteristics_uuid.is_null()
+          ? UMAGetCharacteristicOutcome::NO_CHARACTERISTICS
+          : UMAGetCharacteristicOutcome::NOT_FOUND);
+  callback.Run(characteristics_uuid.is_null()
+                   ? blink::mojom::WebBluetoothError::NO_CHARACTERISTICS_FOUND
+                   : blink::mojom::WebBluetoothError::CHARACTERISTIC_NOT_FOUND,
+               nullptr /* characteristics */);
+  return;
+}
+
 void WebBluetoothServiceImpl::RemoteCharacteristicReadValue(
     const mojo::String& characteristic_instance_id,
     const RemoteCharacteristicReadValueCallback& callback) {
@@ -151,8 +265,7 @@ void WebBluetoothServiceImpl::RemoteCharacteristicReadValue(
       UMAWebBluetoothFunction::CHARACTERISTIC_READ_VALUE);
 
   const CacheQueryResult query_result =
-      GetBluetoothDispatcherHost()->QueryCacheForCharacteristic(
-          GetOrigin(), characteristic_instance_id);
+      QueryCacheForCharacteristic(characteristic_instance_id);
 
   if (query_result.outcome == CacheQueryOutcome::BAD_RENDERER) {
     return;
@@ -196,11 +309,9 @@ void WebBluetoothServiceImpl::RemoteCharacteristicWriteValue(
   }
 
   const CacheQueryResult query_result =
-      GetBluetoothDispatcherHost()->QueryCacheForCharacteristic(
-          GetOrigin(), characteristic_instance_id);
+      QueryCacheForCharacteristic(characteristic_instance_id);
 
   if (query_result.outcome == CacheQueryOutcome::BAD_RENDERER) {
-    binding_.Close();
     return;
   }
 
@@ -243,11 +354,9 @@ void WebBluetoothServiceImpl::RemoteCharacteristicStartNotifications(
   }
 
   const CacheQueryResult query_result =
-      GetBluetoothDispatcherHost()->QueryCacheForCharacteristic(
-          GetOrigin(), characteristic_instance_id);
+      QueryCacheForCharacteristic(characteristic_instance_id);
 
   if (query_result.outcome == CacheQueryOutcome::BAD_RENDERER) {
-    binding_.Close();
     return;
   }
 
@@ -281,11 +390,9 @@ void WebBluetoothServiceImpl::RemoteCharacteristicStopNotifications(
       UMAWebBluetoothFunction::CHARACTERISTIC_STOP_NOTIFICATIONS);
 
   const CacheQueryResult query_result =
-      GetBluetoothDispatcherHost()->QueryCacheForCharacteristic(
-          GetOrigin(), characteristic_instance_id);
+      QueryCacheForCharacteristic(characteristic_instance_id);
 
   if (query_result.outcome == CacheQueryOutcome::BAD_RENDERER) {
-    binding_.Close();
     return;
   }
 
@@ -364,6 +471,33 @@ void WebBluetoothServiceImpl::OnStopNotifySessionComplete(
   callback.Run();
 }
 
+CacheQueryResult WebBluetoothServiceImpl::QueryCacheForCharacteristic(
+    const std::string& characteristic_instance_id) {
+  auto characteristic_iter =
+      characteristic_to_service_.find(characteristic_instance_id);
+
+  // Kill the render, see "ID Not in Map Note" above.
+  if (characteristic_iter == characteristic_to_service_.end()) {
+    CrashRendererAndClosePipe(bad_message::BDH_INVALID_CHARACTERISTIC_ID);
+    return CacheQueryResult(CacheQueryOutcome::BAD_RENDERER);
+  }
+
+  CacheQueryResult result = GetBluetoothDispatcherHost()->QueryCacheForService(

[Jeffrey Yasskin, 2016/04/22 01:16:38]

Do you need to close the binding_ if this returns BAD_RENDERER?

[ortuno, 2016/04/25 15:29:45]

Done.

+      GetOrigin(), characteristic_iter->second);
+  if (result.outcome != CacheQueryOutcome::SUCCESS) {
+    return result;
+  }
+
+  result.characteristic =
+      result.service->GetCharacteristic(characteristic_instance_id);
+
+  if (result.characteristic == nullptr) {
+    result.outcome = CacheQueryOutcome::NO_CHARACTERISTIC;
+  }
+
+  return result;
+}
+
 RenderProcessHost* WebBluetoothServiceImpl::GetRenderProcessHost() {
   return render_frame_host_->GetProcess();
 }
@@ -386,6 +520,7 @@ url::Origin WebBluetoothServiceImpl::GetOrigin() {
 
 void WebBluetoothServiceImpl::ClearState() {
   characteristic_id_to_notify_session_.clear();
+  characteristic_to_service_.clear();
 }
 
 }  // namespace content