From a86fa542fff877da2589b284d800b3924d30710f Mon Sep 17 00:00:00 2001 From: Ken Rockot Date: Tue, 13 Nov 2018 21:47:13 +0000 Subject: [PATCH] [service-manager] Clean up Connector API Updates the Connector mojom and C++ APIs with clarifying naming and type usage. Connector has accumulated lots of cruft over the years and its API conveys a number of muddled concepts which have lead to general confusion, misuse, and abuse. This attempts to sort all that out. Namely: - All use of Identity in the Connector API has been migrated to ServiceFilter where appropriate. - StartService (first version) has been renamed to WarmService to convey the correct intent. Bugs filed and TODOs dropped in places where this is being used in an undesirable fashion. - StartService (second version) has been renamed to RegisterServiceInstance to convey more accurately what it does. It also requires a complete Identity to be provided by the trusted caller. - QueryService takes only a service name instead of an Identity, since it only needs to take a service name. It also makes no service connection so it no longer returns a ConnectResult. - FilterInterfaces is marked as deprecated on the mojom interface, because we want to obliterate it. - Test-only methods have been properly suffixed and made public, eliminating the need for the weird TestApi wrapper thing. A follow-up CL will delete TestApi and migrate its users. - Connector methods which have async replies have optional callbacks now instead of the Connector weirdly having a single catch-all (and dubiously named) "StartServiceCallback" to handle all replies. Bug: 902590 Change-Id: I5a986a5c114da914fd624dac7e6fd021cf139958 Reviewed-on: https://chromium-review.googlesource.com/c/1329845 Reviewed-by: Tom Sepez Reviewed-by: Oksana Zhuravlova Reviewed-by: Scott Violet Reviewed-by: John Abd-El-Malek Commit-Queue: Ken Rockot Cr-Commit-Position: refs/heads/master@{#607750} --- .../renderer/aw_content_renderer_client.cc | 3 +- ash/accelerators/debug_commands.cc | 4 +- ash/app_launch_unittest.cc | 11 +- ash/ash_service_unittest.cc | 2 - ash/assistant/assistant_controller.cc | 4 +- .../ash_keyboard_controller_unittest.cc | 3 +- .../multi_device_notification_presenter.cc | 4 +- ..._device_notification_presenter_unittest.cc | 2 +- ash/shell.cc | 6 +- .../client/shell_browser_main_parts.cc | 28 +-- ash/system/flag_warning/flag_warning_tray.cc | 5 +- chrome/browser/chrome_browser_main.cc | 6 +- .../browser/chrome_content_browser_client.cc | 5 +- chrome/browser/chrome_service.cc | 7 +- .../chrome_site_per_process_browsertest.cc | 7 +- ...display_info_provider_chromeos_unittest.cc | 2 +- .../test/spellcheck_content_browser_client.cc | 7 +- .../chrome_browser_main_extra_parts_ash.cc | 8 +- .../browser_child_process_host_impl.cc | 14 +- content/browser/browser_context.cc | 7 +- content/browser/browser_main_loop.cc | 5 +- .../media/media_devices_manager.cc | 11 +- .../renderer_host/render_process_host_impl.cc | 16 +- .../service_manager_context.cc | 17 +- .../service_manager/child_connection.cc | 4 +- content/public/browser/render_process_host.h | 11 +- .../render_thread_impl_browsertest.cc | 14 +- ios/web/browser_state.mm | 7 +- ios/web/service_manager_context.mm | 5 +- mash/session/session.cc | 7 +- .../pref_service_factory_unittest.cc | 5 +- .../resource_coordinator_service_unittest.cc | 6 - .../background_service_manager_unittest.cc | 7 +- services/service_manager/connect_params.cc | 4 +- services/service_manager/connect_params.h | 18 +- services/service_manager/connect_util.cc | 2 +- .../service_manager/public/cpp/connector.cc | 94 ++++----- .../service_manager/public/cpp/connector.h | 188 +++++++++++------- .../public/cpp/service_filter.cc | 7 + .../public/cpp/service_filter.h | 2 + .../public/cpp/test/test_connector_factory.cc | 20 +- .../public/mojom/connector.mojom | 113 ++++++----- services/service_manager/service_manager.cc | 85 +++++--- .../tests/connect/connect_test_app.cc | 15 +- .../tests/connect/connect_test_package.cc | 12 +- .../tests/connect/connect_unittest.cc | 92 ++++----- .../tests/lifecycle/lifecycle_unittest.cc | 6 +- .../service_manager_unittest.cc | 36 ++-- services/service_manager/tests/util.cc | 16 +- services/service_manager/tests/util.h | 6 +- services/tracing/tracing_service_unittest.cc | 5 - services/ws/ime/ime_driver_bridge.cc | 7 - services/ws/ime/ime_driver_bridge.h | 5 - services/ws/ime/ime_unittest.cc | 4 +- 54 files changed, 528 insertions(+), 459 deletions(-) diff --git a/android_webview/renderer/aw_content_renderer_client.cc b/android_webview/renderer/aw_content_renderer_client.cc index 5be63fb954c372..7e76648b447bae 100644 --- a/android_webview/renderer/aw_content_renderer_client.cc +++ b/android_webview/renderer/aw_content_renderer_client.cc @@ -300,7 +300,8 @@ void AwContentRendererClient::GetInterface( // TODO(crbug.com/806394): Use a WebView-specific service for SpellCheckHost // and SafeBrowsing, instead of |content_browser|. RenderThread::Get()->GetConnector()->BindInterface( - service_manager::Identity(content::mojom::kBrowserServiceName), + service_manager::ServiceFilter::ByName( + content::mojom::kBrowserServiceName), interface_name, std::move(interface_pipe)); } diff --git a/ash/accelerators/debug_commands.cc b/ash/accelerators/debug_commands.cc index 41554aaf5e6d05..d966698cd3619a 100644 --- a/ash/accelerators/debug_commands.cc +++ b/ash/accelerators/debug_commands.cc @@ -115,7 +115,9 @@ void HandlePrintWindowHierarchy() { } void HandleShowQuickLaunch() { - Shell::Get()->connector()->StartService(quick_launch::mojom::kServiceName); + // TODO(https://crbug.com/904148): This should not use |WarmService()|. + Shell::Get()->connector()->WarmService(service_manager::ServiceFilter::ByName( + quick_launch::mojom::kServiceName)); } gfx::ImageSkia CreateWallpaperImage(SkColor fill, SkColor rect) { diff --git a/ash/app_launch_unittest.cc b/ash/app_launch_unittest.cc index a675e1c73ba2e2..138d22e3560f32 100644 --- a/ash/app_launch_unittest.cc +++ b/ash/app_launch_unittest.cc @@ -42,11 +42,16 @@ TEST_F(AppLaunchTest, TestQuickLaunch) { if (features::IsSingleProcessMash()) return; - connector()->StartService(mojom::kServiceName); - connector()->StartService(quick_launch::mojom::kServiceName); + // TODO(https://crbug.com/904148): These should not use |WarmService()|. + connector()->WarmService( + service_manager::ServiceFilter::ByName(mojom::kServiceName)); + connector()->WarmService(service_manager::ServiceFilter::ByName( + quick_launch::mojom::kServiceName)); ws::mojom::WindowServerTestPtr test_interface; - connector()->BindInterface(ws::mojom::kServiceName, &test_interface); + connector()->BindInterface( + service_manager::ServiceFilter::ByName(ws::mojom::kServiceName), + mojo::MakeRequest(&test_interface)); base::RunLoop run_loop; bool success = false; diff --git a/ash/ash_service_unittest.cc b/ash/ash_service_unittest.cc index 67a0c2c9591fa1..67e32c4887d90d 100644 --- a/ash/ash_service_unittest.cc +++ b/ash/ash_service_unittest.cc @@ -105,8 +105,6 @@ TEST_F(AshServiceTest, OpenWindow) { WindowTreeClientDelegate window_tree_delegate; - connector()->StartService(mojom::kServiceName); - // Connect to mus and create a new top level window. The request goes to // |ash|, but is async. std::unique_ptr client = diff --git a/ash/assistant/assistant_controller.cc b/ash/assistant/assistant_controller.cc index eb3c7b36ff9345..6b8ebc5158de4c 100644 --- a/ash/assistant/assistant_controller.cc +++ b/ash/assistant/assistant_controller.cc @@ -251,8 +251,8 @@ void AssistantController::GetNavigableContentsFactory( } Shell::Get()->connector()->BindInterface( - service_manager::Identity(content::mojom::kServiceName, - service_instance_group), + service_manager::ServiceFilter::ByNameInGroup( + content::mojom::kServiceName, *service_instance_group), std::move(request)); } diff --git a/ash/keyboard/ash_keyboard_controller_unittest.cc b/ash/keyboard/ash_keyboard_controller_unittest.cc index 316ac422e651e1..5882462618d0ef 100644 --- a/ash/keyboard/ash_keyboard_controller_unittest.cc +++ b/ash/keyboard/ash_keyboard_controller_unittest.cc @@ -182,7 +182,8 @@ class AshKeyboardControllerTest : public AshTestBase { service_manager::Connector::TestApi test_api(connector_.get()); test_api.OverrideBinderForTesting( - service_manager::Identity("test"), mojom::KeyboardController::Name_, + service_manager::ServiceFilter::ByName("test"), + mojom::KeyboardController::Name_, base::BindRepeating( &AshKeyboardControllerTest::AddKeyboardControllerBinding, base::Unretained(this))); diff --git a/ash/multi_device_setup/multi_device_notification_presenter.cc b/ash/multi_device_setup/multi_device_notification_presenter.cc index 2ee430f9602958..cda53aeef99dca 100644 --- a/ash/multi_device_setup/multi_device_notification_presenter.cc +++ b/ash/multi_device_setup/multi_device_notification_presenter.cc @@ -234,9 +234,9 @@ void MultiDeviceNotificationPresenter::ObserveMultiDeviceSetupIfPossible() { return; connector_->BindInterface( - service_manager::Identity( + service_manager::ServiceFilter::ByNameInGroup( chromeos::multidevice_setup::mojom::kServiceName, - service_instance_group), + *service_instance_group), &multidevice_setup_ptr_); // Add this object as the delegate of the MultiDeviceSetup Service. diff --git a/ash/multi_device_setup/multi_device_notification_presenter_unittest.cc b/ash/multi_device_setup/multi_device_notification_presenter_unittest.cc index 7e050fd5985fd7..4460775bb45dea 100644 --- a/ash/multi_device_setup/multi_device_notification_presenter_unittest.cc +++ b/ash/multi_device_setup/multi_device_notification_presenter_unittest.cc @@ -136,7 +136,7 @@ class MultiDeviceNotificationPresenterTest : public NoSessionAshTestBase { std::make_unique(); service_manager::Connector::TestApi test_api(connector_.get()); test_api.OverrideBinderForTesting( - service_manager::Identity( + service_manager::ServiceFilter::ByNameInGroup( chromeos::multidevice_setup::mojom::kServiceName, kTestServiceInstanceGroup), chromeos::multidevice_setup::mojom::MultiDeviceSetup::Name_, diff --git a/ash/shell.cc b/ash/shell.cc index 54d24ed1e356e5..03bed3fd01ddad 100644 --- a/ash/shell.cc +++ b/ash/shell.cc @@ -1254,9 +1254,11 @@ void Shell::Init( // |connector_| is null in unit tests. if (connector_ && base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kShowTaps)) { - // The show taps feature is a separate mojo app. + // The show taps feature is a separate service. // TODO(jamescook): Make this work in ash_shell_with_content. - connector_->StartService(tap_visualizer::mojom::kServiceName); + // TODO(https://crbug.com/904148): This should not use |WarmService()|. + connector_->WarmService(service_manager::ServiceFilter::ByName( + tap_visualizer::mojom::kServiceName)); } if (!::features::IsMultiProcessMash()) { diff --git a/ash/shell/content/client/shell_browser_main_parts.cc b/ash/shell/content/client/shell_browser_main_parts.cc index df62cdf3368409..192a89ff506acf 100644 --- a/ash/shell/content/client/shell_browser_main_parts.cc +++ b/ash/shell/content/client/shell_browser_main_parts.cc @@ -85,14 +85,16 @@ void ShellBrowserMainParts::PreMainMessageLoopRun() { chromeos::PowerPolicyController::Initialize( chromeos::DBusThreadManager::Get()->GetPowerManagerClient()); + service_manager::Connector* const connector = + content::ServiceManagerConnection::GetForProcess()->GetConnector(); + ui::MaterialDesignController::Initialize(); ash::ShellInitParams init_params; init_params.delegate = std::make_unique(); init_params.context_factory = content::GetContextFactory(); init_params.context_factory_private = content::GetContextFactoryPrivate(); init_params.gpu_interface_provider = content::CreateGpuInterfaceProvider(); - init_params.connector = - content::ServiceManagerConnection::GetForProcess()->GetConnector(); + init_params.connector = connector; ash::Shell::CreateInstance(std::move(init_params)); // Initialize session controller client and create fake user sessions. The @@ -111,19 +113,17 @@ void ShellBrowserMainParts::PreMainMessageLoopRun() { ash::Shell::GetPrimaryRootWindow()->GetHost()->Show(); - content::ServiceManagerConnection::GetForProcess() - ->GetConnector() - ->StartService(test_ime_driver::mojom::kServiceName); - content::ServiceManagerConnection::GetForProcess() - ->GetConnector() - ->StartService(quick_launch::mojom::kServiceName); - content::ServiceManagerConnection::GetForProcess() - ->GetConnector() - ->StartService(tap_visualizer::mojom::kServiceName); + // TODO(https://crbug.com/904148): These should not use |WarmService()|. + connector->WarmService(service_manager::ServiceFilter::ByName( + test_ime_driver::mojom::kServiceName)); + connector->WarmService(service_manager::ServiceFilter::ByName( + quick_launch::mojom::kServiceName)); + connector->WarmService(service_manager::ServiceFilter::ByName( + tap_visualizer::mojom::kServiceName)); shortcut_viewer::mojom::ShortcutViewerPtr shortcut_viewer; - content::ServiceManagerConnection::GetForProcess() - ->GetConnector() - ->BindInterface(shortcut_viewer::mojom::kServiceName, &shortcut_viewer); + connector->BindInterface(service_manager::ServiceFilter::ByName( + shortcut_viewer::mojom::kServiceName), + mojo::MakeRequest(&shortcut_viewer)); shortcut_viewer->Toggle(base::TimeTicks::Now()); ash::Shell::Get()->InitWaylandServer(nullptr); } diff --git a/ash/system/flag_warning/flag_warning_tray.cc b/ash/system/flag_warning/flag_warning_tray.cc index 9aa38e09ed901a..3d258240a5babe 100644 --- a/ash/system/flag_warning/flag_warning_tray.cc +++ b/ash/system/flag_warning/flag_warning_tray.cc @@ -59,7 +59,10 @@ void FlagWarningTray::ButtonPressed(views::Button* sender, DCHECK_EQ(button_, sender); // Open the quick launch mojo mini-app to demonstrate that mini-apps work. - Shell::Get()->connector()->StartService(quick_launch::mojom::kServiceName); + // + // TODO(https://crbug.com/904148): This should not use |WarmService()|. + Shell::Get()->connector()->WarmService(service_manager::ServiceFilter::ByName( + quick_launch::mojom::kServiceName)); } void FlagWarningTray::GetAccessibleNodeData(ui::AXNodeData* node_data) { diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc index 0af7d1de4ce964..cf00acf62dfda0 100644 --- a/chrome/browser/chrome_browser_main.cc +++ b/chrome/browser/chrome_browser_main.cc @@ -1873,8 +1873,10 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() { if (parsed_command_line().HasSwitch(switches::kLaunchSimpleBrowserSwitch) || parsed_command_line().HasSwitch( switches::kLaunchInProcessSimpleBrowserSwitch)) { - content::BrowserContext::GetConnectorFor(profile_)->StartService( - service_manager::Identity(simple_browser::mojom::kServiceName)); + // TODO(https://crbug.com/904148): This should not use |WarmService()|. + content::BrowserContext::GetConnectorFor(profile_)->WarmService( + service_manager::ServiceFilter::ByName( + simple_browser::mojom::kServiceName)); } #endif diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc index 573bb4a43e1406..38ab70c26e0c3e 100644 --- a/chrome/browser/chrome_content_browser_client.cc +++ b/chrome/browser/chrome_content_browser_client.cc @@ -1337,10 +1337,11 @@ void ChromeContentBrowserClient::RenderProcessWillLaunch( *service_request = mojo::MakeRequest(&service); service_manager::mojom::PIDReceiverPtr pid_receiver; service_manager::Identity renderer_identity = host->GetChildIdentity(); - ChromeService::GetInstance()->connector()->StartService( + ChromeService::GetInstance()->connector()->RegisterServiceInstance( service_manager::Identity(chrome::mojom::kRendererServiceName, renderer_identity.instance_group(), - renderer_identity.instance_id()), + renderer_identity.instance_id(), + base::Token::CreateRandom()), std::move(service), mojo::MakeRequest(&pid_receiver)); } diff --git a/chrome/browser/chrome_service.cc b/chrome/browser/chrome_service.cc index 4abaf85b0d2a69..4607f4fbd154f2 100644 --- a/chrome/browser/chrome_service.cc +++ b/chrome/browser/chrome_service.cc @@ -135,12 +135,13 @@ class ChromeService::ExtraParts : public ChromeBrowserMainExtraParts { void ServiceManagerConnectionStarted( content::ServiceManagerConnection* connection) override { // Initializing the connector asynchronously configures the Connector on the - // IO thread. This needs to be done before StartService() is called or + // IO thread. This needs to be done before WarmService() is called or // ChromeService::BindConnector() can race with ChromeService::OnStart(). ChromeService::GetInstance()->InitConnector(); - connection->GetConnector()->StartService( - service_manager::Identity(chrome::mojom::kServiceName)); + // TODO(https://crbug.com/904148): This should not use |WarmService()|. + connection->GetConnector()->WarmService( + service_manager::ServiceFilter::ByName(chrome::mojom::kServiceName)); } DISALLOW_COPY_AND_ASSIGN(ExtraParts); diff --git a/chrome/browser/chrome_site_per_process_browsertest.cc b/chrome/browser/chrome_site_per_process_browsertest.cc index baab7c123a9899..943b9f9d145514 100644 --- a/chrome/browser/chrome_site_per_process_browsertest.cc +++ b/chrome/browser/chrome_site_per_process_browsertest.cc @@ -1021,12 +1021,9 @@ class TestBrowserClientForSpellCheck : public ChromeContentBrowserClient { void BindSpellCheckHostRequest( spellcheck::mojom::SpellCheckHostRequest request, const service_manager::BindSourceInfo& source_info) { - service_manager::Identity renderer_identity( - content::mojom::kRendererServiceName, - source_info.identity.instance_group(), - source_info.identity.instance_id()); content::RenderProcessHost* host = - content::RenderProcessHost::FromRendererIdentity(renderer_identity); + content::RenderProcessHost::FromRendererInstanceId( + *source_info.identity.instance_id()); auto spell_check_host = std::make_unique(host); spell_check_host->SpellCheckHostRequest(std::move(request)); spell_check_hosts_.push_back(std::move(spell_check_host)); diff --git a/chrome/browser/extensions/system_display/display_info_provider_chromeos_unittest.cc b/chrome/browser/extensions/system_display/display_info_provider_chromeos_unittest.cc index cb5000bea21a21..77f245e464425d 100644 --- a/chrome/browser/extensions/system_display/display_info_provider_chromeos_unittest.cc +++ b/chrome/browser/extensions/system_display/display_info_provider_chromeos_unittest.cc @@ -69,7 +69,7 @@ class DisplayInfoProviderChromeosTest : public ash::AshTestBase { connector_ = service_manager::Connector::Create(&request); service_manager::Connector::TestApi test_api(connector_.get()); test_api.OverrideBinderForTesting( - service_manager::Identity(ash::mojom::kServiceName), + service_manager::ServiceFilter::ByName(ash::mojom::kServiceName), ash::mojom::CrosDisplayConfigController::Name_, base::BindRepeating(&DisplayInfoProviderChromeosTest:: AddCrosDisplayConfigControllerBinding, diff --git a/chrome/browser/spellchecker/test/spellcheck_content_browser_client.cc b/chrome/browser/spellchecker/test/spellcheck_content_browser_client.cc index 46ea07d99da1b9..27ea69855d53b2 100644 --- a/chrome/browser/spellchecker/test/spellcheck_content_browser_client.cc +++ b/chrome/browser/spellchecker/test/spellcheck_content_browser_client.cc @@ -53,12 +53,9 @@ void SpellCheckContentBrowserClient::RunUntilBind() { void SpellCheckContentBrowserClient::BindSpellCheckPanelHostRequest( spellcheck::mojom::SpellCheckPanelHostRequest request, const service_manager::BindSourceInfo& source_info) { - service_manager::Identity renderer_identity( - content::mojom::kRendererServiceName, - source_info.identity.instance_group(), - source_info.identity.instance_id()); content::RenderProcessHost* render_process_host = - content::RenderProcessHost::FromRendererIdentity(renderer_identity); + content::RenderProcessHost::FromRendererInstanceId( + *source_info.identity.instance_id()); auto spell_check_panel_host = std::make_unique(render_process_host); spell_check_panel_host->SpellCheckPanelHostRequest(std::move(request)); diff --git a/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc b/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc index 3bdb63c591f1c2..51b85ff2ff15f3 100644 --- a/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc +++ b/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc @@ -153,10 +153,10 @@ void ChromeBrowserMainExtraPartsAsh::ServiceManagerConnectionStarted( if (features::IsUsingWindowService()) { // Start up the window service and the ash system UI service. // NOTE: ash::Shell is still created below for SingleProcessMash. - connection->GetConnector()->StartService( - service_manager::Identity(ws::mojom::kServiceName)); - connection->GetConnector()->StartService( - service_manager::Identity(ash::mojom::kServiceName)); + connection->GetConnector()->WarmService( + service_manager::ServiceFilter::ByName(ws::mojom::kServiceName)); + connection->GetConnector()->WarmService( + service_manager::ServiceFilter::ByName(ash::mojom::kServiceName)); views::MusClient::InitParams params; params.connector = connection->GetConnector(); diff --git a/content/browser/browser_child_process_host_impl.cc b/content/browser/browser_child_process_host_impl.cc index b078387f6823b3..ac4f3f7cf7f465 100644 --- a/content/browser/browser_child_process_host_impl.cc +++ b/content/browser/browser_child_process_host_impl.cc @@ -52,6 +52,7 @@ #include "content/public/common/service_manager_connection.h" #include "mojo/public/cpp/system/platform_handle.h" #include "services/service_manager/embedder/switches.h" +#include "services/service_manager/public/cpp/constants.h" #if defined(OS_MACOSX) #include "content/browser/mach_broker_mac.h" @@ -169,13 +170,12 @@ BrowserChildProcessHostImpl::BrowserChildProcessHostImpl( if (!service_name.empty()) { DCHECK_CURRENTLY_ON(BrowserThread::IO); - service_manager::Identity child_identity(service_name, - base::nullopt /* instance_group */, - base::Token::CreateRandom()); - child_connection_.reset( - new ChildConnection(child_identity, &mojo_invitation_, - ServiceManagerContext::GetConnectorForIOThread(), - base::ThreadTaskRunnerHandle::Get())); + child_connection_ = std::make_unique( + service_manager::Identity( + service_name, service_manager::kSystemInstanceGroup, + base::Token::CreateRandom(), base::Token::CreateRandom()), + &mojo_invitation_, ServiceManagerContext::GetConnectorForIOThread(), + base::ThreadTaskRunnerHandle::Get()); data_.metrics_name = service_name; } diff --git a/content/browser/browser_context.cc b/content/browser/browser_context.cc index ce64af11873c11..ab4166e3daab50 100644 --- a/content/browser/browser_context.cc +++ b/content/browser/browser_context.cc @@ -582,12 +582,13 @@ void BrowserContext::Initialize( auto service_request = mojo::MakeRequest(&service); service_manager::mojom::PIDReceiverPtr pid_receiver; - service_manager::Identity identity(mojom::kBrowserServiceName, new_group); - service_manager_connection->GetConnector()->StartService( + service_manager::Identity identity(mojom::kBrowserServiceName, new_group, + base::Token{}, + base::Token::CreateRandom()); + service_manager_connection->GetConnector()->RegisterServiceInstance( identity, std::move(service), mojo::MakeRequest(&pid_receiver)); pid_receiver->SetPID(base::GetCurrentProcId()); - service_manager_connection->GetConnector()->StartService(identity); BrowserContextServiceManagerConnectionHolder* connection_holder = new BrowserContextServiceManagerConnectionHolder( std::move(service_request)); diff --git a/content/browser/browser_main_loop.cc b/content/browser/browser_main_loop.cc index 3c053b6b97f2ab..872a9c6a41a203 100644 --- a/content/browser/browser_main_loop.cc +++ b/content/browser/browser_main_loop.cc @@ -1674,8 +1674,9 @@ void BrowserMainLoop::InitializeAudio() { if (connection) { // The browser is not shutting down: |connection| would be null // otherwise. - connection->GetConnector()->StartService( - audio::mojom::kServiceName); + connection->GetConnector()->WarmService( + service_manager::ServiceFilter::ByName( + audio::mojom::kServiceName)); } })); } diff --git a/content/browser/renderer_host/media/media_devices_manager.cc b/content/browser/renderer_host/media/media_devices_manager.cc index d79886ff495f4e..d746128d51eab7 100644 --- a/content/browser/renderer_host/media/media_devices_manager.cc +++ b/content/browser/renderer_host/media/media_devices_manager.cc @@ -311,19 +311,20 @@ class MediaDevicesManager::AudioServiceDeviceListener void TryConnectToService(service_manager::Connector* connector) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); // Check if the service manager is managing the audio service. + // + // TODO: Is this necessary? We should know at build how this will respond. connector->QueryService( - service_manager::Identity(audio::mojom::kServiceName), + audio::mojom::kServiceName, base::BindOnce(&AudioServiceDeviceListener::ServiceQueried, weak_factory_.GetWeakPtr(), connector)); } void ServiceQueried(service_manager::Connector* connector, - service_manager::mojom::ConnectResult connect_result, - const std::string& ignore) { + service_manager::mojom::ServiceInfoPtr info) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); // Do not connect if the service manager is not managing the audio service. - if (connect_result != service_manager::mojom::ConnectResult::SUCCEEDED) { - LOG(WARNING) << "Audio service not available: " << connect_result; + if (!info) { + LOG(WARNING) << "Audio service not available."; return; } DoConnectToService(connector); diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc index b3839ccb4c1b79..f1de863c18c85e 100644 --- a/content/browser/renderer_host/render_process_host_impl.cc +++ b/content/browser/renderer_host/render_process_host_impl.cc @@ -1870,12 +1870,12 @@ void RenderProcessHostImpl::InitializeChannelProxy() { // Establish a ServiceManager connection for the new render service instance. mojo_invitation_ = {}; - service_manager::Identity child_identity( - mojom::kRendererServiceName, - BrowserContext::GetServiceInstanceGroupFor(GetBrowserContext()), - base::Token::CreateRandom()); child_connection_ = std::make_unique( - child_identity, &mojo_invitation_, connector, io_task_runner); + service_manager::Identity( + mojom::kRendererServiceName, + BrowserContext::GetServiceInstanceGroupFor(GetBrowserContext()), + base::Token::CreateRandom(), base::Token::CreateRandom()), + &mojo_invitation_, connector, io_task_runner); // Send an interface request to bootstrap the IPC::Channel. Note that this // request will happily sit on the pipe until the process is launched and @@ -3805,12 +3805,12 @@ RenderProcessHost* RenderProcessHost::FromID(int render_process_id) { } // static -RenderProcessHost* RenderProcessHost::FromRendererIdentity( - const service_manager::Identity& identity) { +RenderProcessHost* RenderProcessHost::FromRendererInstanceId( + const base::Token& instance_id) { for (RenderProcessHost::iterator i(RenderProcessHost::AllHostsIterator()); !i.IsAtEnd(); i.Advance()) { RenderProcessHost* process = i.GetCurrentValue(); - if (process->GetChildIdentity().Matches(identity)) + if (process->GetChildIdentity().instance_id() == instance_id) return process; } return nullptr; diff --git a/content/browser/service_manager/service_manager_context.cc b/content/browser/service_manager/service_manager_context.cc index 2b1f9415e14ea8..1403a2f4755382 100644 --- a/content/browser/service_manager/service_manager_context.cc +++ b/content/browser/service_manager/service_manager_context.cc @@ -130,10 +130,10 @@ void StartServiceInUtilityProcess( base::Optional process_group, service_manager::mojom::ServiceRequest request, service_manager::mojom::PIDReceiverPtr pid_receiver, - service_manager::mojom::ConnectResult query_result, - const std::string& sandbox_string) { + service_manager::mojom::ServiceInfoPtr service_info) { + DCHECK(service_info); service_manager::SandboxType sandbox_type = - service_manager::UtilitySandboxTypeFromString(sandbox_string); + service_manager::UtilitySandboxTypeFromString(service_info->sandbox_type); // Look for an existing process group. base::WeakPtr* weak_host = nullptr; @@ -182,7 +182,7 @@ void QueryAndStartServiceInUtilityProcess( service_manager::mojom::ServiceRequest request, service_manager::mojom::PIDReceiverPtr pid_receiver) { ServiceManagerContext::GetConnectorForIOThread()->QueryService( - service_manager::Identity(service_name), + service_name, base::BindOnce(&StartServiceInUtilityProcess, service_name, process_name_callback, std::move(process_group), std::move(request), std::move(pid_receiver))); @@ -583,9 +583,10 @@ ServiceManagerContext::ServiceManagerContext( auto* browser_connection = ServiceManagerConnection::GetForProcess(); service_manager::mojom::PIDReceiverPtr pid_receiver; - packaged_services_connection_->GetConnector()->StartService( + packaged_services_connection_->GetConnector()->RegisterServiceInstance( service_manager::Identity(mojom::kBrowserServiceName, - service_manager::kSystemInstanceGroup), + service_manager::kSystemInstanceGroup, + base::Token{}, base::Token::CreateRandom()), std::move(root_browser_service), mojo::MakeRequest(&pid_receiver)); pid_receiver->SetPID(base::GetCurrentProcId()); @@ -843,8 +844,8 @@ void ServiceManagerContext::StartBrowserConnection() { } else if (!network_service_in_process) { // Start the network service process as soon as possible, since it is // critical to start up performance. - browser_connection->GetConnector()->StartService( - mojom::kNetworkServiceName); + browser_connection->GetConnector()->WarmService( + service_manager::ServiceFilter::ByName(mojom::kNetworkServiceName)); } } diff --git a/content/common/service_manager/child_connection.cc b/content/common/service_manager/child_connection.cc index dd7b1bd3e483ea..58f8c26fba99f7 100644 --- a/content/common/service_manager/child_connection.cc +++ b/content/common/service_manager/child_connection.cc @@ -84,8 +84,8 @@ class ChildConnection::IOThreadContext auto pid_receiver_request = mojo::MakeRequest(&pid_receiver_); if (connector_) { - connector_->StartService(child_identity, std::move(service), - std::move(pid_receiver_request)); + connector_->RegisterServiceInstance(child_identity, std::move(service), + std::move(pid_receiver_request)); connector_->BindInterface(child_identity, &child_); } } diff --git a/content/public/browser/render_process_host.h b/content/public/browser/render_process_host.h index c4a44ad4fb0e63..222d83bfbc58e3 100644 --- a/content/public/browser/render_process_host.h +++ b/content/public/browser/render_process_host.h @@ -36,6 +36,7 @@ class GURL; namespace base { class SharedPersistentMemoryAllocator; class TimeDelta; +class Token; } namespace service_manager { @@ -516,11 +517,11 @@ class CONTENT_EXPORT RenderProcessHost : public IPC::Sender, // not correspond to a live RenderProcessHost. static RenderProcessHost* FromID(int render_process_id); - // Returns the RenderProcessHost given its renderer's service Identity. - // Returns nullptr if the Identity does not correspond to a live - // RenderProcessHost. - static RenderProcessHost* FromRendererIdentity( - const service_manager::Identity& identity); + // Returns the RenderProcessHost given its renderer's service instance ID, + // generated randomly when launching the renderer. Returns nullptr if the + // instance does not correspond to a live RenderProcessHost. + static RenderProcessHost* FromRendererInstanceId( + const base::Token& instance_id); // Returns whether the process-per-site model is in use (globally or just for // the current site), in which case we should ensure there is only one diff --git a/content/renderer/render_thread_impl_browsertest.cc b/content/renderer/render_thread_impl_browsertest.cc index 9d21811ab5d204..e94b3fbcaa1e86 100644 --- a/content/renderer/render_thread_impl_browsertest.cc +++ b/content/renderer/render_thread_impl_browsertest.cc @@ -56,6 +56,7 @@ #include "mojo/core/embedder/embedder.h" #include "mojo/core/embedder/scoped_ipc_support.h" #include "mojo/public/cpp/system/invitation.h" +#include "services/service_manager/public/cpp/constants.h" #include "testing/gtest/include/gtest/gtest.h" #include "third_party/blink/public/platform/scheduler/test/renderer_scheduler_test_support.h" #include "third_party/blink/public/platform/scheduler/web_thread_scheduler.h" @@ -191,13 +192,12 @@ class RenderThreadImplBrowserTest : public testing::Test { io_task_runner, mojo::core::ScopedIPCSupport::ShutdownPolicy::FAST)); shell_context_.reset(new TestServiceManagerContext); mojo::OutgoingInvitation invitation; - service_manager::Identity child_identity(mojom::kRendererServiceName, - base::nullopt /* instance_group */, - base::Token{}); - child_connection_.reset(new ChildConnection( - child_identity, &invitation, - ServiceManagerConnection::GetForProcess()->GetConnector(), - io_task_runner)); + child_connection_ = std::make_unique( + service_manager::Identity(mojom::kRendererServiceName, + service_manager::kSystemInstanceGroup, + base::Token{}, base::Token::CreateRandom()), + &invitation, ServiceManagerConnection::GetForProcess()->GetConnector(), + io_task_runner); mojo::MessagePipe pipe; child_connection_->BindInterface(IPC::mojom::ChannelBootstrap::Name_, diff --git a/ios/web/browser_state.mm b/ios/web/browser_state.mm index b0106ccdb0d52b..517dd9297726e0 100644 --- a/ios/web/browser_state.mm +++ b/ios/web/browser_state.mm @@ -262,12 +262,13 @@ explicit BrowserStateServiceManagerConnectionHolder( auto service_request = mojo::MakeRequest(&service); service_manager::mojom::PIDReceiverPtr pid_receiver; - service_manager::Identity identity(mojom::kBrowserServiceName, new_group); - service_manager_connection->GetConnector()->StartService( + service_manager::Identity identity(mojom::kBrowserServiceName, new_group, + base::Token{}, + base::Token::CreateRandom()); + service_manager_connection->GetConnector()->RegisterServiceInstance( identity, std::move(service), mojo::MakeRequest(&pid_receiver)); pid_receiver->SetPID(base::GetCurrentProcId()); - service_manager_connection->GetConnector()->StartService(identity); auto connection_holder = std::make_unique( std::move(service_request)); diff --git a/ios/web/service_manager_context.mm b/ios/web/service_manager_context.mm index 3bf63f879571c7..506c16f5888ae7 100644 --- a/ios/web/service_manager_context.mm +++ b/ios/web/service_manager_context.mm @@ -180,9 +180,10 @@ void ShutDownOnIOThread() { auto* browser_connection = ServiceManagerConnection::Get(); service_manager::mojom::PIDReceiverPtr pid_receiver; - packaged_services_connection_->GetConnector()->StartService( + packaged_services_connection_->GetConnector()->RegisterServiceInstance( service_manager::Identity(mojom::kBrowserServiceName, - service_manager::kSystemInstanceGroup), + service_manager::kSystemInstanceGroup, + base::Token{}, base::Token::CreateRandom()), std::move(root_browser_service), mojo::MakeRequest(&pid_receiver)); pid_receiver->SetPID(base::GetCurrentProcId()); diff --git a/mash/session/session.cc b/mash/session/session.cc index c0cf5090d96337..598e10d34ed243 100644 --- a/mash/session/session.cc +++ b/mash/session/session.cc @@ -28,7 +28,9 @@ void Session::OnStart() { // (crbug.com/594852) if (base::CommandLine::ForCurrentProcess()->HasSwitch( quick_launch::mojom::kServiceName)) { - context()->connector()->StartService(quick_launch::mojom::kServiceName); + // TODO(https://crbug.com/904148): This should not use |WarmService()|. + context()->connector()->WarmService(service_manager::ServiceFilter::ByName( + quick_launch::mojom::kServiceName)); } #endif // defined(OS_CHROMEOS) } @@ -36,7 +38,8 @@ void Session::OnStart() { void Session::StartWindowManager() { // TODO(beng): monitor this service for death & bring down the whole system // if necessary. - context()->connector()->StartService(common::GetWindowManagerServiceName()); + context()->connector()->WarmService(service_manager::ServiceFilter::ByName( + common::GetWindowManagerServiceName())); } } // namespace session diff --git a/services/preferences/pref_service_factory_unittest.cc b/services/preferences/pref_service_factory_unittest.cc index cccf3e1f01a09a..362ec579a1d98a 100644 --- a/services/preferences/pref_service_factory_unittest.cc +++ b/services/preferences/pref_service_factory_unittest.cc @@ -115,7 +115,10 @@ class PrefServiceFactoryTest : public service_manager::test::ServiceTest { base::Unretained(this), run_loop.QuitClosure()); ServiceTest::SetUp(); ASSERT_TRUE(profile_dir_.CreateUniqueTempDir()); - connector()->StartService("prefs_unittest_helper"); + + // TODO(https://crbug.com/904148): This should not use |WarmService()|. + connector()->WarmService( + service_manager::ServiceFilter::ByName("prefs_unittest_helper")); run_loop.Run(); } diff --git a/services/resource_coordinator/resource_coordinator_service_unittest.cc b/services/resource_coordinator/resource_coordinator_service_unittest.cc index da5f4bb5013bab..bf8fe8d9d9ad94 100644 --- a/services/resource_coordinator/resource_coordinator_service_unittest.cc +++ b/services/resource_coordinator/resource_coordinator_service_unittest.cc @@ -53,12 +53,6 @@ class ResourceCoordinatorTest : public service_manager::test::ServiceTest { TestCUImpl(cu); } - protected: - void SetUp() override { - service_manager::test::ServiceTest::SetUp(); - connector()->StartService(mojom::kServiceName); - } - private: base::RunLoop* loop_ = nullptr; diff --git a/services/service_manager/background/tests/background_service_manager_unittest.cc b/services/service_manager/background/tests/background_service_manager_unittest.cc index 046b8e8666c390..55cecc4c452909 100644 --- a/services/service_manager/background/tests/background_service_manager_unittest.cc +++ b/services/service_manager/background/tests/background_service_manager_unittest.cc @@ -54,10 +54,13 @@ TEST(BackgroundServiceManagerTest, MAYBE_Basic) { ServiceContext service_context(std::make_unique(), mojo::MakeRequest(&service)); background_service_manager.RegisterService( - Identity(kTestName, kSystemInstanceGroup), std::move(service), nullptr); + Identity(kTestName, kSystemInstanceGroup, base::Token{}, + base::Token::CreateRandom()), + std::move(service), nullptr); mojom::TestServicePtr test_service; - service_context.connector()->BindInterface(kAppName, &test_service); + service_context.connector()->BindInterface(ServiceFilter::ByName(kAppName), + &test_service); base::RunLoop run_loop; bool got_result = false; test_service->Test( diff --git a/services/service_manager/connect_params.cc b/services/service_manager/connect_params.cc index 44a642892f1e8b..7c0794052ab171 100644 --- a/services/service_manager/connect_params.cc +++ b/services/service_manager/connect_params.cc @@ -8,8 +8,8 @@ namespace service_manager { ConnectParams::ConnectParams() {} ConnectParams::~ConnectParams() { - if (!start_service_callback_.is_null()) - std::move(start_service_callback_).Run(result_, resolved_identity_); + if (!connection_callback_.is_null()) + std::move(connection_callback_).Run(result_, resolved_identity_); } } // namespace service_manager diff --git a/services/service_manager/connect_params.h b/services/service_manager/connect_params.h index a732ac684ed9aa..830af18626890e 100644 --- a/services/service_manager/connect_params.h +++ b/services/service_manager/connect_params.h @@ -10,6 +10,7 @@ #include "base/callback.h" #include "base/macros.h" +#include "base/optional.h" #include "services/service_manager/public/cpp/connector.h" #include "services/service_manager/public/cpp/identity.h" #include "services/service_manager/public/mojom/connector.mojom.h" @@ -21,6 +22,10 @@ namespace service_manager { // application. class ConnectParams { public: + using ConnectionCallback = + base::OnceCallback& identity)>; + ConnectParams(); ~ConnectParams(); @@ -61,13 +66,12 @@ class ConnectParams { return std::move(interface_pipe_); } - void set_start_service_callback( - mojom::Connector::StartServiceCallback callback) { - start_service_callback_ = std::move(callback); + void set_connection_callback(ConnectionCallback callback) { + connection_callback_ = std::move(callback); } void set_response_data(mojom::ConnectResult result, - const Identity& resolved_identity) { + const base::Optional& resolved_identity) { result_ = result; resolved_identity_ = resolved_identity; } @@ -83,12 +87,12 @@ class ConnectParams { mojom::PIDReceiverRequest pid_receiver_request_; std::string interface_name_; mojo::ScopedMessagePipeHandle interface_pipe_; - mojom::Connector::StartServiceCallback start_service_callback_; + ConnectionCallback connection_callback_; - // These values are supplied to the response callback for StartService()/ + // These values are supplied to the response callback for WarmService()/ // BindInterface() etc. when the connection is completed. mojom::ConnectResult result_ = mojom::ConnectResult::INVALID_ARGUMENT; - Identity resolved_identity_; + base::Optional resolved_identity_; DISALLOW_COPY_AND_ASSIGN(ConnectParams); }; diff --git a/services/service_manager/connect_util.cc b/services/service_manager/connect_util.cc index f8636f4b9dd105..482b598dd39b08 100644 --- a/services/service_manager/connect_util.cc +++ b/services/service_manager/connect_util.cc @@ -23,7 +23,7 @@ mojo::ScopedMessagePipeHandle BindInterface( params->set_target(target); mojo::MessagePipe pipe; params->set_interface_request_info(interface_name, std::move(pipe.handle1)); - params->set_start_service_callback(base::DoNothing()); + params->set_connection_callback(base::DoNothing()); service_manager->Connect(std::move(params)); return std::move(pipe.handle0); } diff --git a/services/service_manager/public/cpp/connector.cc b/services/service_manager/public/cpp/connector.cc index 9f1d49979bfae5..48281c095e197f 100644 --- a/services/service_manager/public/cpp/connector.cc +++ b/services/service_manager/public/cpp/connector.cc @@ -8,9 +8,6 @@ namespace service_manager { -//////////////////////////////////////////////////////////////////////////////// -// Connector, public: - Connector::Connector(mojom::ConnectorPtrInfo unbound_state) : unbound_state_(std::move(unbound_state)), weak_factory_(this) { DETACH_FROM_SEQUENCE(sequence_checker_); @@ -30,45 +27,44 @@ std::unique_ptr Connector::Create(mojom::ConnectorRequest* request) { return std::make_unique(proxy.PassInterface()); } -void Connector::StartService(const Identity& identity) { +void Connector::WarmService(const ServiceFilter& filter, + WarmServiceCallback callback) { if (!BindConnectorIfNecessary()) return; - - connector_->StartService(identity, - base::Bind(&Connector::RunStartServiceCallback, - weak_factory_.GetWeakPtr())); + connector_->WarmService(filter, std::move(callback)); } -void Connector::StartService(const std::string& name) { - StartService(Identity(name, base::nullopt /* instance_group */)); -} - -void Connector::StartService(const Identity& identity, - mojom::ServicePtr service, - mojom::PIDReceiverRequest pid_receiver_request) { +void Connector::RegisterServiceInstance( + const Identity& identity, + mojom::ServicePtr service, + mojom::PIDReceiverRequest pid_receiver_request, + RegisterServiceInstanceCallback callback) { if (!BindConnectorIfNecessary()) return; + DCHECK(identity.instance_group() && !identity.instance_group()->is_zero()); + DCHECK(identity.instance_id()); + DCHECK(identity.globally_unique_id() && + !identity.globally_unique_id()->is_zero()); DCHECK(service.is_bound() && pid_receiver_request.is_pending()); - connector_->StartServiceWithProcess( + connector_->RegisterServiceInstance( identity, service.PassInterface().PassHandle(), - std::move(pid_receiver_request), - base::BindOnce(&Connector::RunStartServiceCallback, - weak_factory_.GetWeakPtr())); + std::move(pid_receiver_request), std::move(callback)); } -void Connector::QueryService(const Identity& identity, +void Connector::QueryService(const std::string& service_name, mojom::Connector::QueryServiceCallback callback) { if (!BindConnectorIfNecessary()) return; - connector_->QueryService(identity, std::move(callback)); + connector_->QueryService(service_name, std::move(callback)); } -void Connector::BindInterface(const Identity& target, +void Connector::BindInterface(const ServiceFilter& filter, const std::string& interface_name, - mojo::ScopedMessagePipeHandle interface_pipe) { - auto service_overrides_iter = local_binder_overrides_.find(target); + mojo::ScopedMessagePipeHandle interface_pipe, + BindInterfaceCallback callback) { + auto service_overrides_iter = local_binder_overrides_.find(filter); if (service_overrides_iter != local_binder_overrides_.end()) { auto override_iter = service_overrides_iter->second.find(interface_name); if (override_iter != service_overrides_iter->second.end()) { @@ -80,9 +76,8 @@ void Connector::BindInterface(const Identity& target, if (!BindConnectorIfNecessary()) return; - connector_->BindInterface(target, interface_name, std::move(interface_pipe), - base::Bind(&Connector::RunStartServiceCallback, - weak_factory_.GetWeakPtr())); + connector_->BindInterface(filter, interface_name, std::move(interface_pipe), + std::move(callback)); } std::unique_ptr Connector::Clone() { @@ -117,50 +112,40 @@ base::WeakPtr Connector::GetWeakPtr() { return weak_factory_.GetWeakPtr(); } -//////////////////////////////////////////////////////////////////////////////// -// Connector, private: - -void Connector::OnConnectionError() { - DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); - connector_.reset(); -} - void Connector::OverrideBinderForTesting( - const service_manager::Identity& identity, + const service_manager::ServiceFilter& filter, const std::string& interface_name, const TestApi::Binder& binder) { - local_binder_overrides_[identity][interface_name] = binder; + local_binder_overrides_[filter][interface_name] = binder; } -bool Connector::HasBinderOverride(const service_manager::Identity& identity, - const std::string& interface_name) { - auto service_overrides = local_binder_overrides_.find(identity); +bool Connector::HasBinderOverrideForTesting( + const service_manager::ServiceFilter& filter, + const std::string& interface_name) { + auto service_overrides = local_binder_overrides_.find(filter); if (service_overrides == local_binder_overrides_.end()) return false; return base::ContainsKey(service_overrides->second, interface_name); } -void Connector::ClearBinderOverride(const service_manager::Identity& identity, - const std::string& interface_name) { - auto service_overrides = local_binder_overrides_.find(identity); +void Connector::ClearBinderOverrideForTesting( + const service_manager::ServiceFilter& filter, + const std::string& interface_name) { + auto service_overrides = local_binder_overrides_.find(filter); if (service_overrides == local_binder_overrides_.end()) return; service_overrides->second.erase(interface_name); } -void Connector::ClearBinderOverrides() { +void Connector::ClearBinderOverridesForTesting() { local_binder_overrides_.clear(); } -void Connector::SetStartServiceCallback( - const Connector::StartServiceCallback& callback) { - start_service_callback_ = callback; -} - -void Connector::ResetStartServiceCallback() { - start_service_callback_.Reset(); +void Connector::OnConnectionError() { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + connector_.reset(); } bool Connector::BindConnectorIfNecessary() { @@ -183,11 +168,4 @@ bool Connector::BindConnectorIfNecessary() { return true; } -void Connector::RunStartServiceCallback( - mojom::ConnectResult result, - const base::Optional& identity) { - if (!start_service_callback_.is_null()) - start_service_callback_.Run(result, identity); -} - } // namespace service_manager diff --git a/services/service_manager/public/cpp/connector.h b/services/service_manager/public/cpp/connector.h index a73bb2ff898fc4..66da043facc86d 100644 --- a/services/service_manager/public/cpp/connector.h +++ b/services/service_manager/public/cpp/connector.h @@ -28,39 +28,33 @@ namespace service_manager { // instance to the desired thread before calling any public methods on it. class SERVICE_MANAGER_PUBLIC_CPP_EXPORT Connector { public: - using StartServiceCallback = - base::RepeatingCallback&)>; - + // DEPRECATED: Do not introduce new uses of this API. Just call the public + // *ForTesting methods directly on the Connector. Also see the note on those + // methods about preferring TestConnectorFactory where feasible. class TestApi { public: using Binder = base::RepeatingCallback; explicit TestApi(Connector* connector) : connector_(connector) {} - ~TestApi() { connector_->ResetStartServiceCallback(); } + ~TestApi() = default; // Allows caller to specify a callback to bind requests for |interface_name| // from |service_name| locally, rather than passing the request through the // Service Manager. - void OverrideBinderForTesting(const service_manager::Identity& identity, + void OverrideBinderForTesting(const service_manager::ServiceFilter& filter, const std::string& interface_name, const Binder& binder) { - connector_->OverrideBinderForTesting(identity, interface_name, binder); + connector_->OverrideBinderForTesting(filter, interface_name, binder); } - bool HasBinderOverride(const service_manager::Identity& identity, + bool HasBinderOverride(const service_manager::ServiceFilter& filter, const std::string& interface_name) { - return connector_->HasBinderOverride(identity, interface_name); + return connector_->HasBinderOverrideForTesting(filter, interface_name); } - void ClearBinderOverride(const service_manager::Identity& identity, + void ClearBinderOverride(const service_manager::ServiceFilter& filter, const std::string& interface_name) { - connector_->ClearBinderOverride(identity, interface_name); + connector_->ClearBinderOverrideForTesting(filter, interface_name); } - void ClearBinderOverrides() { connector_->ClearBinderOverrides(); } - - // Register a callback to be run with the result of an attempt to start a - // service. This will be run in response to calls to StartService() or - // BindInterface(). - void SetStartServiceCallback(const StartServiceCallback& callback) { - connector_->SetStartServiceCallback(callback); + void ClearBinderOverrides() { + connector_->ClearBinderOverridesForTesting(); } private: @@ -75,60 +69,115 @@ class SERVICE_MANAGER_PUBLIC_CPP_EXPORT Connector { // for the other end the Connector's interface. static std::unique_ptr Create(mojom::ConnectorRequest* request); - // Creates an instance of a service for |identity|. - void StartService(const Identity& identity); - - // Creates an instance of the service |name| inheriting the caller's identity. - void StartService(const std::string& name); + // Asks the Service Manager to ensure that there's a running service instance + // which would match |filter| from the caller's perspective. Useful when + // future |BindInterface()| calls will be made with the same |filter| and + // reducing their latency is a high priority. + // + // Note that there is no value in calling this *immediately* before + // |BindInterface()|, so only use it if there is some delay between when you + // know you will want to talk to a service and when you actually need to for + // the first time. For example, Chrome warms the Network Service ASAP on + // startup so that it can be brought up in parallel while the browser is + // initializing other subsystems. + // + // |callback| conveys information about the result of the request. See + // documentation on the mojom ConnectResult definition for the meaning of + // |result|. If |result| is |SUCCEEDED|, then |identity| will contain the full + // identity of the matching service instance, which was either already running + // or was started as a result of this request. + using WarmServiceCallback = + base::OnceCallback& identity)>; + void WarmService(const ServiceFilter& filter, + WarmServiceCallback callback = {}); // Creates an instance of a service for |identity| in a process started by the - // client (or someone else). Must be called before BindInterface() may be - // called to |identity|. - void StartService(const Identity& identity, - mojom::ServicePtr service, - mojom::PIDReceiverRequest pid_receiver_request); - - // Determines if the service for |Identity| is known, and returns information - // about it from the catalog. - void QueryService(const Identity& identity, + // client (or someone else). Must be called before any |BindInterface()| + // reaches the Service Manager expecting the instance to exist, otherwise the + // Service Manager may attempt to start a new instance on its own, which will + // invariably fail (if the Service Manager knew how to start the instance, the + // caller wouldn't need to use this API). + // + // NOTE: |identity| must be a complete service Identity (including a random + // globally unique ID), and this call will only succeed if the calling service + // has set |can_create_other_service_instances| option to |true| in its + // manifest. This is considered privileged behavior. + using RegisterServiceInstanceCallback = + base::OnceCallback; + void RegisterServiceInstance(const Identity& identity, + mojom::ServicePtr service, + mojom::PIDReceiverRequest pid_receiver_request, + RegisterServiceInstanceCallback callback = {}); + + // Determines if the service for |service_name| is known, and returns + // information about it from the catalog. + void QueryService(const std::string& service_name, mojom::Connector::QueryServiceCallback callback); - // Connect to |target| & request to bind |Interface|. + // All variants of |BindInterface()| ask the Service Manager to route an + // interface request to a service instance matching |filter|. If no running + // service instance matches |filter|, the Service Manager may start a new + // service instance which does, and then route the request to it. See the + // comments in connector.mojom regarding restrictions on when the Service + // Manager will *not* create a new instance or when |filter| may otherwise be + // rejected. + // + // For variants which take a callback, |callback| conveys information about + // the result of the request. See documentation on the mojom ConnectResult + // definition for the meaning of |result|. If |result| is |SUCCEEDED|, then + // |identity| will contain the full identity of the matching service instance + // to which the interface request was routed. This service instance was either + // already running, or was started as a result of this request. + // + // TODO: We should consider removing some of these overloads as they're quite + // redundant. It would be cleaner to just have callers explicitly construct a + // ServiceFilter and InterfaceRequest rather than having a bunch of template + // helpers to do the same in various combinations. The first and last variants + // of |BindInterface()| here are sufficient for all use cases. + using BindInterfaceCallback = + base::OnceCallback& identity)>; template - void BindInterface(const Identity& target, - mojo::InterfacePtr* ptr) { - mojo::MessagePipe pipe; - ptr->Bind(mojo::InterfacePtrInfo(std::move(pipe.handle0), 0u)); - BindInterface(target, Interface::Name_, std::move(pipe.handle1)); + void BindInterface(const ServiceFilter& filter, + mojo::InterfaceRequest request, + BindInterfaceCallback callback = {}) { + BindInterface(filter, Interface::Name_, request.PassMessagePipe(), + std::move(callback)); } + template - void BindInterface(const Identity& target, - mojo::InterfaceRequest request) { - BindInterface(target, Interface::Name_, request.PassMessagePipe()); + void BindInterface(const ServiceFilter& filter, + mojo::InterfacePtr* ptr) { + BindInterface(filter, mojo::MakeRequest(ptr)); } + template - void BindInterface(const std::string& name, + void BindInterface(const std::string& service_name, mojo::InterfacePtr* ptr) { - return BindInterface(Identity(name, base::nullopt /* instance_group */), - ptr); + return BindInterface(ServiceFilter::ByName(service_name), + mojo::MakeRequest(ptr)); } + template - void BindInterface(const std::string& name, + void BindInterface(const std::string& service_name, mojo::InterfaceRequest request) { - return BindInterface(Identity(name, base::nullopt /* instance_group */), - Interface::Name_, request.PassMessagePipe()); + return BindInterface(ServiceFilter::ByName(service_name), + std::move(request)); } - void BindInterface(const Identity& target, + + void BindInterface(const ServiceFilter& filter, const std::string& interface_name, - mojo::ScopedMessagePipeHandle interface_pipe); + mojo::ScopedMessagePipeHandle interface_pipe, + BindInterfaceCallback callback = {}); // Creates a new instance of this class which may be passed to another thread. - // The returned object may be passed multiple times until StartService() or - // BindInterface() is called, at which point this method must be called again - // to pass again. + // The returned object may be passed across sequences until any of its public + // methods are called, at which point it becomes bound to the calling + // sequence. std::unique_ptr Clone(); - // Returns true if this Connector instance is already bound to a thread. + // Returns |true| if this Connector instance is already bound to a thread. bool IsBound() const; void FilterInterfaces(const std::string& spec, @@ -141,36 +190,33 @@ class SERVICE_MANAGER_PUBLIC_CPP_EXPORT Connector { base::WeakPtr GetWeakPtr(); + // Test-only methods for interception of requests on Connectors. Consider + // using TestConnectorFactory to create and inject fake Connectors into + // production code instead of using these methods to monkey-patch existing + // Connector objects. + void OverrideBinderForTesting(const service_manager::ServiceFilter& filter, + const std::string& interface_name, + const TestApi::Binder& binder); + bool HasBinderOverrideForTesting(const service_manager::ServiceFilter& filter, + const std::string& interface_name); + void ClearBinderOverrideForTesting( + const service_manager::ServiceFilter& filter, + const std::string& interface_name); + void ClearBinderOverridesForTesting(); + private: using BinderOverrideMap = std::map; void OnConnectionError(); - - void OverrideBinderForTesting(const service_manager::Identity& identity, - const std::string& interface_name, - const TestApi::Binder& binder); - bool HasBinderOverride(const service_manager::Identity& identity, - const std::string& interface_name); - void ClearBinderOverride(const service_manager::Identity& identity, - const std::string& interface_name); - void ClearBinderOverrides(); - void SetStartServiceCallback(const StartServiceCallback& callback); - void ResetStartServiceCallback(); - bool BindConnectorIfNecessary(); - // Callback passed to mojom methods StartService()/BindInterface(). - void RunStartServiceCallback(mojom::ConnectResult result, - const base::Optional& user_id); - mojom::ConnectorPtrInfo unbound_state_; mojom::ConnectorPtr connector_; SEQUENCE_CHECKER(sequence_checker_); - std::map + std::map local_binder_overrides_; - StartServiceCallback start_service_callback_; base::WeakPtrFactory weak_factory_; diff --git a/services/service_manager/public/cpp/service_filter.cc b/services/service_manager/public/cpp/service_filter.cc index 82fc2a7933f7d9..76f04eaa2d012b 100644 --- a/services/service_manager/public/cpp/service_filter.cc +++ b/services/service_manager/public/cpp/service_filter.cc @@ -61,6 +61,13 @@ ServiceFilter ServiceFilter::ForExactIdentity(const Identity& identity) { identity.instance_id(), identity.globally_unique_id()); } +bool ServiceFilter::operator<(const ServiceFilter& other) const { + return std::tie(service_name_, instance_group_, instance_id_, + globally_unique_id_) < + std::tie(other.service_name_, other.instance_group_, + other.instance_id_, other.globally_unique_id_); +} + ServiceFilter::ServiceFilter( const std::string& service_name, const base::Optional& instance_group, diff --git a/services/service_manager/public/cpp/service_filter.h b/services/service_manager/public/cpp/service_filter.h index ff231f66dfe8f3..f57526fbd79ebd 100644 --- a/services/service_manager/public/cpp/service_filter.h +++ b/services/service_manager/public/cpp/service_filter.h @@ -110,6 +110,8 @@ class COMPONENT_EXPORT(SERVICE_MANAGER_CPP_TYPES) ServiceFilter { globally_unique_id_ = globally_unique_id; } + bool operator<(const ServiceFilter& other) const; + private: ServiceFilter(const std::string& service_name, const base::Optional& instance_group, diff --git a/services/service_manager/public/cpp/test/test_connector_factory.cc b/services/service_manager/public/cpp/test/test_connector_factory.cc index e0814b3ba593f4..efe106cf3badff 100644 --- a/services/service_manager/public/cpp/test/test_connector_factory.cc +++ b/services/service_manager/public/cpp/test/test_connector_factory.cc @@ -98,21 +98,21 @@ class TestConnectorImplBase : public mojom::Connector { std::move(callback).Run(mojom::ConnectResult::SUCCEEDED, Identity()); } - void StartService(const ServiceFilter& service_filter, - StartServiceCallback callback) override { + void WarmService(const ServiceFilter& service_filter, + WarmServiceCallback callback) override { NOTREACHED(); } - void QueryService(const ServiceFilter& service_filter, + void QueryService(const std::string& service_name, QueryServiceCallback callback) override { NOTREACHED(); } - void StartServiceWithProcess( + void RegisterServiceInstance( const Identity& identity, mojo::ScopedMessagePipeHandle service, mojom::PIDReceiverRequest pid_receiver_request, - StartServiceWithProcessCallback callback) override { + RegisterServiceInstanceCallback callback) override { NOTREACHED(); } @@ -235,21 +235,21 @@ class ProxiedServiceConnector : public mojom::Connector { std::move(callback).Run(mojom::ConnectResult::SUCCEEDED, Identity()); } - void StartService(const ServiceFilter& target, - StartServiceCallback callback) override { + void WarmService(const ServiceFilter& filter, + WarmServiceCallback callback) override { NOTREACHED(); } - void QueryService(const ServiceFilter& target, + void QueryService(const std::string& service_name, QueryServiceCallback callback) override { NOTREACHED(); } - void StartServiceWithProcess( + void RegisterServiceInstance( const Identity& identity, mojo::ScopedMessagePipeHandle service, mojom::PIDReceiverRequest pid_receiver_request, - StartServiceWithProcessCallback callback) override { + RegisterServiceInstanceCallback callback) override { NOTREACHED(); } diff --git a/services/service_manager/public/mojom/connector.mojom b/services/service_manager/public/mojom/connector.mojom index a866ed5cc21be0..cb036e4097cf77 100644 --- a/services/service_manager/public/mojom/connector.mojom +++ b/services/service_manager/public/mojom/connector.mojom @@ -10,7 +10,7 @@ import "services/service_manager/public/mojom/interface_provider.mojom"; import "services/service_manager/public/mojom/service_filter.mojom"; // TODO(beng): Evaluate the utility of this enum. There are some inconsistencies -// in its use with BindInterface/StartService. +// in its use with BindInterface/WarmService. enum ConnectResult { // The operation was completed successfully. SUCCEEDED, @@ -81,6 +81,16 @@ struct Identity { mojo_base.mojom.Token? globally_unique_id; }; +// Static metadata about a service the Service Manager knows about. This is +// information pertaining to the service definition itself (e.g. registered +// manifest contents), not to any particular service instance running in the +// system. +struct ServiceInfo { + // A string describing the kind of sandboxing the service has declared for + // itself in its manifest. + string sandbox_type; +}; + // Implemented by an object in the service manager associated with a specific // instance. Tells the service manager the PID for a process launched by the // client. @@ -125,65 +135,57 @@ interface Connector { handle interface_pipe) => (ConnectResult result, Identity? identity); - // Asks the service manager whether it knows how to manage a given service, - // and how the service is to be sandboxed per the mainifest. No processes are - // started as a result. QueryService() is typically used before a call to - // StartProcessWithService() so the client can determine how to launch a - // process. - // - // |filter| is interpreted similarly to |BindInterface()| above. - // - // Response parameters: - // - // result - // Indicates the result of the QueryService() operation. - // - // sandbox_type - // A string describing the kind of sandboxing the service has declared - // for itself in the manifest. - // - QueryService(ServiceFilter filter) - => (ConnectResult result, string sandbox_type); - - // Asks the Service Manager to create a service instance which would match - // |filter|. This is essentially equivalent to |BindInterface()| above but - // no interface request given to bind. This may be used to elicit service - // launching if no matching service instance is already running. See comments - // on |BindInterface()| above for details on filtering constraints, instance - // creation, and response values. - StartService(ServiceFilter filter) + // Asks the Service Manager for details about the service named + // |service_name|. No processes are started as a result of this request. On + // response, |info| provides information about the service. If the Service + // Manager is not familiar the named service, |info| will be null. + QueryService(string service_name) => (ServiceInfo? info); + + // Asks the Service Manager to ensure that a service instance which would + // match |filter| (from the caller's perspective) is running. This is + // essentially equivalent to |BindInterface()| above but no interface request + // is given to bind. This is mainly here as a performance measure, as a client + // may wish to ensure that a service is warmed up by the time the client + // actually needs it. + // + // See comments on |BindInterface()| above for details on filtering + // constraints, instance creation, and response values. + WarmService(ServiceFilter filter) => (ConnectResult result, Identity? identity); - // Typically, the service manager will start a process for a service the first - // time it receives a bind interface request for it, or when StartService() is - // called. This struct allows a client to start the process itself and provide - // the service manager the pipes it needs to communicate with it. When this - // function is called, the client owns the lifetime of the child process it - // started, not the service manager. The service manager binds the |service| - // pipe, and when it closes destroys the associated instance but the process - // stays alive. + // Typically, the Service Manager will start a process for a service the first + // time it receives a |BindInterface()| or |WarmService()| request for a + // filter it can't match to an existing instance. // - // Parameters: + // This call allows new service instances to be explicitly injected into the + // Service Manager's set of running services. As such, this is considered to + // be a highly privileged API. A service calling this API must have the + // |can_create_other_service_instances| option set to |true| in its own + // manifest or the request will be rejected. // - // target - // The identity of the service to create the instance for. + // |identity| is the full identity of the service instance to introduce. This + // must include a valid instance group and a valid, random, globally unique + // ID. // - // service - // A pipe to an implementation of Service that the service manager can use - // to communicate with the service. + // |service| is a |Service| interface pipe (i.e. a ServicePtr in C++). // - // pid_receiver_request - // Allows the client process launcher to tell the service manager the PID of - // the process it created (the pid isn't supplied directly here as the - // process may not have been launched by the time BindInterface() is - // called.) + // |pid_receiver_request| is the receiving end of a PIDReceiver pipe. The + // caller is expected to call |SetPid()| on the other end of this pipe in + // order to inform the Service Manager about which process is actually hosting + // the service instance. // - StartServiceWithProcess(Identity target, + // TODO(https://crbug.com/904137): We should be able to be more typesafe here + // for the |service| argument, but Service's definition requires Connector's + // definition and we don't yet support circular mojom imports. Hence the raw + // message pipe. + RegisterServiceInstance(Identity identity, handle service, PIDReceiver& pid_receiver_request) => - (ConnectResult result, Identity? identity); + (ConnectResult result); - // Clones this Connector so it can be passed to another thread. + // Binds another Connector pipe to this same implementation. This effectively + // allows the client to "clone" its Connector arbitrarily many times, for + // example to mint Connector endpoints for different threads to use. Clone(Connector& request); // Filter interface requests received from |source| according to the policy @@ -197,6 +199,17 @@ interface Connector { // in |target|. The Service Manager will only forward interface requests that // were permitted by intersecting |source|'s manifest requirements with the // contents of |spec|. + // + // DEPRECATED: Please do not introduce new uses of this API, as it will be + // removed soon. Please instead avoid using InterfaceProvider altogether in + // favor of explicit factory interfaces. i.e., instead of an InterfaceProvider + // hooked up to a BinderRegistry, prefer to define something like: + // + // interface MyInterfaceBroker { + // GetFoo(Foo& request); + // GetBar(Bar& request); + // }; + // FilterInterfaces(string spec, Identity source, InterfaceProvider& source_request, diff --git a/services/service_manager/service_manager.cc b/services/service_manager/service_manager.cc index 7df7d2b6ac580a..55662432e69ecb 100644 --- a/services/service_manager/service_manager.cc +++ b/services/service_manager/service_manager.cc @@ -468,23 +468,24 @@ class ServiceManager::Instance params->set_target(target); params->set_interface_request_info(interface_name, std::move(interface_pipe)); - params->set_start_service_callback(std::move(callback)); + params->set_connection_callback(std::move(callback)); service_manager_->Connect(std::move(params)); } - void QueryService(const ServiceFilter& service_filter, + void QueryService(const std::string& service_name, QueryServiceCallback callback) override { - std::string sandbox; + std::string sandbox_type; bool success = service_manager_->QueryCatalog( - Identity(service_filter.service_name(), identity_.instance_group()), - &sandbox); - std::move(callback).Run(success ? mojom::ConnectResult::SUCCEEDED - : mojom::ConnectResult::INVALID_ARGUMENT, - std::move(sandbox)); + Identity(service_name, identity_.instance_group()), &sandbox_type); + if (success) { + std::move(callback).Run(mojom::ServiceInfo::New(sandbox_type)); + } else { + std::move(callback).Run(nullptr); + } } - void StartService(const ServiceFilter& service_filter, - StartServiceCallback callback) override { + void WarmService(const ServiceFilter& service_filter, + WarmServiceCallback callback) override { Identity target( service_filter.service_name(), service_filter.instance_group(), service_filter.instance_id(), service_filter.globally_unique_id()); @@ -499,32 +500,37 @@ class ServiceManager::Instance std::unique_ptr params(new ConnectParams); params->set_source(identity_); params->set_target(target); - params->set_start_service_callback(std::move(callback)); + params->set_connection_callback(std::move(callback)); service_manager_->Connect(std::move(params)); } - void StartServiceWithProcess( - const Identity& in_target, + void RegisterServiceInstance( + const Identity& identity, mojo::ScopedMessagePipeHandle service_handle, mojom::PIDReceiverRequest pid_receiver_request, - StartServiceWithProcessCallback callback) override { - Identity target = in_target; + RegisterServiceInstanceCallback callback) override { + Identity target = identity; mojom::ServicePtr service; service.Bind(mojom::ServicePtrInfo(std::move(service_handle), 0)); mojom::ConnectResult result = ValidateConnectParams( &target, &service, &pid_receiver_request, nullptr); if (!Succeeded(result)) { - std::move(callback).Run(result, Identity()); + std::move(callback).Run(result); return; } std::unique_ptr params(new ConnectParams); params->set_source(identity_); params->set_target(target); - params->set_client_process_info(std::move(service), std::move(pid_receiver_request)); - params->set_start_service_callback(std::move(callback)); + params->set_connection_callback(base::BindOnce( + [](RegisterServiceInstanceCallback callback, + mojom::ConnectResult result, + const base::Optional& identity) { + std::move(callback).Run(result); + }, + std::move(callback))); service_manager_->Connect(std::move(params)); } @@ -1060,8 +1066,9 @@ void ServiceManager::Connect(std::unique_ptr params) { // If there was no existing instance but the source specified a specific // globally unique ID for the target, ignore the request. That instance is - // obviously no longer running. - if (params->target().globally_unique_id()) + // obviously no longer running. If it has "client info" (i.e. it's an + // explicit service instance registration), we allow it. + if (!params->HasClientProcessInfo() && params->target().globally_unique_id()) return; const catalog::Entry* entry = @@ -1106,7 +1113,17 @@ void ServiceManager::Connect(std::unique_ptr params) { instance_type = InstanceType::kRegular; Identity target; - if (instance_type == InstanceType::kSingleton) { + // |HasClientProcessInfo()| implies that this Connect call came from + // |Connector.RegisterServiceInstance()| or |ServiceManager::RegisterService| + // directly. In both cases, we are given an explicit Identity we should use + // and should therefore ignore the various overriding behaviors below. + if (params->HasClientProcessInfo()) { + source_identity_for_creation = params->source(); + target = original_target; + DCHECK(target.instance_group()); + DCHECK(target.instance_id()); + DCHECK(target.globally_unique_id()); + } else if (instance_type == InstanceType::kSingleton) { source_identity_for_creation = CreateServiceManagerIdentity(); target = Identity(params->target().name(), base::Token::CreateRandom(), base::Token{}); @@ -1229,10 +1246,12 @@ void ServiceManager::RegisterService( pid_receiver->SetPID(GetCurrentPid()); } - Identity target = identity; - DCHECK(target.instance_group()); - if (!target.instance_id()) - target.set_instance_id(base::Token{}); + DCHECK(identity.instance_group()); + Identity target( + identity.name(), identity.instance_group(), + identity.instance_id() ? identity.instance_id() : base::Token{}, + identity.globally_unique_id() ? identity.globally_unique_id() + : base::Token::CreateRandom()); params->set_source(target); params->set_target(target); params->set_client_process_info( @@ -1349,11 +1368,19 @@ ServiceManager::Instance* ServiceManager::CreateInstance( if (target.instance_id()) instance_id = *target.instance_id(); - // We add a newly minted globally unique ID to the target Identity here. This - // ensures that every new Instance has a globally unique ID as part of its - // Identity. + // We add a newly minted globally unique ID to the target Identity here if + // one wasn't provided. This ensures that every new Instance has a globally + // unique ID as part of its Identity. + base::Token globally_unique_id; + if (target.globally_unique_id()) { + DCHECK(!target.globally_unique_id()->is_zero()); + globally_unique_id = *target.globally_unique_id(); + } else { + globally_unique_id = base::Token::CreateRandom(); + } + Identity target_with_unique_id(target.name(), target.instance_group(), - instance_id, base::Token::CreateRandom()); + instance_id, globally_unique_id); auto instance = std::make_unique(this, target_with_unique_id, specs, options); Instance* raw_instance = instance.get(); diff --git a/services/service_manager/tests/connect/connect_test_app.cc b/services/service_manager/tests/connect/connect_test_app.cc index f9202bf548e9bd..746aa13e987d5e 100644 --- a/services/service_manager/tests/connect/connect_test_app.cc +++ b/services/service_manager/tests/connect/connect_test_app.cc @@ -172,17 +172,14 @@ class ConnectTestApp : public Service, void ConnectToClassAppWithIdentity( const service_manager::Identity& target, ConnectToClassAppWithIdentityCallback callback) override { - context()->connector()->StartService(target); mojom::ConnectResult result; base::Optional resolved_identity; - { - base::RunLoop loop; - Connector::TestApi test_api(context()->connector()); - test_api.SetStartServiceCallback(base::BindRepeating( - &OnConnectResult, loop.QuitClosure(), &result, &resolved_identity)); - base::MessageLoopCurrent::ScopedNestableTaskAllower allow; - loop.Run(); - } + base::RunLoop loop; + context()->connector()->WarmService( + target, base::BindOnce(&OnConnectResult, loop.QuitClosure(), &result, + &resolved_identity)); + base::MessageLoopCurrent::ScopedNestableTaskAllower allow; + loop.Run(); std::move(callback).Run(static_cast(result), *resolved_identity); } diff --git a/services/service_manager/tests/connect/connect_test_package.cc b/services/service_manager/tests/connect/connect_test_package.cc index ff7b7d2bbaa03a..a53fa7578c0f2e 100644 --- a/services/service_manager/tests/connect/connect_test_package.cc +++ b/services/service_manager/tests/connect/connect_test_package.cc @@ -133,16 +133,12 @@ class ProvidedService : public Service, void ConnectToClassAppWithIdentity( const service_manager::Identity& target, ConnectToClassAppWithIdentityCallback callback) override { - service_binding_.GetConnector()->StartService(target); mojom::ConnectResult result; base::Optional resolved_identity; - { - base::RunLoop loop(base::RunLoop::Type::kNestableTasksAllowed); - Connector::TestApi test_api(service_binding_.GetConnector()); - test_api.SetStartServiceCallback( - base::BindRepeating(&QuitLoop, &loop, &result, &resolved_identity)); - loop.Run(); - } + base::RunLoop loop(base::RunLoop::Type::kNestableTasksAllowed); + service_binding_.GetConnector()->WarmService( + target, base::BindOnce(&QuitLoop, &loop, &result, &resolved_identity)); + loop.Run(); std::move(callback).Run(static_cast(result), *resolved_identity); } diff --git a/services/service_manager/tests/connect/connect_unittest.cc b/services/service_manager/tests/connect/connect_unittest.cc index d564b7eceeea90..d2f94123e03e24 100644 --- a/services/service_manager/tests/connect/connect_unittest.cc +++ b/services/service_manager/tests/connect/connect_unittest.cc @@ -65,13 +65,10 @@ void ReceiveTwoStrings(std::string* out_string_1, loop->Quit(); } -void ReceiveQueryResult(mojom::ConnectResult* out_result, - std::string* out_string, +void ReceiveQueryResult(mojom::ServiceInfoPtr* out_info, base::RunLoop* loop, - mojom::ConnectResult in_result, - const std::string& in_string) { - *out_result = in_result; - *out_string = in_string; + mojom::ServiceInfoPtr info) { + *out_info = std::move(info); loop->Quit(); } @@ -79,7 +76,7 @@ void ReceiveConnectionResult(mojom::ConnectResult* out_result, Identity* out_target, base::RunLoop* loop, int32_t in_result, - const service_manager::Identity& in_identity) { + const Identity& in_identity) { *out_result = static_cast(in_result); *out_target = in_identity; loop->Quit(); @@ -332,29 +329,24 @@ TEST_F(ConnectTest, ConnectWithGloballyUniqueId) { } TEST_F(ConnectTest, QueryService) { - mojom::ConnectResult result; - std::string sandbox_type; + mojom::ServiceInfoPtr service_info; base::RunLoop run_loop; connector()->QueryService( - Identity(kTestSandboxedAppName, base::nullopt /* instance_group */, - base::Token{1, 2}), - base::BindOnce(&ReceiveQueryResult, &result, &sandbox_type, &run_loop)); + kTestSandboxedAppName, + base::BindOnce(&ReceiveQueryResult, &service_info, &run_loop)); run_loop.Run(); - EXPECT_EQ(mojom::ConnectResult::SUCCEEDED, result); - EXPECT_EQ("superduper", sandbox_type); + ASSERT_TRUE(service_info); + EXPECT_EQ("superduper", service_info->sandbox_type); } TEST_F(ConnectTest, QueryNonexistentService) { - mojom::ConnectResult result; - std::string sandbox_type; + mojom::ServiceInfoPtr service_info; base::RunLoop run_loop; connector()->QueryService( - Identity(kTestNonexistentAppName, base::nullopt /* instance_group */, - base::Token{1, 2}), - base::BindOnce(&ReceiveQueryResult, &result, &sandbox_type, &run_loop)); + kTestNonexistentAppName, + base::BindOnce(&ReceiveQueryResult, &service_info, &run_loop)); run_loop.Run(); - EXPECT_EQ(mojom::ConnectResult::INVALID_ARGUMENT, result); - EXPECT_EQ("", sandbox_type); + EXPECT_FALSE(service_info); } #if DCHECK_IS_ON() @@ -379,13 +371,13 @@ TEST_F(ConnectTest, MAYBE_BlockedInterface) { // Connects to an app provided by a package. TEST_F(ConnectTest, PackagedApp) { - test::mojom::ConnectTestServicePtr service_a; - connector()->BindInterface(kTestAppAName, &service_a); - Connector::TestApi test_api(connector()); base::Optional resolved_identity; - test_api.SetStartServiceCallback(base::BindRepeating( - &StartServiceResponse, nullptr, nullptr, &resolved_identity)); base::RunLoop run_loop; + test::mojom::ConnectTestServicePtr service_a; + connector()->BindInterface(ServiceFilter::ByName(kTestAppAName), + mojo::MakeRequest(&service_a), + base::BindOnce(&StartServiceResponse, nullptr, + nullptr, &resolved_identity)); std::string a_name; service_a->GetTitle(base::Bind(&ReceiveOneString, &a_name, &run_loop)); run_loop.Run(); @@ -444,13 +436,12 @@ TEST_F(ConnectTest, MAYBE_PackagedApp_BlockedInterface) { // Connection to another application provided by the same package, blocked // because it's not in the capability filter whitelist. TEST_F(ConnectTest, MAYBE_BlockedPackagedApplication) { - test::mojom::ConnectTestServicePtr service_b; - connector()->BindInterface(kTestAppBName, &service_b); - Connector::TestApi test_api(connector()); mojom::ConnectResult result; - test_api.SetStartServiceCallback( - base::Bind(&StartServiceResponse, nullptr, &result, nullptr)); base::RunLoop run_loop; + test::mojom::ConnectTestServicePtr service_b; + connector()->BindInterface( + ServiceFilter::ByName(kTestAppBName), mojo::MakeRequest(&service_b), + base::BindOnce(&StartServiceResponse, nullptr, &result, nullptr)); service_b.set_connection_error_handler(run_loop.QuitClosure()); run_loop.Run(); EXPECT_EQ(mojom::ConnectResult::ACCESS_DENIED, result); @@ -556,8 +547,8 @@ TEST_F(ConnectTest, ConnectToClientProcess_Blocked) { #else "connect_test_exe", #endif - service_manager::Identity("connect_test_exe", - base::nullopt /* instance_group */), + service_manager::Identity("connect_test_exe", kSystemInstanceGroup, + base::Token{}, base::Token::CreateRandom()), connector(), &process); EXPECT_EQ(result, mojom::ConnectResult::ACCESS_DENIED); } @@ -566,37 +557,36 @@ TEST_F(ConnectTest, ConnectToClientProcess_Blocked) { // "instance_sharing" option can receive connections from clients run as other // users. TEST_F(ConnectTest, AllUsersSingleton) { - // Connect to an instance with an explicitly different user_id. This supplied - // user id should be ignored by the service manager (which will generate its - // own synthetic user id for all-user singleton instances). - const base::Token singleton_instance_group = base::Token::CreateRandom(); - Identity singleton_id(kTestSingletonAppName, singleton_instance_group); - connector()->StartService(singleton_id); base::Optional first_resolved_identity; { base::RunLoop loop; - Connector::TestApi test_api(connector()); - test_api.SetStartServiceCallback(base::BindRepeating( - &StartServiceResponse, &loop, nullptr, &first_resolved_identity)); + const base::Token singleton_instance_group = base::Token::CreateRandom(); + // Connect to an instance with an explicit different instance group. This + // supplied group should be ignored by the Service Manager because the + // target service is a singleton, and the Service Manager always generates a + // random instance group to host singleton service instances. + connector()->WarmService( + service_manager::ServiceFilter::ByNameInGroup(kTestSingletonAppName, + singleton_instance_group), + base::BindOnce(&StartServiceResponse, &loop, nullptr, + &first_resolved_identity)); loop.Run(); ASSERT_TRUE(first_resolved_identity); EXPECT_NE(*first_resolved_identity->instance_group(), singleton_instance_group); } - // This connects using the current client's user_id. It should be bound to the - // same service started above, with the same service manager-generated user - // id. - connector()->StartService(kTestSingletonAppName); { base::RunLoop loop; - Connector::TestApi test_api(connector()); base::Optional resolved_identity; - test_api.SetStartServiceCallback(base::BindRepeating( - &StartServiceResponse, &loop, nullptr, &resolved_identity)); + // This connects using the current client's instance group. It should be + // get routed to the same service instance started above. + connector()->WarmService( + service_manager::ServiceFilter::ByName(kTestSingletonAppName), + base::BindOnce(&StartServiceResponse, &loop, nullptr, + &resolved_identity)); loop.Run(); ASSERT_TRUE(resolved_identity); - EXPECT_EQ(resolved_identity->instance_group(), - first_resolved_identity->instance_group()); + EXPECT_EQ(*resolved_identity, *first_resolved_identity); } } diff --git a/services/service_manager/tests/lifecycle/lifecycle_unittest.cc b/services/service_manager/tests/lifecycle/lifecycle_unittest.cc index d27db049015151..ec2c5a5be9e944 100644 --- a/services/service_manager/tests/lifecycle/lifecycle_unittest.cc +++ b/services/service_manager/tests/lifecycle/lifecycle_unittest.cc @@ -14,6 +14,7 @@ #include "base/run_loop.h" #include "build/build_config.h" #include "mojo/public/cpp/bindings/binding.h" +#include "services/service_manager/public/cpp/constants.h" #include "services/service_manager/public/cpp/identity.h" #include "services/service_manager/public/cpp/service_test.h" #include "services/service_manager/public/mojom/constants.mojom.h" @@ -161,8 +162,9 @@ class LifecycleTest : public test::ServiceTest { #else "lifecycle_unittest_exe", #endif - Identity(kTestExeName, base::nullopt /* instance_group */), connector(), - &process); + Identity(kTestExeName, kSystemInstanceGroup, base::Token{}, + base::Token::CreateRandom()), + connector(), &process); return process; } diff --git a/services/service_manager/tests/service_manager/service_manager_unittest.cc b/services/service_manager/tests/service_manager/service_manager_unittest.cc index 3b15318c3be55b..106951817272bb 100644 --- a/services/service_manager/tests/service_manager/service_manager_unittest.cc +++ b/services/service_manager/tests/service_manager/service_manager_unittest.cc @@ -273,10 +273,11 @@ class ServiceManagerTest : public test::ServiceTest, service_manager::PassServiceRequestOnCommandLine(&invitation, &child_command_line); service_manager::mojom::PIDReceiverPtr receiver; - - service_manager::Identity target("service_manager_unittest_target"); - connector()->StartService(target, std::move(client), - MakeRequest(&receiver)); + connector()->RegisterServiceInstance( + service_manager::Identity("service_manager_unittest_target", + kSystemInstanceGroup, base::Token{}, + base::Token::CreateRandom()), + std::move(client), mojo::MakeRequest(&receiver)); target_ = base::LaunchProcess(child_command_line, options); DCHECK(target_.IsValid()); @@ -297,7 +298,8 @@ class ServiceManagerTest : public test::ServiceTest, set_service_failed_to_start_callback(base::BindRepeating( &OnServiceFailedToStartCallback, &failed_to_start, loop.QuitClosure())); - connector()->StartService("service_manager_unittest_embedder"); + connector()->WarmService(service_manager::ServiceFilter::ByName( + "service_manager_unittest_embedder")); loop.Run(); EXPECT_FALSE(failed_to_start); EXPECT_EQ(1, start_count); @@ -315,7 +317,7 @@ class ServiceManagerTest : public test::ServiceTest, set_service_failed_to_start_callback(base::BindRepeating( &OnServiceFailedToStartCallback, &failed_to_start, loop.QuitClosure())); - connector()->StartService(identity); + connector()->WarmService(service_manager::ServiceFilter(identity)); if (!expect_service_started) { // Wait briefly and test no new service was created. base::MessageLoopCurrent::Get()->task_runner()->PostDelayedTask( @@ -527,7 +529,8 @@ TEST_F(ServiceManagerTest, PIDReceivedCallback) { set_service_failed_to_start_callback(base::BindRepeating( &OnServiceFailedToStartCallback, &failed_to_start, loop.QuitClosure())); - connector()->StartService("service_manager_unittest_embedder"); + connector()->WarmService( + ServiceFilter::ByName("service_manager_unittest_embedder")); loop.Run(); EXPECT_FALSE(failed_to_start); EXPECT_EQ("service_manager_unittest_embedder", service_name); @@ -540,16 +543,19 @@ TEST_F(ServiceManagerTest, ClientProcessCapabilityEnforced) { const std::string kTestService = "service_manager_unittest_target"; const Identity kInstance1Id(kTestService, kSystemInstanceGroup, - base::Token{1, 2}); - const Identity kInstance2Id(kTestService, kSystemInstanceGroup); + base::Token{1, 2}, base::Token::CreateRandom()); + const Identity kInstance2Id(kTestService, kSystemInstanceGroup, + base::Token{3, 4}, base::Token::CreateRandom()); // Introduce a new service instance for service_manager_unittest_target, - // using the client_process capability. + // which should be allowed because the test service has + // |can_create_other_service_instances| set to |true| in its manifest. mojom::ServicePtr test_service_proxy1; SimpleService test_service1(mojo::MakeRequest(&test_service_proxy1)); mojom::PIDReceiverPtr pid_receiver1; - connector()->StartService(kInstance1Id, std::move(test_service_proxy1), - mojo::MakeRequest(&pid_receiver1)); + connector()->RegisterServiceInstance(kInstance1Id, + std::move(test_service_proxy1), + mojo::MakeRequest(&pid_receiver1)); pid_receiver1->SetPID(42); WaitForInstanceToStart(kInstance1Id); EXPECT_EQ(1u, instances().size()); @@ -560,9 +566,9 @@ TEST_F(ServiceManagerTest, ClientProcessCapabilityEnforced) { mojom::ServicePtr test_service_proxy2; SimpleService test_service2(mojo::MakeRequest(&test_service_proxy2)); mojom::PIDReceiverPtr pid_receiver2; - test_service1.connector()->StartService(kInstance2Id, - std::move(test_service_proxy2), - mojo::MakeRequest(&pid_receiver2)); + test_service1.connector()->RegisterServiceInstance( + kInstance2Id, std::move(test_service_proxy2), + mojo::MakeRequest(&pid_receiver2)); pid_receiver2->SetPID(43); // The new service should be disconnected immediately. diff --git a/services/service_manager/tests/util.cc b/services/service_manager/tests/util.cc index b824c02793b514..5b5a4509399021 100644 --- a/services/service_manager/tests/util.cc +++ b/services/service_manager/tests/util.cc @@ -31,8 +31,7 @@ namespace { void GrabConnectResult(base::RunLoop* loop, mojom::ConnectResult* out_result, - mojom::ConnectResult result, - const base::Optional& resolved_identity) { + mojom::ConnectResult result) { loop->Quit(); *out_result = result; } @@ -73,15 +72,12 @@ mojom::ConnectResult LaunchAndConnectToProcess( std::move(pipe), 0u)); service_manager::mojom::PIDReceiverPtr receiver; - connector->StartService(target, std::move(client), MakeRequest(&receiver)); mojom::ConnectResult result; - { - base::RunLoop loop(base::RunLoop::Type::kNestableTasksAllowed); - Connector::TestApi test_api(connector); - test_api.SetStartServiceCallback( - base::Bind(&GrabConnectResult, &loop, &result)); - loop.Run(); - } + base::RunLoop loop(base::RunLoop::Type::kNestableTasksAllowed); + connector->RegisterServiceInstance( + target, std::move(client), MakeRequest(&receiver), + base::BindOnce(&GrabConnectResult, &loop, &result)); + loop.Run(); base::LaunchOptions options; #if defined(OS_WIN) diff --git a/services/service_manager/tests/util.h b/services/service_manager/tests/util.h index f4396ad57da8fe..48ed2a1fd80e54 100644 --- a/services/service_manager/tests/util.h +++ b/services/service_manager/tests/util.h @@ -21,9 +21,9 @@ class Identity; namespace test { // Starts the process @ |target_exe_name| and connects to it as |target| using -// |connector|, returning a ConnectResult for the StartService() call. -// This blocks until the connection is established/rejected by the service -// manager. +// |connector|, returning a ConnectResult for the RegisterServiceInstance() +// call. This runs a nested loop until the connection is established or rejected +// by the Service Manager. service_manager::mojom::ConnectResult LaunchAndConnectToProcess( const std::string& target_exe_name, const Identity& target, diff --git a/services/tracing/tracing_service_unittest.cc b/services/tracing/tracing_service_unittest.cc index ccc44b439344bc..445805fd520b7f 100644 --- a/services/tracing/tracing_service_unittest.cc +++ b/services/tracing/tracing_service_unittest.cc @@ -22,11 +22,6 @@ class TracingServiceTest : public service_manager::test::ServiceTest { ~TracingServiceTest() override {} protected: - void SetUp() override { - service_manager::test::ServiceTest::SetUp(); - connector()->StartService(mojom::kServiceName); - } - void SetRunLoopToQuit(base::RunLoop* loop) { loop_ = loop; } private: diff --git a/services/ws/ime/ime_driver_bridge.cc b/services/ws/ime/ime_driver_bridge.cc index 9af9a62d76b4a3..a7e5a7d11f167d 100644 --- a/services/ws/ime/ime_driver_bridge.cc +++ b/services/ws/ime/ime_driver_bridge.cc @@ -13,13 +13,6 @@ IMEDriverBridge::IMEDriverBridge() {} IMEDriverBridge::~IMEDriverBridge() {} -void IMEDriverBridge::Init(service_manager::Connector* connector, - bool is_test_config) { - if (is_test_config) - connector->StartService("test_ime_driver"); - // For non test configs we assume a client registers with us. -} - void IMEDriverBridge::AddBinding(mojom::IMEDriverRequest request) { bindings_.AddBinding(this, std::move(request)); } diff --git a/services/ws/ime/ime_driver_bridge.h b/services/ws/ime/ime_driver_bridge.h index e1675f9b07d045..cf66cf33a56071 100644 --- a/services/ws/ime/ime_driver_bridge.h +++ b/services/ws/ime/ime_driver_bridge.h @@ -11,10 +11,6 @@ #include "mojo/public/cpp/bindings/binding_set.h" #include "services/ws/public/mojom/ime/ime.mojom.h" -namespace service_manager { -class Connector; -} - namespace ws { class IMEDriverBridge : public mojom::IMEDriver { @@ -22,7 +18,6 @@ class IMEDriverBridge : public mojom::IMEDriver { IMEDriverBridge(); ~IMEDriverBridge() override; - void Init(service_manager::Connector* connector, bool is_test_config); void AddBinding(mojom::IMEDriverRequest request); void SetDriver(mojom::IMEDriverPtr driver); diff --git a/services/ws/ime/ime_unittest.cc b/services/ws/ime/ime_unittest.cc index 94bed60486688a..5c168ba1fac2be 100644 --- a/services/ws/ime/ime_unittest.cc +++ b/services/ws/ime/ime_unittest.cc @@ -63,7 +63,9 @@ class IMEAppTest : public service_manager::test::ServiceTest { void SetUp() override { ServiceTest::SetUp(); // test_ime_driver will register itself as the current IMEDriver. - connector()->StartService(test_ime_driver::mojom::kServiceName); + // TODO(https://crbug.com/904148): This should not use |WarmService()|. + connector()->WarmService(service_manager::ServiceFilter::ByName( + test_ime_driver::mojom::kServiceName)); connector()->BindInterface(ws::mojom::kServiceName, &ime_driver_); }