From 3639085734ed6499db7f6e33025aa3ce3c9129ae Mon Sep 17 00:00:00 2001 From: "Andrii Kalinich (GitHub)" Date: Wed, 30 Sep 2020 14:29:42 -0400 Subject: Replace direct access with accessor for apps (#3487) There was found a few places inside of ApplicationManager where `applications_` and `apps_to_register_` members were used without locks however these members are commonly used and must be synchronized in all places. To avoid potential data races, direct usage of these elements were replaced with accessor for read-only operations. Also was added a missing lock. --- .../src/application_manager_impl.cc | 38 ++++++++++++---------- 1 file changed, 21 insertions(+), 17 deletions(-) (limited to 'src/components/application_manager/src/application_manager_impl.cc') diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc index df58871c80..2b6a8bc5e9 100644 --- a/src/components/application_manager/src/application_manager_impl.cc +++ b/src/components/application_manager/src/application_manager_impl.cc @@ -1218,10 +1218,13 @@ void ApplicationManagerImpl::CreatePendingApplication( application->set_hybrid_app_preference(hybrid_app_preference_enum); application->set_cloud_app_certificate(app_properties.certificate); - sync_primitives::AutoLock lock(apps_to_register_list_lock_ptr_); - SDL_LOG_DEBUG("apps_to_register_ size before: " << apps_to_register_.size()); - apps_to_register_.insert(application); - SDL_LOG_DEBUG("apps_to_register_ size after: " << apps_to_register_.size()); + { + sync_primitives::AutoLock lock(apps_to_register_list_lock_ptr_); + SDL_LOG_DEBUG( + "apps_to_register_ size before: " << apps_to_register_.size()); + apps_to_register_.insert(application); + SDL_LOG_DEBUG("apps_to_register_ size after: " << apps_to_register_.size()); + } SendUpdateAppList(); } @@ -1345,10 +1348,8 @@ void ApplicationManagerImpl::SetPendingApplicationState( app->app_id(), mobile_apis::Result::INVALID_ENUM, true, true); app->MarkUnregistered(); - { - sync_primitives::AutoLock lock(apps_to_register_list_lock_ptr_); - apps_to_register_.insert(app); - } + sync_primitives::AutoLock lock(apps_to_register_list_lock_ptr_); + apps_to_register_.insert(app); } void ApplicationManagerImpl::OnConnectionStatusUpdated() { @@ -1639,11 +1640,14 @@ void ApplicationManagerImpl::SendUpdateAppList() { (*request)[strings::msg_params][strings::applications] = SmartObject(SmartType_Array); - SmartObject& applications = + SmartObject& applications_so = (*request)[strings::msg_params][strings::applications]; - PrepareApplicationListSO(applications_, applications, *this); - PrepareApplicationListSO(apps_to_register_, applications, *this); + const auto applications_list = applications().GetData(); + PrepareApplicationListSO(applications_list, applications_so, *this); + + const auto pending_apps_list = AppsWaitingForRegistration().GetData(); + PrepareApplicationListSO(pending_apps_list, applications_so, *this); rpc_service_->ManageHMICommand(request); } @@ -1891,8 +1895,8 @@ bool ApplicationManagerImpl::StartNaviService( /* Fix: For NaviApp1 Switch to NaviApp2, App1's Endcallback() arrives later than App2's Startcallback(). Cause streaming issue on HMI. */ - sync_primitives::AutoLock lock(applications_list_lock_ptr_); - for (auto app : applications_) { + auto accessor = applications(); + for (auto app : accessor.GetData()) { if (!app || (!app->is_navi() && !app->mobile_projection_enabled())) { SDL_LOG_DEBUG("Continue, Not Navi App Id: " << app->app_id()); continue; @@ -2862,6 +2866,7 @@ void ApplicationManagerImpl::ProcessQueryApp( CreateApplications(*obj_array, connection_key); SendUpdateAppList(); + sync_primitives::AutoLock lock(apps_to_register_list_lock_ptr_); AppsWaitRegistrationSet::const_iterator it = apps_to_register_.begin(); for (; it != apps_to_register_.end(); ++it) { const std::string full_icon_path((*it)->app_icon_path()); @@ -3298,7 +3303,8 @@ void ApplicationManagerImpl::UnregisterApplication( plugin_manager_->ForEachPlugin(on_app_unregistered); request_ctrl_.terminateAppRequests(app_id); - if (applications_.empty()) { + const bool is_applications_list_empty = applications().GetData().empty(); + if (is_applications_list_empty) { policy_handler_->StopRetrySequence(); } return; @@ -4104,9 +4110,7 @@ void ApplicationManagerImpl::OnApplicationListUpdateTimer() { const bool is_new_app_registered = registered_during_timer_execution_; registered_during_timer_execution_ = false; - apps_to_register_list_lock_ptr_->Acquire(); - const bool trigger_ptu = apps_size_ != applications_.size(); - apps_to_register_list_lock_ptr_->Release(); + const bool trigger_ptu = apps_size_ != applications().GetData().size(); if (is_new_app_registered) { SendUpdateAppList(); -- cgit v1.2.1 From efb3c1d5ecc3688db65110d3a3c10560d249f33e Mon Sep 17 00:00:00 2001 From: "Andrii Kalinich (GitHub)" Date: Wed, 30 Sep 2020 15:09:05 -0400 Subject: Use atomic bool in ApplicatinManager (#3491) Replace bool flag 'is_stopping_' in class ApplicationManagerImpl and remove mutex 'stopping_application_mng_lock_'. --- .../application_manager/src/application_manager_impl.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) (limited to 'src/components/application_manager/src/application_manager_impl.cc') diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc index 2b6a8bc5e9..1823458ce0 100644 --- a/src/components/application_manager/src/application_manager_impl.cc +++ b/src/components/application_manager/src/application_manager_impl.cc @@ -229,7 +229,7 @@ ApplicationManagerImpl::ApplicationManagerImpl( ApplicationManagerImpl::~ApplicationManagerImpl() { SDL_LOG_AUTO_TRACE(); - is_stopping_ = true; + is_stopping_.store(true); SendOnSDLClose(); media_manager_ = NULL; hmi_handler_ = NULL; @@ -2538,9 +2538,7 @@ bool ApplicationManagerImpl::Init( bool ApplicationManagerImpl::Stop() { SDL_LOG_AUTO_TRACE(); - stopping_application_mng_lock_.Acquire(); - is_stopping_ = true; - stopping_application_mng_lock_.Release(); + is_stopping_.store(true); application_list_update_timer_.Stop(); try { if (unregister_reason_ == @@ -2965,9 +2963,7 @@ void ApplicationManagerImpl::SetUnregisterAllApplicationsReason( void ApplicationManagerImpl::HeadUnitReset( mobile_api::AppInterfaceUnregisteredReason::eType reason) { SDL_LOG_AUTO_TRACE(); - stopping_application_mng_lock_.Acquire(); - is_stopping_ = true; - stopping_application_mng_lock_.Release(); + is_stopping_.store(true); switch (reason) { case mobile_api::AppInterfaceUnregisteredReason::MASTER_RESET: { SDL_LOG_TRACE("Performing MASTER_RESET"); @@ -3374,7 +3370,6 @@ mobile_apis::Result::eType ApplicationManagerImpl::CheckPolicyPermissions( } bool ApplicationManagerImpl::is_stopping() const { - sync_primitives::AutoLock lock(stopping_application_mng_lock_); return is_stopping_; } -- cgit v1.2.1 From 01c9705cf408e010bd9f98c5230347975da69c4a Mon Sep 17 00:00:00 2001 From: "Yana Chernysheva (GitHub)" <59469418+ychernysheva@users.noreply.github.com> Date: Thu, 1 Oct 2020 18:24:00 +0300 Subject: Fix Cppcheck issues (#3453) * Fix noExplicitConstructor issue * Fix functionConst issues * Fix unusedFunction and unusedField issues * Fix redundantInitialization * Fix unreadVariable and unusedVariable issues * Fix postfixOperator issue * Fix variableScope issue * Fix invalidPrintfArgType_sint and unsignedLessThanZero issues * Fix other errors * Add changes, related to functions marked as unused * Fix new issues * Fixe review comment * Fix codestyle * Fix constParameter errors * Fix functionConst errors * Fix noExplicitConstructor, redundantInitialization errors * Fix unreadVariable errors * Fix shadowVariable error * Fix useStlAlgorithm errors * Fixe variableScope error, add FIXME comment * Fix code style * Fix compile error * Remove unsued function * Fix compile error Co-authored-by: Vladislav Semenyuk --- .../src/application_manager_impl.cc | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) (limited to 'src/components/application_manager/src/application_manager_impl.cc') diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc index 1823458ce0..90855fa9ee 100644 --- a/src/components/application_manager/src/application_manager_impl.cc +++ b/src/components/application_manager/src/application_manager_impl.cc @@ -693,9 +693,9 @@ ApplicationSharedPtr ApplicationManagerImpl::RegisterApplication( bool is_mismatched_cloud_app = false; if (apps_to_register_.end() == it) { - DevicePredicate finder(application->device()); + DevicePredicate device_finder(application->device()); it = std::find_if( - apps_to_register_.begin(), apps_to_register_.end(), finder); + apps_to_register_.begin(), apps_to_register_.end(), device_finder); bool found = apps_to_register_.end() != it; is_mismatched_cloud_app = found && (*it)->is_cloud_app() && @@ -3083,11 +3083,11 @@ void ApplicationManagerImpl::UnregisterAllApplications() { SDL_LOG_DEBUG("Unregister reason " << unregister_reason_); SetHMICooperating(false); - bool is_ignition_off = false; + using namespace mobile_api::AppInterfaceUnregisteredReason; using namespace helpers; - is_ignition_off = + bool is_ignition_off = Compare(unregister_reason_, IGNITION_OFF, INVALID_ENUM); bool is_unexpected_disconnect = Compare( @@ -3846,9 +3846,7 @@ bool ApplicationManagerImpl::ResetVrHelpTitleItems( } const std::string& vr_help_title = get_settings().vr_help_title(); - smart_objects::SmartObject so_vr_help_title = - smart_objects::SmartObject(smart_objects::SmartType_String); - so_vr_help_title = vr_help_title; + smart_objects::SmartObject so_vr_help_title(vr_help_title); app->reset_vr_help_title(); app->reset_vr_help(); @@ -4380,7 +4378,7 @@ void ApplicationManagerImpl::OnUpdateHMIAppType( std::vector hmi_types_from_policy; smart_objects::SmartObject transform_app_hmi_types( smart_objects::SmartType_Array); - bool flag_diffirence_app_hmi_type = false; + bool flag_diffirence_app_hmi_type; DataAccessor accessor(applications()); for (ApplicationSetIt it = accessor.GetData().begin(); it != accessor.GetData().end(); @@ -4389,7 +4387,6 @@ void ApplicationManagerImpl::OnUpdateHMIAppType( if (it_app_hmi_types_from_policy != app_hmi_types.end() && ((it_app_hmi_types_from_policy->second).size())) { - flag_diffirence_app_hmi_type = false; hmi_types_from_policy = (it_app_hmi_types_from_policy->second); if (transform_app_hmi_types.length()) { @@ -4611,7 +4608,6 @@ void ApplicationManagerImpl::SendGetIconUrlNotifications( continue; } - std::string endpoint = app_icon_it->second.endpoint; bool pending_request = app_icon_it->second.pending_request; if (pending_request) { @@ -4942,7 +4938,7 @@ void ApplicationManagerImpl::SetMockMediaManager( #endif // BUILD_TESTS struct MobileAppIdPredicate { std::string policy_app_id_; - MobileAppIdPredicate(const std::string& policy_app_id) + explicit MobileAppIdPredicate(const std::string& policy_app_id) : policy_app_id_(policy_app_id) {} bool operator()(const ApplicationSharedPtr app) const { return app ? policy_app_id_ == app->policy_app_id() : false; @@ -4951,7 +4947,8 @@ struct MobileAppIdPredicate { struct TakeDeviceHandle { public: - TakeDeviceHandle(const ApplicationManager& app_mngr) : app_mngr_(app_mngr) {} + explicit TakeDeviceHandle(const ApplicationManager& app_mngr) + : app_mngr_(app_mngr) {} std::string operator()(ApplicationSharedPtr& app) { DCHECK_OR_RETURN(app, ""); return MessageHelper::GetDeviceMacAddressForHandle(app->device(), -- cgit v1.2.1