From dbb573689e674ce91da47c5dd8a916838148dde9 Mon Sep 17 00:00:00 2001 From: Collin Date: Tue, 9 Nov 2021 12:27:50 -0500 Subject: Do not modify application list while iterating it (#3806) * do not modify applications array while iterating it * remove unnecessary and dangerous iterator++ * fixup! do not modify applications array while iterating it restore sending on language change to mobile * deref app ptr while holding app list lock * SendNotificationToMobile can also invalidate apps list iterator by calling unregister application * apply fix to TTS and VR on language change * Revert "Fix early IsRegistered() state of application (#3796)" This reverts commit 0ed38e8d2f7cb3a61a303cca2806a06ce9a1d58d. * revert #3799 * prevent crash with null app in rai response * do not deactivate app before unregistering in case of vr language change * make copy of ApplicationSet instead of pushing app_ids to vector * address failure of 3797_3798 atf test in extprop policy mode * fixup! address failure of 3797_3798 atf test in extprop policy mode only send NO_APPS_REGISTERED when no apps are registered * fix ext prop unit test and APPLICATION_NOT_REGISTERED log message * fixup! fix ext prop unit test and APPLICATION_NOT_REGISTERED log message fix style * simplify code copying applicationset --- .../hmi/on_tts_language_change_notification.cc | 16 ++--- .../hmi/on_ui_language_change_notification.cc | 15 ++--- .../hmi/on_vr_language_change_notification.cc | 22 ++----- .../src/commands/hmi/sdl_activate_app_request.cc | 68 +++++++++------------- .../mobile/register_app_interface_request.cc | 3 +- .../mobile/register_app_interface_response.cc | 2 +- .../test/commands/hmi/hmi_notifications_test.cc | 10 ---- .../commands/hmi/sdl_activate_app_request_test.cc | 24 ++++---- .../src/application_manager_impl.cc | 5 +- .../src/message_helper/message_helper.cc | 2 +- 10 files changed, 58 insertions(+), 109 deletions(-) diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_tts_language_change_notification.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_tts_language_change_notification.cc index d693c00ac3..51df69d0bc 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_tts_language_change_notification.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_tts_language_change_notification.cc @@ -88,20 +88,14 @@ void OnTTSLanguageChangeNotification::Run() { (*message_)[strings::params][strings::function_id] = static_cast(mobile_apis::FunctionID::OnLanguageChangeID); - const auto applications = application_manager_.applications().GetData(); - for (const auto& app : applications) { - if (!app->IsRegistered()) { - SDL_LOG_DEBUG("Skipping app " - << app->app_id() - << " which has not finished the registration process"); - continue; - } + auto apps = ApplicationSet(application_manager_.applications().GetData()); + auto message_language = + (*message_)[strings::msg_params][strings::language].asInt(); + for (auto app : apps) { (*message_)[strings::params][strings::connection_key] = app->app_id(); SendNotificationToMobile(message_); - - if (static_cast(app->language()) != - (*message_)[strings::msg_params][strings::language].asInt()) { + if (app->language() != message_language) { rpc_service_.ManageMobileCommand( MessageHelper::GetOnAppInterfaceUnregisteredNotificationToMobile( app->app_id(), diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_ui_language_change_notification.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_ui_language_change_notification.cc index 7f6ffaa4eb..8da7da28e7 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_ui_language_change_notification.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_ui_language_change_notification.cc @@ -80,19 +80,14 @@ void OnUILanguageChangeNotification::Run() { (*message_)[strings::params][strings::function_id] = static_cast(mobile_apis::FunctionID::OnLanguageChangeID); - const ApplicationSet& accessor = - application_manager_.applications().GetData(); + auto apps = ApplicationSet(application_manager_.applications().GetData()); - ApplicationSetConstIt it = accessor.begin(); - for (; accessor.end() != it;) { - ApplicationSharedPtr app = *it; - ++it; + auto message_language = + (*message_)[strings::msg_params][strings::hmi_display_language].asInt(); + for (auto app : apps) { (*message_)[strings::params][strings::connection_key] = app->app_id(); SendNotificationToMobile(message_); - - if (app->ui_language() != - (*message_)[strings::msg_params][strings::hmi_display_language] - .asInt()) { + if (app->ui_language() != message_language) { rpc_service_.ManageMobileCommand( MessageHelper::GetOnAppInterfaceUnregisteredNotificationToMobile( app->app_id(), diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_vr_language_change_notification.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_vr_language_change_notification.cc index 23c1ec1b80..9cb4653548 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_vr_language_change_notification.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/on_vr_language_change_notification.cc @@ -78,26 +78,14 @@ void OnVRLanguageChangeNotification::Run() { (*message_)[strings::params][strings::function_id] = static_cast(mobile_apis::FunctionID::OnLanguageChangeID); - const auto applications = application_manager_.applications().GetData(); - for (auto app : applications) { - if (!app->IsRegistered()) { - SDL_LOG_DEBUG("Skipping app " - << app->app_id() - << " which has not finished the registration process"); - continue; - } + auto apps = ApplicationSet(application_manager_.applications().GetData()); + auto message_language = + (*message_)[strings::msg_params][strings::language].asInt(); + for (auto app : apps) { (*message_)[strings::params][strings::connection_key] = app->app_id(); SendNotificationToMobile(message_); - - if (static_cast(app->language()) != - (*message_)[strings::msg_params][strings::language].asInt()) { - application_manager_.state_controller().SetRegularState( - app, - mobile_apis::PredefinedWindows::DEFAULT_WINDOW, - mobile_apis::HMILevel::HMI_NONE, - false); - + if (app->language() != message_language) { rpc_service_.ManageMobileCommand( MessageHelper::GetOnAppInterfaceUnregisteredNotificationToMobile( app->app_id(), diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/sdl_activate_app_request.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/sdl_activate_app_request.cc index e25ee93400..d79a397651 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/sdl_activate_app_request.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/sdl_activate_app_request.cc @@ -136,10 +136,27 @@ void SDLActivateAppRequest::Run() { static_cast(function_id()), hmi_apis::Common_Result::REJECTED, "HMIDeactivate is active"); - return; - } - - if (app->app_id() > 0 || app->IsRegistered()) { + } else if (!app->IsRegistered()) { + if (app->is_cloud_app()) { + SDL_LOG_DEBUG("Starting cloud application."); + const ApplicationManagerSettings& settings = + application_manager_.get_settings(); + uint32_t total_retry_timeout = (settings.cloud_app_retry_timeout() * + settings.cloud_app_max_retry_attempts()); + application_manager_.UpdateRequestTimeout( + 0, correlation_id(), default_timeout_ + total_retry_timeout); + subscribe_on_event(BasicCommunication_OnAppRegistered); + application_manager_.connection_handler().ConnectToDevice(app->device()); + } else { + SDL_LOG_ERROR( + "Can't find registered app with the specified app id: " << app_id()); + SendErrorResponse(correlation_id(), + SDL_ActivateApp, + hmi_apis::Common_Result::APPLICATION_NOT_REGISTERED, + "Application is not registered"); + } + } else { + const uint32_t application_id = app_id(); auto main_state = app->CurrentHmiState(mobile_apis::PredefinedWindows::DEFAULT_WINDOW); if (mobile_apis::HMILevel::INVALID_ENUM == main_state->hmi_level()) { @@ -148,40 +165,12 @@ void SDLActivateAppRequest::Run() { "yet, postpone activation"); auto& postponed_activation_ctrl = application_manager_.state_controller() .GetPostponedActivationController(); - postponed_activation_ctrl.AddAppToActivate(app->app_id(), + postponed_activation_ctrl.AddAppToActivate(application_id, correlation_id()); return; } - } - - const uint32_t application_id = app_id(); - if (app->IsRegistered()) { - SDL_LOG_DEBUG("Application is registered. Activating."); policy_handler_.OnActivateApp(application_id, correlation_id()); - return; - } - - if (app->is_cloud_app()) { - SDL_LOG_DEBUG("Starting cloud application."); - const ApplicationManagerSettings& settings = - application_manager_.get_settings(); - uint32_t total_retry_timeout = (settings.cloud_app_retry_timeout() * - settings.cloud_app_max_retry_attempts()); - application_manager_.UpdateRequestTimeout( - 0, correlation_id(), default_timeout_ + total_retry_timeout); - subscribe_on_event(BasicCommunication_OnAppRegistered); - application_manager_.connection_handler().ConnectToDevice(app->device()); - return; } - - connection_handler::DeviceHandle device_handle = app->device(); - SDL_LOG_ERROR( - "Can't find regular foreground app with the same connection id: " - << device_handle); - SendErrorResponse(correlation_id(), - SDL_ActivateApp, - hmi_apis::Common_Result::NO_APPS_REGISTERED, - ""); } #else // EXTERNAL_PROPRIETARY_MODE @@ -225,7 +214,8 @@ void SDLActivateAppRequest::Run() { return; } - if (app_to_activate->app_id() > 0 || app_to_activate->IsRegistered()) { + if (app_to_activate->IsRegistered()) { + SDL_LOG_DEBUG("Application is registered. Activating."); auto main_state = app_to_activate->CurrentHmiState( mobile_apis::PredefinedWindows::DEFAULT_WINDOW); if (mobile_apis::HMILevel::INVALID_ENUM == main_state->hmi_level()) { @@ -234,19 +224,13 @@ void SDLActivateAppRequest::Run() { "yet, postpone activation"); auto& postponed_activation_ctrl = application_manager_.state_controller() .GetPostponedActivationController(); - postponed_activation_ctrl.AddAppToActivate(app_to_activate->app_id(), + postponed_activation_ctrl.AddAppToActivate(application_id, correlation_id()); return; } - } - - if (app_to_activate->IsRegistered()) { - SDL_LOG_DEBUG("Application is registered. Activating."); policy_handler_.OnActivateApp(application_id, correlation_id()); return; - } - - if (app_to_activate->is_cloud_app()) { + } else if (app_to_activate->is_cloud_app()) { SDL_LOG_DEBUG("Starting cloud application."); const ApplicationManagerSettings& settings = application_manager_.get_settings(); diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/register_app_interface_request.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/register_app_interface_request.cc index 9a9aa0c533..2224f17c4e 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/register_app_interface_request.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/register_app_interface_request.cc @@ -440,8 +440,6 @@ void FinishSendingResponseToMobile(const smart_objects::SmartObject& msg_params, &(msg_params[strings::app_hmi_type])); } - application->MarkRegistered(); - // Default HMI level should be set before any permissions validation, since // it relies on HMI level. app_manager.OnApplicationRegistered(application); @@ -734,6 +732,7 @@ void RegisterAppInterfaceRequest::Run() { } CheckLanguage(); + SendRegisterAppInterfaceResponseToMobile( ApplicationType::kNewApplication, status_notifier, add_info); } diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/register_app_interface_response.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/register_app_interface_response.cc index 0c74105e56..6e751a9b91 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/register_app_interface_response.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/register_app_interface_response.cc @@ -96,7 +96,7 @@ void RegisterAppInterfaceResponse::Run() { } SendResponse(success, result_code, last_message); - if (success) { + if (app && success) { app->set_is_ready(true); } event_engine::MobileEvent event( diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/hmi_notifications_test.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/hmi_notifications_test.cc index 53b1fdfc67..b4d34e180e 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/hmi_notifications_test.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/hmi_notifications_test.cc @@ -1368,7 +1368,6 @@ TEST_F(HMICommandsNotificationsTest, EXPECT_CALL(mock_hmi_capabilities_, set_active_vr_language(_)); EXPECT_CALL(mock_rpc_service_, ManageMobileCommand(_, Command::CommandSource::SOURCE_SDL)); - EXPECT_CALL(*app_ptr_, IsRegistered()).WillOnce(Return(true)); EXPECT_CALL(*app_ptr_, app_id()).WillOnce(Return(kAppId_)); EXPECT_CALL(*app_ptr_, language()).WillRepeatedly(ReturnRef(kLang)); @@ -1413,15 +1412,8 @@ TEST_F(HMICommandsNotificationsTest, EXPECT_CALL(mock_hmi_capabilities_, set_active_vr_language(_)); EXPECT_CALL(mock_rpc_service_, ManageMobileCommand(_, Command::CommandSource::SOURCE_SDL)); - EXPECT_CALL(*app_ptr_, IsRegistered()).WillOnce(Return(true)); EXPECT_CALL(*app_ptr_, app_id()).WillRepeatedly(Return(kAppId_)); EXPECT_CALL(*app_ptr_, language()).WillRepeatedly(ReturnRef(kLang)); - EXPECT_CALL(app_mngr_, state_controller()) - .WillOnce(ReturnRef(mock_state_controller_)); - EXPECT_CALL( - mock_state_controller_, - SetRegularState( - app_, kDefaultWindowId, mobile_apis::HMILevel::HMI_NONE, false)); EXPECT_CALL(mock_message_helper_, GetOnAppInterfaceUnregisteredNotificationToMobile( kAppId_, @@ -1699,7 +1691,6 @@ TEST_F(HMICommandsNotificationsTest, EXPECT_CALL(mock_hmi_capabilities_, set_active_tts_language(_)); EXPECT_CALL(mock_rpc_service_, ManageMobileCommand(_, Command::CommandSource::SOURCE_SDL)); - EXPECT_CALL(*app_ptr_, IsRegistered()).WillOnce(Return(true)); EXPECT_CALL(*app_ptr_, app_id()).WillOnce(Return(kAppId_)); EXPECT_CALL(*app_ptr_, language()).WillRepeatedly(ReturnRef(kLang)); @@ -1745,7 +1736,6 @@ TEST_F(HMICommandsNotificationsTest, EXPECT_CALL(mock_hmi_capabilities_, set_active_tts_language(_)); EXPECT_CALL(mock_rpc_service_, ManageMobileCommand(_, Command::CommandSource::SOURCE_SDL)); - EXPECT_CALL(*app_ptr_, IsRegistered()).WillOnce(Return(true)); EXPECT_CALL(*app_ptr_, app_id()).WillRepeatedly(Return(kAppId_)); EXPECT_CALL(*app_ptr_, language()).WillRepeatedly(ReturnRef(kLang)); EXPECT_CALL(mock_message_helper_, diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/sdl_activate_app_request_test.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/sdl_activate_app_request_test.cc index b110d4bf9e..8057a2c518 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/sdl_activate_app_request_test.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/test/commands/hmi/sdl_activate_app_request_test.cc @@ -157,7 +157,6 @@ TEST_F(SDLActivateAppRequestTest, Run_ActivateApp_SUCCESS) { CurrentHmiState(mobile_apis::PredefinedWindows::DEFAULT_WINDOW)) .WillOnce(Return(state)); - EXPECT_CALL(*mock_app, app_id()).WillOnce(Return(kAppID)); EXPECT_CALL(*mock_app, IsRegistered()).WillOnce(Return(true)); EXPECT_CALL(mock_policy_handler_, OnActivateApp(kAppID, kCorrelationID)); @@ -205,8 +204,8 @@ TEST_F(SDLActivateAppRequestTest, FindAppToRegister_SUCCESS) { IsStateActive(am::HmiState::StateID::STATE_ID_DEACTIVATE_HMI)) .WillOnce(Return(false)); - ON_CALL(*mock_app, IsRegistered()).WillByDefault(Return(false)); - ON_CALL(*mock_app, is_cloud_app()).WillByDefault(Return(false)); + EXPECT_CALL(*mock_app, IsRegistered()).WillOnce(Return(false)); + EXPECT_CALL(*mock_app, is_cloud_app()).WillOnce(Return(false)); ON_CALL(*mock_app, device()).WillByDefault(Return(kHandle)); MockAppPtr mock_app_first(CreateMockApp()); @@ -267,8 +266,8 @@ TEST_F(SDLActivateAppRequestTest, DevicesAppsEmpty_SUCCESS) { IsStateActive(am::HmiState::StateID::STATE_ID_DEACTIVATE_HMI)) .WillOnce(Return(false)); - ON_CALL(*mock_app, IsRegistered()).WillByDefault(Return(false)); - ON_CALL(*mock_app, is_cloud_app()).WillByDefault(Return(false)); + EXPECT_CALL(*mock_app, IsRegistered()).WillOnce(Return(false)); + EXPECT_CALL(*mock_app, is_cloud_app()).WillOnce(Return(false)); ON_CALL(*mock_app, device()).WillByDefault(Return(kHandle)); DataAccessor accessor(app_list_, lock_); @@ -330,11 +329,10 @@ TEST_F(SDLActivateAppRequestTest, FirstAppNotActiveNONE_SUCCESS) { EXPECT_CALL(mock_state_controller_, IsStateActive(am::HmiState::StateID::STATE_ID_DEACTIVATE_HMI)) .WillOnce(Return(false)); - - ON_CALL(*mock_app, IsRegistered()).WillByDefault(Return(true)); - + EXPECT_CALL(*mock_app, IsRegistered()).WillOnce(Return(true)); am::HmiStatePtr state = std::make_shared(mock_app, app_mngr_); state->set_hmi_level(mobile_apis::HMILevel::HMI_NONE); + EXPECT_CALL(*mock_app, CurrentHmiState(mobile_apis::PredefinedWindows::DEFAULT_WINDOW)) .WillOnce(Return(state)); @@ -361,9 +359,8 @@ TEST_F(SDLActivateAppRequestTest, FirstAppIsForeground_SUCCESS) { ON_CALL(app_mngr_, application(kAppID)).WillByDefault(Return(mock_app)); EXPECT_CALL(*mock_app, device()).WillOnce(Return(kHandle)); - ON_CALL(*mock_app, IsRegistered()).WillByDefault(Return(false)); - ON_CALL(*mock_app, is_cloud_app()).WillByDefault(Return(false)); - + EXPECT_CALL(*mock_app, IsRegistered()).WillOnce(Return(false)); + EXPECT_CALL(*mock_app, is_cloud_app()).WillOnce(Return(false)); EXPECT_CALL(app_mngr_, state_controller()) .WillOnce(ReturnRef(mock_state_controller_)); EXPECT_CALL(mock_state_controller_, @@ -434,7 +431,6 @@ TEST_F(SDLActivateAppRequestTest, FirstAppNotRegistered_SUCCESS) { EXPECT_CALL(mock_state_controller_, IsStateActive(am::HmiState::StateID::STATE_ID_DEACTIVATE_HMI)) .WillOnce(Return(false)); - DataAccessor accessor(app_list_, lock_); EXPECT_CALL(app_mngr_, applications()).WillRepeatedly(Return(accessor)); @@ -466,8 +462,8 @@ TEST_F(SDLActivateAppRequestTest, WaitingCloudApplication_ConnectDevice) { MockAppPtr mock_app(CreateMockApp()); EXPECT_CALL(*mock_app, device()).WillOnce(Return(kHandle)); - ON_CALL(*mock_app, IsRegistered()).WillByDefault(Return(false)); - ON_CALL(*mock_app, is_cloud_app()).WillByDefault(Return(true)); + EXPECT_CALL(*mock_app, IsRegistered()).WillOnce(Return(false)); + EXPECT_CALL(*mock_app, is_cloud_app()).WillOnce(Return(true)); #ifndef EXTERNAL_PROPRIETARY_MODE EXPECT_CALL(app_mngr_, application(kAppID)) diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc index 7a5d5fc0dc..6065743d1d 100644 --- a/src/components/application_manager/src/application_manager_impl.cc +++ b/src/components/application_manager/src/application_manager_impl.cc @@ -3344,7 +3344,7 @@ void ApplicationManagerImpl::UnregisterApplication( while (applications_.end() != it_app) { if (app_id == (*it_app)->app_id()) { app_to_remove = *it_app; - applications_.erase(it_app++); + applications_.erase(it_app); break; } else { ++it_app; @@ -4770,6 +4770,9 @@ void ApplicationManagerImpl::AddAppToRegisteredAppList( SDL_LOG_AUTO_TRACE(); DCHECK_OR_RETURN_VOID(application); sync_primitives::AutoLock lock(applications_list_lock_ptr_); + + // Add application to registered app list and set appropriate mark. + application->MarkRegistered(); applications_.insert(application); SDL_LOG_DEBUG("App with app_id: " << application->app_id() diff --git a/src/components/application_manager/src/message_helper/message_helper.cc b/src/components/application_manager/src/message_helper/message_helper.cc index bbe6189bc9..606bc4c3f0 100644 --- a/src/components/application_manager/src/message_helper/message_helper.cc +++ b/src/components/application_manager/src/message_helper/message_helper.cc @@ -1848,7 +1848,7 @@ bool MessageHelper::CreateHMIApplicationStruct( if (file_system::FileExists(app->app_icon_path())) { message[strings::icon] = icon_path; } - if (app->app_id() > 0 || app->IsRegistered()) { + if (app->IsRegistered()) { message[strings::hmi_display_language_desired] = app->ui_language(); message[strings::is_media_application] = app->is_media_application(); } else { -- cgit v1.2.1