Commit ed46d84d authored by James Cook's avatar James Cook Committed by Commit Bot

cros/mash: Terminate the browser process if the ash process crashes

The window manager process (ash) caches some state about the browser
process, and the browser caches some state about ash. We eventually
want to make these processes restartable, but for now we need to
terminate the browser if ash crashes. When the browser terminates on
Chrome OS the OS-level session manager daemon will restart it.

This broke when we moved the mojo service manager back into the
browser process.

Also clean up the old ShouldTerminateServiceManagerOnInstanceQuit
support we were using for the standalone and background run modes
for service manager, since we're not using those modes right now.

Ctrl-Alt-Shift-K to crash ash, see the browser exit too

Bug: 678683, 772098
Test: unit_tests, run chrome --mash --ash-debug-shortcuts and press
Change-Id: Icc62d46b36039c12a72ba3281babd234553a3f79
Reviewed-on: https://chromium-review.googlesource.com/716930Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarCharlie Reis <creis@chromium.org>
Reviewed-by: default avatarKen Rockot <rockot@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508782}
parent 95092097
......@@ -110,12 +110,6 @@
#include "components/metrics/leak_detector/leak_detector.h"
#endif
#if BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES)
#include "mash/common/config.h" // nogncheck
#include "services/ui/public/interfaces/constants.mojom.h" // nogncheck
#endif // BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES)
#if defined(OS_ANDROID)
#include "base/android/java_exception_reporter.h"
#include "chrome/browser/android/crash/pure_java_exception_handler.h"
......@@ -1120,22 +1114,3 @@ service_manager::ProcessType ChromeMainDelegate::OverrideProcessType() {
}
return service_manager::ProcessType::kDefault;
}
bool ChromeMainDelegate::ShouldTerminateServiceManagerOnInstanceQuit(
const service_manager::Identity& identity,
int* exit_code) {
#if BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES)
if (identity.name() == mash::common::GetWindowManagerServiceName() ||
identity.name() == ui::mojom::kServiceName ||
identity.name() == content::mojom::kPackagedServicesServiceName) {
// Quit the main process if an important child (e.g. window manager) dies.
// On Chrome OS the OS-level session_manager will restart the main process.
*exit_code = 1;
LOG(ERROR) << "Main process exiting because service " << identity.name()
<< " quit unexpectedly.";
return true;
}
#endif // BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES)
return false;
}
......@@ -52,9 +52,6 @@ class ChromeMainDelegate : public content::ContentMainDelegate {
#endif
bool ShouldEnableProfilerRecording() override;
service_manager::ProcessType OverrideProcessType() override;
bool ShouldTerminateServiceManagerOnInstanceQuit(
const service_manager::Identity& identity,
int* exit_code) override;
content::ContentBrowserClient* CreateContentBrowserClient() override;
content::ContentGpuClient* CreateContentGpuClient() override;
......
......@@ -1850,7 +1850,7 @@ void ChromeContentBrowserClient::AdjustUtilityServiceProcessCommandLine(
base::CommandLine* command_line) {
#if BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES)
// Mash services do their own resource loading.
if (IsMashServiceName(identity.name())) {
if (mash_service_registry::IsMashServiceName(identity.name())) {
// This switch is used purely for debugging to make it easier to know what
// service a process is running.
command_line->AppendSwitchASCII("mash-service-name", identity.name());
......@@ -3102,10 +3102,18 @@ void ChromeContentBrowserClient::RegisterOutOfProcessServices(
#if BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES)
if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kMash))
RegisterOutOfProcessServicesForMash(services);
mash_service_registry::RegisterOutOfProcessServices(services);
#endif
}
bool ChromeContentBrowserClient::ShouldTerminateOnServiceQuit(
const service_manager::Identity& id) {
#if BUILDFLAG(ENABLE_PACKAGE_MASH_SERVICES)
return mash_service_registry::ShouldTerminateOnServiceQuit(id.name());
#endif
return false;
}
std::unique_ptr<base::Value>
ChromeContentBrowserClient::GetServiceManifestOverlay(base::StringPiece name) {
ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
......
......@@ -311,6 +311,8 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
void RegisterInProcessServices(StaticServiceMap* services) override;
void RegisterOutOfProcessServices(
OutOfProcessServiceMap* services) override;
bool ShouldTerminateOnServiceQuit(
const service_manager::Identity& id) override;
std::unique_ptr<base::Value> GetServiceManifestOverlay(
base::StringPiece name) override;
std::vector<content::ContentBrowserClient::ServiceManifestInfo>
......
......@@ -48,6 +48,12 @@
#include "chrome/test/base/search_test_utils.h"
#endif
#if defined(OS_CHROMEOS)
#include "ash/public/interfaces/constants.mojom.h"
#include "content/public/common/service_names.mojom.h"
#include "services/ui/public/interfaces/constants.mojom.h"
#endif
using content::BrowsingDataFilterBuilder;
using testing::_;
using ChromeContentBrowserClientTest = testing::Test;
......@@ -406,3 +412,33 @@ TEST(ChromeContentBrowserClientTest, GetMetricSuffixForURL) {
EXPECT_EQ("", client.GetMetricSuffixForURL(
GURL("https://www.google.com/search?notaquery=nope")));
}
#if defined(OS_CHROMEOS)
// This behavior only matters on Chrome OS, which is why this isn't wrapped in
// ENABLE_MASH_PACKAGED_SERVICES (which is used for Linux Ozone).
TEST(ChromeContentBrowserClientTest, ShouldTerminateOnServiceQuit) {
const struct {
std::string service_name;
bool expect_terminate;
} kTestCases[] = {
// Don't terminate for invalid service names.
{"", false},
{"unknown-name", false},
// Don't terminate for some well-known browser services.
{content::mojom::kBrowserServiceName, false},
{content::mojom::kGpuServiceName, false},
{content::mojom::kRendererServiceName, false},
// Do terminate for some mash-specific cases.
{ui::mojom::kServiceName, true},
{ash::mojom::kServiceName, true},
};
ChromeContentBrowserClient client;
for (const auto& test : kTestCases) {
service_manager::Identity id(test.service_name);
EXPECT_EQ(test.expect_terminate, client.ShouldTerminateOnServiceQuit(id))
<< "for service name " << test.service_name;
}
}
#endif // defined(OS_CHROMEOS)
......@@ -18,6 +18,7 @@
#include "components/font_service/public/interfaces/constants.mojom.h"
#endif // defined(OS_LINUX) && !defined(OS_ANDROID)
namespace mash_service_registry {
namespace {
struct Service {
......@@ -40,7 +41,7 @@ constexpr Service kServices[] = {
} // namespace
void RegisterOutOfProcessServicesForMash(
void RegisterOutOfProcessServices(
content::ContentBrowserClient::OutOfProcessServiceMap* services) {
for (size_t i = 0; i < arraysize(kServices); ++i) {
(*services)[kServices[i].name] =
......@@ -55,3 +56,18 @@ bool IsMashServiceName(const std::string& name) {
}
return false;
}
bool ShouldTerminateOnServiceQuit(const std::string& name) {
// Some services going down are treated as catastrophic failures, usually
// because both the browser and the service cache data about each other's
// state that is not rebuilt when the service restarts.
if (name == ui::mojom::kServiceName)
return true;
#if defined(OS_CHROMEOS)
if (name == ash::mojom::kServiceName)
return true;
#endif
return false;
}
} // namespace mash_service_registry
......@@ -9,11 +9,18 @@
#include "content/public/browser/content_browser_client.h"
namespace mash_service_registry {
// Starts one of Mash's embedded services.
void RegisterOutOfProcessServicesForMash(
void RegisterOutOfProcessServices(
content::ContentBrowserClient::OutOfProcessServiceMap* services);
// Returns true if |name| identifies a mash related service.
bool IsMashServiceName(const std::string& name);
// Returns true if the browser should exit when service |name| quits.
bool ShouldTerminateOnServiceQuit(const std::string& name);
} // namespace mash_service_registry
#endif // CHROME_BROWSER_MASH_SERVICE_REGISTRY_H_
......@@ -109,14 +109,6 @@ void ContentServiceManagerMainDelegate::AdjustServiceProcessCommandLine(
command_line->AppendArgNative(arg);
}
bool ContentServiceManagerMainDelegate::
ShouldTerminateServiceManagerOnInstanceQuit(
const service_manager::Identity& identity,
int* exit_code) {
return content_main_params_.delegate
->ShouldTerminateServiceManagerOnInstanceQuit(identity, exit_code);
}
void ContentServiceManagerMainDelegate::OnServiceManagerInitialized(
const base::Closure& quit_closure,
service_manager::BackgroundServiceManager* service_manager) {
......
......@@ -34,9 +34,6 @@ class ContentServiceManagerMainDelegate : public service_manager::MainDelegate {
void AdjustServiceProcessCommandLine(
const service_manager::Identity& identity,
base::CommandLine* command_line) override;
bool ShouldTerminateServiceManagerOnInstanceQuit(
const service_manager::Identity& identity,
int* exit_code) override;
void OnServiceManagerInitialized(
const base::Closure& quit_closure,
service_manager::BackgroundServiceManager* service_manager) override;
......
......@@ -269,6 +269,20 @@ class ServiceManagerContext::InProcessServiceManagerContext
service_manager::Identity(mojom::kPackagedServicesServiceName,
service_manager::mojom::kRootUserID),
std::move(packaged_services_service), nullptr);
service_manager_->SetInstanceQuitCallback(
base::Bind(&OnInstanceQuitOnIOThread));
}
static void OnInstanceQuitOnIOThread(const service_manager::Identity& id) {
BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)
->PostTask(FROM_HERE, base::BindOnce(&OnInstanceQuit, id));
}
static void OnInstanceQuit(const service_manager::Identity& id) {
if (GetContentClient()->browser()->ShouldTerminateOnServiceQuit(id)) {
LOG(FATAL) << "Terminating because service '" << id.name()
<< "' quit unexpectedly.";
}
}
void ShutDownOnIOThread() {
......
......@@ -5,7 +5,6 @@
#include "content/public/app/content_main_delegate.h"
#include "build/build_config.h"
#include "content/public/gpu/content_gpu_client.h"
#include "content/public/renderer/content_renderer_client.h"
#include "content/public/utility/content_utility_client.h"
......@@ -61,12 +60,6 @@ void ContentMainDelegate::AdjustServiceProcessCommandLine(
const service_manager::Identity& identity,
base::CommandLine* command_line) {}
bool ContentMainDelegate::ShouldTerminateServiceManagerOnInstanceQuit(
const service_manager::Identity& identity,
int* exit_code) {
return false;
}
void ContentMainDelegate::OnServiceManagerInitialized(
const base::Closure& quit_closure,
service_manager::BackgroundServiceManager* service_manager) {}
......
......@@ -9,12 +9,19 @@
#include <string>
#include <vector>
#include "base/callback_forward.h"
#include "build/build_config.h"
#include "content/common/content_export.h"
#include "services/service_manager/background/background_service_manager.h"
#include "services/service_manager/embedder/process_type.h"
#include "services/service_manager/public/cpp/identity.h"
#include "services/service_manager/public/cpp/service.h"
namespace base {
class CommandLine;
}
namespace service_manager {
class BackgroundServiceManager;
class Identity;
} // namespace service_manager
namespace content {
......@@ -95,13 +102,6 @@ class CONTENT_EXPORT ContentMainDelegate {
const service_manager::Identity& identity,
base::CommandLine* command_line);
// Indicates if the Service Manager should be terminated in response to a
// specific service instance quitting. If this returns |true|, the value in
// |*exit_code| will be returned from the Service Manager's process on exit.
virtual bool ShouldTerminateServiceManagerOnInstanceQuit(
const service_manager::Identity& identity,
int* exit_code);
// Allows the embedder to perform arbitrary initialization within the Service
// Manager process immediately before the Service Manager runs its main loop.
//
......
......@@ -484,6 +484,11 @@ std::unique_ptr<base::Value> ContentBrowserClient::GetServiceManifestOverlay(
return nullptr;
}
bool ContentBrowserClient::ShouldTerminateOnServiceQuit(
const service_manager::Identity& id) {
return false;
}
std::vector<ContentBrowserClient::ServiceManifestInfo>
ContentBrowserClient::GetExtraServiceManifests() {
return std::vector<ContentBrowserClient::ServiceManifestInfo>();
......
......@@ -735,6 +735,11 @@ class CONTENT_EXPORT ContentBrowserClient {
// to use for the service's host process when launched.
virtual void RegisterOutOfProcessServices(OutOfProcessServiceMap* services) {}
// Allows the embedder to terminate the browser if a specific service instance
// quits or crashes.
virtual bool ShouldTerminateOnServiceQuit(
const service_manager::Identity& id);
// Allow the embedder to provide a dictionary loaded from a JSON file
// resembling a service manifest whose capabilities section will be merged
// with content's own for |name|. Additional entries will be appended to their
......
......@@ -25,19 +25,6 @@
#include "services/service_manager/standalone/context.h"
namespace service_manager {
namespace {
// Calls |callback| on |callback_task_runner|'s thread.
void CallCallbackWithIdentity(
const scoped_refptr<base::TaskRunner> callback_task_runner,
const base::Callback<void(const Identity&)>& callback,
const Identity& identity) {
DCHECK(callback);
DCHECK(identity.IsValid());
callback_task_runner->PostTask(FROM_HERE, base::Bind(callback, identity));
}
} // namespace
BackgroundServiceManager::BackgroundServiceManager(
service_manager::ServiceProcessLauncherDelegate* launcher_delegate,
......@@ -82,29 +69,6 @@ void BackgroundServiceManager::RegisterService(
base::Passed(&pid_receiver_request)));
}
void BackgroundServiceManager::SetInstanceQuitCallback(
base::Callback<void(const Identity&)> callback) {
DCHECK(callback);
// Hop to the background thread. The provided callback will be called on
// whichever thread called this function.
background_thread_.task_runner()->PostTask(
FROM_HERE,
base::Bind(
&BackgroundServiceManager::SetInstanceQuitCallbackOnBackgroundThread,
base::Unretained(this), base::ThreadTaskRunnerHandle::Get(),
callback));
}
void BackgroundServiceManager::SetInstanceQuitCallbackOnBackgroundThread(
const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner,
const base::Callback<void(const Identity&)>& callback) {
DCHECK(callback_task_runner);
DCHECK(callback);
// Calls |callback| with the identity of the service that is quitting.
context_->service_manager()->SetInstanceQuitCallback(
base::Bind(&CallCallbackWithIdentity, callback_task_runner, callback));
}
void BackgroundServiceManager::InitializeOnBackgroundThread(
service_manager::ServiceProcessLauncherDelegate* launcher_delegate,
std::unique_ptr<base::Value> catalog_contents) {
......
......@@ -7,18 +7,15 @@
#include <memory>
#include "base/callback_forward.h"
#include "base/macros.h"
#include "base/threading/thread.h"
#include "base/values.h"
#include "build/build_config.h"
#include "services/service_manager/public/cpp/identity.h"
#include "services/service_manager/public/interfaces/connector.mojom.h"
#include "services/service_manager/public/interfaces/service.mojom.h"
#include "services/service_manager/runner/host/service_process_launcher_delegate.h"
namespace base {
class SingleThreadTaskRunner;
class WaitableEvent;
}
......@@ -50,12 +47,6 @@ class BackgroundServiceManager {
mojom::ServicePtr service,
mojom::PIDReceiverRequest pid_receiver_request);
// Provide a callback to be notified whenever a service is destroyed.
// Typically the creator of BackgroundServiceManager will use this to shut
// down when some set of services it created is destroyed. The |callback| is
// called on whichever thread called this function.
void SetInstanceQuitCallback(base::Callback<void(const Identity&)> callback);
private:
void InitializeOnBackgroundThread(
service_manager::ServiceProcessLauncherDelegate* launcher_delegate,
......@@ -66,10 +57,6 @@ class BackgroundServiceManager {
const Identity& identity,
mojom::ServicePtrInfo service_info,
mojom::PIDReceiverRequest pid_receiver_request);
void SetInstanceQuitCallbackOnBackgroundThread(
const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner,
const base::Callback<void(const Identity&)>& callback);
void OnInstanceQuitOnBackgroundThread(const Identity& identity);
base::Thread background_thread_;
......
......@@ -66,41 +66,5 @@ TEST(BackgroundServiceManagerTest, MAYBE_Basic) {
EXPECT_TRUE(got_result);
}
// The out param cannot be last due to base::Bind() currying.
void QuitCallback(bool* callback_called,
const base::Closure& quit_closure,
const Identity& identity) {
EXPECT_EQ(kAppName, identity.name());
*callback_called = true;
quit_closure.Run();
}
// Verifies that quitting a service invokes the quit callback.
TEST(BackgroundServiceManagerTest, SetInstanceQuitCallback) {
BackgroundServiceManager background_service_manager(nullptr, nullptr);
base::MessageLoop message_loop;
mojom::ServicePtr service;
ServiceContext service_context(base::MakeUnique<ServiceImpl>(),
mojo::MakeRequest(&service));
background_service_manager.RegisterService(
Identity(kTestName, mojom::kRootUserID), std::move(service), nullptr);
mojom::TestServicePtr test_service;
service_context.connector()->BindInterface(kAppName, &test_service);
// Set a callback for when the service quits that will quit |run_loop|.
base::RunLoop run_loop;
bool callback_called = false;
background_service_manager.SetInstanceQuitCallback(
base::Bind(&QuitCallback, &callback_called, run_loop.QuitClosure()));
// Ask the service to quit itself.
test_service->Quit();
run_loop.Run();
// The run loop was quit by the callback and not by something else.
EXPECT_TRUE(callback_called);
}
} // namespace
} // namespace service_manager
......@@ -219,18 +219,6 @@ void WaitForDebuggerIfNecessary() {
base::debug::WaitForDebugger(120, true);
}
// Quits |run_loop| if the |identity| of the quitting service is critical to the
// system (e.g. the window manager). Used in the main process.
void OnInstanceQuit(MainDelegate* delegate,
base::RunLoop* run_loop,
int* exit_code,
const service_manager::Identity& identity) {
if (delegate->ShouldTerminateServiceManagerOnInstanceQuit(identity,
exit_code)) {
run_loop->Quit();
}
}
int RunServiceManager(MainDelegate* delegate) {
NonEmbedderProcessInit();
......@@ -251,10 +239,6 @@ int RunServiceManager(MainDelegate* delegate) {
&service_process_launcher_delegate, delegate->CreateServiceCatalog());
base::RunLoop run_loop;
int exit_code = 0;
background_service_manager.SetInstanceQuitCallback(
base::Bind(&OnInstanceQuit, delegate, &run_loop, &exit_code));
delegate->OnServiceManagerInitialized(run_loop.QuitClosure(),
&background_service_manager);
run_loop.Run();
......@@ -262,7 +246,7 @@ int RunServiceManager(MainDelegate* delegate) {
ipc_thread.Stop();
base::TaskScheduler::GetInstance()->Shutdown();
return exit_code;
return 0;
}
void InitializeResources() {
......
......@@ -39,12 +39,6 @@ void MainDelegate::AdjustServiceProcessCommandLine(
const Identity& identity,
base::CommandLine* command_line) {}
bool MainDelegate::ShouldTerminateServiceManagerOnInstanceQuit(
const Identity& identity,
int* exit_code) {
return false;
}
void MainDelegate::OnServiceManagerInitialized(
const base::Closure& quit_closure,
BackgroundServiceManager* service_manager) {}
......
......@@ -85,13 +85,6 @@ class SERVICE_MANAGER_EMBEDDER_EXPORT MainDelegate {
virtual void AdjustServiceProcessCommandLine(const Identity& identity,
base::CommandLine* command_line);
// Allows the embedder to terminate its Service Manager if any specific
// service instances quit. If this returns |true|, |*exit_code| will be
// returned from the Service Manager's process on exit.
virtual bool ShouldTerminateServiceManagerOnInstanceQuit(
const Identity& identity,
int* exit_code);
// Allows the embedder to perform arbitrary initialization within the Service
// Manager process immediately before the Service Manager runs its main loop.
//
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment