[fuchsia] Fix WebContentRunner to handle Context failures.

WebContentRunner used to exit() if its Context terminated unexpectedly.
This was removed when support was added for a single Runner service to
run WebComponents in their own isolated Contexts. Launch of the Context
requires move()ing the contents of the CreateContextParams, resulting in
some of its fields (in particular the service-directory) still being
set, but to nulled values.

Failure of the default Context would therefore leave the
WebContentRunner active but unable to launch any new non-isolated
WebComponents. This affected both the Cast and web Runners.

WebContentRunner & CastRunner are fixed to consume a callback which they
can Run() to get a fresh fuchsia.web.CreateContextParams table from the
caller.

Bug: 1066826, b/152696080
Change-Id: I3610765751c402cd0c3d2acca13c3f0e164f85d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2134231
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: David Dorwin <ddorwin@chromium.org>
Auto-Submit: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757011}

fuchsia/runners/cast/cast_component.cc

@@ -102,6 +102,10 @@ void CastComponent::StartComponent() {
   }
 }
 
+CastRunner* CastComponent::runner() const {
+  return static_cast<CastRunner*>(WebComponent::runner());
+}
+
 void CastComponent::DestroyComponent(int termination_exit_code,
                                      fuchsia::sys::TerminationReason reason) {
   DCHECK(!constructor_active_);

fuchsia/runners/cast/cast_component.h

@@ -71,6 +71,8 @@ class CastComponent : public WebComponent,
 
   cr_fuchsia::AgentManager* agent_manager() { return agent_manager_.get(); }
 
+  CastRunner* runner() const;
+
  private:
   FRIEND_TEST_ALL_PREFIXES(HeadlessCastRunnerIntegrationTest, Headless);
 

fuchsia/runners/cast/cast_runner.h

@@ -29,16 +29,18 @@ class FilteredServiceDirectory;
 // sys::Runner which instantiates Cast activities specified via cast/casts URIs.
 class CastRunner : public WebContentRunner {
  public:
-  using OnDestructionCallback = base::OnceCallback<void(CastRunner*)>;
-
+  // Creates a Runner for Cast components and publishes it into the specified
+  // OutgoingDirectory.
+  // |get_context_params_callback|: Returns the context parameters to use.
+  // |is_headless|: True if |get_context_params_callback| sets the HEADLESS
+  //     feature flag.
   // |outgoing_directory|: The directory that this CastRunner will publish
   //     itself to.
-  // |context_feature_flags|: The feature flags to use when creating the
-  //     runner's Context.
-  CastRunner(fuchsia::web::CreateContextParams create_context_params,
+  CastRunner(GetContextParamsCallback get_context_params_callback,
+             bool is_headless,
              sys::OutgoingDirectory* outgoing_directory);
-
   ~CastRunner() override;
+
   CastRunner(const CastRunner&) = delete;
   CastRunner& operator=(const CastRunner&) = delete;
 
@@ -54,10 +56,15 @@ class CastRunner : public WebContentRunner {
   // Used to connect to the CastAgent to access Cast-specific services.
   static const char kAgentComponentUrl[];
 
+  // Returns true if this Runner is configured not to use Scenic.
+  bool is_headless() const { return is_headless_; }
+
   // Returns the number of active CastRunner instances.
   size_t GetChildCastRunnerCountForTest();
 
  private:
+  using OnDestructionCallback = base::OnceCallback<void(CastRunner*)>;
+
   // Constructor used for creating CastRunners that run apps in dedicated
   // Contexts. Child CastRunners may only spawn one Component and will be
   // destroyed by their parents when their singleton Components are destroyed.
@@ -66,35 +73,43 @@ class CastRunner : public WebContentRunner {
              fuchsia::web::ContextPtr context,
              bool is_headless);
 
-  // Initializes the service directory that's passed to the web context. Must be
-  // called during initialization, before the context is created.
-  void InitializeServiceDirectory();
+  // Creates and returns the service directory that is passed to the main web
+  // context.
+  std::unique_ptr<base::fuchsia::FilteredServiceDirectory>
+  CreateServiceDirectory();
 
   // Starts a component once all configuration data is available.
   void MaybeStartComponent(
       CastComponent::CastComponentParams* pending_component_params);
 
   // Cancels the launch of a component.
-  void CancelComponentLaunch(CastComponent::CastComponentParams* params);
+  void CancelComponentLaunch(
+      CastComponent::CastComponentParams* pending_component_params);
 
   void CreateAndRegisterCastComponent(
-      CastComponent::CastComponentParams params);
-  void GetConfigCallback(CastComponent::CastComponentParams* pending_component,
-                         chromium::cast::ApplicationConfig app_config);
-  void GetBindingsCallback(
-      CastComponent::CastComponentParams* pending_component,
-      std::vector<chromium::cast::ApiBinding> bindings);
+      CastComponent::CastComponentParams component_params);
+  void OnApplicationConfigReceived(
+      CastComponent::CastComponentParams* pending_component_params,
+      chromium::cast::ApplicationConfig app_config);
   void OnChildRunnerDestroyed(CastRunner* cast_runner);
 
   // Creates a CastRunner configured to serve data from content directories in
   // |params|. Returns nullptr if an error occurred during CastRunner creation.
   CastRunner* CreateChildRunnerForIsolatedComponent(
-      CastComponent::CastComponentParams* params);
+      CastComponent::CastComponentParams* component_params);
 
   // Handler for fuchsia.media.Audio requests in |service_directory_|.
   void ConnectAudioProtocol(
       fidl::InterfaceRequest<fuchsia::media::Audio> request);
 
+  // Returns the parameters with which the main context should be created.
+  fuchsia::web::CreateContextParams GetMainContextParams();
+
+  const WebContentRunner::GetContextParamsCallback get_context_params_callback_;
+
+  // True if this Runner uses Context(s) with the HEADLESS feature set.
+  const bool is_headless_;
+
   // Holds StartComponent() requests while the ApplicationConfig is being
   // fetched from the ApplicationConfigManager.
   base::flat_set<std::unique_ptr<CastComponent::CastComponentParams>,
@@ -112,7 +127,10 @@ class CastRunner : public WebContentRunner {
   base::flat_set<std::unique_ptr<CastRunner>, base::UniquePtrComparator>
       isolated_runners_;
 
-  std::unique_ptr<base::fuchsia::FilteredServiceDirectory> service_directory_;
+  // ServiceDirectory passed to the main Context, to allow Audio capturer
+  // service requests to be routed to the relevant Agent.
+  const std::unique_ptr<base::fuchsia::FilteredServiceDirectory>
+      service_directory_;
 
   // Last component that was created with permission to access MICROPHONE.
   CastComponent* audio_capturer_component_ = nullptr;

fuchsia/runners/cast/cast_runner_integration_test.cc

@@ -222,16 +222,25 @@ class CastRunnerIntegrationTest : public testing::Test {
     EnsureFuchsiaDirSchemeInitialized();
 
     // Create the CastRunner, published into |outgoing_directory_|.
-    fuchsia::web::CreateContextParams create_context_params;
-    create_context_params.set_features(feature_flags);
-    create_context_params.set_service_directory(base::fuchsia::OpenDirectory(
-        base::FilePath(base::fuchsia::kServiceDirectoryPath)));
-    CHECK(create_context_params.service_directory());
-
-    const uint16_t kRemoteDebuggingAnyPort = 0;
-    create_context_params.set_remote_debugging_port(kRemoteDebuggingAnyPort);
+    WebContentRunner::GetContextParamsCallback get_context_params =
+        base::BindLambdaForTesting([feature_flags]() {
+          fuchsia::web::CreateContextParams create_context_params;
+          create_context_params.set_features(feature_flags);
+          create_context_params.set_service_directory(
+              base::fuchsia::OpenDirectory(
+                  base::FilePath(base::fuchsia::kServiceDirectoryPath)));
+          CHECK(create_context_params.service_directory());
+
+          const uint16_t kRemoteDebuggingAnyPort = 0;
+          create_context_params.set_remote_debugging_port(
+              kRemoteDebuggingAnyPort);
+          return create_context_params;
+        });
     cast_runner_ = std::make_unique<CastRunner>(
-        std::move(create_context_params), &outgoing_directory_);
+        std::move(get_context_params),
+        (feature_flags & fuchsia::web::ContextFeatureFlags::HEADLESS) ==
+            fuchsia::web::ContextFeatureFlags::HEADLESS,
+        &outgoing_directory_);
 
     cast_runner_->SetContextProviderForTest(cr_fuchsia::ConnectContextProvider(
         context_provider_controller_.NewRequest()));
@@ -257,7 +266,7 @@ class CastRunnerIntegrationTest : public testing::Test {
       base::StringPiece component_url) {
     auto component_state = std::make_unique<FakeComponentState>(
         component_url, &app_config_manager_, &api_bindings_,
-        &url_request_rewrite_rules_provider_);
+        url_request_rewrite_rules_provider_.get());
     component_state_ = component_state.get();
 
     if (init_component_state_callback_)
@@ -291,6 +300,8 @@ class CastRunnerIntegrationTest : public testing::Test {
   }
 
   void CreateComponentContext(const base::StringPiece& component_url) {
+    url_request_rewrite_rules_provider_ =
+        std::make_unique<FakeUrlRequestRewriteRulesProvider>();
     component_context_ = std::make_unique<cr_fuchsia::FakeComponentContext>(
         base::BindRepeating(&CastRunnerIntegrationTest::OnComponentConnect,
                             base::Unretained(this)),
@@ -336,7 +347,7 @@ class CastRunnerIntegrationTest : public testing::Test {
   }
 
   void WaitComponentCreated() {
-    EXPECT_FALSE(cast_component_);
+    ASSERT_FALSE(cast_component_);
 
     base::RunLoop run_loop;
     cr_fuchsia::ResultReceiver<WebComponent*> component_receiver(
@@ -359,13 +370,39 @@ class CastRunnerIntegrationTest : public testing::Test {
     listener.RunUntilUrlAndTitleEquals(url, title);
   }
 
+  void WaitForComponentDestroyed() {
+    ASSERT_TRUE(cast_component_);
+    ASSERT_TRUE(component_state_);
+
+    base::RunLoop state_loop;
+    component_state_->set_on_delete(state_loop.QuitClosure());
+
+    if (component_controller_) {
+      base::RunLoop controller_loop;
+      component_controller_.set_error_handler(
+          [&controller_loop](zx_status_t status) {
+            EXPECT_EQ(status, ZX_ERR_PEER_CLOSED);
+            controller_loop.Quit();
+          });
+      controller_loop.Run();
+    }
+
+    state_loop.Run();
+
+    component_context_ = nullptr;
+    component_services_client_ = nullptr;
+    component_state_ = nullptr;
+    cast_component_ = nullptr;
+  }
+
   base::test::SingleThreadTaskEnvironment task_environment_{
       base::test::SingleThreadTaskEnvironment::MainThreadType::IO};
   net::EmbeddedTestServer test_server_;
 
   FakeApplicationConfigManager app_config_manager_;
   TestApiBindings api_bindings_;
-  FakeUrlRequestRewriteRulesProvider url_request_rewrite_rules_provider_;
+  std::unique_ptr<FakeUrlRequestRewriteRulesProvider>
+      url_request_rewrite_rules_provider_;
 
   // Incoming service directory, ComponentContext and per-component state.
   sys::OutgoingDirectory component_services_;
@@ -414,10 +451,41 @@ TEST_F(CastRunnerIntegrationTest, BasicRequest) {
 
   // Verify that the component is torn down when |component_controller| is
   // unbound.
-  base::RunLoop run_loop;
-  component_state_->set_on_delete(run_loop.QuitClosure());
   component_controller_.Unbind();
-  run_loop.Run();
+  WaitForComponentDestroyed();
+}
+
+// Verify that the Runner can continue to be used even after its Context has
+// crashed. Regression test for https://crbug.com/1066826).
+// TODO(https://crbug.com/1066833): Make this a WebRunner test.
+TEST_F(CastRunnerIntegrationTest, CanRecreateContext) {
+  // Execute two iterations of launching the component and verifying that it
+  // reaches the expected URL.
+  for (int i = 0; i < 2; ++i) {
+    const GURL app_url = test_server_.GetURL(kBlankAppUrl);
+    app_config_manager_.AddApp(kTestAppId, app_url);
+
+    CreateComponentContextAndStartComponent();
+
+    fuchsia::web::NavigationControllerPtr nav_controller;
+    cast_component_->frame()->GetNavigationController(
+        nav_controller.NewRequest());
+
+    base::RunLoop run_loop;
+    cr_fuchsia::ResultReceiver<fuchsia::web::NavigationState> nav_entry(
+        run_loop.QuitClosure());
+    nav_controller->GetVisibleEntry(
+        cr_fuchsia::CallbackToFitFunction(nav_entry.GetReceiveCallback()));
+    run_loop.Run();
+
+    ASSERT_TRUE(nav_entry->has_url());
+    EXPECT_EQ(nav_entry->url(), app_url.spec());
+
+    // Fake teardown of the Context, forcing the next StartComponent to create
+    // a new one.
+    cast_runner_->DisconnectContextForTest();
+    WaitForComponentDestroyed();
+  }
 }
 
 TEST_F(CastRunnerIntegrationTest, ApiBindings) {
@@ -562,10 +630,8 @@ TEST_F(CastRunnerIntegrationTest, IsolatedContext) {
 
   // Verify that the component is torn down when |component_controller| is
   // unbound.
-  base::RunLoop run_loop;
-  component_state_->set_on_delete(run_loop.QuitClosure());
   component_controller_.Unbind();
-  run_loop.Run();
+  WaitForComponentDestroyed();
 
   EXPECT_EQ(cast_runner_->GetChildCastRunnerCountForTest(), 0u);
 }
@@ -787,10 +853,8 @@ TEST_F(HeadlessCastRunnerIntegrationTest, IsolatedAndHeadless) {
 
   // Verify that the component is torn down when |component_controller| is
   // unbound.
-  base::RunLoop run_loop;
-  component_state_->set_on_delete(run_loop.QuitClosure());
   component_controller_.Unbind();
-  run_loop.Run();
+  WaitForComponentDestroyed();
 
   EXPECT_EQ(cast_runner_->GetChildCastRunnerCountForTest(), 0u);
 }

fuchsia/runners/cast/fake_application_config_manager.cc

@@ -54,6 +54,6 @@ void FakeApplicationConfigManager::GetConfig(std::string id,
     return;
   }
 
-  callback(std::move(std::move(id_to_config_[id])));
+  callback(std::move(id_to_config_[id]));
   id_to_config_.erase(id);
 }

fuchsia/runners/cast/main.cc

@@ -8,6 +8,7 @@
 #include "base/fuchsia/default_context.h"
 #include "base/fuchsia/file_utils.h"
 #include "base/message_loop/message_pump_type.h"
+#include "base/no_destructor.h"
 #include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/task/single_thread_task_executor.h"
@@ -33,19 +34,7 @@ bool IsHeadless() {
   return false;
 }
 
-}  // namespace
-
-int main(int argc, char** argv) {
-  base::SingleThreadTaskExecutor io_task_executor(base::MessagePumpType::IO);
-  base::RunLoop run_loop;
-
-  base::CommandLine::Init(argc, argv);
-  CHECK(cr_fuchsia::InitLoggingFromCommandLine(
-      *base::CommandLine::ForCurrentProcess()))
-      << "Failed to initialize logging.";
-
-  cr_fuchsia::RegisterFuchsiaDirScheme();
-
+fuchsia::web::CreateContextParams CreateMainContextParams() {
   fuchsia::web::ContextFeatureFlags features =
       fuchsia::web::ContextFeatureFlags::NETWORK |
       fuchsia::web::ContextFeatureFlags::AUDIO |
@@ -82,8 +71,27 @@ int main(int argc, char** argv) {
   create_context_params.set_unsafely_treat_insecure_origins_as_secure(
       {"allow-running-insecure-content"});
 
+  return create_context_params;
+}
+
+}  // namespace
+
+int main(int argc, char** argv) {
+  base::SingleThreadTaskExecutor io_task_executor(base::MessagePumpType::IO);
+  base::RunLoop run_loop;
+
+  base::CommandLine::Init(argc, argv);
+  CHECK(cr_fuchsia::InitLoggingFromCommandLine(
+      *base::CommandLine::ForCurrentProcess()))
+      << "Failed to initialize logging.";
+
+  cr_fuchsia::RegisterFuchsiaDirScheme();
+
+  WebContentRunner::GetContextParamsCallback get_context_params_callback =
+      base::BindRepeating(&CreateMainContextParams);
+
   CastRunner runner(
-      std::move(create_context_params),
+      std::move(get_context_params_callback), IsHeadless(),
       base::fuchsia::ComponentContextForCurrentProcess()->outgoing().get());
 
   base::fuchsia::ComponentContextForCurrentProcess()

fuchsia/runners/common/web_content_runner.cc

@@ -23,25 +23,21 @@
 #include "url/gurl.h"
 
 WebContentRunner::WebContentRunner(
-    fuchsia::web::CreateContextParams create_params,
+    GetContextParamsCallback get_context_params_callback,
     sys::OutgoingDirectory* outgoing_directory)
-    : create_params_(std::move(create_params)),
-      is_headless_((create_params_.features() &
-                    fuchsia::web::ContextFeatureFlags::HEADLESS) ==
-                   fuchsia::web::ContextFeatureFlags::HEADLESS) {
+    : get_context_params_callback_(std::move(get_context_params_callback)) {
   service_binding_.emplace(outgoing_directory, this);
 }
 
-WebContentRunner::WebContentRunner(fuchsia::web::ContextPtr context,
-                                   bool is_headless)
-    : context_(std::move(context)), is_headless_(is_headless) {}
+WebContentRunner::WebContentRunner(fuchsia::web::ContextPtr context)
+    : context_(std::move(context)) {}
 
 WebContentRunner::~WebContentRunner() = default;
 
 fuchsia::web::ContextPtr WebContentRunner::CreateWebContext(
-    fuchsia::web::CreateContextParams create_params) {
+    fuchsia::web::CreateContextParams context_params) {
   fuchsia::web::ContextPtr web_context;
-  GetContextProvider()->Create(std::move(create_params),
+  GetContextProvider()->Create(std::move(context_params),
                                web_context.NewRequest());
   web_context.set_error_handler([](zx_status_t status) {
     // If the browser instance died, then exit everything and do not attempt to
@@ -54,8 +50,7 @@ fuchsia::web::ContextPtr WebContentRunner::CreateWebContext(
 
 fuchsia::web::Context* WebContentRunner::GetContext() {
   if (!context_)
-    context_ = CreateWebContext(std::move(create_params_));
-
+    context_ = CreateWebContext(get_context_params_callback_.Run());
   return context_.get();
 }
 
@@ -106,6 +101,10 @@ void WebContentRunner::SetContextProviderForTest(
   context_provider_ = std::move(context_provider);
 }
 
+void WebContentRunner::DisconnectContextForTest() {
+  context_.Unbind();
+}
+
 fuchsia::web::ContextProvider* WebContentRunner::GetContextProvider() {
   if (!context_provider_) {
     context_provider_ = base::fuchsia::ComponentContextForCurrentProcess()

fuchsia/runners/common/web_content_runner.h

@@ -22,23 +22,29 @@ class WebComponent;
 // sys::Runner that instantiates components hosting standard web content.
 class WebContentRunner : public fuchsia::sys::Runner {
  public:
-  using CreateContextCallback = base::OnceCallback<fuchsia::web::ContextPtr()>;
+  using GetContextParamsCallback =
+      base::RepeatingCallback<fuchsia::web::CreateContextParams()>;
 
-  // |create_params|: Parameters to use for the Runner's web.Context.
+  // Creates a Runner for web-based components, and publishes it to the
+  // specified OutgoingDirectory.
+  // |get_context_params_callback|: Returns parameters for the Runner's
+  //     web.Context.
   // |outgoing_directory|: The directory that the Runner's services will be
-  //                       published to.
-  WebContentRunner(fuchsia::web::CreateContextParams create_params,
+  //     published to.
+  WebContentRunner(GetContextParamsCallback get_context_params_callback,
                    sys::OutgoingDirectory* outgoing_directory);
 
-  // Alternative constructor for unpublished Runners.
-  explicit WebContentRunner(fuchsia::web::ContextPtr context, bool is_headless);
+  // Creates a Runner which launches components using the specified |context|.
+  // The caller may publish the Runner, or call StartComponent() manually to
+  // create new components with it.
+  explicit WebContentRunner(fuchsia::web::ContextPtr context);
 
   ~WebContentRunner() override;
 
   // TODO(crbug.com/1046615): Make this static when the injected ContextProvider
   // goes away.
   fuchsia::web::ContextPtr CreateWebContext(
-      fuchsia::web::CreateContextParams create_params);
+      fuchsia::web::CreateContextParams context_params);
 
   // Gets a pointer to this runner's Context, creating one if needed.
   fuchsia::web::Context* GetContext();
@@ -47,9 +53,6 @@ class WebContentRunner : public fuchsia::sys::Runner {
   // channel was dropped, and therefore the component should be destroyed.
   virtual void DestroyComponent(WebComponent* component);
 
-  // Set if Cast applications are to be run without graphical rendering.
-  bool is_headless() const { return is_headless_; }
-
   // fuchsia::sys::Runner implementation.
   void StartComponent(fuchsia::sys::Package package,
                       fuchsia::sys::StartupInfo startup_info,
@@ -68,17 +71,20 @@ class WebContentRunner : public fuchsia::sys::Runner {
   void SetContextProviderForTest(
       fuchsia::web::ContextProviderPtr context_provider);
 
+  // Disconnects the Context used by this Runner.
+  void DisconnectContextForTest();
+
  protected:
   base::RepeatingCallback<void(WebComponent*)>
   web_component_created_callback_for_test() const {
     return web_component_created_callback_for_test_;
   }
 
-  fuchsia::web::CreateContextParams create_params_;
-
  private:
   fuchsia::web::ContextProvider* GetContextProvider();
 
+  const GetContextParamsCallback get_context_params_callback_;
+
   // If set, invoked whenever a WebComponent is created.
   base::RepeatingCallback<void(WebComponent*)>
       web_component_created_callback_for_test_;
@@ -87,7 +93,6 @@ class WebContentRunner : public fuchsia::sys::Runner {
   fuchsia::web::ContextPtr context_;
   std::set<std::unique_ptr<WebComponent>, base::UniquePtrComparator>
       components_;
-  const bool is_headless_;
 
   // Publishes this Runner into the service directory specified at construction.
   // This is not set for child runner instances.

fuchsia/runners/web/main.cc

@@ -15,17 +15,9 @@
 #include "fuchsia/runners/buildflags.h"
 #include "fuchsia/runners/common/web_content_runner.h"
 
-int main(int argc, char** argv) {
-  base::SingleThreadTaskExecutor io_task_executor(base::MessagePumpType::IO);
-  base::RunLoop run_loop;
-
-  base::CommandLine::Init(argc, argv);
-  CHECK(cr_fuchsia::InitLoggingFromCommandLine(
-      *base::CommandLine::ForCurrentProcess()))
-      << "Failed to initialize logging.";
-
-  cr_fuchsia::RegisterFuchsiaDirScheme();
+namespace {
 
+fuchsia::web::CreateContextParams GetContextParams() {
   fuchsia::web::CreateContextParams create_context_params;
   create_context_params.set_features(
       fuchsia::web::ContextFeatureFlags::NETWORK |
@@ -46,9 +38,27 @@ int main(int argc, char** argv) {
   create_context_params.set_remote_debugging_port(
       BUILDFLAG(WEB_RUNNER_REMOTE_DEBUGGING_PORT));
 #endif
+  return create_context_params;
+}
+
+}  // namespace
+
+int main(int argc, char** argv) {
+  base::SingleThreadTaskExecutor io_task_executor(base::MessagePumpType::IO);
+  base::RunLoop run_loop;
+
+  base::CommandLine::Init(argc, argv);
+  CHECK(cr_fuchsia::InitLoggingFromCommandLine(
+      *base::CommandLine::ForCurrentProcess()))
+      << "Failed to initialize logging.";
+
+  cr_fuchsia::RegisterFuchsiaDirScheme();
+
+  WebContentRunner::GetContextParamsCallback get_context_params_callback =
+      base::BindRepeating(&GetContextParams);
 
   WebContentRunner runner(
-      std::move(create_context_params),
+      std::move(get_context_params_callback),
       base::fuchsia::ComponentContextForCurrentProcess()->outgoing().get());
 
   base::fuchsia::ComponentContextForCurrentProcess()