Make BluetoothRemoteGATTCharacteristic an ActiveScriptWrappable

This patch makes BluetoothRemoteGATTCharacteristic implement ActiveScriptWrappable so that when event listeners are registered the V8 wrappers will not be destroyed. This prevents event listeners from being silently destroyed if script does not maintain a reference to the characteristic object. Bug: 846977 Change-Id: I235e55a1fef41f81b45b1b281036ce24bc5d0279 Reviewed-on: https://chromium-review.googlesource.com/1079629Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org> Commit-Queue: Reilly Grant <reillyg@chromium.org> Cr-Commit-Position: refs/heads/master@{#563113}

Make BluetoothRemoteGATTCharacteristic an ActiveScriptWrappable
This patch makes BluetoothRemoteGATTCharacteristic implement ActiveScriptWrappable so that when event listeners are registered the V8 wrappers will not be destroyed. This prevents event listeners from being silently destroyed if script does not maintain a reference to the characteristic object. Bug: 846977 Change-Id: I235e55a1fef41f81b45b1b281036ce24bc5d0279 Reviewed-on: https://chromium-review.googlesource.com/1079629Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org> Commit-Queue: Reilly Grant <reillyg@chromium.org> Cr-Commit-Position: refs/heads/master@{#563113}
22ea6534 · Reilly Grant · Commit Bot · 38e0b7d8 · 22ea6534 · 22ea6534
Commit 22ea6534 authored May 31, 2018 by Reilly Grant Committed by Commit Bot May 31, 2018
4 changed files
--- a/third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/gc-with-event-listener.html
+++ b/third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/gc-with-event-listener.html
+<!DOCTYPE html>
+<script src="../../../resources/testharness.js"></script>
+<script src="../../../resources/testharnessreport.js"></script>
+<script src="../../../resources/testdriver.js"></script>
+<script src="../../../resources/testdriver-vendor.js"></script>
+<script src="../../../external/wpt/bluetooth/resources/bluetooth-helpers.js"></script>
+<script>
+'use strict';
+bluetooth_test(async () => {
+  let expectingEvent = false;
+  let eventHandler;
+  let eventPromise = new Promise((resolve, reject) => {
+    eventHandler = () => {
+      if (expectingEvent) {
+        resolve();
+      } else {
+        reject('Event fired before garbage collection.');
+      }
+    };
+  });
+
+  await (async () => {
+    await setBluetoothFakeAdapter('HeartRateAdapter');
+    let device = await requestDeviceWithTrustedClick({
+        filters: [{services: ['heart_rate']}]});
+    let gattServer = await device.gatt.connect();
+    let service = await gattServer.getPrimaryService('heart_rate');
+    let characteristic =
+        await service.getCharacteristic('heart_rate_measurement');
+    await characteristic.startNotifications();
+
+    characteristic.addEventListener(
+        'characteristicvaluechanged', eventHandler, { once: true });
+
+    // The characteristic must hold the last reference to the event handler.
+    eventHandler = undefined;
+  })();
+
+  // At this point all the variables above should be out of scope and so this
+  // will free them.
+  GCController.collect();
+
+  // The event listener should still be active.
+  expectingEvent = true;
+  await eventPromise;
+}, 'Event listeners remain registered after garbage collection.');
+</script>
--- a/third_party/blink/renderer/modules/bluetooth/bluetooth_remote_gatt_characteristic.cc
+++ b/third_party/blink/renderer/modules/bluetooth/bluetooth_remote_gatt_characteristic.cc
@@ -76,6 +76,13 @@ ExecutionContext* BluetoothRemoteGATTCharacteristic::GetExecutionContext()
  return ContextLifecycleObserver::GetExecutionContext();
 }

+bool BluetoothRemoteGATTCharacteristic::HasPendingActivity() const {
+  // This object should be considered active as long as there are registered
+  // event listeners. Even if script drops all references this can still be
+  // found again through the BluetoothRemoteGATTServer object.
+  return GetExecutionContext() && HasEventListeners();
+}
+
 void BluetoothRemoteGATTCharacteristic::AddedEventListener(
    const AtomicString& event_type,
    RegisteredEventListener& registered_listener) {

--- a/third_party/blink/renderer/modules/bluetooth/bluetooth_remote_gatt_characteristic.h
+++ b/third_party/blink/renderer/modules/bluetooth/bluetooth_remote_gatt_characteristic.h
@@ -7,6 +7,7 @@

 #include "mojo/public/cpp/bindings/associated_binding_set.h"
 #include "third_party/blink/public/platform/modules/bluetooth/web_bluetooth.mojom-blink.h"
+#include "third_party/blink/renderer/bindings/core/v8/active_script_wrappable.h"
 #include "third_party/blink/renderer/core/dom/context_lifecycle_observer.h"
 #include "third_party/blink/renderer/core/typed_arrays/dom_array_piece.h"
 #include "third_party/blink/renderer/core/typed_arrays/dom_data_view.h"
@@ -34,6 +35,7 @@ class ScriptState;
 // CallbackPromiseAdapter class comments.
 class BluetoothRemoteGATTCharacteristic final
    : public EventTargetWithInlineData,
+      public ActiveScriptWrappable<BluetoothRemoteGATTCharacteristic>,
      public ContextLifecycleObserver,
      public mojom::blink::WebBluetoothCharacteristicClient {
  USING_PRE_FINALIZER(BluetoothRemoteGATTCharacteristic, Dispose);
@@ -71,6 +73,9 @@ class BluetoothRemoteGATTCharacteristic final
  const AtomicString& InterfaceName() const override;
  ExecutionContext* GetExecutionContext() const override;

+  // ActiveScriptWrappable methods:
+  bool HasPendingActivity() const override;
+
  // Interface required by garbage collection.
  void Trace(blink::Visitor*) override;


--- a/third_party/blink/renderer/modules/bluetooth/bluetooth_remote_gatt_characteristic.idl
+++ b/third_party/blink/renderer/modules/bluetooth/bluetooth_remote_gatt_characteristic.idl
@@ -7,6 +7,7 @@
 // Implement BluetoothRemoteGATTCharacteristic interface: https://crbug.com/483344

 [
+    ActiveScriptWrappable,
    RuntimeEnabled=WebBluetooth
 ] interface BluetoothRemoteGATTCharacteristic : EventTarget {//: CharacteristicEventHandlers {
    [SameObject] readonly attribute BluetoothRemoteGATTService service;