From 0e1170c52e9729cf4f7746cbf4b8980a28dcefc3 Mon Sep 17 00:00:00 2001 From: Anay Wadhera Date: Mon, 15 May 2023 11:38:30 -0700 Subject: [PATCH 01/11] remove obsolete todo PiperOrigin-RevId: 532176213 --- presence/implementation/scan_manager.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/presence/implementation/scan_manager.cc b/presence/implementation/scan_manager.cc index 3d209e1c..6f35e49b 100644 --- a/presence/implementation/scan_manager.cc +++ b/presence/implementation/scan_manager.cc @@ -58,7 +58,6 @@ ScanSessionId ScanManager::StartScan(ScanRequest scan_request, absl::Status ble_status) mutable { start_scan_client(ble_status); }, - // TODO(b/256686710): Track known devices .advertisement_found_cb = [this, id](BlePeripheral& peripheral, BleAdvertisementData data) { From 6d3704f50a6799338ba96fc6cf117275c913ab92 Mon Sep 17 00:00:00 2001 From: Qin Wang Date: Mon, 15 May 2023 12:18:09 -0700 Subject: [PATCH 02/11] Add battery notification and not discoverable advertisement PiperOrigin-RevId: 532187475 --- fastpair/common/BUILD | 21 +++ fastpair/common/battery_notification.cc | 86 ++++++++++ fastpair/common/battery_notification.h | 66 ++++++++ fastpair/common/battery_notification_test.cc | 152 ++++++++++++++++++ fastpair/common/constant.h | 8 +- .../common/non_discoverable_advertisement.h | 59 +++++++ 6 files changed, 391 insertions(+), 1 deletion(-) create mode 100644 fastpair/common/battery_notification.cc create mode 100644 fastpair/common/battery_notification.h create mode 100644 fastpair/common/battery_notification_test.cc create mode 100644 fastpair/common/non_discoverable_advertisement.h diff --git a/fastpair/common/BUILD b/fastpair/common/BUILD index b4235e89..ac60ad24 100644 --- a/fastpair/common/BUILD +++ b/fastpair/common/BUILD @@ -3,6 +3,7 @@ licenses(["notice"]) cc_library( name = "common", srcs = [ + "battery_notification.cc", "fast_pair_device.cc", "fast_pair_http_result.cc", "pair_failure.cc", @@ -10,9 +11,11 @@ cc_library( ], hdrs = [ "account_key.h", + "battery_notification.h", "constant.h", "fast_pair_device.h", "fast_pair_http_result.h", + "non_discoverable_advertisement.h", "pair_failure.h", "protocol.h", ], @@ -21,6 +24,7 @@ cc_library( ], deps = [ "//internal/crypto", + "//internal/platform:logging", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/status", "@com_google_absl//absl/strings", @@ -36,6 +40,7 @@ cc_test( shard_count = 16, deps = [ ":common", + "//internal/platform/implementation/g3", # build_cleaner: keep "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_absl//absl/strings", "@com_google_googletest//:gtest_main", @@ -51,6 +56,22 @@ cc_test( shard_count = 16, deps = [ ":common", + "//internal/platform/implementation/g3", # build_cleaner: keep + "@com_github_protobuf_matchers//protobuf-matchers", + "@com_google_googletest//:gtest_main", + ], +) + +cc_test( + name = "battery_notification_test", + size = "small", + srcs = [ + "battery_notification_test.cc", + ], + shard_count = 16, + deps = [ + ":common", + "//internal/platform/implementation/g3", # build_cleaner: keep "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_googletest//:gtest_main", ], diff --git a/fastpair/common/battery_notification.cc b/fastpair/common/battery_notification.cc new file mode 100644 index 00000000..d46d6b86 --- /dev/null +++ b/fastpair/common/battery_notification.cc @@ -0,0 +1,86 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "fastpair/common/battery_notification.h" + +#include +#include +#include + +#include "fastpair/common/constant.h" +#include "internal/platform/logging.h" + +namespace nearby { +namespace fastpair { +BatteryInfo::BatteryInfo(bool is_charging) + : is_charging(is_charging), percentage(std::nullopt) {} + +BatteryInfo::BatteryInfo(bool is_charging, int8_t percentage) + : is_charging(is_charging), percentage(percentage) {} + +// static +BatteryInfo BatteryInfo::FromByte(uint8_t byte) { + // Battery value is in the form 0bSVVVVVVV. + // S = charging (0b1) or not (0b0). + // V = value, Ranges from 0-100, or 0bS1111111 if unknown. + bool is_charging = byte & kBatteryChargingMask; + uint8_t percentage = byte & kBatteryPercentageMask; + int8_t percentage_signed = static_cast(percentage); + if (percentage_signed < 0 || percentage_signed > 100) { + NEARBY_LOGS(INFO) << __func__ << "Invalid battery percentage."; + return BatteryInfo(is_charging); + } + return BatteryInfo(is_charging, percentage_signed); +} + +uint8_t BatteryInfo::ToByte() const { + // Battery value is in the form 0bSVVVVVVV. + // S = charging (0b1) or not (0b0). + // V = value, Ranges from 0-100, or 1111111 if unknown. + if (!percentage) { + return is_charging ? kBatteryIsChargingByte : kBatteryNotChargingByte; + } else { + return percentage.value() | (is_charging ? kBatteryChargingMask : 0); + } +} + +BatteryNotification::BatteryNotification( + Type type, const std::vector& battery_infos) + : type(type), battery_infos(battery_infos) {} + +// static +std::optional BatteryNotification::FromBytes( + const std::vector& bytes, Type type) { + if (bytes.size() == 1) { + // Single component device. + NEARBY_LOGS(INFO) << __func__ << " : Single component device."; + std::vector battery_infos = {BatteryInfo::FromByte(bytes[0])}; + return std::make_optional(type, battery_infos); + } else if (bytes.size() == 3) { + // True wireless headset expecting 3 bytes - Left bud, Right bud and case. + NEARBY_LOGS(INFO) << __func__ << " : True wireless headset."; + std::vector battery_infos = { + /* left bud info */ BatteryInfo::FromByte(bytes[0]), + /* right bud info */ BatteryInfo::FromByte(bytes[1]), + /* case info */ BatteryInfo::FromByte(bytes[2])}; + return std::make_optional(type, battery_infos); + } + NEARBY_LOGS(WARNING) << __func__ + << " : Unexpected battery notification length :" + << bytes.size(); + return std::nullopt; +} + +} // namespace fastpair +} // namespace nearby diff --git a/fastpair/common/battery_notification.h b/fastpair/common/battery_notification.h new file mode 100644 index 00000000..3c82060f --- /dev/null +++ b/fastpair/common/battery_notification.h @@ -0,0 +1,66 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef THIRD_PARTY_NEARBY_FASTPAIR_COMMON_BATTERY_NOTIFICATION_H_ +#define THIRD_PARTY_NEARBY_FASTPAIR_COMMON_BATTERY_NOTIFICATION_H_ + +#include +#include +#include + +namespace nearby { +namespace fastpair { + +// Fast Pair battery information from notification. See +// https://developers.google.com/nearby/fast-pair/spec#BatteryNotification +struct BatteryInfo { + BatteryInfo() = default; + explicit BatteryInfo(bool is_charging); + BatteryInfo(bool is_charging, int8_t percentage); + ~BatteryInfo() = default; + + static BatteryInfo FromByte(uint8_t byte); + + uint8_t ToByte() const; + + bool is_charging = false; + std::optional percentage; +}; + +// Fast Pair battery notification. See +// https://developers.google.com/nearby/fast-pair/spec#BatteryNotification +struct BatteryNotification { + // Represents if the provider wants to show an indication + // of the battery values + enum class Type { + kNone = 0, + kShowUi = 3, /* Show UI indication: 0b0011 */ + kHideUi = 4, /* Hide UI indication: 0b0100 */ + }; + + BatteryNotification() = default; + BatteryNotification(Type type, const std::vector& battery_infos); + ~BatteryNotification() = default; + + static std::optional FromBytes( + const std::vector& bytes, Type type); + + Type type = Type::kNone; + std::vector battery_infos; +}; + +} // namespace fastpair +} // namespace nearby + +#endif // THIRD_PARTY_NEARBY_FASTPAIR_COMMON_BATTERY_NOTIFICATION_H_ diff --git a/fastpair/common/battery_notification_test.cc b/fastpair/common/battery_notification_test.cc new file mode 100644 index 00000000..48c86b60 --- /dev/null +++ b/fastpair/common/battery_notification_test.cc @@ -0,0 +1,152 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "fastpair/common/battery_notification.h" + +#include +#include + +#include "gtest/gtest.h" + +namespace nearby { +namespace fastpair { +namespace { +// Test data comes from: +// https://developers.google.com/nearby/fast-pair/specifications/appendix/testcases#test_cases + +TEST(BatteryNotificationTest, TestBatteryInfo) { + // Tests default constructor + BatteryInfo battery_info_1; + EXPECT_FALSE(battery_info_1.is_charging); + + // Tests Constructor with is_charging + BatteryInfo battery_info_2(true); + EXPECT_TRUE(battery_info_2.is_charging); + // Tests ToByte and FromByte + BatteryInfo battery_info_3 = BatteryInfo::FromByte(battery_info_2.ToByte()); + EXPECT_TRUE(battery_info_3.is_charging); + EXPECT_FALSE(battery_info_3.percentage.has_value()); + + // Tests Constructor with is_charging and percentage value + BatteryInfo battery_info_4(true, 80); + EXPECT_TRUE(battery_info_4.is_charging); + EXPECT_EQ(battery_info_4.percentage.value(), 80); + // Tests ToByte and FromByte + BatteryInfo battery_info_5 = BatteryInfo::FromByte(battery_info_4.ToByte()); + EXPECT_TRUE(battery_info_5.is_charging); + EXPECT_EQ(battery_info_5.percentage.value(), 80); + + // Tests Constructor with is_charging and wrong percentage value + BatteryInfo battery_info_6(true, 110); + // Tests ToByte and FromByte + BatteryInfo battery_info_7 = BatteryInfo::FromByte(battery_info_6.ToByte()); + EXPECT_TRUE(battery_info_7.is_charging); + EXPECT_FALSE(battery_info_7.percentage.has_value()); + + // Tests FromByte with battery percentage = 0 + BatteryInfo battery_info_8 = BatteryInfo::FromByte(0); + EXPECT_FALSE(battery_info_8.is_charging); + EXPECT_EQ(battery_info_8.percentage.value(), 0); + + BatteryInfo battery_info_10 = BatteryInfo::FromByte(0x80); + EXPECT_TRUE(battery_info_10.is_charging); + EXPECT_EQ(battery_info_10.percentage.value(), 0); + + // Tests FromByte with battery percentage = 100 + BatteryInfo battery_info_9 = BatteryInfo::FromByte(100); + EXPECT_FALSE(battery_info_9.is_charging); + EXPECT_EQ(battery_info_9.percentage.value(), 100); + + BatteryInfo battery_info_11 = BatteryInfo::FromByte(0x80 | 100); + EXPECT_TRUE(battery_info_11.is_charging); + EXPECT_EQ(battery_info_11.percentage.value(), 100); + + // Test battery with invalid battery percentage + BatteryInfo battery_info_12 = BatteryInfo::FromByte(0x7F); + EXPECT_FALSE(battery_info_12.is_charging); + EXPECT_FALSE(battery_info_12.percentage.has_value()); + + BatteryInfo battery_info_13 = BatteryInfo::FromByte(0xFF); + EXPECT_TRUE(battery_info_13.is_charging); + EXPECT_FALSE(battery_info_13.percentage.has_value()); +} + +TEST(BatteryNotificationTest, TestBatteryNotificationDefaultConstructor) { + // Tests default constructor + BatteryNotification battery_notification; + EXPECT_EQ(battery_notification.type, BatteryNotification::Type::kNone); + EXPECT_EQ(battery_notification.battery_infos.size(), 0); +} + +TEST(BatteryNotificationTest, TestBatteryNotificationForSingleComponentDevice) { + std::vector battery_infos = {BatteryInfo(false, 70)}; + BatteryNotification battery_notification(BatteryNotification::Type::kShowUi, + battery_infos); + EXPECT_EQ(battery_notification.type, BatteryNotification::Type::kShowUi); + EXPECT_FALSE(battery_notification.battery_infos.at(0).is_charging); + EXPECT_EQ(battery_notification.battery_infos.at(0).percentage.value(), 70); +} + +TEST(BatteryNotificationTest, TestBatteryNotificationForTrueWirelessHeadset) { + std::vector battery_infos = { + BatteryInfo(false, 70), BatteryInfo(false, 80), BatteryInfo(true, 90)}; + BatteryNotification battery_notification(BatteryNotification::Type::kShowUi, + battery_infos); + EXPECT_EQ(battery_notification.type, BatteryNotification::Type::kShowUi); + // Left bud + EXPECT_FALSE(battery_notification.battery_infos.at(0).is_charging); + EXPECT_EQ(battery_notification.battery_infos.at(0).percentage.value(), 70); + // Right bud + EXPECT_FALSE(battery_notification.battery_infos.at(1).is_charging); + EXPECT_EQ(battery_notification.battery_infos.at(1).percentage.value(), 80); + // Case + EXPECT_TRUE(battery_notification.battery_infos.at(2).is_charging); + EXPECT_EQ(battery_notification.battery_infos.at(2).percentage.value(), 90); +} + +TEST(BatteryNotificationTest, + TestBatteryNotificationFromBytesForSingleComponentDevice) { + const std::vector batteryData{0b01000000}; + BatteryNotification battery_notification = + BatteryNotification::FromBytes(batteryData, + BatteryNotification::Type::kShowUi) + .value(); + EXPECT_EQ(battery_notification.type, BatteryNotification::Type::kShowUi); + EXPECT_EQ(battery_notification.battery_infos.at(0).percentage.value(), 64); +} + +TEST(BatteryNotificationTest, + TestBatteryNotificationFromBytesForTrueWirelessHeadset) { + const std::vector batteryData{0b01000000, 0b01000000, 0b01000000}; + BatteryNotification battery_notification = + BatteryNotification::FromBytes(batteryData, + BatteryNotification::Type::kShowUi) + .value(); + EXPECT_EQ(battery_notification.type, BatteryNotification::Type::kShowUi); + EXPECT_EQ(battery_notification.battery_infos.at(0).percentage.value(), 64); + EXPECT_EQ(battery_notification.battery_infos.at(1).percentage.value(), 64); + EXPECT_EQ(battery_notification.battery_infos.at(2).percentage.value(), 64); +} + +TEST(BatteryNotificationTest, TestBatteryNotificationFromWrongBytes) { + const std::vector batteryDataWithWrongLenth{0b01000000, 0b01000000}; + EXPECT_FALSE( + BatteryNotification::FromBytes(batteryDataWithWrongLenth, + BatteryNotification::Type::kShowUi) + .has_value()); +} +} // namespace + +} // namespace fastpair +} // namespace nearby diff --git a/fastpair/common/constant.h b/fastpair/common/constant.h index d4614336..200eb0ac 100644 --- a/fastpair/common/constant.h +++ b/fastpair/common/constant.h @@ -24,7 +24,6 @@ namespace fastpair { constexpr char kServiceId[] = "Fast Pair"; constexpr char kRfcommUuid[] = "df21fe2c-2515-4fdb-8886-f12c4d67927c"; - constexpr int kAccountKeySize = 16; // Key pair @@ -60,6 +59,13 @@ constexpr uint8_t kAccountKeyStartByte = 0x04; constexpr uint8_t kKeyBasedPairingType = 0x00; constexpr uint8_t kInitialOrSubsequentFlags = 0x00; constexpr uint8_t kRetroactiveFlags = 0x10; + +// Battery Info +constexpr int kBatteryChargingMask = 0b10000000; +constexpr int kBatteryPercentageMask = 0b01111111; +constexpr int kBatteryIsChargingByte = 0b11111111; +constexpr int kBatteryNotChargingByte = 0b01111111; + } // namespace fastpair } // namespace nearby diff --git a/fastpair/common/non_discoverable_advertisement.h b/fastpair/common/non_discoverable_advertisement.h new file mode 100644 index 00000000..a03fc421 --- /dev/null +++ b/fastpair/common/non_discoverable_advertisement.h @@ -0,0 +1,59 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef THIRD_PARTY_NEARBY_FASTPAIR_COMMON_NON_DISCOVERABLE_ADVERTISEMENT_H_ +#define THIRD_PARTY_NEARBY_FASTPAIR_COMMON_NON_DISCOVERABLE_ADVERTISEMENT_H_ + +#include +#include +#include +#include + +#include "fastpair/common/battery_notification.h" + +namespace nearby { +namespace fastpair { + +// Fast Pair 'Not Discoverable' advertisement. See +// https://developers.google.com/nearby/fast-pair/specifications/service/provider#AdvertisingWhenNotDiscoverable +struct NonDiscoverableAdvertisement { + // Represents showing UI indication + enum class Type { + kShowUi = 0, /* Show UI indication: 0b0000*/ + kNone = 1, + kHideUi = 2, /* Hide UI indication: 0b0010*/ + }; + + NonDiscoverableAdvertisement() = default; + NonDiscoverableAdvertisement( + std::vector account_key_filter, Type type, + std::vector salt, + std::optional battery_notification) + : account_key_filter(std::move(account_key_filter)), + type(type), + salt(std::move(salt)), + battery_notification(std::move(battery_notification)) {} + + ~NonDiscoverableAdvertisement() = default; + + std::vector account_key_filter; + Type type = Type::kNone; + std::vector salt; + std::optional battery_notification; +}; + +} // namespace fastpair +} // namespace nearby + +#endif // THIRD_PARTY_NEARBY_FASTPAIR_COMMON_NON_DISCOVERABLE_ADVERTISEMENT_H_ From ae35b1a2422e865fe54354a9ea8e1273f74f7015 Mon Sep 17 00:00:00 2001 From: Nick Bourdakos Date: Mon, 15 May 2023 13:55:07 -0700 Subject: [PATCH 03/11] Internal change PiperOrigin-RevId: 532215519 --- .../implementation/flags/nearby_connections_feature_flags.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/connections/implementation/flags/nearby_connections_feature_flags.h b/connections/implementation/flags/nearby_connections_feature_flags.h index d18a9d72..dd9f3a47 100644 --- a/connections/implementation/flags/nearby_connections_feature_flags.h +++ b/connections/implementation/flags/nearby_connections_feature_flags.h @@ -37,8 +37,8 @@ constexpr auto kBlePeripheralLostTimeoutMillis = flags::Flag(kConfigPackage, "45411439", 12000); // LINT.ThenChange( -// //depot/google3/location/nearby/cpp/sharing/clients/windows/nearby_sharing_service_adapter_dart.h, -// //depot/google3/location/nearby/cpp/sharing/clients/windows/nearby_sharing_service_adapter_dart.cc, +// //depot/google3/location/nearby/cpp/sharing/clients/cpp/nearby_sharing_service_adapter_dart.h, +// //depot/google3/location/nearby/cpp/sharing/clients/cpp/nearby_sharing_service_adapter_dart.cc, // //depot/google3/location/nearby/cpp/sharing/clients/dart/platform/lib/ffi_types.dart, // //depot/google3/location/nearby/cpp/sharing/clients/dart/platform/lib/types/models.dart // ) From a925ed1072d346dcab79719772148164730e9318 Mon Sep 17 00:00:00 2001 From: Chun Zhang Date: Mon, 15 May 2023 15:02:48 -0700 Subject: [PATCH 04/11] Move FakeAuthenticationManager from location to third_party. PiperOrigin-RevId: 532237745 --- Package.swift | 1 + internal/platform/implementation/g3/BUILD | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Package.swift b/Package.swift index b690d341..941fec31 100644 --- a/Package.swift +++ b/Package.swift @@ -569,6 +569,7 @@ let package = Package( "internal/network/http_request_test.cc", "internal/network/http_client_impl_test.cc", "internal/network/http_status_code_test.cc", + "internal/test/google3_only/fake_authentication_manager_test.cc", "internal/test/fake_clock_test.cc", "internal/test/fake_timer_test.cc", "internal/test/fake_device_info_test.cc", diff --git a/internal/platform/implementation/g3/BUILD b/internal/platform/implementation/g3/BUILD index 1d4b5f3b..79c8d9a7 100644 --- a/internal/platform/implementation/g3/BUILD +++ b/internal/platform/implementation/g3/BUILD @@ -36,7 +36,10 @@ cc_library( "single_thread_executor.h", "timer.h", ], - visibility = ["//location/nearby/cpp:__subpackages__"], + visibility = [ + "//internal/test:__subpackages__", + "//location/nearby/cpp:__subpackages__", + ], deps = [ ":preferences_repository", "//internal/platform:base", From 99e951dc530e0381ae011cc7a06fa04c6dd2c996 Mon Sep 17 00:00:00 2001 From: Anay Wadhera Date: Mon, 15 May 2023 16:16:08 -0700 Subject: [PATCH 05/11] ClientProxy: Wire in DeviceProvider PiperOrigin-RevId: 532259220 --- connections/implementation/BUILD | 2 ++ connections/implementation/client_proxy.cc | 11 +++++++---- connections/implementation/client_proxy.h | 9 ++++++++- connections/implementation/client_proxy_test.cc | 12 ++++++++++++ 4 files changed, 29 insertions(+), 5 deletions(-) diff --git a/connections/implementation/BUILD b/connections/implementation/BUILD index e0b8c2cf..fa928ef3 100644 --- a/connections/implementation/BUILD +++ b/connections/implementation/BUILD @@ -140,6 +140,7 @@ cc_library( "//connections/implementation/mediums:utils", "//connections/implementation/mediums/webrtc", "//connections/implementation/proto:offline_wire_formats_cc_proto", + "//connections/v3:v3_types", "//internal/analytics:event_logger", "//internal/flags:nearby_flags", "//internal/interop:device", @@ -247,6 +248,7 @@ cc_test( "//connections/implementation/flags:connections_flags", "//connections/implementation/mediums", "//connections/implementation/proto:offline_wire_formats_cc_proto", + "//connections/v3:v3_types", "//internal/analytics:event_logger", "//internal/flags:nearby_flags", "//internal/platform:base", diff --git a/connections/implementation/client_proxy.cc b/connections/implementation/client_proxy.cc index 15621b9d..17cb5dd0 100644 --- a/connections/implementation/client_proxy.cc +++ b/connections/implementation/client_proxy.cc @@ -27,6 +27,7 @@ #include "absl/container/flat_hash_set.h" #include "absl/strings/escaping.h" #include "absl/strings/str_format.h" +#include "connections/v3/connections_device_provider.h" #include "internal/analytics/event_logger.h" #include "internal/platform/error_code_recorder.h" #include "internal/platform/feature_flags.h" @@ -70,11 +71,13 @@ std::int64_t ClientProxy::GetClientId() const { return client_id_; } std::string ClientProxy::GetLocalEndpointId() { MutexLock lock(&mutex_); - if (local_endpoint_id_.empty()) { + if (!local_endpoint_id_.empty()) { + return local_endpoint_id_; + } + if (device_provider_ == nullptr) { local_endpoint_id_ = GenerateLocalEndpointId(); - NEARBY_LOGS(INFO) << "ClientProxy [Local Endpoint Generated]: client=" - << GetClientId() - << "; endpoint_id=" << local_endpoint_id_; + } else { + local_endpoint_id_ = device_provider_->GetLocalDevice()->GetEndpointId(); } return local_endpoint_id_; } diff --git a/connections/implementation/client_proxy.h b/connections/implementation/client_proxy.h index f3bc81bf..fbf2669f 100644 --- a/connections/implementation/client_proxy.h +++ b/connections/implementation/client_proxy.h @@ -20,6 +20,7 @@ #include #include #include +#include #include #include "connections/advertising_options.h" @@ -31,6 +32,7 @@ #include "connections/strategy.h" #include "internal/analytics/event_logger.h" #include "internal/interop/device.h" +#include "internal/interop/device_provider.h" #include "internal/platform/byte_array.h" #include "internal/platform/cancelable_alarm.h" #include "internal/platform/cancellation_flag.h" @@ -206,6 +208,10 @@ class ClientProxy final { absl::string_view endpoint_id, const location::nearby::connections::OsInfo& remote_os_info); + void RegisterDeviceProvider(std::unique_ptr provider) { + device_provider_ = std::move(provider); + } + private: struct Connection { // Status: may be either: @@ -282,7 +288,7 @@ class ClientProxy final { // Bluetooth Classic enabled. When high_visibility_mode_ is true, the endpoint // id is stable for 30s. When high_visibility_mode_ is false, the endpoint id // always rotates. - bool high_vis_mode_{false}; + bool high_vis_mode_ = false; // Caches the endpoint id when it is in high visibility mode advertisement for // 30s. Currently, Nearby Connections keeps rotating endpoint id. The client // (Nearby Share) treats different endpoints as different receivers, duplicate @@ -339,6 +345,7 @@ class ClientProxy final { std::unique_ptr error_code_recorder_; // Local device OS information. location::nearby::connections::OsInfo local_os_info_; + std::unique_ptr device_provider_; }; } // namespace connections diff --git a/connections/implementation/client_proxy_test.cc b/connections/implementation/client_proxy_test.cc index a8b28833..2e74c1f7 100644 --- a/connections/implementation/client_proxy_test.cc +++ b/connections/implementation/client_proxy_test.cc @@ -15,6 +15,7 @@ #include "connections/implementation/client_proxy.h" #include +#include #include #include @@ -28,6 +29,7 @@ #include "absl/types/span.h" #include "connections/listeners.h" #include "connections/strategy.h" +#include "connections/v3/connections_device_provider.h" #include "internal/analytics/event_logger.h" #include "internal/platform/byte_array.h" #include "internal/platform/feature_flags.h" @@ -427,6 +429,16 @@ TEST_F(ClientProxyTest, GeneratedEndpointIdIsUnique) { EXPECT_NE(client1_.GetLocalEndpointId(), client2_.GetLocalEndpointId()); } +TEST_F(ClientProxyTest, GeneratedEndpointIdIsUniqueWithDeviceProvider) { + client1_.RegisterDeviceProvider( + std::make_unique( + v3::ConnectionsDeviceProvider("", {}))); + client2_.RegisterDeviceProvider( + std::make_unique( + v3::ConnectionsDeviceProvider("", {}))); + EXPECT_NE(client1_.GetLocalEndpointId(), client2_.GetLocalEndpointId()); +} + TEST_F(ClientProxyTest, ResetClearsState) { client1_.Reset(); EXPECT_FALSE(client1_.IsAdvertising()); From 4ea5e4cf8a28d00c6c45edc869ec534edb58c1c7 Mon Sep 17 00:00:00 2001 From: Anay Wadhera Date: Tue, 16 May 2023 10:54:39 -0700 Subject: [PATCH 06/11] nearby swift: Update include paths for new ukey2 revisions --- Package.resolved | 8 ++-- Package.swift | 48 +++---------------- .../proto/device_to_device_messages.pb.cc | 0 .../proto/device_to_device_messages.pb.h | 0 .../proto/passwordless_auth_payloads.pb.cc | 0 .../proto/passwordless_auth_payloads.pb.h | 0 .../main}/proto/proximity_payloads.pb.cc | 0 .../main}/proto/proximity_payloads.pb.h | 0 .../{ => src/main}/proto/securegcm.pb.cc | 0 .../{ => src/main}/proto/securegcm.pb.h | 0 .../{ => src/main}/proto/securemessage.pb.cc | 0 .../{ => src/main}/proto/securemessage.pb.h | 0 .../{ => src/main}/proto/ukey.pb.cc | 0 .../{ => src/main}/proto/ukey.pb.h | 0 14 files changed, 10 insertions(+), 46 deletions(-) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/device_to_device_messages.pb.cc (100%) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/device_to_device_messages.pb.h (100%) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/passwordless_auth_payloads.pb.cc (100%) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/passwordless_auth_payloads.pb.h (100%) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/proximity_payloads.pb.cc (100%) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/proximity_payloads.pb.h (100%) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/securegcm.pb.cc (100%) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/securegcm.pb.h (100%) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/securemessage.pb.cc (100%) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/securemessage.pb.h (100%) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/ukey.pb.cc (100%) rename third_party/ukey2/compiled_proto/{ => src/main}/proto/ukey.pb.h (100%) diff --git a/Package.resolved b/Package.resolved index 5b233429..677a6086 100644 --- a/Package.resolved +++ b/Package.resolved @@ -3,10 +3,10 @@ "pins": [ { "package": "abseil", - "repositoryURL": "https://github.com/firebase/abseil-cpp-SwiftPM.git", + "repositoryURL": "https://github.com/bourdakos1/abseil-cpp-SwiftPM.git", "state": { - "branch": "main", - "revision": "06e7506b74bfc47c70f3353e2927ea3e2275230c", + "branch": "cxx17", + "revision": "a7042563d167160f56e614cd1e2a32616510d9ec", "version": null } }, @@ -22,4 +22,4 @@ ] }, "version": 1 -} \ No newline at end of file +} diff --git a/Package.swift b/Package.swift index 941fec31..ec463cfc 100644 --- a/Package.swift +++ b/Package.swift @@ -168,69 +168,30 @@ let package = Package( .headerSearchPath("smhasher/src/") ] ), - .target( - name: "securemessage", - dependencies: [ - "protobuf", - .product(name: "openssl_grpc", package: "BoringSSL-GRPC"), - ], - path: "third_party/securemessage", - exclude: [ - "securemessage/cmake", - "securemessage/cpp/src/securemessage/CMakeLists.txt", - "securemessage/cpp/test", - "securemessage/cpp/CMakeLists.txt", - "securemessage/cpp/Makefile", - "securemessage/cpp/README.md", - "securemessage/java", - "securemessage/js", - "securemessage/proto", - "securemessage/third_party", - "securemessage/CMakeLists.txt", - "securemessage/CONTRIBUTORS", - "securemessage/CONTRIBUTING.md", - "securemessage/LICENSE", - "securemessage/README.md", - ], - sources: [ - "securemessage/cpp/src/securemessage", - "compiled_proto", - ], - publicHeadersPath: "include", - cSettings: [ - .headerSearchPath("securemessage/cpp/include/"), - .headerSearchPath("compiled_proto/"), - ] - ), .target( name: "ukey2", dependencies: [ - "securemessage", "protobuf", .product(name: "abseil", package: "abseil"), + .product(name: "openssl_grpc", package: "BoringSSL-GRPC"), ], path: "third_party/ukey2", exclude: [ - "ukey2/cmake", - "ukey2/src/main/cpp/src/securegcm/CMakeLists.txt", "ukey2/src/main/cpp/src/securegcm/ukey2_shell.cc", "ukey2/src/main/cpp/test", - "ukey2/src/main/cpp/CMakeLists.txt", "ukey2/src/main/java", "ukey2/src/main/javatest", "ukey2/src/main/proto", - "ukey2/src/main/CMakeLists.txt", "ukey2/third_party", "ukey2/Android.bp", "ukey2/build.gradle", - "ukey2/CMakeLists.txt", "ukey2/CONTRIBUTING.md", "ukey2/LICENSE", "ukey2/MODULE_LICENSE_APACHE2", "ukey2/NOTICE", "ukey2/README", "ukey2/README.md", - "compiled_proto/proto/securemessage.pb.cc", + "compiled_proto/src/main/proto/securemessage.pb.cc", ], sources: [ "ukey2/src/main/cpp/src/securegcm", @@ -238,8 +199,9 @@ let package = Package( ], publicHeadersPath: "include", cSettings: [ - .headerSearchPath("ukey2/src/main/cpp/include/"), + .headerSearchPath("ukey2/"), .headerSearchPath("compiled_proto/"), + .headerSearchPath("compiled_proto/src/main/"), ] ), .target( @@ -605,6 +567,8 @@ let package = Package( cSettings: [ .headerSearchPath("./"), .headerSearchPath("compiled_proto/"), + .headerSearchPath("third_party/ukey2/ukey2/"), + .headerSearchPath("third_party/ukey2/compiled_proto/"), .define("NO_WEBRTC"), .define("NEARBY_SWIFTPM"), ] diff --git a/third_party/ukey2/compiled_proto/proto/device_to_device_messages.pb.cc b/third_party/ukey2/compiled_proto/src/main/proto/device_to_device_messages.pb.cc similarity index 100% rename from third_party/ukey2/compiled_proto/proto/device_to_device_messages.pb.cc rename to third_party/ukey2/compiled_proto/src/main/proto/device_to_device_messages.pb.cc diff --git a/third_party/ukey2/compiled_proto/proto/device_to_device_messages.pb.h b/third_party/ukey2/compiled_proto/src/main/proto/device_to_device_messages.pb.h similarity index 100% rename from third_party/ukey2/compiled_proto/proto/device_to_device_messages.pb.h rename to third_party/ukey2/compiled_proto/src/main/proto/device_to_device_messages.pb.h diff --git a/third_party/ukey2/compiled_proto/proto/passwordless_auth_payloads.pb.cc b/third_party/ukey2/compiled_proto/src/main/proto/passwordless_auth_payloads.pb.cc similarity index 100% rename from third_party/ukey2/compiled_proto/proto/passwordless_auth_payloads.pb.cc rename to third_party/ukey2/compiled_proto/src/main/proto/passwordless_auth_payloads.pb.cc diff --git a/third_party/ukey2/compiled_proto/proto/passwordless_auth_payloads.pb.h b/third_party/ukey2/compiled_proto/src/main/proto/passwordless_auth_payloads.pb.h similarity index 100% rename from third_party/ukey2/compiled_proto/proto/passwordless_auth_payloads.pb.h rename to third_party/ukey2/compiled_proto/src/main/proto/passwordless_auth_payloads.pb.h diff --git a/third_party/ukey2/compiled_proto/proto/proximity_payloads.pb.cc b/third_party/ukey2/compiled_proto/src/main/proto/proximity_payloads.pb.cc similarity index 100% rename from third_party/ukey2/compiled_proto/proto/proximity_payloads.pb.cc rename to third_party/ukey2/compiled_proto/src/main/proto/proximity_payloads.pb.cc diff --git a/third_party/ukey2/compiled_proto/proto/proximity_payloads.pb.h b/third_party/ukey2/compiled_proto/src/main/proto/proximity_payloads.pb.h similarity index 100% rename from third_party/ukey2/compiled_proto/proto/proximity_payloads.pb.h rename to third_party/ukey2/compiled_proto/src/main/proto/proximity_payloads.pb.h diff --git a/third_party/ukey2/compiled_proto/proto/securegcm.pb.cc b/third_party/ukey2/compiled_proto/src/main/proto/securegcm.pb.cc similarity index 100% rename from third_party/ukey2/compiled_proto/proto/securegcm.pb.cc rename to third_party/ukey2/compiled_proto/src/main/proto/securegcm.pb.cc diff --git a/third_party/ukey2/compiled_proto/proto/securegcm.pb.h b/third_party/ukey2/compiled_proto/src/main/proto/securegcm.pb.h similarity index 100% rename from third_party/ukey2/compiled_proto/proto/securegcm.pb.h rename to third_party/ukey2/compiled_proto/src/main/proto/securegcm.pb.h diff --git a/third_party/ukey2/compiled_proto/proto/securemessage.pb.cc b/third_party/ukey2/compiled_proto/src/main/proto/securemessage.pb.cc similarity index 100% rename from third_party/ukey2/compiled_proto/proto/securemessage.pb.cc rename to third_party/ukey2/compiled_proto/src/main/proto/securemessage.pb.cc diff --git a/third_party/ukey2/compiled_proto/proto/securemessage.pb.h b/third_party/ukey2/compiled_proto/src/main/proto/securemessage.pb.h similarity index 100% rename from third_party/ukey2/compiled_proto/proto/securemessage.pb.h rename to third_party/ukey2/compiled_proto/src/main/proto/securemessage.pb.h diff --git a/third_party/ukey2/compiled_proto/proto/ukey.pb.cc b/third_party/ukey2/compiled_proto/src/main/proto/ukey.pb.cc similarity index 100% rename from third_party/ukey2/compiled_proto/proto/ukey.pb.cc rename to third_party/ukey2/compiled_proto/src/main/proto/ukey.pb.cc diff --git a/third_party/ukey2/compiled_proto/proto/ukey.pb.h b/third_party/ukey2/compiled_proto/src/main/proto/ukey.pb.h similarity index 100% rename from third_party/ukey2/compiled_proto/proto/ukey.pb.h rename to third_party/ukey2/compiled_proto/src/main/proto/ukey.pb.h From d677f0c3c3196707e803cdf45ff0921b9f1ef5a7 Mon Sep 17 00:00:00 2001 From: Anay Wadhera Date: Tue, 16 May 2023 11:26:43 -0700 Subject: [PATCH 07/11] nearby: Upgrade all submodules --- third_party/absl | 2 +- third_party/depot_tools | 2 +- third_party/google-toolbox-for-mac/google-toolbox-for-mac | 2 +- third_party/gtest | 2 +- third_party/json/json | 2 +- third_party/mbedtls | 2 +- third_party/protobuf | 2 +- third_party/ukey2/ukey2 | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/third_party/absl b/third_party/absl index 9336be04..abe63eb9 160000 --- a/third_party/absl +++ b/third_party/absl @@ -1 +1 @@ -Subproject commit 9336be04a242237cd41a525bedfcf3be1bb55377 +Subproject commit abe63eb9bd1213c018bf82765ab747334d3b33d8 diff --git a/third_party/depot_tools b/third_party/depot_tools index e1197f06..6316ac23 160000 --- a/third_party/depot_tools +++ b/third_party/depot_tools @@ -1 +1 @@ -Subproject commit e1197f06a8f45c0328d341b30e337d3a4b609716 +Subproject commit 6316ac234e70d983ae758e283ed6ac8d4d0cb36b diff --git a/third_party/google-toolbox-for-mac/google-toolbox-for-mac b/third_party/google-toolbox-for-mac/google-toolbox-for-mac index 33941504..1b2da5e6 160000 --- a/third_party/google-toolbox-for-mac/google-toolbox-for-mac +++ b/third_party/google-toolbox-for-mac/google-toolbox-for-mac @@ -1 +1 @@ -Subproject commit 339415048005a9eba957357a02459a977a2e3007 +Subproject commit 1b2da5e6e6b5edb1fa1427cc77e23f04069c04ad diff --git a/third_party/gtest b/third_party/gtest index d61d4d8e..d6fb5e3b 160000 --- a/third_party/gtest +++ b/third_party/gtest @@ -1 +1 @@ -Subproject commit d61d4d8e64c08a662055e82904bbf90e108a704f +Subproject commit d6fb5e3bf76c0363d7519373a07c2435e57c1073 diff --git a/third_party/json/json b/third_party/json/json index 6af826d0..bc889afb 160000 --- a/third_party/json/json +++ b/third_party/json/json @@ -1 +1 @@ -Subproject commit 6af826d0bdb55e4b69e3ad817576745335f243ca +Subproject commit bc889afb4c5bf1c0d8ee29ef35eaaf4c8bef8a5d diff --git a/third_party/mbedtls b/third_party/mbedtls index 3e0418fe..ff7a3462 160000 --- a/third_party/mbedtls +++ b/third_party/mbedtls @@ -1 +1 @@ -Subproject commit 3e0418fe502b2a2194e118d362efdfc0e558be73 +Subproject commit ff7a3462017d1fafa823f6e081fc12802f96f29c diff --git a/third_party/protobuf b/third_party/protobuf index 4812107b..12f743d4 160000 --- a/third_party/protobuf +++ b/third_party/protobuf @@ -1 +1 @@ -Subproject commit 4812107b9d0fb9fdcca933766c237c38f2150379 +Subproject commit 12f743d4e7bc2a22918371a286e9ab09748b0f40 diff --git a/third_party/ukey2/ukey2 b/third_party/ukey2/ukey2 index c2436e55..03290b58 160000 --- a/third_party/ukey2/ukey2 +++ b/third_party/ukey2/ukey2 @@ -1 +1 @@ -Subproject commit c2436e55116964d88532080784f6ed496b0d11f9 +Subproject commit 03290b58d9f93cdd5c9f9f7a7f9e8efc640d0c40 From 601d5f6b36b3f9925aa325622e1de4c3de7879cc Mon Sep 17 00:00:00 2001 From: Anay Wadhera Date: Tue, 16 May 2023 11:52:02 -0700 Subject: [PATCH 08/11] fixup! nearby swift: Update include paths for new ukey2 revisions --- Package.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Package.swift b/Package.swift index ec463cfc..21e142b1 100644 --- a/Package.swift +++ b/Package.swift @@ -191,9 +191,9 @@ let package = Package( "ukey2/NOTICE", "ukey2/README", "ukey2/README.md", - "compiled_proto/src/main/proto/securemessage.pb.cc", ], sources: [ + "ukey2/src/securemessage/src/securemessage", "ukey2/src/main/cpp/src/securegcm", "compiled_proto", ], From e4be4677c49f2d5667b80cdfe8b90f34be9bbfd3 Mon Sep 17 00:00:00 2001 From: Anay Wadhera Date: Tue, 16 May 2023 12:04:39 -0700 Subject: [PATCH 09/11] update ukey2 to use bazel PiperOrigin-RevId: 532532700 --- WORKSPACE | 6 +++--- connections/implementation/BUILD | 28 ++-------------------------- 2 files changed, 5 insertions(+), 29 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index b317f629..0fc42261 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -59,10 +59,10 @@ http_archive( urls = ["https://github.com/google/glog/archive/v0.4.0.tar.gz"], ) -new_local_repository( +http_archive( name = "com_google_ukey2", - path = "./third_party/ukey2/ukey2", - build_file_content = _ALL_CONTENT, + strip_prefix = "ukey2-master", + urls = ["https://github.com/google/ukey2/archive/master.zip"], ) http_archive( diff --git a/connections/implementation/BUILD b/connections/implementation/BUILD index fa928ef3..67bd965b 100644 --- a/connections/implementation/BUILD +++ b/connections/implementation/BUILD @@ -1,5 +1,3 @@ -load("@rules_foreign_cc//foreign_cc:defs.bzl", "cmake") - # Copyright 2020 Google LLC # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -15,28 +13,6 @@ load("@rules_foreign_cc//foreign_cc:defs.bzl", "cmake") # limitations under the License. licenses(["notice"]) -cmake( - name = "ukey2", - env = { - "CC": "clang", - "CXX": "clang++", - }, - generate_args = [ - "-DBYPASS_TESTING=ON", # Set the flags in CMakeLists.txt (ukey2 & its dependencies) here. - "-Dukey2_USE_LOCAL_ABSL=ON", - "-Dukey2_USE_LOCAL_PROTOBUF=ON", # Add -DCMAKE_BUILD_TYPE=Debug for debugger & Valgrind - ], - lib_source = "@com_google_ukey2//:all_srcs", - out_static_libs = [ - "libproto_device_to_device_messages_cc_proto.a", - "libproto_securegcm_cc_proto.a", - "libproto_securemessage_cc_proto.a", - "libproto_ukey_cc_proto.a", - "libsecuremessage.a", - "libukey2.a", - ], -) - cc_library( name = "internal", srcs = [ @@ -132,7 +108,6 @@ cc_library( ], deps = [ ":message_lite", - ":ukey2", "//connections:core_types", "//connections/implementation/analytics", "//connections/implementation/flags:connections_flags", @@ -165,6 +140,7 @@ cc_library( "@com_google_absl//absl/strings:str_format", "@com_google_absl//absl/time", "@com_google_absl//absl/types:span", + "@com_google_ukey2//:ukey2", ], ) @@ -243,7 +219,6 @@ cc_test( deps = [ ":internal", ":internal_test", - ":ukey2", "//connections:core_types", "//connections/implementation/flags:connections_flags", "//connections/implementation/mediums", @@ -267,5 +242,6 @@ cc_test( "@com_google_absl//absl/types:span", "@com_google_googletest//:gtest", "@com_google_googletest//:gtest_main", + "@com_google_ukey2//:ukey2", ], ) From 275743e462f81c808cc1b59459d4da7f8ddead9e Mon Sep 17 00:00:00 2001 From: hai007 Date: Tue, 16 May 2023 13:25:09 -0700 Subject: [PATCH 10/11] Fix a number of Auth crashes by upgrading the callbacks to almost pure functions PiperOrigin-RevId: 532556608 --- internal/network/http_client_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/network/http_client_impl.cc b/internal/network/http_client_impl.cc index ef74495a..a5dbacc1 100644 --- a/internal/network/http_client_impl.cc +++ b/internal/network/http_client_impl.cc @@ -38,7 +38,7 @@ void NearbyHttpClient::StartRequest( CleanThreads(); std::future http_thread = std::async( - std::launch::async, [&, request, callback = std::move(callback)]() { + std::launch::async, [=]() { NEARBY_LOGS(INFO) << __func__ << ": Start async request to url=" << request.GetUrl().GetUrlPath(); absl::StatusOr response = InternalGetResponse(request); From 8a78212bb841a7efcff74b8608b5194dc7047417 Mon Sep 17 00:00:00 2001 From: Juliet Levesque Date: Tue, 16 May 2023 14:52:11 -0700 Subject: [PATCH 11/11] [Nearby Connections] Check for shutdown before accessing ClientProxy. During the destruction of NearbyConnections, Core (which owns ClientProxy) is destructed before ServiceController (which owns EndpointManager), which means any pending tasks on the EndpointManager executor that use ClientProxy will be using garbage memory. To fix this issue, EndpointManager::DiscardEndpoint will check for an `is_shutdown` boolean set during ~EndpointManager before accessing ClientProxy. The assumption is that any accessing of ClientProxy after the destruction will be invalid. PiperOrigin-RevId: 532581122 --- connections/implementation/BUILD | 3 +- .../implementation/endpoint_manager.cc | 58 ++++++++++++++--- connections/implementation/endpoint_manager.h | 18 +++++- .../implementation/endpoint_manager_test.cc | 63 +++++++++++++++++-- internal/platform/single_thread_executor.h | 2 +- internal/platform/submittable_executor.h | 3 +- internal/test/BUILD | 2 + internal/test/fake_single_thread_executor.cc | 48 ++++++++++++++ internal/test/fake_single_thread_executor.h | 52 +++++++++++++++ 9 files changed, 231 insertions(+), 18 deletions(-) create mode 100644 internal/test/fake_single_thread_executor.cc create mode 100644 internal/test/fake_single_thread_executor.h diff --git a/connections/implementation/BUILD b/connections/implementation/BUILD index 67bd965b..0e93ee0a 100644 --- a/connections/implementation/BUILD +++ b/connections/implementation/BUILD @@ -107,13 +107,11 @@ cc_library( "//location/nearby/cpp/sharing/implementation:__pkg__", ], deps = [ - ":message_lite", "//connections:core_types", "//connections/implementation/analytics", "//connections/implementation/flags:connections_flags", "//connections/implementation/mediums", "//connections/implementation/mediums:utils", - "//connections/implementation/mediums/webrtc", "//connections/implementation/proto:offline_wire_formats_cc_proto", "//connections/v3:v3_types", "//internal/analytics:event_logger", @@ -232,6 +230,7 @@ cc_test( "//internal/platform:test_util", "//internal/platform:types", "//internal/platform/implementation/g3", # build_cleaner: keep + "//internal/test", "//proto:connections_enums_cc_proto", "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_absl//absl/container:flat_hash_set", diff --git a/connections/implementation/endpoint_manager.cc b/connections/implementation/endpoint_manager.cc index 2233eab2..cd7ef40e 100644 --- a/connections/implementation/endpoint_manager.cc +++ b/connections/implementation/endpoint_manager.cc @@ -287,10 +287,16 @@ bool operator<(const EndpointManager::FrameProcessor& lhs, } EndpointManager::EndpointManager(EndpointChannelManager* manager) - : channel_manager_(manager) {} + : EndpointManager(manager, std::make_unique()) {} + +EndpointManager::EndpointManager( + EndpointChannelManager* manager, + std::unique_ptr serial_executor) + : channel_manager_(manager), serial_executor_(std::move(serial_executor)) {} EndpointManager::~EndpointManager() { NEARBY_LOG(INFO, "Initiating shutdown of EndpointManager."); + is_shutdown_ = true; analytics::ThroughputRecorderContainer::GetInstance().Shutdown(); CountDownLatch latch(1); RunOnEndpointManagerThread("bring-down-endpoints", [this, &latch]() { @@ -301,7 +307,7 @@ EndpointManager::~EndpointManager() { latch.Await(); NEARBY_LOG(INFO, "Bringing down control thread"); - serial_executor_.Shutdown(); + serial_executor_->Shutdown(); NEARBY_LOG(INFO, "EndpointManager is down"); } @@ -524,10 +530,48 @@ std::vector EndpointManager::SendPayloadChunk( void EndpointManager::DiscardEndpoint(ClientProxy* client, const std::string& endpoint_id) { NEARBY_LOGS(VERBOSE) << "DiscardEndpoint for endpoint " << endpoint_id; - RunOnEndpointManagerThread("discard-endpoint", [this, client, endpoint_id]() { - RemoveEndpoint(client, endpoint_id, - /*notify=*/client->IsConnectedToEndpoint(endpoint_id)); - }); + RunOnEndpointManagerThread( + "discard-endpoint", [this, client, endpoint_id]() { + // `ClientProxy` is destroyed before `EndpointManager` in + // `~NearbyConnections`, which means "discard-endpoint" needs to check + // if this task is being executing during `~EndpointManager` to + // prevent accessing an invalid `ClientProxy` pointer. There are two + // cases where "discard-endpoint" can be executed during destruction, + // both of which can safely use `is_shutdown_` to check if this is being + // executed during the destruction of the object: + // + // Case 1: "discard-endpoints" is posted to the thread before + // destruction, but not executed yet: `~EndpointManager` blocks on + // "bring-down-endpoints" and because the executor is a single thread + // executor, tasks are guaranteed to execute sequentially + // (see + // https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/Executors.html#newSingleThreadExecutor--) + // and this means that the "discard-endpoints" will be executed before + // "bring-down-endpoints", blocking the destruction of `is_shutdown_` + // and therefore `is_shutdown_` is not garbage memory. + // + // Case 2: "discard-endpoints" is posted to the thread during + // destruction, after "bring-down-endpoints" is called: the executor + // will be destructed before `is_shutdown_` because of the ordering of + // `EndpointManager`'s member variables, and the executor's destructor + // blocks on running all pending tasks + // (see + // https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:chrome/services/sharing/nearby/platform/scheduled_executor.cc;l=67;drc=e0e0d24aaa54727dc0a8bc4b159ccdf80d3f5d8d), + // which means that "discard-endpoints" will run during the destruction + // of `serial_executor_` and will still have access to a valid + // `is_shutdown_`. + // + // TODO(b/280653613): Develop a more robost solution to prevent + // accessing an already destroyed `ClientProxy` during destruction. + if (is_shutdown_) { + NEARBY_LOGS(VERBOSE) + << "DiscardEndpoint called during destruction, returning early."; + return; + } + + RemoveEndpoint(client, endpoint_id, + /*notify=*/client->IsConnectedToEndpoint(endpoint_id)); + }); } std::vector EndpointManager::SendControlMessage( @@ -698,7 +742,7 @@ void EndpointManager::EndpointState::StartEndpointKeepAliveManager( void EndpointManager::RunOnEndpointManagerThread(const std::string& name, Runnable runnable) { - serial_executor_.Execute(name, std::move(runnable)); + serial_executor_->Execute(name, std::move(runnable)); } } // namespace connections diff --git a/connections/implementation/endpoint_manager.h b/connections/implementation/endpoint_manager.h index 53c16bf0..7e7fdaae 100644 --- a/connections/implementation/endpoint_manager.h +++ b/connections/implementation/endpoint_manager.h @@ -153,6 +153,11 @@ class EndpointManager { // blocked here. void DiscardEndpoint(ClientProxy* client, const std::string& endpoint_id); + protected: + // For unit tests only to control executing tasks on the executor. + EndpointManager(EndpointChannelManager* manager, + std::unique_ptr serial_executor); + private: class EndpointState { public: @@ -288,7 +293,18 @@ class EndpointManager { // We keep track of all registered channel endpoints here. absl::flat_hash_map endpoints_; - SingleThreadExecutor serial_executor_; + // Indicates whether the destructor has been called yet. If `is_shutdown_` + // is true, assume any `ClientProxy` pointers are invalid, and should not + // be used. + // + // The ordering of these objects is important: `serial_executor_` must be + // destroyed before `is_shutdown_` because `serial_executor_` runs all + // pending tasks during it's destruction, and the "discard-endpoints" + // task checks `is_shutdown_` to prevent accessing an invalid `ClientProxy` + // pointer. + bool is_shutdown_ = false; + + std::unique_ptr serial_executor_; }; // Operator overloads when comparing FrameProcessor*. diff --git a/connections/implementation/endpoint_manager_test.cc b/connections/implementation/endpoint_manager_test.cc index df790c70..58c83162 100644 --- a/connections/implementation/endpoint_manager_test.cc +++ b/connections/implementation/endpoint_manager_test.cc @@ -34,6 +34,7 @@ #include "internal/platform/count_down_latch.h" #include "internal/platform/exception.h" #include "internal/platform/logging.h" +#include "internal/test/fake_single_thread_executor.h" #include "proto/connections_enums.pb.h" namespace nearby { @@ -111,6 +112,13 @@ class MockFrameProcessor : public EndpointManager::FrameProcessor { (override)); }; +class TestEndpointManager : public EndpointManager { + public: + TestEndpointManager(EndpointChannelManager* manager, + std::unique_ptr serial_executor) + : EndpointManager(manager, std::move(serial_executor)) {} +}; + class EndpointManagerTest : public ::testing::Test { protected: void RegisterEndpoint(std::unique_ptr channel, @@ -127,14 +135,15 @@ class EndpointManagerTest : public ::testing::Test { EXPECT_CALL(*channel, GetLastWriteTimestamp()) .WillRepeatedly(Return(start_time_)); EXPECT_CALL(mock_listener_.initiated_cb, Call).Times(1); - em_.RegisterEndpoint(&client_, endpoint_id_, info_, connection_options_, - std::move(channel), listener_, connection_token); + em_.RegisterEndpoint(client_.get(), endpoint_id_, info_, + connection_options_, std::move(channel), listener_, + connection_token); if (should_close) { EXPECT_TRUE(done.Await(absl::Milliseconds(1000)).result()); } } - ClientProxy client_; + std::unique_ptr client_ = std::make_unique(); ConnectionOptions connection_options_{ .keep_alive_interval_millis = 5000, .keep_alive_timeout_millis = 30000, @@ -195,7 +204,7 @@ TEST_F(EndpointManagerTest, UnregisterEndpointCallsOnDisconnected) { // (IMO, it should be called as long as any connection callback was called // before. (in this case initiated_cb is called)). // Test captures current protocol behavior. - em_.UnregisterEndpoint(&client_, endpoint_id_); + em_.UnregisterEndpoint(client_.get(), endpoint_id_); } TEST_F(EndpointManagerTest, RegisterFrameProcessorWorks) { @@ -250,7 +259,7 @@ TEST_F(EndpointManagerTest, UnregisterFrameProcessorWorks) { processors_.emplace_back(std::move(connect_request)); // Endpoint will not send OnDisconnect notification to frame processor. RegisterEndpoint(std::move(endpoint_channel), false); - em_.UnregisterEndpoint(&client_, endpoint_id_); + em_.UnregisterEndpoint(client_.get(), endpoint_id_); } TEST_F(EndpointManagerTest, SendControlMessageWorks) { @@ -286,7 +295,7 @@ TEST_F(EndpointManagerTest, SendControlMessageWorks) { em_.SendControlMessage(header, control, std::vector{endpoint_id_}); EXPECT_EQ(failed_ids, std::vector{}); NEARBY_LOG(INFO, "Will unregister endpoint now"); - em_.UnregisterEndpoint(&client_, endpoint_id_); + em_.UnregisterEndpoint(client_.get(), endpoint_id_); NEARBY_LOG(INFO, "Will call destructors now"); } @@ -301,6 +310,48 @@ TEST_F(EndpointManagerTest, SingleReadOnInvalidPayload) { RegisterEndpoint(std::move(endpoint_channel)); } +// Regression test for b/278729669. +// +// During the destruction of NearbyConnections, Core (which owns ClientProxy) +// is destructed before ServiceController (which owns EndpointManager), which +// means any pending tasks on the EndpointManager than use ClientProxy will +// be using garbage memory, and cause crashes. This test enforces the fix. +TEST_F(EndpointManagerTest, DisconnectEndpointDuringDestruction) { + // This test uses a `FakeSingleThreadExecutor` in order to control when + // tasks are executed in order to simulate the scenario where + // `DiscardEndpoint` is posted to the executor before the EndpointManager + // is destructed, and executed during it's destruction. + std::unique_ptr serial_executor = + std::make_unique(); + FakeSingleThreadExecutor* fake_serial_executor = + static_cast(serial_executor.get()); + std::unique_ptr endpoint_manager = + std::make_unique(&ecm_, std::move(serial_executor)); + + // DiscardEndpoint posts a task to the executor to run "discard-endpoint", + // however the `FakeSingleThreadExecutor` will not run this task + // immediately. + fake_serial_executor->SetRunExecutablesImmediately( + /*run_executables_immediately=*/false); + endpoint_manager->DiscardEndpoint(client_.get(), endpoint_id_); + + // Simulate Core destruction of ClientProxy by destroying `client_`. + client_.reset(); + + // Simulate ServiceController destruction of EndpointManager by destroying + // `endpoint_manager`, and set the `FakeSingleThreadExecutor` to run + // executables on calls `Execute`. When `endpoint_manager` is destructed, it + // will block on calls to `Execute` to run all pending executables, notably + // "discard-endpoint" from above. However, "discard-endpoint" will have a + // reference to a destroyed ClientProxy. + // + // Expect no crash when "discard-endpoints" is executed during the + // destruction. + fake_serial_executor->SetRunExecutablesImmediately( + /*run_executables_immediately=*/true); + endpoint_manager.reset(); +} + } // namespace } // namespace connections } // namespace nearby diff --git a/internal/platform/single_thread_executor.h b/internal/platform/single_thread_executor.h index 27a80f40..eb53bbee 100644 --- a/internal/platform/single_thread_executor.h +++ b/internal/platform/single_thread_executor.h @@ -24,7 +24,7 @@ namespace nearby { // queue. // // https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/Executors.html#newSingleThreadExecutor-- -class ABSL_LOCKABLE SingleThreadExecutor final : public SubmittableExecutor { +class ABSL_LOCKABLE SingleThreadExecutor : public SubmittableExecutor { public: using Platform = api::ImplementationPlatform; SingleThreadExecutor() diff --git a/internal/platform/submittable_executor.h b/internal/platform/submittable_executor.h index 47aa9e80..aba0375d 100644 --- a/internal/platform/submittable_executor.h +++ b/internal/platform/submittable_executor.h @@ -17,6 +17,7 @@ #include #include +#include #include #include "absl/base/thread_annotations.h" @@ -53,7 +54,7 @@ class ABSL_LOCKABLE SubmittableExecutor : public api::SubmittableExecutor, } return *this; } - void Execute(const std::string& name, Runnable&& runnable) + virtual void Execute(const std::string& name, Runnable&& runnable) ABSL_LOCKS_EXCLUDED(mutex_) { MutexLock lock(&mutex_); if (impl_) diff --git a/internal/test/BUILD b/internal/test/BUILD index dc5407b7..d843e66c 100644 --- a/internal/test/BUILD +++ b/internal/test/BUILD @@ -18,12 +18,14 @@ cc_library( name = "test", srcs = [ "fake_clock.cc", + "fake_single_thread_executor.cc", "fake_task_runner.cc", "fake_timer.cc", ], hdrs = [ "fake_clock.h", "fake_device_info.h", + "fake_single_thread_executor.h", "fake_task_runner.h", "fake_timer.h", ], diff --git a/internal/test/fake_single_thread_executor.cc b/internal/test/fake_single_thread_executor.cc new file mode 100644 index 00000000..dc50918a --- /dev/null +++ b/internal/test/fake_single_thread_executor.cc @@ -0,0 +1,48 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "internal/test/fake_single_thread_executor.h" + +#include +#include +#include + +namespace nearby { + +FakeSingleThreadExecutor::FakeSingleThreadExecutor() = default; + +FakeSingleThreadExecutor::~FakeSingleThreadExecutor() { DoShutdown(); } + +void FakeSingleThreadExecutor::Execute(const std::string& name, + Runnable&& runnable) { + runnables_.push_back(std::make_pair(name, std::move(runnable))); + + if (!run_executables_immediately_) return; + + RunAllExecutables(); +} + +void FakeSingleThreadExecutor::RunAllExecutables() { + // Because `SingleThreadExecutor` ensures sequencing, run all pending + // executables in order they were added to the vector. + for (auto& runnable_pair : runnables_) { + runnable_pair.second(); + } + + runnables_.clear(); +} + +void FakeSingleThreadExecutor::DoShutdown() { RunAllExecutables(); } + +} // namespace nearby diff --git a/internal/test/fake_single_thread_executor.h b/internal/test/fake_single_thread_executor.h new file mode 100644 index 00000000..d9c86d35 --- /dev/null +++ b/internal/test/fake_single_thread_executor.h @@ -0,0 +1,52 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef PLATFORM_PUBLIC_FAKE_SINGLE_THREAD_EXECUTOR_H_ +#define PLATFORM_PUBLIC_FAKE_SINGLE_THREAD_EXECUTOR_H_ + +#include +#include +#include + +#include "absl/base/thread_annotations.h" +#include "internal/platform/single_thread_executor.h" + +namespace nearby { + +class ABSL_LOCKABLE FakeSingleThreadExecutor final + : public SingleThreadExecutor { + public: + FakeSingleThreadExecutor(); + ~FakeSingleThreadExecutor() override; + FakeSingleThreadExecutor(FakeSingleThreadExecutor&&) = default; + FakeSingleThreadExecutor& operator=(FakeSingleThreadExecutor&&) = default; + + void Execute(const std::string& name, Runnable&& runnable) override; + + void SetRunExecutablesImmediately(bool run_executables_immediately) { + run_executables_immediately_ = run_executables_immediately; + } + + void RunAllExecutables(); + + private: + void DoShutdown(); + + bool run_executables_immediately_ = false; + std::vector> runnables_; +}; + +} // namespace nearby + +#endif // PLATFORM_PUBLIC_FAKE_SINGLE_THREAD_EXECUTOR_H_