Remove RemoteServerDisconnect() from web_bluetooth.mojom

third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTServer.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "modules/bluetooth/BluetoothRemoteGATTServer.h"
 
 #include "bindings/core/v8/CallbackPromiseAdapter.h"
 #include "bindings/core/v8/ScriptPromise.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "core/dom/DOMException.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/events/Event.h"
 #include "modules/bluetooth/Bluetooth.h"
 #include "modules/bluetooth/BluetoothDevice.h"
 #include "modules/bluetooth/BluetoothError.h"
 #include "modules/bluetooth/BluetoothRemoteGATTService.h"
 #include "modules/bluetooth/BluetoothUUID.h"
-#include "mojo/public/cpp/bindings/associated_interface_ptr.h"
 #include <utility>
 
 namespace blink {
 
 BluetoothRemoteGATTServer::BluetoothRemoteGATTServer(ExecutionContext* context,
                                                      BluetoothDevice* device)
-    : ContextLifecycleObserver(context), m_device(device), m_connected(false) {}
+    : ContextLifecycleObserver(context),
+      m_clientBinding(this),
+      m_device(device),
+      m_connected(false) {}
 
 BluetoothRemoteGATTServer* BluetoothRemoteGATTServer::Create(
     ExecutionContext* context,
     BluetoothDevice* device) {
   return new BluetoothRemoteGATTServer(context, device);
 }
 
 void BluetoothRemoteGATTServer::contextDestroyed(ExecutionContext*) {
   Dispose();
 }
 
 void BluetoothRemoteGATTServer::GATTServerDisconnected() {
-  DispatchDisconnected();
+  // This function can only be called if |m_clientBinding| is bound and
+  // |m_clientBinding| can only be bound if |m_connected| is true.

[ortuno, 2017/03/30 04:20:48]

I might be missing something here but m_clientBinding is bound as soon as we
call connect during which m_connected stays false.

[juncai, 2017/03/30 18:44:57]

Done.

+  // Therefore we skip Step 2 of Responding to Disconnection.
+  // https://webbluetoothcg.github.io/web-bluetooth/#disconnection-events
+  DCHECK(m_clientBinding.is_bound());
+  CleanupDisconnectedDevice(true /* dispatchEvent */);
 }
 
 void BluetoothRemoteGATTServer::AddToActiveAlgorithms(
     ScriptPromiseResolver* resolver) {
   auto result = m_activeAlgorithms.insert(resolver);
   CHECK(result.isNewEntry);
 }
 
 bool BluetoothRemoteGATTServer::RemoveFromActiveAlgorithms(
     ScriptPromiseResolver* resolver) {
   if (!m_activeAlgorithms.contains(resolver)) {
     return false;
   }
   m_activeAlgorithms.erase(resolver);
   return true;
 }
 
-void BluetoothRemoteGATTServer::DisconnectIfConnected() {
-  if (m_connected) {
-    SetConnected(false);
-    ClearActiveAlgorithms();
-    mojom::blink::WebBluetoothService* service =
-        m_device->bluetooth()->Service();
-    service->RemoteServerDisconnect(m_device->id());
+void BluetoothRemoteGATTServer::CleanupDisconnectedDevice(bool dispatchEvent) {
+  // This implements the specification "clean up the disconnected device"
+  // algorithm:
+  // https://webbluetoothcg.github.io/web-bluetooth/#clean-up-the-disconnected-device
+  DCHECK(m_connected);
+  SetConnected(false);
+  ClearActiveAlgorithms();
+  m_device->ClearAttributeInstanceMap();
+  // TODO(juncai): Below this line there are steps in the above specification
+  // which we are not explicitly handling here (clearing representedService,
+  // notification & indications). Need to add comment here explaining why no

[ortuno, 2017/03/30 04:20:48]

Clearing the represented services, characteristics and descriptors is done by
ClearAttributeInstanceMap(). The notification context is implementing in the
browser so it cleans up up itself.

[juncai, 2017/03/30 18:44:57]

Done.

+  // explicit actions are needed.
+  if (dispatchEvent) {
+    m_device->dispatchEvent(
+        Event::createBubble(EventTypeNames::gattserverdisconnected));
   }
 }
 
-void BluetoothRemoteGATTServer::CleanupDisconnectedDeviceAndFireEvent() {
-  DCHECK(m_connected);
-  SetConnected(false);
-  ClearActiveAlgorithms();
-  m_device->ClearAttributeInstanceMapAndFireEvent();
-}
-
-void BluetoothRemoteGATTServer::DispatchDisconnected() {
+void BluetoothRemoteGATTServer::HandleClientConnectionError() {
   if (!m_connected) {
     return;
   }
-  CleanupDisconnectedDeviceAndFireEvent();
+  CleanupDisconnectedDevice(true /* dispatchEvent */);
 }
 
 void BluetoothRemoteGATTServer::Dispose() {
-  DisconnectIfConnected();
-  // The pipe to this object must be closed when is marked unreachable to
-  // prevent messages from being dispatched before lazy sweeping.
-  m_clientBindings.CloseAllBindings();
+  if (!m_connected) {
+    return;
+  }
+  // The object is being garbage collected or the context is being destroyed,
+  // so no need to dispatch a gattserverdisconnected event.
+  CleanupDisconnectedDevice(false /* dispatchEvent */);
+  DCHECK(m_clientBinding.is_bound());
+  m_clientBinding.Close();
 }
 
 DEFINE_TRACE(BluetoothRemoteGATTServer) {
   visitor->trace(m_activeAlgorithms);
   visitor->trace(m_device);
   ContextLifecycleObserver::trace(visitor);
 }
 
 void BluetoothRemoteGATTServer::ConnectCallback(
     ScriptPromiseResolver* resolver,
     mojom::blink::WebBluetoothResult result) {
   if (!resolver->getExecutionContext() ||
       resolver->getExecutionContext()->isContextDestroyed())
     return;
 
   if (result == mojom::blink::WebBluetoothResult::SUCCESS) {
     SetConnected(true);
     resolver->resolve(this);
   } else {
     resolver->reject(BluetoothError::CreateDOMException(result));
   }
 }
 
 ScriptPromise BluetoothRemoteGATTServer::connect(ScriptState* scriptState) {
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
   ScriptPromise promise = resolver->promise();
 
-  mojom::blink::WebBluetoothService* service = m_device->bluetooth()->Service();
-  mojom::blink::WebBluetoothServerClientAssociatedPtrInfo ptrInfo;
-  auto request = mojo::MakeRequest(&ptrInfo);
-  m_clientBindings.AddBinding(this, std::move(request));
+  if (m_connected) {

[ortuno, 2017/03/30 04:20:48]

hmm I think this is breaking spec'ed behavior e.g.

connect().then(() => {
  connect();
  disconnect();
});

Based on the spec'ed algorithm the second call to connect should reject: Step
5.2 says to queue a task, at which point disconnect runs, the algorithm for
disconnect immediately removes the promise from the active algorithms map so
when step 5.2.1 runs the promise should reject.

https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver...

[juncai, 2017/03/30 18:44:57]

hmm... I am not sure I understand it. When the second connect() is called, the
connect() code is not aware of the following code has a disconnect() call, so it
can't act upon it. Similar to the following:
connect().then(() => {
  connect();
  ...
  ...
});
And since this is the second call connect(), it makes sense to just resolve the
promise if |m_connected| is true.

[ortuno, 2017/03/31 06:07:28]

According to step 5.2 of the connect() algorithm in the spec we *have* to post a
task before resolving the promise. If we don't then the bug I described before
will happen.

This is what should happen in the code above based on the spec'ed functions:

// When the second connect is called:
1. Steps 1, 2, 3, 4, and 5.1 of the connect()[1] algorithm run
2. Step 5.2 queues a task to execute steps 5.2.*
// disconnect is called
3. Step 1 of the disconnect()[2] algorithm immediately clears the active
algorithms map
4. Steps 2, 3, 4, 5 of the disconnect() algorithm run
5. The task posted by 5.2 of the connect() algorithm starts running
6. Step 5.2.1 checks the active algorithms map and since the connect() promise
is no longer there (it was removed by step 1 of the disconnect() algorithm),
then the promise rejects.

But as is this code will resolve the promise immediately and the spec'ed
behavior will not happen. Before the "queue a task" part of the algorithm was
being done by calling RemoteGATTServerConnect.

[1]
https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver...
[2]
https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver...

+    resolver->resolve(this);
+  } else {
+    mojom::blink::WebBluetoothService* service =
+        m_device->bluetooth()->Service();
+    mojom::blink::WebBluetoothServerClientAssociatedPtrInfo ptrInfo;
+    m_clientBinding.Bind(&ptrInfo);
+    m_clientBinding.set_connection_error_handler(convertToBaseCallback(

[ortuno, 2017/03/30 04:20:48]

Since you are binding this here and in the browser wouldn't you get a
notification if the connection fails since you are closing the binding?

[juncai, 2017/03/30 18:44:57]

Yes, only if |m_connected| is true. And in that situation, a notification is
necessary, right?

[ortuno, 2017/03/31 06:07:28]

I think the fact that the client is receiving connection notifications before an
actual connection is made doesn't make much sense, is very subtle and could end
up causing bugs. We should refactor the code so that we only get notifications
after the connection is made.

+        WTF::bind(&BluetoothRemoteGATTServer::HandleClientConnectionError,
+                  wrapWeakPersistent(this))));
 
-  service->RemoteServerConnect(
-      m_device->id(), std::move(ptrInfo),
-      convertToBaseCallback(
-          WTF::bind(&BluetoothRemoteGATTServer::ConnectCallback,
-                    wrapPersistent(this), wrapPersistent(resolver))));
+    service->RemoteServerConnect(
+        m_device->id(), std::move(ptrInfo),
+        convertToBaseCallback(
+            WTF::bind(&BluetoothRemoteGATTServer::ConnectCallback,
+                      wrapPersistent(this), wrapPersistent(resolver))));
+  }
 
   return promise;
 }
 
 void BluetoothRemoteGATTServer::disconnect(ScriptState* scriptState) {
-  if (!m_connected)
+  ClearActiveAlgorithms();

[dcheng, 2017/03/28 02:25:01]

Just curious: how come we need to ClearActiveAlgorithms() here before the early
return? Might be worth a comment, since CleanupDisconnectedDevice() also does
that.

[ortuno, 2017/03/30 04:20:48]

I asked the same in the spec a while ago :) The answer is in the spec:
"1. Clear this.[[activeAlgorithms]] to abort any active connect() calls."

https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothremotegattserver...

Basically connect() calls will add a promise to this active algorithms map. When
connect's callback is run we check that the promise is still in this map and if
not we reject the promise.

During this, m_connected stays false so if we want to actually cancel the
connection we need to do so before the m_connected check.

[juncai, 2017/03/30 18:44:57]

comment added.

Done.

[juncai, 2017/03/30 18:44:57]

Thanks Gio for the explanation!

+  if (!m_connected) {
     return;
-  CleanupDisconnectedDeviceAndFireEvent();
-  m_clientBindings.CloseAllBindings();
-  mojom::blink::WebBluetoothService* service = m_device->bluetooth()->Service();
-  service->RemoteServerDisconnect(m_device->id());
+  }
+  CleanupDisconnectedDevice(true /* dispatchEvent */);
+  DCHECK(m_clientBinding.is_bound());
+  // Implicitly signals disconnect via the connection_error_handler.
+  m_clientBinding.Close();
 }
 
 // Callback that allows us to resolve the promise with a single service or
 // with a vector owning the services.
 void BluetoothRemoteGATTServer::GetPrimaryServicesCallback(
     const String& requestedServiceUUID,
     mojom::blink::WebBluetoothGATTQueryQuantity quantity,
     ScriptPromiseResolver* resolver,
     mojom::blink::WebBluetoothResult result,
     Optional<Vector<mojom::blink::WebBluetoothRemoteGATTServicePtr>> services) {
@@ skipping 90 matching lines @@
   service->RemoteServerGetPrimaryServices(
       m_device->id(), quantity, servicesUUID,
       convertToBaseCallback(
           WTF::bind(&BluetoothRemoteGATTServer::GetPrimaryServicesCallback,
                     wrapPersistent(this), servicesUUID, quantity,
                     wrapPersistent(resolver))));
   return promise;
 }
 
 }  // namespace blink