Commit 18c4ffcb authored by mhasank's avatar mhasank Committed by Commit Bot

login: ignore new subscribers in concierge_client during events

This is spin-off from https://crrev.com/c/2381050 and addressess the same re-entrancy problem for arcvm

ConciergeClient::Observer is implemented by arc_vm_client_adapter.cc and crostini_stability_monitor.cc
arc_vm_client_adapter.cc has the same issue as the container so the TEST below ensures that the change is same
crostini_stability_monitor.cc is only firing metric

ConciergeClient::VmObserver is implemented by arc_vm_client_adapter.cc, lock_to_single_user_manager.cc, crostini_manager.cc, cros_usb_detector.cc and arc_session_manager.cc
I checked all the consumers and I don't see anything registering another observer in the event handler

ConciergeClient::ContainerObserver is implemented by CrostiniManager
when CrostiniManager gets OnContainerStartupFailed, it fires a callback as well
The callback is ultimately StartLxdContainerFinished which then fires OnContainerStarted (in case of failure) on its own observer
Which is implemented by CrostiniInstaller in which it fires its own callback that comes from Install method which seems to be only used in unit tests.

ConciergeClient::DiskImageObserver is implemented by CrostiniDisk and PluginVMInstaller

CrostiniDisk registers callback in OnResize which is a callback of ResizeDiskImage
ResizeDiskImage is called in OnVMRunning which is a callback for ResizeCrostiniDisk which is called by CrostiniHandler::HandleResizeCrostiniDisk and ResolveResizeCrostiniDiskCallback is the callback that ultimately gets fired
which invokes a javascript function

PluginVMInstaller registers itself in OnConceirgeAvailable
In the event handler, the PluginVMInstaller fires its own observer which is implmeented by PluginVmInstallerView which only updates the labels on UI.

I don't see any place where we're adding another observer during these callbacks which would need to receive the same event that is currently being observed therefore the change is safe.

BUG=b:164816080
TEST=deploy to dut, disable playstore, re-enable playstore, successfully finish the authorization flow
TEST=deploy to dut, turn on crostini, successfully launch terminal, turn off crostini

Change-Id: Ida55a24b04f87053bcfb7c958b63cc0ea883fa33
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2389060
Commit-Queue: Muhammad Hasan Khan <mhasank@chromium.org>
Reviewed-by: default avatarRyo Hashimoto <hashimoto@chromium.org>
Reviewed-by: default avatarYusuke Sato <yusukes@chromium.org>
Reviewed-by: default avatarHidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805382}
parent 56457d4e
......@@ -31,7 +31,7 @@ namespace chromeos {
class ConciergeClientImpl : public ConciergeClient {
public:
ConciergeClientImpl() {}
ConciergeClientImpl() = default;
~ConciergeClientImpl() override = default;
......@@ -421,10 +421,14 @@ class ConciergeClientImpl : public ConciergeClient {
dbus::ObjectProxy* concierge_proxy_ = nullptr;
base::ObserverList<Observer> observer_list_;
base::ObserverList<VmObserver>::Unchecked vm_observer_list_;
base::ObserverList<ContainerObserver>::Unchecked container_observer_list_;
base::ObserverList<DiskImageObserver>::Unchecked disk_image_observer_list_;
base::ObserverList<Observer> observer_list_{
base::ObserverListPolicy::EXISTING_ONLY};
base::ObserverList<VmObserver>::Unchecked vm_observer_list_{
base::ObserverListPolicy::EXISTING_ONLY};
base::ObserverList<ContainerObserver>::Unchecked container_observer_list_{
base::ObserverListPolicy::EXISTING_ONLY};
base::ObserverList<DiskImageObserver>::Unchecked disk_image_observer_list_{
base::ObserverListPolicy::EXISTING_ONLY};
bool is_vm_started_signal_connected_ = false;
bool is_vm_stopped_signal_connected_ = false;
......
......@@ -258,6 +258,8 @@ void ArcSessionRunner::SetRestartDelayForTesting(
restart_delay_ = restart_delay;
}
// TODO(b/164816080) add a test to ensure OnSessionStopped is not called
// when starting ARC session after failed attempt
void ArcSessionRunner::StartArcSession() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(!restart_timer_.IsRunning());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment