assistant: Convert sandbox build flag to feature

This patch converts the sandbox build flag to a feature, which makes it easier to test the sandbox in browser tests. Bug: b/155328340 Test: passed all tests Change-Id: Ifcde90a48311fb14902076c8a7777bd7ea496f4e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2930936 Commit-Queue: Tao Wu <wutao@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org> Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org> Cr-Commit-Position: refs/heads/master@{#891933}
chromium · Jun 13, 2021 · 7db2ffa · 7db2ffa
1 parent 1171b2f
commit 7db2ffa
Show file tree

Hide file tree

Showing 28 changed files with 138 additions and 121 deletions.
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
@@ -2427,7 +2427,7 @@ static_library("browser") {
       ]
     }
 
-    if (enable_libassistant_sandbox) {
+    if (enable_cros_libassistant) {
       deps += [ "//chromeos/services/libassistant/public/mojom" ]
     }
   }

diff --git a/chrome/browser/chromeos/service_sandbox_type.h b/chrome/browser/chromeos/service_sandbox_type.h
@@ -13,21 +13,6 @@
 // This file maps service classes to sandbox types. See
 // ServiceProcessHost::Launch() for how these templates are consumed.
 
-// chromeos::assistant::mojom::AssistantAudioDecoderFactory
-namespace chromeos {
-namespace assistant {
-namespace mojom {
-class AssistantAudioDecoderFactory;
-}  // namespace mojom
-}  // namespace assistant
-}  // namespace chromeos
-
-template <>
-inline sandbox::policy::SandboxType content::GetServiceSandboxType<
-    chromeos::assistant::mojom::AssistantAudioDecoderFactory>() {
-  return sandbox::policy::SandboxType::kUtility;
-}
-
 // chromeos::ime::mojom::ImeService
 namespace chromeos {
 namespace ime {
@@ -58,22 +43,6 @@ content::GetServiceSandboxType<chromeos::tts::mojom::TtsService>() {
   return sandbox::policy::SandboxType::kTts;
 }
 
-#if BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
-namespace chromeos {
-namespace libassistant {
-namespace mojom {
-class LibassistantService;
-}  // namespace mojom
-}  // namespace libassistant
-}  // namespace chromeos
-
-template <>
-inline sandbox::policy::SandboxType content::GetServiceSandboxType<
-    chromeos::libassistant::mojom::LibassistantService>() {
-  return sandbox::policy::SandboxType::kLibassistant;
-}
-#endif  // BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
-
 #if BUILDFLAG(IS_CHROMEOS_ASH)
 // recording::mojom::RecordingService
 namespace recording {
@@ -90,6 +59,38 @@ inline sandbox::policy::SandboxType
 content::GetServiceSandboxType<recording::mojom::RecordingService>() {
   return sandbox::policy::SandboxType::kVideoCapture;
 }
+
+// chromeos::assistant::mojom::AssistantAudioDecoderFactory
+namespace chromeos {
+namespace assistant {
+namespace mojom {
+class AssistantAudioDecoderFactory;
+}  // namespace mojom
+}  // namespace assistant
+}  // namespace chromeos
+
+template <>
+inline sandbox::policy::SandboxType content::GetServiceSandboxType<
+    chromeos::assistant::mojom::AssistantAudioDecoderFactory>() {
+  return sandbox::policy::SandboxType::kUtility;
+}
+
+#if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
+// chromeos::libassistant::mojom::LibassistantService
+namespace chromeos {
+namespace libassistant {
+namespace mojom {
+class LibassistantService;
+}  // namespace mojom
+}  // namespace libassistant
+}  // namespace chromeos
+
+template <>
+inline sandbox::policy::SandboxType content::GetServiceSandboxType<
+    chromeos::libassistant::mojom::LibassistantService>() {
+  return sandbox::policy::SandboxType::kLibassistant;
+}
+#endif  // BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 #endif  // CHROME_BROWSER_CHROMEOS_SERVICE_SANDBOX_TYPE_H_
diff --git a/chrome/browser/ui/ash/assistant/assistant_browsertest.cc b/chrome/browser/ui/ash/assistant/assistant_browsertest.cc
@@ -43,7 +43,12 @@ using chromeos::assistant::test::ExpectResult;
 
 class AssistantBrowserTest : public MixinBasedInProcessBrowserTest {
  public:
-  AssistantBrowserTest() = default;
+  AssistantBrowserTest() {
+    // TODO(b/190633242): enable sandbox in browser tests.
+    feature_list_.InitAndDisableFeature(
+        chromeos::assistant::features::kEnableLibAssistantSandbox);
+  }
+
   ~AssistantBrowserTest() override = default;
 
   AssistantTestMixin* tester() { return &tester_; }

diff --git a/chrome/browser/ui/ash/assistant/assistant_client_impl.cc b/chrome/browser/ui/ash/assistant/assistant_client_impl.cc
@@ -33,9 +33,9 @@
 #include "content/public/common/content_switches.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
-#if BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 #include "chromeos/services/libassistant/public/mojom/service.mojom.h"
-#endif  // BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#endif  // BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 
 AssistantClientImpl::AssistantClientImpl() {
   auto* session_manager = session_manager::SessionManager::Get();
@@ -167,7 +167,7 @@ void AssistantClientImpl::RequestNetworkConfig(
   ash::GetNetworkConfigService(std::move(receiver));
 }
 
-#if BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 void AssistantClientImpl::RequestLibassistantService(
     mojo::PendingReceiver<chromeos::libassistant::mojom::LibassistantService>
         receiver) {
@@ -177,7 +177,7 @@ void AssistantClientImpl::RequestLibassistantService(
                                .WithDisplayName("Libassistant Service")
                                .Pass());
 }
-#endif  // BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#endif  // BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 
 void AssistantClientImpl::OnExtendedAccountInfoUpdated(
     const AccountInfo& info) {

diff --git a/chrome/browser/ui/ash/assistant/assistant_client_impl.h b/chrome/browser/ui/ash/assistant/assistant_client_impl.h
@@ -79,11 +79,11 @@ class AssistantClientImpl : public ash::AssistantClient,
   void RequestNetworkConfig(
       mojo::PendingReceiver<chromeos::network_config::mojom::CrosNetworkConfig>
           receiver) override;
-#if BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
   void RequestLibassistantService(
       mojo::PendingReceiver<chromeos::libassistant::mojom::LibassistantService>
           receiver) override;
-#endif  // BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#endif  // BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 
  private:
   // signin::IdentityManager::Observer:

diff --git a/chrome/utility/BUILD.gn b/chrome/utility/BUILD.gn
@@ -175,14 +175,9 @@ static_library("utility") {
       deps += [
         "//chromeos/services/assistant/audio_decoder:lib",
         "//chromeos/services/assistant/public/mojom",
+        "//chromeos/services/libassistant",
+        "//chromeos/services/libassistant/public/mojom",
       ]
-
-      if (enable_libassistant_sandbox) {
-        deps += [
-          "//chromeos/services/libassistant",
-          "//chromeos/services/libassistant/public/mojom",
-        ]
-      }
     }
   }
 

diff --git a/chrome/utility/services.cc b/chrome/utility/services.cc
@@ -107,10 +107,7 @@
 
 #if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 #include "chromeos/services/assistant/audio_decoder/assistant_audio_decoder_factory.h"  // nogncheck
-
-#if BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
 #include "chromeos/services/libassistant/libassistant_service.h"  // nogncheck
-#endif  // BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
 #endif  // BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
@@ -297,14 +294,12 @@ auto RunAssistantAudioDecoder(
       std::move(receiver));
 }
 
-#if BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
 auto RunLibassistantService(
     mojo::PendingReceiver<chromeos::libassistant::mojom::LibassistantService>
         receiver) {
   return std::make_unique<chromeos::libassistant::LibassistantService>(
       std::move(receiver));
 }
-#endif  // BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
 #endif  // BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
@@ -384,9 +379,7 @@ void RegisterMainThreadServices(mojo::ServiceFactory& services) {
   services.Add(RunLocalSearchService);
 #if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
   services.Add(RunAssistantAudioDecoder);
-#if BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
   services.Add(RunLibassistantService);
-#endif  // BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
 #endif  // BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 }

diff --git a/chromeos/assistant/BUILD.gn b/chromeos/assistant/BUILD.gn
@@ -11,7 +11,6 @@ buildflag_header("buildflags") {
 
   flags = [
     "ENABLE_CROS_LIBASSISTANT=$enable_cros_libassistant",
-    "ENABLE_LIBASSISTANT_SANDBOX=$enable_cros_libassistant && $enable_libassistant_sandbox",
     "ENABLE_CROS_AMBIENT_MODE_BACKEND=$enable_cros_ambient_mode_backend",
   ]
 }
diff --git a/chromeos/assistant/assistant.gni b/chromeos/assistant/assistant.gni
@@ -5,9 +5,6 @@ declare_args() {
   # Enable assistant implementation based on libassistant.
   enable_cros_libassistant = is_chromeos_ash && is_chrome_branded
 
-  # Enable sandboxing LibAssistant service.
-  enable_libassistant_sandbox = false
-
   # Enable a fake microphone, which can replay audio files as microphone input.
   # See chromeos/assistant/tools/send-audio.sh
   enable_fake_assistant_microphone = false

diff --git a/chromeos/services/assistant/BUILD.gn b/chromeos/services/assistant/BUILD.gn
@@ -75,11 +75,10 @@ component("lib") {
   ]
 
   if (enable_cros_libassistant) {
-    deps += [ "//chromeos/services/libassistant" ]
-
-    if (enable_libassistant_sandbox) {
-      deps += [ "//chromeos/services/assistant/public/cpp" ]
-    }
+    deps += [
+      "//chromeos/services/assistant/public/cpp",
+      "//chromeos/services/libassistant",
+    ]
   }
 
   public_deps = [
@@ -102,6 +101,7 @@ source_set("tests") {
     "//ash/public/cpp/assistant/test_support",
     "//base",
     "//base/test:test_support",
+    "//chromeos/assistant:buildflags",
     "//chromeos/assistant/test_support",
     "//chromeos/dbus:test_support",
     "//chromeos/dbus/audio",

diff --git a/chromeos/services/assistant/libassistant_service_host_impl.cc b/chromeos/services/assistant/libassistant_service_host_impl.cc
@@ -10,12 +10,10 @@
 #include "chromeos/assistant/buildflags.h"
 
 #if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
+#include "chromeos/services/assistant/public/cpp/assistant_client.h"
+#include "chromeos/services/assistant/public/cpp/features.h"
 #include "chromeos/services/libassistant/libassistant_service.h"
-
-#if BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
-#include "chromeos/services/assistant/public/cpp/assistant_client.h"  // nogncheck
-#include "chromeos/services/libassistant/public/mojom/service.mojom-forward.h"  // nogncheck
-#endif  // BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#include "chromeos/services/libassistant/public/mojom/service.mojom-forward.h"
 #endif  // BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 
 namespace chromeos {
@@ -24,32 +22,28 @@ namespace assistant {
 #if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 
 LibassistantServiceHostImpl::LibassistantServiceHostImpl() {
-#if !BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
   DETACH_FROM_SEQUENCE(sequence_checker_);
-#endif
 }
 
 LibassistantServiceHostImpl::~LibassistantServiceHostImpl() = default;
 
 void LibassistantServiceHostImpl::Launch(
     mojo::PendingReceiver<chromeos::libassistant::mojom::LibassistantService>
         receiver) {
-#if BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
-  AssistantClient::Get()->RequestLibassistantService(std::move(receiver));
-#else
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(!libassistant_service_);
-  libassistant_service_ =
-      std::make_unique<chromeos::libassistant::LibassistantService>(
-          std::move(receiver));
-#endif  // BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+  if (assistant::features::IsLibAssistantSandboxEnabled()) {
+    AssistantClient::Get()->RequestLibassistantService(std::move(receiver));
+  } else {
+    DCHECK(!libassistant_service_);
+    libassistant_service_ =
+        std::make_unique<chromeos::libassistant::LibassistantService>(
+            std::move(receiver));
+  }
 }
 
 void LibassistantServiceHostImpl::Stop() {
-#if !BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   libassistant_service_ = nullptr;
-#endif
 }
 
 #else

diff --git a/chromeos/services/assistant/libassistant_service_host_impl.h b/chromeos/services/assistant/libassistant_service_host_impl.h
@@ -37,8 +37,7 @@ class LibassistantServiceHostImpl : public LibassistantServiceHost {
   void Stop() override;
 
  private:
-#if BUILDFLAG(ENABLE_CROS_LIBASSISTANT) && \
-    !BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
   SEQUENCE_CHECKER(sequence_checker_);
   std::unique_ptr<chromeos::libassistant::LibassistantService>
       libassistant_service_ GUARDED_BY_CONTEXT(sequence_checker_);

diff --git a/chromeos/services/assistant/public/cpp/assistant_client.h b/chromeos/services/assistant/public/cpp/assistant_client.h
@@ -19,9 +19,9 @@
 #include "services/media_session/public/mojom/audio_focus.mojom.h"
 #include "services/media_session/public/mojom/media_controller.mojom.h"
 
-#if BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 #include "chromeos/services/libassistant/public/mojom/service.mojom-forward.h"
-#endif  // BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#endif  // BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 
 namespace chromeos {
 namespace assistant {
@@ -80,12 +80,12 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) AssistantClient {
       mojo::PendingReceiver<chromeos::network_config::mojom::CrosNetworkConfig>
           receiver) = 0;
 
-#if BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
   // Requests a connection to Libassistant service interface via the browser.
   virtual void RequestLibassistantService(
       mojo::PendingReceiver<chromeos::libassistant::mojom::LibassistantService>
           receiver) = 0;
-#endif  // BUILDFLAG(ENABLE_LIBASSISTANT_SANDBOX)
+#endif  // BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
 };
 
 }  // namespace assistant

diff --git a/chromeos/services/assistant/public/cpp/features.cc b/chromeos/services/assistant/public/cpp/features.cc
@@ -54,6 +54,9 @@ const base::Feature kEnableLibAssistantBetaBackend{
 const base::Feature kDisableVoiceMatch{"DisableVoiceMatch",
                                        base::FEATURE_DISABLED_BY_DEFAULT};
 
+const base::Feature kEnableLibAssistantSandbox{
+    "LibAssistantSandbox", base::FEATURE_DISABLED_BY_DEFAULT};
+
 bool IsAppSupportEnabled() {
   return base::FeatureList::IsEnabled(
       assistant::features::kAssistantAppSupport);
@@ -113,6 +116,10 @@ bool IsWaitSchedulingEnabled() {
   return base::FeatureList::IsEnabled(kAssistantWaitScheduling);
 }
 
+bool IsLibAssistantSandboxEnabled() {
+  return base::FeatureList::IsEnabled(kEnableLibAssistantSandbox);
+}
+
 }  // namespace features
 }  // namespace assistant
 }  // namespace chromeos
diff --git a/chromeos/services/assistant/public/cpp/features.h b/chromeos/services/assistant/public/cpp/features.h
@@ -60,6 +60,10 @@ extern const base::Feature kEnablePowerManager;
 COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
 extern const base::Feature kEnableLibAssistantBetaBackend;
 
+// Enables the sandbox of LibAssistant service.
+COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
+extern const base::Feature kEnableLibAssistantSandbox;
+
 COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) bool IsAppSupportEnabled();
 
 COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) bool IsAudioEraserEnabled();
@@ -92,6 +96,9 @@ COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) bool IsVoiceMatchDisabled();
 
 COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) bool IsWaitSchedulingEnabled();
 
+COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
+bool IsLibAssistantSandboxEnabled();
+
 }  // namespace features
 }  // namespace assistant
 }  // namespace chromeos