[fuchsia] Check return value of AddPublicService & RemovePublicService

Most calls to these methods did not check the returned status.
FilteredServiceDirectory was also not checking return values. These are
now returned, which required updating calls to its methods as well.

Since many of these calls are in constructors or return objects, many of
the checks are ZX_CHECK().

Bug: 1196525
Change-Id: Ia6a74e646db57b9c89737556d2cb7d73b2a06202
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2586654
Auto-Submit: David Dorwin <ddorwin@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Fabrice de Gans-Riberi <fdegans@chromium.org>
Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
Commit-Queue: David Dorwin <ddorwin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#870519}

base/fuchsia/filtered_service_directory.cc

@@ -20,8 +20,9 @@ FilteredServiceDirectory::FilteredServiceDirectory(
 
 FilteredServiceDirectory::~FilteredServiceDirectory() {}
 
-void FilteredServiceDirectory::AddService(base::StringPiece service_name) {
-  outgoing_directory_.AddPublicService(
+zx_status_t FilteredServiceDirectory::AddService(
+    base::StringPiece service_name) {
+  return outgoing_directory_.AddPublicService(
       std::make_unique<vfs::Service>(
           [this, service_name = std::string(service_name)](
               zx::channel channel, async_dispatcher_t* dispatcher) {
@@ -31,11 +32,11 @@ void FilteredServiceDirectory::AddService(base::StringPiece service_name) {
       std::string(service_name));
 }
 
-void FilteredServiceDirectory::ConnectClient(
+zx_status_t FilteredServiceDirectory::ConnectClient(
     fidl::InterfaceRequest<::fuchsia::io::Directory> dir_request) {
   // sys::OutgoingDirectory puts public services under ./svc . Connect to that
   // directory and return client handle for the connection,
-  outgoing_directory_.GetOrCreateDirectory("svc")->Serve(
+  return outgoing_directory_.GetOrCreateDirectory("svc")->Serve(
       ::fuchsia::io::OPEN_RIGHT_READABLE | ::fuchsia::io::OPEN_RIGHT_WRITABLE,
       dir_request.TakeChannel());
 }

base/fuchsia/filtered_service_directory.h

@@ -13,9 +13,18 @@
 #include <memory>
 
 #include "base/base_export.h"
+#include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/strings/string_piece.h"
 
+// TODO(crbug.com/1196525): Remove once Chromecast calls are checking results.
+#include "build/chromecast_buildflags.h"
+#if BUILDFLAG(IS_CHROMECAST)
+#define MAYBE_WARN_UNUSED_RESULT
+#else
+#define MAYBE_WARN_UNUSED_RESULT WARN_UNUSED_RESULT
+#endif
+
 namespace base {
 
 // ServiceDirectory that uses the supplied sys::ServiceDirectory to satisfy
@@ -28,12 +37,13 @@ class BASE_EXPORT FilteredServiceDirectory {
   ~FilteredServiceDirectory();
 
   // Adds the specified service to the list of allowed services.
-  void AddService(base::StringPiece service_name);
+  zx_status_t AddService(base::StringPiece service_name)
+      MAYBE_WARN_UNUSED_RESULT;
 
   // Connects a directory client. The directory can be passed to a sandboxed
   // process to be used for /svc namespace.
-  void ConnectClient(
-      fidl::InterfaceRequest<::fuchsia::io::Directory> dir_request);
+  zx_status_t ConnectClient(fidl::InterfaceRequest<::fuchsia::io::Directory>
+                                dir_request) MAYBE_WARN_UNUSED_RESULT;
 
   // Accessor for the OutgoingDirectory, used to add handlers for services
   // in addition to those provided from |directory| via AddService().

base/fuchsia/filtered_service_directory_unittest.cc

@@ -18,7 +18,9 @@ class FilteredServiceDirectoryTest : public ServiceDirectoryTestBase {
     filtered_service_directory_ = std::make_unique<FilteredServiceDirectory>(
         public_service_directory_.get());
     fidl::InterfaceHandle<::fuchsia::io::Directory> directory;
-    filtered_service_directory_->ConnectClient(directory.NewRequest());
+    EXPECT_EQ(
+        filtered_service_directory_->ConnectClient(directory.NewRequest()),
+        ZX_OK);
     filtered_client_ =
         std::make_unique<sys::ServiceDirectory>(std::move(directory));
   }
@@ -30,15 +32,19 @@ class FilteredServiceDirectoryTest : public ServiceDirectoryTestBase {
 
 // Verify that we can connect to an allowed service.
 TEST_F(FilteredServiceDirectoryTest, Connect) {
-  filtered_service_directory_->AddService(testfidl::TestInterface::Name_);
+  EXPECT_EQ(
+      filtered_service_directory_->AddService(testfidl::TestInterface::Name_),
+      ZX_OK);
 
   auto stub = filtered_client_->Connect<testfidl::TestInterface>();
   VerifyTestInterface(&stub, ZX_OK);
 }
 
 // Verify that multiple connections to the same service work properly.
 TEST_F(FilteredServiceDirectoryTest, ConnectMultiple) {
-  filtered_service_directory_->AddService(testfidl::TestInterface::Name_);
+  EXPECT_EQ(
+      filtered_service_directory_->AddService(testfidl::TestInterface::Name_),
+      ZX_OK);
 
   auto stub1 = filtered_client_->Connect<testfidl::TestInterface>();
   auto stub2 = filtered_client_->Connect<testfidl::TestInterface>();
@@ -55,7 +61,9 @@ TEST_F(FilteredServiceDirectoryTest, ServiceBlocked) {
 // Verify that FilteredServiceDirectory handles the case when the target service
 // is not available in the underlying service directory.
 TEST_F(FilteredServiceDirectoryTest, NoService) {
-  filtered_service_directory_->AddService(testfidl::TestInterface::Name_);
+  EXPECT_EQ(
+      filtered_service_directory_->AddService(testfidl::TestInterface::Name_),
+      ZX_OK);
 
   service_binding_.reset();
 
@@ -66,7 +74,9 @@ TEST_F(FilteredServiceDirectoryTest, NoService) {
 // Verify that FilteredServiceDirectory handles the case when the underlying
 // service directory is destroyed.
 TEST_F(FilteredServiceDirectoryTest, NoServiceDir) {
-  filtered_service_directory_->AddService(testfidl::TestInterface::Name_);
+  EXPECT_EQ(
+      filtered_service_directory_->AddService(testfidl::TestInterface::Name_),
+      ZX_OK);
 
   service_binding_.reset();
   outgoing_directory_.reset();

base/fuchsia/startup_context.cc

@@ -9,6 +9,7 @@
 #include <lib/sys/cpp/service_directory.h>
 
 #include "base/fuchsia/file_utils.h"
+#include "base/fuchsia/fuchsia_logging.h"
 #include "base/logging.h"
 #include "base/macros.h"
 
@@ -55,13 +56,15 @@ StartupContext::StartupContext(::fuchsia::sys::StartupInfo startup_info) {
         std::move(startup_info.launch_info.additional_services->provider));
     additional_services_directory_ = std::make_unique<sys::OutgoingDirectory>();
     for (auto& name : startup_info.launch_info.additional_services->names) {
-      additional_services_directory_->AddPublicService(
+      zx_status_t status = additional_services_directory_->AddPublicService(
           std::make_unique<vfs::Service>([this, name](
                                              zx::channel channel,
                                              async_dispatcher_t* dispatcher) {
             additional_services_->ConnectToService(name, std::move(channel));
           }),
           name);
+      ZX_CHECK(status == ZX_OK, status)
+          << "AddPublicService(" << name << ") failed";
     }
 
     // Publish those services to the caller as |incoming_services|.

base/fuchsia/test_component_context_for_process.cc

@@ -39,7 +39,9 @@ TestComponentContextForProcess::TestComponentContextForProcess(
 
   // Create a ServiceDirectory backed by the contents of |incoming_directory|.
   fidl::InterfaceHandle<::fuchsia::io::Directory> incoming_directory;
-  context_services_->ConnectClient(incoming_directory.NewRequest());
+  zx_status_t status =
+      context_services_->ConnectClient(incoming_directory.NewRequest());
+  ZX_CHECK(status == ZX_OK, status) << "ConnectClient failed";
   auto incoming_services =
       std::make_shared<sys::ServiceDirectory>(std::move(incoming_directory));
 
@@ -55,7 +57,7 @@ TestComponentContextForProcess::TestComponentContextForProcess(
   // Connect to the "/svc" directory of the |published_root_directory| and wrap
   // that into a ServiceDirectory.
   fidl::InterfaceHandle<::fuchsia::io::Directory> published_services;
-  zx_status_t status = fdio_service_connect_at(
+  status = fdio_service_connect_at(
       published_root_directory.channel().get(), "svc",
       published_services.NewRequest().TakeChannel().release());
   ZX_CHECK(status == ZX_OK, status) << "fdio_service_connect_at() to /svc";
@@ -73,7 +75,8 @@ sys::OutgoingDirectory* TestComponentContextForProcess::additional_services() {
 
 void TestComponentContextForProcess::AddService(
     const base::StringPiece service) {
-  context_services_->AddService(service);
+  zx_status_t status = context_services_->AddService(service);
+  ZX_CHECK(status == ZX_OK, status) << "AddService(" << service << ") failed";
 }
 
 void TestComponentContextForProcess::AddServices(

fuchsia/engine/web_engine_integration_test_base.cc

@@ -81,13 +81,16 @@ WebEngineIntegrationTestBase::ContextParamsWithFilteredServiceDirectory() {
       std::make_unique<base::FilteredServiceDirectory>(
           base::ComponentContextForProcess()->svc().get());
   fidl::InterfaceHandle<fuchsia::io::Directory> svc_dir;
-  filtered_service_directory_->ConnectClient(svc_dir.NewRequest());
+  EXPECT_EQ(filtered_service_directory_->ConnectClient(svc_dir.NewRequest()),
+            ZX_OK);
 
   // Push all services from /svc to the service directory.
   base::FileEnumerator file_enum(base::FilePath("/svc"), false,
                                  base::FileEnumerator::FILES);
   for (auto file = file_enum.Next(); !file.empty(); file = file_enum.Next()) {
-    filtered_service_directory_->AddService(file.BaseName().value().c_str());
+    EXPECT_EQ(filtered_service_directory_->AddService(
+                  file.BaseName().value().c_str()),
+              ZX_OK);
   }
 
   fuchsia::web::CreateContextParams create_params;

fuchsia/runners/cast/cast_runner.cc

@@ -282,23 +282,36 @@ CastRunner::CastRunner(bool is_headless)
 
   // Specify the services to connect via the Runner process' service directory.
   for (const char* name : kServices) {
-    main_services_->AddService(name);
-    isolated_services_->AddService(name);
+    zx_status_t status = main_services_->AddService(name);
+    ZX_CHECK(status == ZX_OK, status)
+        << "AddService(" << name << ") to main failed";
+    status = isolated_services_->AddService(name);
+    ZX_CHECK(status == ZX_OK, status)
+        << "AddService(" << name << ") to isolated failed";
   }
 
   // Add handlers to main context's service directory for redirected services.
-  main_services_->outgoing_directory()->AddPublicService<fuchsia::media::Audio>(
-      fit::bind_member(this, &CastRunner::OnAudioServiceRequest));
-  main_services_->outgoing_directory()
-      ->AddPublicService<fuchsia::camera3::DeviceWatcher>(
-          fit::bind_member(this, &CastRunner::OnCameraServiceRequest));
-  main_services_->outgoing_directory()
-      ->AddPublicService<fuchsia::legacymetrics::MetricsRecorder>(
-          fit::bind_member(this, &CastRunner::OnMetricsRecorderServiceRequest));
+  zx_status_t status =
+      main_services_->outgoing_directory()
+          ->AddPublicService<fuchsia::media::Audio>(
+              fit::bind_member(this, &CastRunner::OnAudioServiceRequest));
+  ZX_CHECK(status == ZX_OK, status) << "AddPublicService(Audio) to main failed";
+  status = main_services_->outgoing_directory()
+               ->AddPublicService<fuchsia::camera3::DeviceWatcher>(
+                   fit::bind_member(this, &CastRunner::OnCameraServiceRequest));
+  ZX_CHECK(status == ZX_OK, status)
+      << "AddPublicService(DeviceWatcher) to main failed";
+  status = main_services_->outgoing_directory()
+               ->AddPublicService<fuchsia::legacymetrics::MetricsRecorder>(
+                   fit::bind_member(
+                       this, &CastRunner::OnMetricsRecorderServiceRequest));
+  ZX_CHECK(status == ZX_OK, status)
+      << "AddPublicService(MetricsRecorder) to main failed";
 
   // Isolated contexts can use the normal Audio service, and don't record
   // metrics.
-  isolated_services_->AddService(fuchsia::media::Audio::Name_);
+  status = isolated_services_->AddService(fuchsia::media::Audio::Name_);
+  ZX_CHECK(status == ZX_OK, status) << "AddService(Audio) to isolated failed";
 }
 
 CastRunner::~CastRunner() = default;
@@ -545,8 +558,9 @@ fuchsia::web::CreateContextParams CastRunner::GetMainContextParams() {
   *params.mutable_features() |=
       fuchsia::web::ContextFeatureFlags::NETWORK |
       fuchsia::web::ContextFeatureFlags::LEGACYMETRICS;
-  main_services_->ConnectClient(
+  zx_status_t status = main_services_->ConnectClient(
       params.mutable_service_directory()->NewRequest());
+  ZX_CHECK(status == ZX_OK, status) << "ConnectClient failed";
 
   if (!disable_vulkan_for_test_)
     SetCdmParamsForMainContext(&params);
@@ -571,8 +585,9 @@ CastRunner::GetIsolatedContextParamsWithFuchsiaDirs(
   fuchsia::web::CreateContextParams params = GetCommonContextParams();
   params.set_remote_debugging_port(kEphemeralRemoteDebuggingPort);
   params.set_content_directories(std::move(content_directories));
-  isolated_services_->ConnectClient(
+  zx_status_t status = isolated_services_->ConnectClient(
       params.mutable_service_directory()->NewRequest());
+  ZX_CHECK(status == ZX_OK, status) << "ConnectClient failed";
   return params;
 }
 
@@ -583,8 +598,9 @@ CastRunner::GetIsolatedContextParamsForCastStreaming() {
   ApplyCastStreamingContextParams(&params);
   // TODO(crbug.com/1069746): Use a different FilteredServiceDirectory for Cast
   // Streaming Contexts.
-  main_services_->ConnectClient(
+  zx_status_t status = main_services_->ConnectClient(
       params.mutable_service_directory()->NewRequest());
+  ZX_CHECK(status == ZX_OK, status) << "ConnectClient failed";
   return params;
 }