0

Remove BrowserMainParts::ServiceManagerConnectionStarted

This is no longer necessary since the Service Manager is initialized
before browser threads are even created. None of the implementations
of this override needed the ServiceManagerConnection specifically,
but only its Connector. Additionally, with recent changes to Service
Manager initialization the PostCreateThreads() method is effectively
equivalent in timing to what ServiceManagerConnectionStarted was.

As such all prior implementations of ServiceManagerConnectionStarted
have been moved into a PostCreateThreads override, and references to
the connection argument have been replaced with GetSystemConnector().

Bug: 904240
Change-Id: I70a445f5dcf9d16df654b95d971672f8c611e108
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1652798
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669025}
This commit is contained in:
Ken Rockot
2019-06-13 23:10:27 +00:00
committed by Commit Bot
parent 9fcfaa7623
commit acbf3d8c63
21 changed files with 69 additions and 130 deletions

@@ -38,10 +38,10 @@
#include "content/public/browser/android/synchronous_compositor.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/system_connector.h"
#include "content/public/common/content_client.h"
#include "content/public/common/content_switches.h"
#include "content/public/common/result_codes.h"
#include "content/public/common/service_manager_connection.h"
#include "content/public/common/service_names.mojom.h"
#include "net/android/network_change_notifier_factory_android.h"
#include "net/base/network_change_notifier.h"
@@ -128,12 +128,11 @@ bool AwBrowserMainParts::MainMessageLoopRun(int* result_code) {
return true;
}
void AwBrowserMainParts::ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) {
void AwBrowserMainParts::PostCreateThreads() {
heap_profiling::Mode mode = heap_profiling::GetModeForStartup();
if (mode != heap_profiling::Mode::kNone) {
heap_profiling::Supervisor::GetInstance()->Start(connection,
base::OnceClosure());
heap_profiling::Supervisor::GetInstance()->Start(
content::GetSystemConnector(), base::OnceClosure());
}
}

@@ -26,8 +26,7 @@ class AwBrowserMainParts : public content::BrowserMainParts {
int PreCreateThreads() override;
void PreMainMessageLoopRun() override;
bool MainMessageLoopRun(int* result_code) override;
void ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) override;
void PostCreateThreads() override;
private:
// Android specific UI SingleThreadTaskExecutor.

@@ -1143,6 +1143,8 @@ void BrowserProcessImpl::PreCreateThreads(
extensions::kExtensionScheme, true);
#endif
battery_metrics_ = std::make_unique<BatteryMetrics>();
secure_origin_prefs_observer_ =
std::make_unique<SecureOriginPrefsObserver>(local_state());
site_isolation_prefs_observer_ =
@@ -1174,12 +1176,6 @@ void BrowserProcessImpl::PreCreateThreads(
io_thread_ = std::make_unique<IOThread>(net_log_.get());
}
void BrowserProcessImpl::ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) {
// This uses the service manager so it must happen after it's started.
battery_metrics_ = std::make_unique<BatteryMetrics>();
}
void BrowserProcessImpl::PreMainMessageLoopRun() {
TRACE_EVENT0("startup", "BrowserProcessImpl::PreMainMessageLoopRun");
SCOPED_UMA_HISTOGRAM_TIMER(

@@ -109,11 +109,6 @@ class BrowserProcessImpl : public BrowserProcess,
// Called before the browser threads are created.
void PreCreateThreads(const base::CommandLine& command_line);
// Called after the browser threads are created, and service manager is set
// up.
void ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection);
// Called after the threads have been created but before the message loops
// starts running. Allows the browser process to do any initialization that
// requires all threads running.

@@ -1179,19 +1179,11 @@ void ChromeBrowserMainParts::PostCreateThreads() {
FROM_HERE, {BrowserThread::IO},
base::BindOnce(&tracing::TracingSamplerProfiler::CreateOnChildThread));
#endif
}
void ChromeBrowserMainParts::ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) {
// This should be called after the creation of the tracing controller. The
// tracing controller is created when the service manager connection is
// started.
tracing::SetupBackgroundTracingFieldTrial();
for (size_t i = 0; i < chrome_extra_parts_.size(); ++i)
chrome_extra_parts_[i]->ServiceManagerConnectionStarted(connection);
browser_process_->ServiceManagerConnectionStarted(connection);
chrome_extra_parts_[i]->PostCreateThreads();
}
void ChromeBrowserMainParts::PreMainMessageLoopRun() {

@@ -65,8 +65,6 @@ class ChromeBrowserMainParts : public content::BrowserMainParts {
void PostMainMessageLoopStart() override;
int PreCreateThreads() override;
void PostCreateThreads() override;
void ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) override;
void PreMainMessageLoopRun() override;
bool MainMessageLoopRun(int* result_code) override;
void PostMainMessageLoopRun() override;

@@ -5,10 +5,6 @@
#ifndef CHROME_BROWSER_CHROME_BROWSER_MAIN_EXTRA_PARTS_H_
#define CHROME_BROWSER_CHROME_BROWSER_MAIN_EXTRA_PARTS_H_
namespace content {
class ServiceManagerConnection;
}
// Interface class for Parts owned by ChromeBrowserMainParts.
// The default implementation for all methods is empty.
@@ -38,8 +34,7 @@ class ChromeBrowserMainExtraParts {
// MainMessageLoopRun methods.
virtual void PreCreateThreads() {}
virtual void ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) {}
virtual void PostCreateThreads() {}
virtual void PreProfileInit() {}
virtual void PostProfileInit() {}
virtual void PreBrowserStart() {}

@@ -15,6 +15,7 @@
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/content_browser_client.h"
#include "content/public/browser/system_connector.h"
#include "content/public/common/content_client.h"
#include "content/public/common/service_manager_connection.h"
#include "services/service_manager/public/cpp/binder_registry.h"
@@ -112,15 +113,14 @@ class ChromeService::ExtraParts : public ChromeBrowserMainExtraParts {
~ExtraParts() override = default;
private:
void ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) override {
void PostCreateThreads() override {
// Initializing the connector asynchronously configures the Connector on the
// IO thread. This needs to be done before WarmService() is called or
// ChromeService::BindConnector() can race with ChromeService::OnStart().
ChromeService::GetInstance()->InitConnector();
// TODO(https://crbug.com/904148): This should not use |WarmService()|.
connection->GetConnector()->WarmService(
content::GetSystemConnector()->WarmService(
service_manager::ServiceFilter::ByName(chrome::mojom::kServiceName));
}

@@ -16,11 +16,8 @@ ChromeBrowserMainExtraPartsPerformanceManager::
ChromeBrowserMainExtraPartsPerformanceManager::
~ChromeBrowserMainExtraPartsPerformanceManager() = default;
void ChromeBrowserMainExtraPartsPerformanceManager::
ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) {
void ChromeBrowserMainExtraPartsPerformanceManager::PostCreateThreads() {
performance_manager_ = performance_manager::PerformanceManager::Create();
browser_child_process_watcher_ =
std::make_unique<performance_manager::BrowserChildProcessWatcher>();
}

@@ -24,8 +24,7 @@ class ChromeBrowserMainExtraPartsPerformanceManager
private:
// ChromeBrowserMainExtraParts overrides.
void ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) override;
void PostCreateThreads() override;
void PostMainMessageLoopRun() override;
std::unique_ptr<performance_manager::PerformanceManager> performance_manager_;

@@ -11,6 +11,7 @@
#include "chrome/common/chrome_switches.h"
#include "components/heap_profiling/supervisor.h"
#include "components/services/heap_profiling/public/cpp/settings.h"
#include "content/public/browser/system_connector.h"
namespace {
@@ -29,22 +30,19 @@ ChromeBrowserMainExtraPartsProfiling::ChromeBrowserMainExtraPartsProfiling() =
ChromeBrowserMainExtraPartsProfiling::~ChromeBrowserMainExtraPartsProfiling() =
default;
void ChromeBrowserMainExtraPartsProfiling::ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) {
void ChromeBrowserMainExtraPartsProfiling::PostCreateThreads() {
heap_profiling::Supervisor::GetInstance()
->SetClientConnectionManagerConstructor(&CreateClientConnectionManager);
#if defined(ADDRESS_SANITIZER)
#if !defined(ADDRESS_SANITIZER)
// Memory sanitizers are using large memory shadow to keep track of memory
// state. Using memlog and memory sanitizers at the same time is slowing down
// user experience, causing the browser to be barely responsive. In theory,
// memlog and memory sanitizers are compatible and can run at the same time.
(void)connection; // Unused variable.
#else
heap_profiling::Mode mode = heap_profiling::GetModeForStartup();
if (mode != heap_profiling::Mode::kNone) {
heap_profiling::Supervisor::GetInstance()->Start(
connection,
content::GetSystemConnector(),
base::BindOnce(
&heap_profiling::ProfilingProcessHost::Start,
base::Unretained(

@@ -17,8 +17,7 @@ class ChromeBrowserMainExtraPartsProfiling
private:
// ChromeBrowserMainExtraParts overrides.
void ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) override;
void PostCreateThreads() override;
DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainExtraPartsProfiling);
};

@@ -59,9 +59,9 @@ class CrostiniBrowserTestChromeBrowserMainExtraParts
browser_process_platform_part_test_api_->InitializeCrosComponentManager(
std::move(cros_component_manager));
}
// Ideally we'd call SetConnectionType in ServiceManagerConnectionStarted,
// but currently we have to wait for PreProfileInit to complete, since that
// creates the ash::Shell that AshService needs in order to start.
// Ideally we'd call SetConnectionType in PostCreateThreads, but currently we
// have to wait for PreProfileInit to complete, since that creatse the
// ash::Shell that AshService needs in order to start.
void PostProfileInit() override {
connection_change_simulator_.SetConnectionType(
network::mojom::ConnectionType::CONNECTION_WIFI);

@@ -33,12 +33,12 @@
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/child_process_data.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/system_connector.h"
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_ui.h"
#include "content/public/browser/web_ui_data_source.h"
#include "content/public/browser/web_ui_message_handler.h"
#include "content/public/common/process_type.h"
#include "content/public/common/service_manager_connection.h"
#include "mojo/public/cpp/system/platform_handle.h"
#include "services/service_manager/public/cpp/connector.h"
#include "ui/shell_dialogs/select_file_dialog.h"
@@ -269,7 +269,7 @@ void MemoryInternalsDOMHandler::HandleStartProfiling(
supervisor->StartManualProfiling(pid);
} else {
supervisor->Start(
content::ServiceManagerConnection::GetForProcess(),
content::GetSystemConnector(),
base::BindOnce(&heap_profiling::Supervisor::StartManualProfiling,
base::Unretained(supervisor), pid));
}

@@ -60,6 +60,7 @@
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/child_process_security_policy.h"
#include "content/public/browser/storage_partition.h"
#include "content/public/browser/system_connector.h"
#include "content/public/common/content_switches.h"
#include "gpu/command_buffer/service/gpu_switches.h"
#include "media/base/media.h"
@@ -710,22 +711,21 @@ void CastBrowserMainParts::PostMainMessageLoopRun() {
#endif
}
void CastBrowserMainParts::PostCreateThreads() {
#if !defined(OS_FUCHSIA)
heap_profiling::Supervisor* supervisor =
heap_profiling::Supervisor::GetInstance();
supervisor->SetClientConnectionManagerConstructor(
&CreateClientConnectionManager);
supervisor->Start(content::GetSystemConnector(), base::NullCallback());
#endif // !defined(OS_FUCHSIA)
}
void CastBrowserMainParts::PostDestroyThreads() {
#if !defined(OS_ANDROID)
cast_content_browser_client_->ResetMediaResourceTracker();
#endif // !defined(OS_ANDROID)
}
void CastBrowserMainParts::ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) {
#if !defined(OS_FUCHSIA)
heap_profiling::Supervisor* supervisor =
heap_profiling::Supervisor::GetInstance();
supervisor->SetClientConnectionManagerConstructor(
&CreateClientConnectionManager);
supervisor->Start(connection, base::NullCallback());
#endif // !defined(OS_FUCHSIA)
}
} // namespace shell
} // namespace chromecast

@@ -87,9 +87,8 @@ class CastBrowserMainParts : public content::BrowserMainParts {
void PreMainMessageLoopRun() override;
bool MainMessageLoopRun(int* result_code) override;
void PostMainMessageLoopRun() override;
void PostCreateThreads() override;
void PostDestroyThreads() override;
void ServiceManagerConnectionStarted(
content::ServiceManagerConnection* connection) override;
private:
std::unique_ptr<CastBrowserProcess> cast_browser_process_;

@@ -14,7 +14,6 @@
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/tracing_controller.h"
#include "content/public/common/service_manager_connection.h"
#include "services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h"
#include "services/service_manager/public/cpp/connector.h"
@@ -58,13 +57,13 @@ void Supervisor::SetClientConnectionManagerConstructor(
constructor_ = constructor;
}
void Supervisor::Start(content::ServiceManagerConnection* connection,
void Supervisor::Start(service_manager::Connector* connector,
base::OnceClosure closure) {
Start(connection, GetModeForStartup(), GetStackModeForStartup(),
Start(connector, GetModeForStartup(), GetStackModeForStartup(),
GetSamplingRateForStartup(), std::move(closure));
}
void Supervisor::Start(content::ServiceManagerConnection* connection,
void Supervisor::Start(service_manager::Connector* connector,
Mode mode,
mojom::StackMode stack_mode,
uint32_t sampling_rate,
@@ -73,11 +72,10 @@ void Supervisor::Start(content::ServiceManagerConnection* connection,
DCHECK(!started_);
base::CreateSingleThreadTaskRunnerWithTraits({content::BrowserThread::IO})
->PostTask(FROM_HERE,
base::BindOnce(&Supervisor::StartServiceOnIOThread,
base::Unretained(this),
connection->GetConnector()->Clone(), mode,
stack_mode, sampling_rate, std::move(closure)));
->PostTask(FROM_HERE, base::BindOnce(&Supervisor::StartServiceOnIOThread,
base::Unretained(this),
connector->Clone(), mode, stack_mode,
sampling_rate, std::move(closure)));
}
Mode Supervisor::GetMode() {

@@ -10,10 +10,6 @@
#include "base/process/process.h"
#include "components/services/heap_profiling/public/mojom/heap_profiling_client.mojom.h"
namespace content {
class ServiceManagerConnection;
} // namespace content
namespace service_manager {
class Connector;
} // namespace service_manager
@@ -67,9 +63,8 @@ class Supervisor {
// * Relying on the assumption that in all other cases, the object is either
// fully initialized or not initialized. There are DCHECKs to enforce this
// assumption.
void Start(content::ServiceManagerConnection* connection,
base::OnceClosure callback);
void Start(content::ServiceManagerConnection* connection,
void Start(service_manager::Connector* connector, base::OnceClosure callback);
void Start(service_manager::Connector* connector,
Mode mode,
mojom::StackMode stack_mode,
uint32_t sampling_rate,

@@ -26,6 +26,7 @@
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/system_connector.h"
#include "content/public/browser/tracing_controller.h"
#include "content/public/common/service_manager_connection.h"
@@ -684,11 +685,10 @@ bool TestDriver::CheckOrStartProfilingOnUIThreadWithAsyncSignalling() {
return true;
}
content::ServiceManagerConnection* connection =
content::ServiceManagerConnection::GetForProcess();
if (!connection) {
LOG(ERROR) << "A ServiceManagerConnection was not available for the "
"current process.";
service_manager::Connector* connector = content::GetSystemConnector();
content::ServiceManagerConnection::GetForProcess();
if (!connector) {
LOG(ERROR) << "A system Connector is not available in this environment.";
return false;
}
@@ -710,7 +710,7 @@ bool TestDriver::CheckOrStartProfilingOnUIThreadWithAsyncSignalling() {
uint32_t sampling_rate = options_.should_sample
? (options_.sample_everything ? 2 : kSampleRate)
: 1;
Supervisor::GetInstance()->Start(connection, options_.mode,
Supervisor::GetInstance()->Start(connector, options_.mode,
options_.stack_mode, sampling_rate,
std::move(start_callback));
@@ -738,11 +738,10 @@ bool TestDriver::CheckOrStartProfilingOnUIThreadWithNestedRunLoops() {
return true;
}
content::ServiceManagerConnection* connection =
content::ServiceManagerConnection::GetForProcess();
if (!connection) {
LOG(ERROR) << "A ServiceManagerConnection was not available for the "
"current process.";
service_manager::Connector* connector = content::GetSystemConnector();
content::ServiceManagerConnection::GetForProcess();
if (!connector) {
LOG(ERROR) << "A system Connector is not available in this environment.";
return false;
}
@@ -763,7 +762,7 @@ bool TestDriver::CheckOrStartProfilingOnUIThreadWithNestedRunLoops() {
uint32_t sampling_rate = options_.should_sample
? (options_.sample_everything ? 2 : kSampleRate)
: 1;
Supervisor::GetInstance()->Start(connection, options_.mode,
Supervisor::GetInstance()->Start(connector, options_.mode,
options_.stack_mode, sampling_rate,
std::move(start_callback));

@@ -108,6 +108,7 @@
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/site_isolation_policy.h"
#include "content/public/browser/swap_metrics_driver.h"
#include "content/public/browser/system_connector.h"
#include "content/public/common/content_client.h"
#include "content/public/common/content_features.h"
#include "content/public/common/content_switches.h"
@@ -975,6 +976,10 @@ int BrowserMainLoop::CreateThreads() {
}
int BrowserMainLoop::PostCreateThreads() {
tracing_controller_ = std::make_unique<content::TracingControllerImpl>();
content::BackgroundTracingManagerImpl::GetInstance()
->AddMetadataGeneratorFunction();
if (parts_) {
TRACE_EVENT0("startup", "BrowserMainLoop::PostCreateThreads");
parts_->PostCreateThreads();
@@ -1318,9 +1323,7 @@ int BrowserMainLoop::BrowserThreadsStarted() {
{
TRACE_EVENT0("startup", "BrowserThreadsStarted::Subsystem:GamepadService");
device::GamepadService::GetInstance()->StartUp(
content::ServiceManagerConnection::GetForProcess()
->GetConnector()
->Clone());
GetSystemConnector()->Clone());
}
#if defined(OS_WIN)
@@ -1448,8 +1451,7 @@ int BrowserMainLoop::BrowserThreadsStarted() {
now_playing::RemoteCommandCenterDelegate::Create();
#endif
media_keys_listener_manager_ =
std::make_unique<MediaKeysListenerManagerImpl>(
content::ServiceManagerConnection::GetForProcess()->GetConnector());
std::make_unique<MediaKeysListenerManagerImpl>(GetSystemConnector());
}
#if defined(OS_MACOSX)
@@ -1560,16 +1562,10 @@ void BrowserMainLoop::InitializeMojo() {
// know they're running in the same process as the service.
content::NavigableContentsView::SetClientRunningInServiceProcess();
tracing_controller_ = std::make_unique<content::TracingControllerImpl>();
content::BackgroundTracingManagerImpl::GetInstance()
->AddMetadataGeneratorFunction();
// Registers the browser process as a memory-instrumentation client, so
// that data for the browser process will be available in memory dumps.
service_manager::Connector* connector =
content::ServiceManagerConnection::GetForProcess()->GetConnector();
memory_instrumentation::ClientProcessImpl::Config config(
connector, resource_coordinator::mojom::kServiceName,
GetSystemConnector(), resource_coordinator::mojom::kServiceName,
memory_instrumentation::mojom::ProcessType::BROWSER);
memory_instrumentation::ClientProcessImpl::CreateInstance(config);
@@ -1580,11 +1576,6 @@ void BrowserMainLoop::InitializeMojo() {
// MessagePumpForUI::ScheduleWork() before MessagePumpForUI::Start().
TracingControllerImpl::GetInstance()->StartStartupTracingIfNeeded();
if (parts_) {
parts_->ServiceManagerConnectionStarted(
ServiceManagerConnection::GetForProcess());
}
#if BUILDFLAG(MOJO_RANDOM_DELAYS_ENABLED)
mojo::BeginRandomMojoDelays();
#endif
@@ -1629,22 +1620,17 @@ void BrowserMainLoop::InitializeAudio() {
{content::BrowserThread::UI, base::TaskPriority::BEST_EFFORT},
base::BindOnce([]() {
TRACE_EVENT0("audio", "Starting audio service");
ServiceManagerConnection* connection =
content::ServiceManagerConnection::GetForProcess();
if (connection) {
auto* connector = GetSystemConnector();
if (connector) {
// The browser is not shutting down: |connection| would be null
// otherwise.
connection->GetConnector()->WarmService(
service_manager::ServiceFilter::ByName(
audio::mojom::kServiceName));
connector->WarmService(service_manager::ServiceFilter::ByName(
audio::mojom::kServiceName));
}
}));
}
audio_system_ = audio::CreateAudioSystem(
content::ServiceManagerConnection::GetForProcess()
->GetConnector()
->Clone());
audio_system_ = audio::CreateAudioSystem(GetSystemConnector()->Clone());
CHECK(audio_system_);
}

@@ -10,8 +10,6 @@
namespace content {
class ServiceManagerConnection;
// This class contains different "stages" to be executed by |BrowserMain()|,
// Each stage is represented by a single BrowserMainParts method, called from
// the corresponding method in |BrowserMainLoop| (e.g., EarlyInitialization())
@@ -79,9 +77,6 @@ class CONTENT_EXPORT BrowserMainParts {
// are created.
virtual void PostCreateThreads() {}
virtual void ServiceManagerConnectionStarted(
ServiceManagerConnection* connection) {}
// This is called just before the main message loop is run. The
// various browser threads have all been created at this point
virtual void PreMainMessageLoopRun() {}