diff --git a/src/developer/crashpad_agent/crashpad_agent.h b/src/developer/crashpad_agent/crashpad_agent.h index adf206bd18a1cd56db9b0b2d0035209f199cd5d8..9b36f8377e25800d72dec2c18bc0e8047bb21d3f 100644 --- a/src/developer/crashpad_agent/crashpad_agent.h +++ b/src/developer/crashpad_agent/crashpad_agent.h @@ -94,6 +94,8 @@ class CrashpadAgent : public Analyzer { const std::unique_ptr<crashpad::CrashReportDatabase> database_; const std::unique_ptr<CrashServer> crash_server_; + // TODO(DX-1499): we should have a connection to fuchsia.feedback.DataProvider + // per GetData() call, not a single one overall. fuchsia::feedback::DataProviderPtr feedback_data_provider_; FXL_DISALLOW_COPY_AND_ASSIGN(CrashpadAgent); diff --git a/src/developer/feedback_agent/BUILD.gn b/src/developer/feedback_agent/BUILD.gn index f0f83cdd7535b4ebe7a6efa2b333302b7debf8a5..fbf8273f7c0a9eb49b0fba89eadf26a6c37b50ea 100644 --- a/src/developer/feedback_agent/BUILD.gn +++ b/src/developer/feedback_agent/BUILD.gn @@ -7,6 +7,7 @@ import("//build/package/component.gni") package("feedback_agent") { deps = [ + ":data_provider", ":main", ] @@ -14,11 +15,14 @@ package("feedback_agent") { { name = "feedback_agent" }, + { + name = "data_provider" + }, ] meta = [ { - path = rebase_path("meta/feedback_agent.cmx") + path = "meta/feedback_agent.cmx" dest = "feedback_agent.cmx" }, ] @@ -31,13 +35,31 @@ executable("main") { "main.cc", ] + deps = [ + "//garnet/public/lib/syslog/cpp", + "//sdk/fidl/fuchsia.feedback", + "//sdk/lib/fidl/cpp", + "//sdk/lib/sys/cpp", + "//src/lib/fxl", + "//zircon/public/lib/async-loop-cpp", + "//zircon/public/lib/zx", + ] +} + +executable("data_provider") { + sources = [ + "data_provider_main.cc", + ] + deps = [ ":src", "//garnet/public/lib/syslog/cpp", "//sdk/fidl/fuchsia.feedback", "//sdk/lib/fidl/cpp", "//sdk/lib/sys/cpp", + "//src/lib/fxl", "//zircon/public/lib/async-loop-cpp", + "//zircon/public/lib/zx", ] } @@ -47,8 +69,8 @@ source_set("src") { "annotations.h", "attachments.cc", "attachments.h", - "feedback_agent.cc", - "feedback_agent.h", + "data_provider.cc", + "data_provider.h", "image_conversion.cc", "image_conversion.h", "log_listener.cc", diff --git a/src/developer/feedback_agent/feedback_agent.cc b/src/developer/feedback_agent/data_provider.cc similarity index 89% rename from src/developer/feedback_agent/feedback_agent.cc rename to src/developer/feedback_agent/data_provider.cc index 06124fff016e050a093216b5af82804223f95c70..0068bd98f0512ac4cf7c52f50b0178d2003e24ec 100644 --- a/src/developer/feedback_agent/feedback_agent.cc +++ b/src/developer/feedback_agent/data_provider.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "src/developer/feedback_agent/feedback_agent.h" +#include "src/developer/feedback_agent/data_provider.h" #include <fuchsia/feedback/cpp/fidl.h> #include <fuchsia/ui/scenic/cpp/fidl.h> @@ -18,11 +18,12 @@ namespace fuchsia { namespace feedback { -FeedbackAgent::FeedbackAgent(async_dispatcher_t* dispatcher, - std::shared_ptr<::sys::ServiceDirectory> services) +DataProviderImpl::DataProviderImpl( + async_dispatcher_t* dispatcher, + std::shared_ptr<::sys::ServiceDirectory> services) : executor_(dispatcher), services_(services) {} -void FeedbackAgent::GetData(GetDataCallback callback) { +void DataProviderImpl::GetData(GetDataCallback callback) { // Today attachments are fetched asynchronously, but annotations are not. // In the future, we can use fit::join_promises() if annotations need to be // fetched asynchronously as well. @@ -54,8 +55,8 @@ void FeedbackAgent::GetData(GetDataCallback callback) { executor_.schedule_task(std::move(promise)); } -void FeedbackAgent::GetScreenshot(ImageEncoding encoding, - GetScreenshotCallback callback) { +void DataProviderImpl::GetScreenshot(ImageEncoding encoding, + GetScreenshotCallback callback) { // We add the provided callback to the vector of pending callbacks we maintain // and save a reference to pass to the Scenic callback. auto& saved_callback = get_png_screenshot_callbacks_.emplace_back( @@ -95,7 +96,7 @@ void FeedbackAgent::GetScreenshot(ImageEncoding encoding, }); } -void FeedbackAgent::ConnectToScenic() { +void DataProviderImpl::ConnectToScenic() { scenic_ = services_->Connect<fuchsia::ui::scenic::Scenic>(); scenic_.set_error_handler([this](zx_status_t status) { FX_LOGS(ERROR) << "Lost connection to Scenic service"; @@ -103,7 +104,7 @@ void FeedbackAgent::ConnectToScenic() { }); } -void FeedbackAgent::TerminateAllGetScreenshotCallbacks() { +void DataProviderImpl::TerminateAllGetScreenshotCallbacks() { for (const auto& callback : get_png_screenshot_callbacks_) { (*callback)(/*screenshot=*/nullptr); } diff --git a/src/developer/feedback_agent/feedback_agent.h b/src/developer/feedback_agent/data_provider.h similarity index 74% rename from src/developer/feedback_agent/feedback_agent.h rename to src/developer/feedback_agent/data_provider.h index 74c2886542d71aba52411f213d864a21ebb28459..e2e8348f93ddb54676e93eccbadef17132e79525 100644 --- a/src/developer/feedback_agent/feedback_agent.h +++ b/src/developer/feedback_agent/data_provider.h @@ -2,8 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef SRC_DEVELOPER_FEEDBACK_AGENT_FEEDBACK_AGENT_H_ -#define SRC_DEVELOPER_FEEDBACK_AGENT_FEEDBACK_AGENT_H_ +#ifndef SRC_DEVELOPER_FEEDBACK_AGENT_DATA_PROVIDER_H_ +#define SRC_DEVELOPER_FEEDBACK_AGENT_DATA_PROVIDER_H_ #include <fuchsia/feedback/cpp/fidl.h> #include <fuchsia/ui/scenic/cpp/fidl.h> @@ -19,16 +19,13 @@ namespace fuchsia { namespace feedback { // Provides data useful to attach in feedback reports (crash or user feedback). -class FeedbackAgent : public DataProvider { +class DataProviderImpl : public DataProvider { public: - FeedbackAgent(async_dispatcher_t* dispatcher, - std::shared_ptr<::sys::ServiceDirectory> services); + DataProviderImpl(async_dispatcher_t* dispatcher, + std::shared_ptr<::sys::ServiceDirectory> services); - // Returns all the feedback data except the screenshot, which is provided - // separately. + // |fuchsia.feedback.DataProvider| void GetData(GetDataCallback callback) override; - - // Returns an image of the current view encoded in the provided |encoding|. void GetScreenshot(ImageEncoding encoding, GetScreenshotCallback callback) override; @@ -44,6 +41,8 @@ class FeedbackAgent : public DataProvider { async::Executor executor_; const std::shared_ptr<::sys::ServiceDirectory> services_; + // TODO(DX-1499): we should have a connection to Scenic per GetScreenshot() + // call, not a single one overall. fuchsia::ui::scenic::ScenicPtr scenic_; // We keep track of the pending GetScreenshot callbacks so we can terminate // all of them when we lose the connection with Scenic. @@ -54,4 +53,4 @@ class FeedbackAgent : public DataProvider { } // namespace feedback } // namespace fuchsia -#endif // SRC_DEVELOPER_FEEDBACK_AGENT_FEEDBACK_AGENT_H_ +#endif // SRC_DEVELOPER_FEEDBACK_AGENT_DATA_PROVIDER_H_ diff --git a/src/developer/feedback_agent/data_provider_main.cc b/src/developer/feedback_agent/data_provider_main.cc new file mode 100644 index 0000000000000000000000000000000000000000..6065bf15477d516c1cfe0f67c4a75f0def39b05e --- /dev/null +++ b/src/developer/feedback_agent/data_provider_main.cc @@ -0,0 +1,56 @@ +// Copyright 2019 The Fuchsia Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include <fuchsia/feedback/cpp/fidl.h> +#include <lib/async-loop/cpp/loop.h> +#include <lib/fidl/cpp/interface_request.h> +#include <lib/sys/cpp/component_context.h> +#include <lib/syslog/cpp/logger.h> +#include <lib/zx/channel.h> +#include <stdlib.h> +#include <zircon/errors.h> +#include <zircon/process.h> +#include <zircon/processargs.h> +#include <zircon/status.h> + +#include "src/developer/feedback_agent/data_provider.h" +#include "src/lib/fxl/strings/string_printf.h" + +int main(int argc, const char** argv) { + syslog::InitLogger({"feedback"}); + + // This process is spawned by the feedback_agent process, which forwards it + // the incoming request through PA_USER0. + fidl::InterfaceRequest<fuchsia::feedback::DataProvider> request( + zx::channel(zx_take_startup_handle(PA_HND(PA_USER0, 0)))); + if (!request.is_valid()) { + FX_LOGS(ERROR) << "Invalid incoming fuchsia.feedback.DataProvider request"; + return EXIT_FAILURE; + } + + async::Loop loop(&kAsyncLoopConfigAttachToThread); + auto context = sys::ComponentContext::Create(); + fuchsia::feedback::DataProviderImpl data_provider(loop.dispatcher(), + context->svc()); + fidl::Binding<fuchsia::feedback::DataProvider> binding(&data_provider); + // TODO(DX-1497): in addition to exiting the process when the connection is + // closed, we should have an internal timeout since the last call and exit the + // process then in case clients don't close the connection themselves. + binding.set_error_handler([&loop](zx_status_t status) { + loop.Shutdown(); + // We exit successfully when the client closes the connection. + if (status == ZX_ERR_PEER_CLOSED) { + exit(0); + } else { + FX_LOGS(ERROR) << fxl::StringPrintf("Received channel error: %d (%s)", + status, zx_status_get_string(status)); + exit(1); + } + }); + binding.Bind(std::move(request)); + + loop.Run(); + + return EXIT_SUCCESS; +} diff --git a/src/developer/feedback_agent/main.cc b/src/developer/feedback_agent/main.cc index febe8fbf11a926576668cdd82261abd86279c5a4..f9d2c0444a7bfdb8494ae7aae7110984ace7b018 100644 --- a/src/developer/feedback_agent/main.cc +++ b/src/developer/feedback_agent/main.cc @@ -4,22 +4,56 @@ #include <fuchsia/feedback/cpp/fidl.h> #include <lib/async-loop/cpp/loop.h> -#include <lib/fidl/cpp/binding_set.h> +#include <lib/fdio/spawn.h> +#include <lib/fidl/cpp/interface_request.h> #include <lib/sys/cpp/component_context.h> #include <lib/syslog/cpp/logger.h> #include <stdlib.h> +#include <zircon/errors.h> +#include <zircon/processargs.h> +#include <zircon/status.h> -#include "src/developer/feedback_agent/feedback_agent.h" +#include "src/lib/fxl/strings/string_printf.h" + +fidl::InterfaceRequestHandler<fuchsia::feedback::DataProvider> +SpawnNewDataProvider() { + return [](fidl::InterfaceRequest<fuchsia::feedback::DataProvider> request) { + // We spawn a new process to which we forward the channel of the incoming + // request so it can handle it. + // + // Note that today we do not keep track of the spawned process. + fdio_spawn_action_t actions = {}; + actions.action = FDIO_SPAWN_ACTION_ADD_HANDLE; + actions.h.id = PA_HND(PA_USER0, 0); + actions.h.handle = request.TakeChannel().release(); + + const char* args[] = { + "/pkg/bin/data_provider", + nullptr, + }; + char err_msg[FDIO_SPAWN_ERR_MSG_MAX_LENGTH] = {}; + const zx_status_t spawn_status = + fdio_spawn_etc(ZX_HANDLE_INVALID, FDIO_SPAWN_CLONE_ALL, args[0], args, + nullptr, 1, &actions, nullptr, err_msg); + if (spawn_status != ZX_OK) { + FX_LOGS(ERROR) << fxl::StringPrintf( + "Failed to spawn data provider to handle incoming request: %d " + "(%s): %s", + spawn_status, zx_status_get_string(spawn_status), err_msg); + } + }; +} int main(int argc, const char** argv) { - syslog::InitLogger({"feedback_agent"}); + syslog::InitLogger({"feedback"}); async::Loop loop(&kAsyncLoopConfigAttachToThread); auto context = sys::ComponentContext::Create(); - fuchsia::feedback::FeedbackAgent feedback_agent(loop.dispatcher(), - context->svc()); - fidl::BindingSet<fuchsia::feedback::DataProvider> bindings; - context->outgoing()->AddPublicService(bindings.GetHandler(&feedback_agent)); + // We spawn a new process capable of handling fuchsia.feedback.DataProvider + // requests on every incoming request. This has the advantage of tying each + // request to a different process that can be cleaned up once it is done or + // after a timeout and take care of dangling threads for instance, cf. CF-756. + context->outgoing()->AddPublicService(SpawnNewDataProvider()); loop.Run(); diff --git a/src/developer/feedback_agent/meta/feedback_agent.cmx b/src/developer/feedback_agent/meta/feedback_agent.cmx index b9ec6b2b102e8847035d2011cb056cea569135c0..bd5870f7a6ea1a933ae80c866bec29d3bb5e60e7 100644 --- a/src/developer/feedback_agent/meta/feedback_agent.cmx +++ b/src/developer/feedback_agent/meta/feedback_agent.cmx @@ -12,6 +12,7 @@ "services": [ "fuchsia.logger.Log", "fuchsia.logger.LogSink", + "fuchsia.process.Launcher", "fuchsia.ui.scenic.Scenic" ] } diff --git a/src/developer/feedback_agent/tests/BUILD.gn b/src/developer/feedback_agent/tests/BUILD.gn index bd1c0581289875a30f60f3cbcc0237bcd3942ac4..3ac333d87599e24763f39eb2ca20d9a5921aba54 100644 --- a/src/developer/feedback_agent/tests/BUILD.gn +++ b/src/developer/feedback_agent/tests/BUILD.gn @@ -8,7 +8,7 @@ import("//build/testing/environments.gni") test_package("feedback_agent_tests") { tests = [ { - name = "feedback_agent_unittest" + name = "data_provider_unittest" }, { name = "log_listener_unittest" @@ -29,17 +29,17 @@ test_package("feedback_agent_tests") { ] deps = [ + ":data_provider_unittest", ":feedback_agent_integration_test", - ":feedback_agent_unittest", ":log_listener_unittest", ] } -executable("feedback_agent_unittest") { +executable("data_provider_unittest") { testonly = true sources = [ - "feedback_agent_unittest.cc", + "data_provider_unittest.cc", "stub_scenic.cc", "stub_scenic.h", ] diff --git a/src/developer/feedback_agent/tests/feedback_agent_unittest.cc b/src/developer/feedback_agent/tests/data_provider_unittest.cc similarity index 95% rename from src/developer/feedback_agent/tests/feedback_agent_unittest.cc rename to src/developer/feedback_agent/tests/data_provider_unittest.cc index 16266a5146a2609b9dd9921b7fec322cbfe6ad49..86ac2a72d56d3ecd2ffddf322ce6efd6c399722a 100644 --- a/src/developer/feedback_agent/tests/feedback_agent_unittest.cc +++ b/src/developer/feedback_agent/tests/data_provider_unittest.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "src/developer/feedback_agent/feedback_agent.h" +#include "src/developer/feedback_agent/data_provider.h" #include <fuchsia/feedback/cpp/fidl.h> #include <fuchsia/math/cpp/fidl.h> @@ -153,7 +153,7 @@ MATCHER_P2(MatchesAttachment, expected_key, expected_value, // // This does not test the environment service. It directly instantiates the // class, without connecting through FIDL. -class FeedbackAgentTest : public gtest::RealLoopFixture { +class DataProviderImplTest : public gtest::RealLoopFixture { public: void SetUp() override { stub_scenic_.reset(new StubScenic()); @@ -163,7 +163,7 @@ class FeedbackAgentTest : public gtest::RealLoopFixture { FXL_CHECK(service_directory_provider_.AddService( stub_logger_->GetHandler(dispatcher())) == ZX_OK); - agent_.reset(new FeedbackAgent( + agent_.reset(new DataProviderImpl( dispatcher(), service_directory_provider_.service_directory())); } @@ -180,7 +180,7 @@ class FeedbackAgentTest : public gtest::RealLoopFixture { stub_logger_->set_messages(messages); } - std::unique_ptr<FeedbackAgent> agent_; + std::unique_ptr<DataProviderImpl> agent_; private: ::sys::testing::ServiceDirectoryProvider service_directory_provider_; @@ -189,7 +189,7 @@ class FeedbackAgentTest : public gtest::RealLoopFixture { std::unique_ptr<StubLogger> stub_logger_; }; -TEST_F(FeedbackAgentTest, GetScreenshot_SucceedOnScenicReturningSuccess) { +TEST_F(DataProviderImplTest, GetScreenshot_SucceedOnScenicReturningSuccess) { const size_t image_dim_in_px = 100; std::vector<TakeScreenshotResponse> scenic_responses; scenic_responses.emplace_back(CreateCheckerboardScreenshot(image_dim_in_px), @@ -227,7 +227,7 @@ TEST_F(FeedbackAgentTest, GetScreenshot_SucceedOnScenicReturningSuccess) { EXPECT_EQ(actual_pixels, expected_pixels); } -TEST_F(FeedbackAgentTest, GetScreenshot_FailOnScenicReturningFailure) { +TEST_F(DataProviderImplTest, GetScreenshot_FailOnScenicReturningFailure) { std::vector<TakeScreenshotResponse> scenic_responses; scenic_responses.emplace_back(CreateEmptyScreenshot(), kFailure); set_scenic_responses(std::move(scenic_responses)); @@ -248,7 +248,7 @@ TEST_F(FeedbackAgentTest, GetScreenshot_FailOnScenicReturningFailure) { EXPECT_EQ(feedback_response.screenshot, nullptr); } -TEST_F(FeedbackAgentTest, +TEST_F(DataProviderImplTest, GetScreenshot_FailOnScenicReturningNonBGRA8Screenshot) { std::vector<TakeScreenshotResponse> scenic_responses; scenic_responses.emplace_back(CreateNonBGRA8Screenshot(), kSuccess); @@ -270,8 +270,8 @@ TEST_F(FeedbackAgentTest, EXPECT_EQ(feedback_response.screenshot, nullptr); } -TEST_F(FeedbackAgentTest, GetScreenshot_ParallelRequests) { - // We simulate three calls to FeedbackAgent::GetScreenshot(): one for which +TEST_F(DataProviderImplTest, GetScreenshot_ParallelRequests) { + // We simulate three calls to DataProviderImpl::GetScreenshot(): one for which // the stub Scenic will return a checkerboard 10x10, one for a 20x20 and one // failure. const size_t num_calls = 3u; @@ -300,7 +300,7 @@ TEST_F(FeedbackAgentTest, GetScreenshot_ParallelRequests) { EXPECT_TRUE(get_scenic_responses().empty()); - // We cannot assume that the order of the FeedbackAgent::GetScreenshot() + // We cannot assume that the order of the DataProviderImpl::GetScreenshot() // calls match the order of the Scenic::TakeScreenshot() callbacks because of // the async message loop. Thus we need to match them as sets. // @@ -328,7 +328,7 @@ TEST_F(FeedbackAgentTest, GetScreenshot_ParallelRequests) { } } -TEST_F(FeedbackAgentTest, GetData_SmokeTest) { +TEST_F(DataProviderImplTest, GetData_SmokeTest) { // CollectSystemLogs() has its own set of unit tests so we only cover one log // message here to check that we are attaching the logs. set_logger_messages({ diff --git a/src/developer/feedback_agent/tests/feedback_agent_integration_test.cc b/src/developer/feedback_agent/tests/feedback_agent_integration_test.cc index 55d38607455b937c2bc229072bd6dffd379121b9..734747738f63dde4bb1b872c8a50ced3ff4b2978 100644 --- a/src/developer/feedback_agent/tests/feedback_agent_integration_test.cc +++ b/src/developer/feedback_agent/tests/feedback_agent_integration_test.cc @@ -149,19 +149,31 @@ void CheckNumberOfDataProviderProcesses( EXPECT_EQ(num_feedback_agents, 1u); } -// This test case isn't super useful in itself. This is just to demonstrate how -// to use the utility helper that will be needed to check the number of spawned -// data providers, cf. DX-1497. -TEST_F(FeedbackAgentIntegrationTest, CheckFeedbackAgentSpawned) { - DataProviderSyncPtr data_provider; - environment_services_->Connect(data_provider.NewRequest()); +TEST_F(FeedbackAgentIntegrationTest, OneDataProviderPerRequest) { + DataProviderSyncPtr data_provider_1; + environment_services_->Connect(data_provider_1.NewRequest()); // As the connection is asynchronous, we make a call with the SyncPtr to make // sure the connection is established and the process for the service spawned // before checking its existence. DataProvider_GetData_Result out_result; - ASSERT_EQ(data_provider->GetData(&out_result), ZX_OK); - - CheckNumberOfDataProviderProcesses(0u); + ASSERT_EQ(data_provider_1->GetData(&out_result), ZX_OK); + CheckNumberOfDataProviderProcesses(1u); + + DataProviderSyncPtr data_provider_2; + environment_services_->Connect(data_provider_2.NewRequest()); + ASSERT_EQ(data_provider_2->GetData(&out_result), ZX_OK); + CheckNumberOfDataProviderProcesses(2u); + + DataProviderSyncPtr data_provider_3; + environment_services_->Connect(data_provider_3.NewRequest()); + ASSERT_EQ(data_provider_3->GetData(&out_result), ZX_OK); + CheckNumberOfDataProviderProcesses(3u); + + data_provider_1.Unbind(); + data_provider_2.Unbind(); + data_provider_3.Unbind(); + // Ideally we would check after each Unbind() that there is one less + // data_provider process, but the process clean up is asynchronous. } } // namespace diff --git a/src/developer/feedback_agent/tests/meta/feedback_agent_unittest.cmx b/src/developer/feedback_agent/tests/meta/data_provider_unittest.cmx similarity index 84% rename from src/developer/feedback_agent/tests/meta/feedback_agent_unittest.cmx rename to src/developer/feedback_agent/tests/meta/data_provider_unittest.cmx index 3a25fa36a722c9cdd272b5f0958e3642f0c5ca4a..953149cb9cdf02f95764486ae85e87791d545c39 100644 --- a/src/developer/feedback_agent/tests/meta/feedback_agent_unittest.cmx +++ b/src/developer/feedback_agent/tests/meta/data_provider_unittest.cmx @@ -1,6 +1,6 @@ { "program": { - "binary": "test/feedback_agent_unittest" + "binary": "test/data_provider_unittest" }, "sandbox": { "dev": [