Wait DeviceApps config sync with enableAssistantAndWaitForReady
- There is a short period where assistant status is READY but assistant
hasn't received DeviceApps config bit. If assistant does not have the
bit, assistant does not send Android application list to the server
and query for opening an Android app fails.
- This CL modifies the test API as it waits the config sync.
Bug: b:228644597
Test: tast run DUT assistant.OpenAndroidApp
Change-Id: I94fc027d67b13402787d37554cc16903d6e9396e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3592257
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Eric Sum <esum@google.com>
Reviewed-by: Tao Wu <wutao@chromium.org>
Commit-Queue: Yuki Awano <yawano@google.com>
Cr-Commit-Position: refs/heads/main@{#996910}
diff --git a/chrome/browser/chromeos/BUILD.gn b/chrome/browser/chromeos/BUILD.gn
index 3b5df61e..978c8df 100644
--- a/chrome/browser/chromeos/BUILD.gn
+++ b/chrome/browser/chromeos/BUILD.gn
@@ -336,6 +336,7 @@
"//chromeos/network",
"//chromeos/printing",
"//chromeos/scanning",
+ "//chromeos/services/assistant:lib",
"//chromeos/services/assistant/public/cpp",
"//chromeos/services/bluetooth_config:in_process_bluetooth_config",
"//chromeos/services/cros_healthd/public/cpp",
diff --git a/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc b/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc
index 3c1034a..3d49e2af 100644
--- a/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc
+++ b/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc
@@ -139,6 +139,7 @@
#include "chromeos/dbus/session_manager/session_manager_client.h"
#include "chromeos/metrics/login_event_recorder.h"
#include "chromeos/printing/printer_configuration.h"
+#include "chromeos/services/assistant/assistant_manager_service_impl.h"
#include "chromeos/services/assistant/public/cpp/assistant_prefs.h"
#include "chromeos/services/assistant/public/cpp/assistant_service.h"
#include "chromeos/services/machine_learning/public/cpp/service_connection.h"
@@ -3073,14 +3074,26 @@
AutotestPrivateEnableAssistantAndWaitForReadyFunction() = default;
AutotestPrivateEnableAssistantAndWaitForReadyFunction::
- ~AutotestPrivateEnableAssistantAndWaitForReadyFunction() {
- ash::AssistantState::Get()->RemoveObserver(this);
-}
+ ~AutotestPrivateEnableAssistantAndWaitForReadyFunction() = default;
ExtensionFunction::ResponseAction
AutotestPrivateEnableAssistantAndWaitForReadyFunction::Run() {
DVLOG(1) << "AutotestPrivateEnableAssistantAndWaitForReadyFunction";
+ if (ash::AssistantState::Get()->assistant_status() ==
+ chromeos::assistant::AssistantStatus::READY) {
+ return RespondNow(Error("Assistant is already enabled."));
+ }
+
+ // We can set this callback only when assistant status is NOT_READY. We should
+ // call this before we try to enable Assistant to avoid causing some timing
+ // issue.
+ chromeos::assistant::AssistantManagerServiceImpl::
+ SetInitializedInternalCallbackForTesting(base::BindOnce(
+ &AutotestPrivateEnableAssistantAndWaitForReadyFunction::
+ OnInitializedInternal,
+ this));
+
Profile* profile = Profile::FromBrowserContext(browser_context());
const std::string& err_msg =
SetWhitelistedPref(profile, chromeos::assistant::prefs::kAssistantEnabled,
@@ -3088,34 +3101,12 @@
if (!err_msg.empty())
return RespondNow(Error(err_msg));
- // Asynchronously subscribe to status changes to avoid a possible segmentation
- // fault caused by Respond() in the subscriber callback being called before
- // RespondLater() below.
- base::SequencedTaskRunnerHandle::Get()->PostTask(
- FROM_HERE,
- base::BindOnce(&AutotestPrivateEnableAssistantAndWaitForReadyFunction::
- SubscribeToStatusChanges,
- this));
-
- // Prevent |this| from being freed before we get a response from the
- // Assistant.
- self_ = this;
-
return RespondLater();
}
void AutotestPrivateEnableAssistantAndWaitForReadyFunction::
- SubscribeToStatusChanges() {
- // |AddObserver| will immediately trigger |OnAssistantStatusChanged|.
- ash::AssistantState::Get()->AddObserver(this);
-}
-
-void AutotestPrivateEnableAssistantAndWaitForReadyFunction::
- OnAssistantStatusChanged(chromeos::assistant::AssistantStatus status) {
- if (status == chromeos::assistant::AssistantStatus::READY) {
- Respond(NoArguments());
- self_.reset();
- }
+ OnInitializedInternal() {
+ Respond(NoArguments());
}
// AssistantInteractionHelper is a helper class used to interact with Assistant
diff --git a/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.h b/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.h
index 9eaccce..1659cf4 100644
--- a/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.h
+++ b/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.h
@@ -738,8 +738,7 @@
// Bring up the Assistant service, and wait until the ready signal is received.
class AutotestPrivateEnableAssistantAndWaitForReadyFunction
- : public ExtensionFunction,
- public ash::AssistantStateObserver {
+ : public ExtensionFunction {
public:
AutotestPrivateEnableAssistantAndWaitForReadyFunction();
DECLARE_EXTENSION_FUNCTION("autotestPrivate.enableAssistantAndWaitForReady",
@@ -749,15 +748,7 @@
~AutotestPrivateEnableAssistantAndWaitForReadyFunction() override;
ResponseAction Run() override;
- void SubscribeToStatusChanges();
-
- // ash::AssistantStateObserver overrides:
- void OnAssistantStatusChanged(
- chromeos::assistant::AssistantStatus status) override;
-
- // A reference to keep |this| alive while waiting for the Assistant to
- // respond.
- scoped_refptr<ExtensionFunction> self_;
+ void OnInitializedInternal();
};
// Send text query to Assistant and return response.
diff --git a/chromeos/services/assistant/assistant_manager_service_impl.cc b/chromeos/services/assistant/assistant_manager_service_impl.cc
index 86109bb..d53b9a1 100644
--- a/chromeos/services/assistant/assistant_manager_service_impl.cc
+++ b/chromeos/services/assistant/assistant_manager_service_impl.cc
@@ -10,6 +10,7 @@
#include "ash/constants/ash_features.h"
#include "ash/constants/ash_switches.h"
+#include "ash/public/cpp/assistant/assistant_state.h"
#include "ash/public/cpp/assistant/assistant_state_base.h"
#include "ash/public/cpp/assistant/controller/assistant_notification_controller.h"
#include "base/barrier_closure.h"
@@ -54,6 +55,7 @@
namespace assistant {
namespace {
+static base::OnceCallback<void()> initialized_internal_callback_for_testing;
static bool is_first_init = true;
constexpr char kAndroidSettingsAppPackage[] = "com.android.settings";
@@ -145,6 +147,18 @@
};
// static
+void AssistantManagerServiceImpl::SetInitializedInternalCallbackForTesting(
+ base::OnceCallback<void()> callback) {
+ CHECK(initialized_internal_callback_for_testing.is_null());
+ // We expect that the callback is set when AssistantStatus is NOT_READY to
+ // confirm that AssistantStatus has changed from NOT_READY to READY. See more
+ // details at a comment in AssistantManagerServiceImpl::OnDeviceAppsEnabled.
+ CHECK(ash::AssistantState::Get()->assistant_status() ==
+ chromeos::assistant::AssistantStatus::NOT_READY);
+ initialized_internal_callback_for_testing = std::move(callback);
+}
+
+// static
void AssistantManagerServiceImpl::ResetIsFirstInitFlagForTesting() {
is_first_init = true;
}
@@ -555,6 +569,25 @@
return;
display_controller().SetDeviceAppsEnabled(enabled);
+
+ // You can set initialized_internal callback only when AssistantStatus is
+ // NOT_READY. Also this line reaches only after GetState() becomes RUNNING
+ // (i.e. READY). From that reason, test code can assume that status has
+ // changed from NOT_READY to READY between those two points.
+ //
+ // Test code expects those things when Assistant gets initialized:
+ //
+ // - Status becomes READY.
+ // - All necessary settings are passed to LibAssistant.
+ //
+ // We update necessary settings after status becomes READY. For now,
+ // DeviceAppsEnabled is the only settings update which involves async call.
+ // As other settings are sync, if this async call gets completed, we can also
+ // assume that all necessary settings are passed to LibAssistant, i.e.
+ // initialized.
+ if (!initialized_internal_callback_for_testing.is_null()) {
+ std::move(initialized_internal_callback_for_testing).Run();
+ }
}
void AssistantManagerServiceImpl::AddTimeToTimer(const std::string& id,
diff --git a/chromeos/services/assistant/assistant_manager_service_impl.h b/chromeos/services/assistant/assistant_manager_service_impl.h
index bd08fae5..1be0c1f 100644
--- a/chromeos/services/assistant/assistant_manager_service_impl.h
+++ b/chromeos/services/assistant/assistant_manager_service_impl.h
@@ -95,6 +95,10 @@
private chromeos::libassistant::mojom::StateObserver,
public ConversationObserver {
public:
+ // |callback| is called when AssistantManagerServiceImpl got initialized
+ // internally. This waits DeviceApps config value sync.
+ static void SetInitializedInternalCallbackForTesting(
+ base::OnceCallback<void()> callback);
static void ResetIsFirstInitFlagForTesting();
// |service| owns this class and must outlive this class.
