[modular] Fix & renable flaky tests embed_shell and story_shell
There is a race condition between the story shell getting a ViewOwner
for the module, and the module dropping the ViewOwner on the floor
(because its a test module which doesn't care to draw things on the
screen). The issue happens when the latter happens first, and the
ViewOwner that the story shell receives is invalid, which causes the
FIDL call to fail (since the FIDL interface requires it to be valid).
Fixes in this patch:
* Hold onto ViewProvider in these test modules indefinitely so ViewOwner
is not dropped on the floor
* unrelated: fix FindAnchor() to use ModuleData::is_embedded, since the
other signal is not accurate.
Additional cleanup:
* Clearer names which would've helped me follow code quicker
(e.g., ProcessPendingStoryShellViews)
* Fix code to follow fuchsia/google C++ style guide (e.g., data types declared
in the class first).
MF-178
MF-174 #done
TEST=embed_shell and story_shell don't flake for me anymore.
Change-Id: I42250accaf301c10ec66dbdb2df37515f55408a5
diff --git a/bin/sessionmgr/sessionmgr_impl.cc b/bin/sessionmgr/sessionmgr_impl.cc
index 6330dd0..ad00f1b 100644
--- a/bin/sessionmgr/sessionmgr_impl.cc
+++ b/bin/sessionmgr/sessionmgr_impl.cc
@@ -765,7 +765,7 @@
session_shell_app_->services().ConnectToService(view_provider.NewRequest());
view_provider->CreateView(view_owner.NewRequest(), nullptr);
session_shell_view_host_->ConnectView(std::move(view_owner));
-} // namespace modular
+}
void SessionmgrImpl::TerminateSessionShell(const std::function<void()>& done) {
session_shell_app_->Teardown(kBasicTimeout, [this, done] {
diff --git a/bin/sessionmgr/story_runner/story_controller_impl.cc b/bin/sessionmgr/story_runner/story_controller_impl.cc
index 487e4eb..3adaa11 100644
--- a/bin/sessionmgr/story_runner/story_controller_impl.cc
+++ b/bin/sessionmgr/story_runner/story_controller_impl.cc
@@ -389,6 +389,12 @@
story_controller_impl_, fidl::Clone(module_data_),
std::move(module_controller_request_), view_owner_.NewRequest(),
[this, flow] { LoadModuleManifest(flow); }));
+ view_owner_.set_error_handler([module_url =
+ module_data_.module_url](zx_status_t) {
+ FXL_LOG(ERROR) << "ViewOwner associated with module_url=" << module_url
+ << " died. This module likely won't be able to display "
+ "anything on the screen.";
+ });
}
void LoadModuleManifest(FlowToken flow) {
@@ -401,6 +407,10 @@
});
}
+ // We only add a module to story shell if it's either a root module or its
+ // anchor module is already known to story shell. Otherwise, we pend its view
+ // (StoryControllerImpl::pending_story_shell_views_) and pass it to the story
+ // shell once its anchor module is ready.
void MaybeConnectViewToStoryShell(FlowToken flow) {
// If this is called during Stop(), story_shell_ might already have been
// reset. TODO(mesch): Then the whole operation should fail.
@@ -408,10 +418,8 @@
return;
}
- // We only add a module to story shell if its either a root module or its
- // anchor is already known to story shell.
-
if (module_data_.module_path.size() == 1) {
+ // This is a root module; pass it's view on to the story shell.
ConnectViewToStoryShell(flow, "");
return;
}
@@ -439,11 +447,12 @@
module_data_.surface_relation->Clone(surface_relation_clone.get());
}
- story_controller_impl_->pending_views_.emplace(
+ story_controller_impl_->pending_story_shell_views_.emplace(
ModulePathToSurfaceID(module_data_.module_path),
- PendingView{module_data_.module_path, std::move(manifest_clone),
- std::move(surface_relation_clone),
- module_data_.module_source, std::move(view_owner_)});
+ PendingViewForStoryShell{
+ module_data_.module_path, std::move(manifest_clone),
+ std::move(surface_relation_clone), module_data_.module_source,
+ std::move(view_owner_)});
}
void ConnectViewToStoryShell(FlowToken flow,
@@ -463,7 +472,7 @@
std::move(surface_info));
story_controller_impl_->connected_views_.emplace(surface_id);
- story_controller_impl_->ProcessPendingViews();
+ story_controller_impl_->ProcessPendingStoryShellViews();
story_controller_impl_->story_shell_->FocusSurface(surface_id);
}
@@ -554,8 +563,8 @@
story_controller_impl_->DetachView([cont] { cont(false); });
- async::PostDelayedTask(
- async_get_default_dispatcher(), [cont] { cont(true); }, kBasicTimeout);
+ async::PostDelayedTask(async_get_default_dispatcher(),
+ [cont] { cont(true); }, kBasicTimeout);
}
void StopStory() {
@@ -855,7 +864,7 @@
std::move(module_controller_request_),
std::move(view_owner_request_), [this, flow] {
// LaunchModuleInShellCall above already calls
- // ProcessPendingViews(). NOTE(thatguy): This
+ // ProcessPendingStoryShellViews(). NOTE(thatguy): This
// cannot be moved into LaunchModuleCall, because
// LaunchModuleInShellCall uses LaunchModuleCall
// as the very first step of its operation. This
@@ -864,7 +873,7 @@
// surface-relation parent (which we do as the
// second part of LaunchModuleInShellCall). So
// we must defer to here.
- story_controller_impl_->ProcessPendingViews();
+ story_controller_impl_->ProcessPendingStoryShellViews();
}));
}
}
@@ -1153,7 +1162,8 @@
});
FXL_DCHECK(fit != running_mod_infos_.end());
fit->module_controller_impl.release();
- pending_views_.erase(ModulePathToSurfaceID(fit->module_data->module_path));
+ pending_story_shell_views_.erase(
+ ModulePathToSurfaceID(fit->module_data->module_path));
running_mod_infos_.erase(fit);
}
@@ -1243,7 +1253,7 @@
nullptr /* view_owner_request */, std::move(callback)));
}
-void StoryControllerImpl::ProcessPendingViews() {
+void StoryControllerImpl::ProcessPendingStoryShellViews() {
// NOTE(mesch): As it stands, this machinery to send modules in traversal
// order to the story shell is N^3 over the lifetime of the story, where N
// is the number of modules. This function is N^2, and it's called once for
@@ -1257,7 +1267,7 @@
std::vector<fidl::StringPtr> added_keys;
- for (auto& kv : pending_views_) {
+ for (auto& kv : pending_story_shell_views_) {
auto* const running_mod_info = FindRunningModInfo(kv.second.module_path);
if (!running_mod_info) {
continue;
@@ -1292,9 +1302,9 @@
if (added_keys.size()) {
for (auto& key : added_keys) {
- pending_views_.erase(key);
+ pending_story_shell_views_.erase(key);
}
- ProcessPendingViews();
+ ProcessPendingStoryShellViews();
}
}
@@ -1491,10 +1501,8 @@
auto* anchor = FindRunningModInfo(
ParentModulePath(running_mod_info->module_data->module_path));
- // Traverse up until there is a non-embedded module. We recognize
- // non-embedded modules by having a non-null SurfaceRelation. If the root
- // module is there at all, it has a non-null surface relation.
- while (anchor && !anchor->module_data->surface_relation) {
+ // Traverse up until there is a non-embedded module.
+ while (anchor && anchor->module_data->is_embedded) {
anchor =
FindRunningModInfo(ParentModulePath(anchor->module_data->module_path));
}
@@ -1570,4 +1578,4 @@
}
}
-} // namespace modular
+} // namespace modular
\ No newline at end of file
diff --git a/bin/sessionmgr/story_runner/story_controller_impl.h b/bin/sessionmgr/story_runner/story_controller_impl.h
index 1560544..dbd22b2 100644
--- a/bin/sessionmgr/story_runner/story_controller_impl.h
+++ b/bin/sessionmgr/story_runner/story_controller_impl.h
@@ -148,6 +148,54 @@
void StopBulk(bool bulk, StopCallback done);
private:
+ // Operations implemented here.
+ class AddIntentCall;
+ class DefocusCall;
+ class FocusCall;
+ class KillModuleCall;
+ class LaunchModuleCall;
+ class LaunchModuleInShellCall;
+ class OnModuleDataUpdatedCall;
+ class ResolveParameterCall;
+ class StartCall;
+ class StopCall;
+ class StopModuleCall;
+ class StopModuleAndStoryIfEmptyCall;
+ class StartSnapshotLoaderCall;
+ class UpdateSnapshotCall;
+
+ // For each *running* Module in the Story, there is one RunningModInfo.
+ struct RunningModInfo {
+ // NOTE: |module_data| is a cached copy of what is stored in
+ // |story_storage_|, the source of truth. It is updated in two
+ // places:
+ //
+ // 1) In LaunchModuleCall (used by LaunchModuleInShellCall) in the case
+ // that either a) the module isn't running yet or b) ModuleData.intent
+ // differs from what is cached.
+ //
+ // 2) Indirectly from OnModuleDataUpdated(), which is called when another
+ // device updates the Module by calling LaunchModuleInShellCall. However,
+ // this only happens if the Module is EXTERNAL (it was not explicitly added
+ // by another Module).
+ //
+ // TODO(thatguy): we should ensure that the local cached copy is always
+ // up to date no matter what.
+ fuchsia::modular::ModuleDataPtr module_data;
+ std::unique_ptr<ModuleContextImpl> module_context_impl;
+ std::unique_ptr<ModuleControllerImpl> module_controller_impl;
+ };
+
+ // A module's story shell-related information that we pend until we are able
+ // to pass it off to the story shell.
+ struct PendingViewForStoryShell {
+ std::vector<std::string> module_path;
+ fuchsia::modular::ModuleManifestPtr module_manifest;
+ fuchsia::modular::SurfaceRelationPtr surface_relation;
+ fuchsia::modular::ModuleSource module_source;
+ fuchsia::ui::viewsv1token::ViewOwnerPtr view_owner;
+ };
+
// |StoryController|
void Stop(StopCallback done) override;
void GetInfo(GetInfoCallback callback) override;
@@ -184,7 +232,7 @@
void NotifyOneStoryWatcher(
const fuchsia::modular::storymodel::StoryModel& model,
fuchsia::modular::StoryWatcher* watcher);
- void ProcessPendingViews();
+ void ProcessPendingStoryShellViews();
std::set<fuchsia::modular::LinkPath> GetActiveLinksInternal();
bool IsExternalModule(const std::vector<std::string>& module_path);
@@ -202,6 +250,21 @@
// processes.
void DestroyStoryEnvironment();
+ // Finds the active RunningModInfo for a module at the given module path. May
+ // return nullptr if the module at the path is not running, regardless of
+ // whether a module at that path is known to the story.
+ RunningModInfo* FindRunningModInfo(
+ const std::vector<std::string>& module_path);
+
+ // Finds the active RunningModInfo for the story shell anchor of a module
+ // with the given |running_mod_info|. The anchor is the closest ancestor
+ // module of the given module that is not embedded and actually known to the
+ // story shell. This requires that it must be running, otherwise it cannot be
+ // connected to the story shell. May return nullptr if the anchor module, or
+ // any intermediate module, is not running, regardless of whether a module at
+ // such path is known to the story.
+ RunningModInfo* FindAnchor(RunningModInfo* running_mod_info);
+
// The ID of the story, copied from |story_observer_| for convenience in
// transitioning clients. TODO(thatguy): Remove users of this in favor of
// reading from the |story_observer_| directly.
@@ -240,57 +303,14 @@
// not yet sent to story shell.
std::set<fidl::StringPtr> connected_views_;
- // Holds the view of a non-embedded running module (identified by its
- // serialized module path) until its parent is connected to story shell. Story
- // shell cannot display views whose parents are not yet displayed.
- struct PendingView {
- std::vector<std::string> module_path;
- fuchsia::modular::ModuleManifestPtr module_manifest;
- fuchsia::modular::SurfaceRelationPtr surface_relation;
- fuchsia::modular::ModuleSource module_source;
- fuchsia::ui::viewsv1token::ViewOwnerPtr view_owner;
- };
- std::map<std::string, PendingView> pending_views_;
+ // Since story shell cannot display views whose parents are not yet displayed,
+ // |pending_story_shell_views_| holds the view of a non-embedded running
+ // module (identified by its serialized module path) until its parent is
+ // connected to story shell.
+ std::map<std::string, PendingViewForStoryShell> pending_story_shell_views_;
- // The first ingredient of a story: Modules. For each *running* Module in the
- // Story, there is one RunningModInfo.
- struct RunningModInfo {
- // NOTE: |module_data| is a cached copy of what is stored in
- // |story_storage_|, the source of truth. It is updated in two
- // places:
- //
- // 1) In LaunchModuleCall (used by LaunchModuleInShellCall) in the case
- // that either a) the module isn't running yet or b) ModuleData.intent
- // differs from what is cached.
- //
- // 2) Indirectly from OnModuleDataUpdated(), which is called when another
- // device updates the Module by calling LaunchModuleInShellCall. However,
- // this only happens if the Module is EXTERNAL (it was not explicitly added
- // by another Module).
- //
- // TODO(thatguy): we should ensure that the local cached copy is always
- // up to date no matter what.
- fuchsia::modular::ModuleDataPtr module_data;
- std::unique_ptr<ModuleContextImpl> module_context_impl;
- std::unique_ptr<ModuleControllerImpl> module_controller_impl;
- };
std::vector<RunningModInfo> running_mod_infos_;
- // Finds the active RunningModInfo for a module at the given module path. May
- // return nullptr if the module at the path is not running, regardless of
- // whether a module at that path is known to the story.
- RunningModInfo* FindRunningModInfo(
- const std::vector<std::string>& module_path);
-
- // Finds the active RunningModInfo for the story shell anchor of a module
- // with the given |running_mod_info|. The anchor is the closest ancestor
- // module of the given module that is not embedded and actually known to the
- // story shell. This requires that it must be running, otherwise it cannot be
- // connected to the story shell. May return nullptr if the anchor module, or
- // any intermediate module, is not running, regardless of whether a module at
- // such path is known to the story.
- RunningModInfo* FindAnchor(RunningModInfo* running_mod_info);
-
// The second ingredient of a story: Links. They connect Modules.
fidl::BindingSet<Link, std::unique_ptr<LinkImpl>> link_impls_;
@@ -312,22 +332,6 @@
fxl::WeakPtrFactory<StoryControllerImpl> weak_factory_;
- // Operations implemented here.
- class AddIntentCall;
- class DefocusCall;
- class FocusCall;
- class KillModuleCall;
- class LaunchModuleCall;
- class LaunchModuleInShellCall;
- class OnModuleDataUpdatedCall;
- class ResolveParameterCall;
- class StartCall;
- class StopCall;
- class StopModuleCall;
- class StopModuleAndStoryIfEmptyCall;
- class StartSnapshotLoaderCall;
- class UpdateSnapshotCall;
-
FXL_DISALLOW_COPY_AND_ASSIGN(StoryControllerImpl);
};
diff --git a/tests/common/common_null_module.cc b/tests/common/common_null_module.cc
index 1d212b2..c406863 100644
--- a/tests/common/common_null_module.cc
+++ b/tests/common/common_null_module.cc
@@ -18,26 +18,26 @@
// The NullModule just sits there and does nothing until it's terminated.
class NullModule {
public:
- TestPoint initialized_{"Null module initialized"};
-
NullModule(modular::ModuleHost* const module_host,
- fidl::InterfaceRequest<
- fuchsia::ui::app::ViewProvider> /*view_provider_request*/)
- : module_host_(module_host) {
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>
+ view_provider_request)
+ : module_host_(module_host),
+ app_view_provider_(std::move(view_provider_request)) {
modular::testing::Init(module_host_->startup_context(), __FILE__);
initialized_.Pass();
Signal(kCommonNullModuleStarted);
}
NullModule(modular::ModuleHost* const module_host,
- fidl::InterfaceRequest<
- fuchsia::ui::viewsv1::ViewProvider> /*view_provider_request*/)
+ fidl::InterfaceRequest<fuchsia::ui::viewsv1::ViewProvider>
+ view_provider_request)
: NullModule(
module_host,
- fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>(nullptr)) {}
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>(nullptr)) {
+ viewsv1_view_provider_ = std::move(view_provider_request);
+ }
// Called by ModuleDriver.
- TestPoint stopped_{"Null module stopped"};
void Terminate(const std::function<void()>& done) {
Signal(kCommonNullModuleStopped);
stopped_.Pass();
@@ -45,7 +45,15 @@
}
private:
+ TestPoint initialized_{"Null module initialized"};
+ TestPoint stopped_{"Null module stopped"};
+
modular::ModuleHost* const module_host_;
+ // We keep the view provider around so that story shell can hold a view for
+ // us, but don't do anything with it.
+ fidl::InterfaceRequest<fuchsia::ui::viewsv1::ViewProvider>
+ viewsv1_view_provider_;
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider> app_view_provider_;
};
} // namespace
diff --git a/tests/embed_shell/embed_shell_test_child_module.cc b/tests/embed_shell/embed_shell_test_child_module.cc
index cfc7129..fc77731 100644
--- a/tests/embed_shell/embed_shell_test_child_module.cc
+++ b/tests/embed_shell/embed_shell_test_child_module.cc
@@ -20,19 +20,22 @@
class TestModule {
public:
TestModule(modular::ModuleHost* const module_host,
- fidl::InterfaceRequest<
- fuchsia::ui::app::ViewProvider> /*view_provider_request*/)
- : module_host_(module_host) {
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>
+ view_provider_request)
+ : module_host_(module_host),
+ app_view_provider_(std::move(view_provider_request)) {
modular::testing::Init(module_host->startup_context(), __FILE__);
StartChildModule();
}
TestModule(modular::ModuleHost* const module_host,
- fidl::InterfaceRequest<
- fuchsia::ui::viewsv1::ViewProvider> /*view_provider_request*/)
+ fidl::InterfaceRequest<fuchsia::ui::viewsv1::ViewProvider>
+ view_provider_request)
: TestModule(
module_host,
- fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>(nullptr)) {}
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>(nullptr)) {
+ views1_view_provider_ = std::move(view_provider_request);
+ }
// Called from ModuleDriver.
void Terminate(const std::function<void()>& done) {
@@ -62,6 +65,13 @@
}
modular::ModuleHost* const module_host_;
+
+ // We keep the view provider around so that story shell can hold a view for
+ // us, but don't do anything with it.
+ fidl::InterfaceRequest<fuchsia::ui::viewsv1::ViewProvider>
+ views1_view_provider_;
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider> app_view_provider_;
+
fuchsia::modular::ModuleControllerPtr child_module_;
FXL_DISALLOW_COPY_AND_ASSIGN(TestModule);
diff --git a/tests/embed_shell/embed_shell_test_parent_module.cc b/tests/embed_shell/embed_shell_test_parent_module.cc
index f5f7a15..d1dbcee 100644
--- a/tests/embed_shell/embed_shell_test_parent_module.cc
+++ b/tests/embed_shell/embed_shell_test_parent_module.cc
@@ -27,20 +27,23 @@
class TestModule {
public:
TestModule(modular::ModuleHost* const module_host,
- fidl::InterfaceRequest<
- fuchsia::ui::app::ViewProvider> /*view_provider_request*/)
- : module_host_(module_host) {
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>
+ view_provider_request)
+ : module_host_(module_host),
+ app_view_provider_(std::move(view_provider_request)) {
modular::testing::Init(module_host->startup_context(), __FILE__);
ScheduleDone();
StartChildModule();
}
TestModule(modular::ModuleHost* const module_host,
- fidl::InterfaceRequest<
- fuchsia::ui::viewsv1::ViewProvider> /*view_provider_request*/)
+ fidl::InterfaceRequest<fuchsia::ui::viewsv1::ViewProvider>
+ view_provider_request)
: TestModule(
module_host,
- fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>(nullptr)) {}
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>(nullptr)) {
+ views1_view_provider_ = std::move(view_provider_request);
+ }
void Terminate(const std::function<void()>& done) {
modular::testing::Done(done);
@@ -77,6 +80,12 @@
fuchsia::modular::ModuleControllerPtr child_module_;
fuchsia::ui::viewsv1token::ViewOwnerPtr child_view_;
+ // We keep the view provider around so that story shell can hold a view for
+ // us, but don't do anything with it.
+ fidl::InterfaceRequest<fuchsia::ui::viewsv1::ViewProvider>
+ views1_view_provider_;
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider> app_view_provider_;
+
FXL_DISALLOW_COPY_AND_ASSIGN(TestModule);
};
diff --git a/tests/modular_tests.json b/tests/modular_tests.json
index a85e107..fa592ec 100644
--- a/tests/modular_tests.json
+++ b/tests/modular_tests.json
@@ -29,7 +29,6 @@
},
{
"name": "embed_shell",
- "disabled": true,
"exec": [
"basemgr",
"--test",
@@ -94,7 +93,6 @@
},
{
"name": "story_shell",
- "disabled": true,
"exec": [
"fuchsia-pkg://fuchsia.com/basemgr#meta/basemgr.cmx",
"--test",
diff --git a/tests/suggestion/suggestion_test_module.cc b/tests/suggestion/suggestion_test_module.cc
index 0d7b027..62a0757 100644
--- a/tests/suggestion/suggestion_test_module.cc
+++ b/tests/suggestion/suggestion_test_module.cc
@@ -29,9 +29,10 @@
TestPoint proposal_was_accepted_{"fuchsia::modular::Proposal was accepted"};
TestModule(modular::ModuleHost* module_host,
- fidl::InterfaceRequest<
- fuchsia::ui::app::ViewProvider> /*view_provider_request*/)
- : module_host_(module_host) {
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>
+ view_provider_request)
+ : module_host_(module_host),
+ app_view_provider_(std::move(view_provider_request)) {
modular::testing::Init(module_host_->startup_context(), __FILE__);
initialized_.Pass();
@@ -80,11 +81,13 @@
}
TestModule(modular::ModuleHost* const module_host,
- fidl::InterfaceRequest<
- fuchsia::ui::viewsv1::ViewProvider> /*view_provider_request*/)
+ fidl::InterfaceRequest<fuchsia::ui::viewsv1::ViewProvider>
+ view_provider_request)
: TestModule(
module_host,
- fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>(nullptr)) {}
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider>(nullptr)) {
+ views1_view_provider_ = std::move(view_provider_request);
+ }
// Called by ModuleDriver.
TestPoint stopped_{"Root module stopped"};
@@ -105,6 +108,12 @@
fuchsia::modular::ProposalPublisherPtr proposal_publisher_;
fidl::BindingSet<fuchsia::modular::ProposalListener>
proposal_listener_bindings_;
+
+ // We keep the view provider around so that story shell can hold a view for
+ // us, but don't do anything with it.
+ fidl::InterfaceRequest<fuchsia::ui::viewsv1::ViewProvider>
+ views1_view_provider_;
+ fidl::InterfaceRequest<fuchsia::ui::app::ViewProvider> app_view_provider_;
FXL_DISALLOW_COPY_AND_ASSIGN(TestModule);
};
