diff --git a/sharing/common/nearby_share_prefs.cc b/sharing/common/nearby_share_prefs.cc index 3ce0985a..81392a12 100644 --- a/sharing/common/nearby_share_prefs.cc +++ b/sharing/common/nearby_share_prefs.cc @@ -44,6 +44,8 @@ ABSL_CONST_INIT const char "nearby_sharing.background_visibility_expiration_seconds"; ABSL_CONST_INIT const char kNearbySharingContactUploadHashName[] = "nearby_sharing.contact_upload_hash"; +ABSL_CONST_INIT const char kNearbySharingContactUploadTimeName[] = + "nearby_sharing.contact_upload_time"; ABSL_CONST_INIT const char kNearbySharingCustomSavePath[] = "nearby_sharing.custom_save_path"; ABSL_CONST_INIT const char kNearbySharingDataUsageName[] = @@ -72,8 +74,6 @@ ABSL_CONST_INIT const char kNearbySharingSchedulerDownloadDeviceDataName[] = ABSL_CONST_INIT const char kNearbySharingSchedulerDownloadPublicCertificatesName[] = "nearby_sharing.scheduler.download_public_certificates"; -ABSL_CONST_INIT const char kNearbySharingSchedulerPeriodicContactUploadName[] = - "nearby_sharing.scheduler.periodic_contact_upload"; ABSL_CONST_INIT const char kNearbySharingSchedulerPrivateCertificateExpirationName[] = "nearby_sharing.scheduler.private_certificate_expiration"; @@ -140,7 +140,6 @@ void RegisterNearbySharingPrefs(PreferenceManager& preference_manager, preference_manager.Remove(kNearbySharingSchedulerDownloadDeviceDataName); preference_manager.Remove( kNearbySharingSchedulerDownloadPublicCertificatesName); - preference_manager.Remove(kNearbySharingSchedulerPeriodicContactUploadName); preference_manager.Remove( kNearbySharingSchedulerPrivateCertificateExpirationName); preference_manager.Remove( @@ -159,7 +158,6 @@ void ResetSchedulers(PreferenceManager& preference_manager) { preference_manager.Remove(kNearbySharingSchedulerDownloadDeviceDataName); preference_manager.Remove( kNearbySharingSchedulerDownloadPublicCertificatesName); - preference_manager.Remove(kNearbySharingSchedulerPeriodicContactUploadName); preference_manager.Remove( kNearbySharingSchedulerPrivateCertificateExpirationName); preference_manager.Remove( diff --git a/sharing/common/nearby_share_prefs.h b/sharing/common/nearby_share_prefs.h index d99b9272..ee745869 100644 --- a/sharing/common/nearby_share_prefs.h +++ b/sharing/common/nearby_share_prefs.h @@ -29,6 +29,7 @@ ABSL_CONST_INIT extern const char ABSL_CONST_INIT extern const char kNearbySharingBackgroundVisibilityExpirationSeconds[]; ABSL_CONST_INIT extern const char kNearbySharingContactUploadHashName[]; +ABSL_CONST_INIT extern const char kNearbySharingContactUploadTimeName[]; ABSL_CONST_INIT extern const char kNearbySharingCustomSavePath[]; ABSL_CONST_INIT extern const char kNearbySharingDataUsageName[]; ABSL_CONST_INIT extern const char kNearbySharingDeviceIdName[]; @@ -47,8 +48,6 @@ ABSL_CONST_INIT extern const char kNearbySharingSchedulerDownloadDeviceDataName[]; ABSL_CONST_INIT extern const char kNearbySharingSchedulerDownloadPublicCertificatesName[]; -ABSL_CONST_INIT extern const char - kNearbySharingSchedulerPeriodicContactUploadName[]; ABSL_CONST_INIT extern const char kNearbySharingSchedulerPrivateCertificateExpirationName[]; ABSL_CONST_INIT extern const char diff --git a/sharing/contacts/nearby_share_contact_manager_impl.cc b/sharing/contacts/nearby_share_contact_manager_impl.cc index b478379f..5688e2ee 100644 --- a/sharing/contacts/nearby_share_contact_manager_impl.cc +++ b/sharing/contacts/nearby_share_contact_manager_impl.cc @@ -31,6 +31,7 @@ #include "absl/synchronization/notification.h" #include "absl/time/time.h" #include "internal/crypto_cros/secure_hash.h" +#include "internal/platform/clock.h" #include "internal/platform/implementation/account_manager.h" #include "sharing/common/nearby_share_prefs.h" #include "sharing/contacts/nearby_share_contact_manager.h" @@ -57,8 +58,8 @@ using ::nearby::sharing::proto::ContactRecord; using ::nearby::sharing::proto::ListContactPeopleRequest; using ::nearby::sharing::proto::ListContactPeopleResponse; -constexpr absl::Duration kContactUploadPeriod = absl::Hours(24); constexpr absl::Duration kContactDownloadPeriod = absl::Hours(12); +constexpr absl::Duration kMaxContactUploadInterval = absl::Hours(72); // Converts a list of ContactRecord protos, along with the allowlist, into a // list of Contact protos. @@ -160,16 +161,10 @@ NearbyShareContactManagerImpl::NearbyShareContactManagerImpl( NearbyShareLocalDeviceDataManager* local_device_data_manager) : preference_manager_(preference_manager), account_manager_(account_manager), + clock_(context->GetClock()), nearby_client_factory_(nearby_client_factory), nearby_share_client_(nearby_client_factory_->CreateInstance()), local_device_data_manager_(local_device_data_manager), - periodic_contact_upload_scheduler_( - NearbyShareSchedulerFactory::CreatePeriodicScheduler( - context, preference_manager_, kContactUploadPeriod, - /*retry_failures=*/false, - /*require_connectivity=*/true, - prefs::kNearbySharingSchedulerPeriodicContactUploadName, - [&] { OnPeriodicContactsUploadRequested(); })), contact_download_and_upload_scheduler_( NearbyShareSchedulerFactory::CreatePeriodicScheduler( context, preference_manager_, kContactDownloadPeriod, @@ -292,21 +287,13 @@ void NearbyShareContactManagerImpl::GetContacts(ContactsCallback callback) { } void NearbyShareContactManagerImpl::OnStart() { - periodic_contact_upload_scheduler_->Start(); contact_download_and_upload_scheduler_->Start(); } void NearbyShareContactManagerImpl::OnStop() { - periodic_contact_upload_scheduler_->Stop(); contact_download_and_upload_scheduler_->Stop(); } -void NearbyShareContactManagerImpl::OnPeriodicContactsUploadRequested() { - NL_VLOG(1) << __func__ - << ": Periodic Nearby Share contacts upload requested. " - << "Upload will occur after next contacts download."; -} - void NearbyShareContactManagerImpl::OnContactsDownloadSuccess( std::vector contacts, uint32_t num_unreachable_contacts_filtered_out) { @@ -336,19 +323,21 @@ void NearbyShareContactManagerImpl::OnContactsDownloadSuccess( std::string last_contact_upload_hash = preference_manager_.GetString( prefs::kNearbySharingContactUploadHashName, ""); + int64_t last_contact_upload_time = preference_manager_.GetInt64( + prefs::kNearbySharingContactUploadTimeName, 0); + absl::Time now = clock_->Now(); std::string contact_upload_hash = ComputeHash(contacts_to_upload); bool did_contacts_change_since_last_upload = - contact_upload_hash != last_contact_upload_hash; - - if (did_contacts_change_since_last_upload) { - NL_VLOG(1) << __func__ << ": Contact list or allowlist changed since last " - << "successful upload to the Nearby Share server."; - } + (contact_upload_hash != last_contact_upload_hash) || + (now - absl::FromUnixSeconds(last_contact_upload_time) >= + kMaxContactUploadInterval); // Request a contacts upload if the contact list or allowlist has changed - // since the last successful upload. Also request an upload periodically. - if (did_contacts_change_since_last_upload || - periodic_contact_upload_scheduler_->IsWaitingForResult()) { + // since the last successful upload or max upload interval has passed. + if (did_contacts_change_since_last_upload) { + NL_LOG(INFO) << __func__ << ": Contact list changed since last " + << "successful upload at " + << absl::FromUnixSeconds(last_contact_upload_time); absl::Notification notification; bool upload_success = false; local_device_data_manager_->UploadContacts(std::move(contacts_to_upload), @@ -362,7 +351,7 @@ void NearbyShareContactManagerImpl::OnContactsDownloadSuccess( << upload_success; OnContactsUploadFinished(did_contacts_change_since_last_upload, - contact_upload_hash, upload_success); + contact_upload_hash, now, upload_success); return; } @@ -378,24 +367,19 @@ void NearbyShareContactManagerImpl::OnContactsDownloadFailure() { void NearbyShareContactManagerImpl::OnContactsUploadFinished( bool did_contacts_change_since_last_upload, - absl::string_view contact_upload_hash, bool success) { + absl::string_view contact_upload_hash, absl::Time upload_time, + bool success) { NL_LOG(INFO) << __func__ << ": Upload of contacts to Nearby Share server " << (success ? "succeeded." : "failed.") << " Contact upload hash: " << contact_upload_hash; if (success) { - // Only resolve the periodic upload request on success; let the - // download-and-upload scheduler handle any failure retries. The periodic - // upload scheduler will remember that it has an outstanding request even - // after reboot. - if (periodic_contact_upload_scheduler_->IsWaitingForResult()) { - periodic_contact_upload_scheduler_->HandleResult(success); - } - std::string last_contact_upload_hash = preference_manager_.GetString( prefs::kNearbySharingContactUploadHashName, ""); preference_manager_.SetString(prefs::kNearbySharingContactUploadHashName, contact_upload_hash); + preference_manager_.SetInt64(prefs::kNearbySharingContactUploadTimeName, + absl::ToUnixSeconds(upload_time)); if (last_contact_upload_hash.empty()) { // If no contacts are uploaded before, set the flag to false in order to @@ -411,7 +395,6 @@ void NearbyShareContactManagerImpl::OnContactsUploadFinished( contact_download_and_upload_scheduler_->HandleResult(success); } - void NearbyShareContactManagerImpl::NotifyAllObserversContactsDownloaded( const std::vector& contacts, uint32_t num_unreachable_contacts_filtered_out) { diff --git a/sharing/contacts/nearby_share_contact_manager_impl.h b/sharing/contacts/nearby_share_contact_manager_impl.h index d5f45d06..7bf9d329 100644 --- a/sharing/contacts/nearby_share_contact_manager_impl.h +++ b/sharing/contacts/nearby_share_contact_manager_impl.h @@ -19,14 +19,14 @@ #include #include -#include #include #include #include -#include "absl/functional/any_invocable.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" +#include "absl/time/time.h" +#include "internal/platform/clock.h" #include "internal/platform/implementation/account_manager.h" #include "internal/platform/task_runner.h" #include "sharing/contacts/nearby_share_contact_manager.h" @@ -130,9 +130,9 @@ class NearbyShareContactManagerImpl : public NearbyShareContactManager { std::vector<::nearby::sharing::proto::ContactRecord> contacts, uint32_t num_unreachable_contacts_filtered_out); void OnContactsDownloadFailure(); - void OnPeriodicContactsUploadRequested(); void OnContactsUploadFinished(bool did_contacts_change_since_last_upload, absl::string_view contact_upload_hash, + absl::Time upload_time, bool success); // Notify the base-class and mojo observers that contacts were downloaded. @@ -142,10 +142,10 @@ class NearbyShareContactManagerImpl : public NearbyShareContactManager { nearby::sharing::api::PreferenceManager& preference_manager_; AccountManager& account_manager_; + Clock* const clock_; nearby::sharing::api::SharingRpcClientFactory* const nearby_client_factory_; std::unique_ptr nearby_share_client_; NearbyShareLocalDeviceDataManager* local_device_data_manager_ = nullptr; - std::unique_ptr periodic_contact_upload_scheduler_; std::unique_ptr contact_download_and_upload_scheduler_; std::unique_ptr executor_ = nullptr; diff --git a/sharing/contacts/nearby_share_contact_manager_impl_test.cc b/sharing/contacts/nearby_share_contact_manager_impl_test.cc index bf17b29c..a55419a7 100644 --- a/sharing/contacts/nearby_share_contact_manager_impl_test.cc +++ b/sharing/contacts/nearby_share_contact_manager_impl_test.cc @@ -242,8 +242,6 @@ class NearbyShareContactManagerImplTest size_t num_upload_notifications = contacts_uploaded_notifications_.size(); size_t num_download_and_upload_handled_results = download_and_upload_scheduler()->handled_results().size(); - size_t num_periodic_upload_handled_results = - periodic_upload_scheduler()->handled_results().size(); manager_->DownloadContacts(); Sync(); @@ -285,24 +283,6 @@ class NearbyShareContactManagerImplTest // Verify upload notification was sent on success. EXPECT_EQ(contacts_uploaded_notifications_.size(), num_upload_notifications + (upload_success ? 1 : 0)); - if (upload_success) { - // We only expect uploads to occur if contacts have changed since the - // last - // upload or if a periodic upload was requested. - EXPECT_TRUE(contacts_uploaded_notifications_.back() - .did_contacts_change_since_last_upload || - periodic_upload_scheduler()->IsWaitingForResult()); - - if (periodic_upload_scheduler()->IsWaitingForResult()) { - EXPECT_EQ(periodic_upload_scheduler()->handled_results().size(), - num_periodic_upload_handled_results + 1); - EXPECT_TRUE(periodic_upload_scheduler()->handled_results().back()); - periodic_upload_scheduler()->SetIsWaitingForResult(false); - } else { - EXPECT_EQ(periodic_upload_scheduler()->handled_results().size(), - num_periodic_upload_handled_results); - } - } // Verify that the result is sent to download/upload scheduler. EXPECT_EQ(download_and_upload_scheduler()->handled_results().size(), num_download_and_upload_handled_results + 1); @@ -315,12 +295,6 @@ class NearbyShareContactManagerImplTest } } - void MakePeriodicUploadRequest() { - periodic_upload_scheduler()->InvokeRequestCallback(); - periodic_upload_scheduler()->SetIsWaitingForResult(true); - Sync(); - } - PreferenceManager& preference_manager() { return preference_manager_; } std::vector& @@ -328,6 +302,8 @@ class NearbyShareContactManagerImplTest return contacts_downloaded_notifications_; } + FakeContext& fake_context() { return fake_context_; } + private: // NearbyShareContactManager::Observer: void OnContactsDownloaded( @@ -350,12 +326,6 @@ class NearbyShareContactManagerImplTest return nearby_client_factory_.instances().back(); } - FakeNearbyShareScheduler* periodic_upload_scheduler() { - return scheduler_factory_.pref_name_to_periodic_instance() - .at(prefs::kNearbySharingSchedulerPeriodicContactUploadName) - .fake_scheduler; - } - FakeNearbyShareScheduler* download_and_upload_scheduler() { return scheduler_factory_.pref_name_to_periodic_instance() .at(prefs::kNearbySharingSchedulerContactDownloadAndUploadName) @@ -373,16 +343,6 @@ class NearbyShareContactManagerImplTest kContactDownloadPeriod); EXPECT_TRUE(download_and_upload_scheduler_instance.retry_failures); EXPECT_TRUE(download_and_upload_scheduler_instance.require_connectivity); - - FakeNearbyShareSchedulerFactory::PeriodicInstance - periodic_upload_scheduler_instance = - scheduler_factory_.pref_name_to_periodic_instance().at( - prefs::kNearbySharingSchedulerPeriodicContactUploadName); - EXPECT_TRUE(periodic_upload_scheduler_instance.fake_scheduler); - EXPECT_EQ(periodic_upload_scheduler_instance.request_period, - kContactUploadPeriod); - EXPECT_FALSE(periodic_upload_scheduler_instance.retry_failures); - EXPECT_TRUE(periodic_upload_scheduler_instance.require_connectivity); } void TriggerDownloadScheduler() { @@ -460,7 +420,7 @@ TEST_F(NearbyShareContactManagerImplTest, } TEST_F(NearbyShareContactManagerImplTest, - DownloadContacts_PeriodicUploadRequest) { + DownloadContacts_HashExpiration) { std::vector contact_records = TestContactRecordList(/*num_contacts=*/3u); std::set allowlist = TestContactIds(/*num_contacts=*/2u); @@ -474,18 +434,44 @@ TEST_F(NearbyShareContactManagerImplTest, /*upload_success=*/true, /*contacts=*/contact_records); - // Because device records on the server will be removed after a few days if - // the device does not contact the server, we ensure that contacts are - // uploaded periodically. Make that request now. Contacts will be uploaded - // after the next contact download. It will not force a download now, - // however. - MakePeriodicUploadRequest(); + fake_context().fake_clock()->FastForward(absl::Hours(72)); + SetDownloadSuccessResult(contact_records); + SetUploadResult(true); + // When contacts are downloaded again, we detect that contacts have not + // changed, but since the hash expired, we upload again. + DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, + /*upload_success=*/true, + /*contacts=*/contact_records); +} + +TEST_F(NearbyShareContactManagerImplTest, + DownloadContacts_HashExpirationUploadFailed) { + std::vector contact_records = + TestContactRecordList(/*num_contacts=*/3u); + std::set allowlist = TestContactIds(/*num_contacts=*/2u); + + SetDownloadSuccessResult(contact_records); + SetUploadResult(true); + + // Because contacts have never been uploaded, a subsequent upload is + // requested, which succeeds. + DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, + /*upload_success=*/true, + /*contacts=*/contact_records); + + fake_context().fake_clock()->FastForward(absl::Hours(72)); + SetDownloadSuccessResult(contact_records); + SetUploadResult(false); + // When contacts are downloaded again, we detect that contacts have not + // changed, but since the hash expired, we upload again. + DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, + /*upload_success=*/false, + /*contacts=*/contact_records); SetDownloadSuccessResult(contact_records); SetUploadResult(true); // When contacts are downloaded again, we detect that contacts have not - // changed. However, we expect an upload because a periodic request was - // made. + // changed, last upload failed, we upload again. DownloadContacts(/*download_success=*/true, /*expect_upload=*/true, /*upload_success=*/true, /*contacts=*/contact_records); diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index 52878516..885cdbf0 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -1278,11 +1278,6 @@ std::string NearbySharingServiceImpl::Dump() const { preference_manager_, prefs::kNearbySharingSchedulerDownloadPublicCertificatesName) << std::endl; - sstream << " Upload contacts periodically: " - << ConvertToReadableSchedule( - preference_manager_, - prefs::kNearbySharingSchedulerPeriodicContactUploadName) - << std::endl; sstream << " Upload local device certificates: " << ConvertToReadableSchedule(