assistant: delay active conversation closing

This avoids potential race when conversation involves opening
a new window which immediately closes the conversation.

Bug: b/173833583
Change-Id: I56654232099c5bb7318fcd17ed26e0d3bbb0d66d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2623116
Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Jeroen Dhollander <jeroendh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842812}

chrome/browser/ui/ash/assistant/assistant_browsertest.cc

@@ -8,6 +8,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/ui/ash/assistant/assistant_test_mixin.h"
 #include "chrome/test/base/mixin_based_in_process_browser_test.h"
+#include "chromeos/assistant/test_support/expect_utils.h"
 #include "chromeos/audio/cras_audio_handler.h"
 #include "chromeos/dbus/power_manager/backlight.pb.h"
 #include "chromeos/services/assistant/public/cpp/features.h"
@@ -36,6 +37,8 @@ constexpr int kStartBrightnessPercent = 50;
 
 }  // namespace
 
+using chromeos::assistant::test::ExpectResult;
+
 class AssistantBrowserTest : public MixinBasedInProcessBrowserTest {
  public:
   AssistantBrowserTest() = default;
@@ -64,7 +67,7 @@ class AssistantBrowserTest : public MixinBasedInProcessBrowserTest {
     chromeos::PowerManagerClient::Get()->SetScreenBrightness(request);
 
     // Wait for the initial value to settle.
-    tester()->ExpectResult(
+    ExpectResult(
         true, base::BindLambdaForTesting([&]() {
           constexpr double kEpsilon = 0.1;
           auto current_brightness = tester()->SyncCall(base::BindOnce(
@@ -79,7 +82,7 @@ class AssistantBrowserTest : public MixinBasedInProcessBrowserTest {
   void ExpectBrightnessUp() {
     auto* power_manager = chromeos::PowerManagerClient::Get();
     // Check the brightness changes
-    tester()->ExpectResult(
+    ExpectResult(
         true, base::BindLambdaForTesting([&]() {
           constexpr double kEpsilon = 1;
           auto current_brightness = tester()->SyncCall(base::BindOnce(
@@ -94,7 +97,7 @@ class AssistantBrowserTest : public MixinBasedInProcessBrowserTest {
   void ExpectBrightnessDown() {
     auto* power_manager = chromeos::PowerManagerClient::Get();
     // Check the brightness changes
-    tester()->ExpectResult(
+    ExpectResult(
         true, base::BindLambdaForTesting([&]() {
           constexpr double kEpsilon = 1;
           auto current_brightness = tester()->SyncCall(base::BindOnce(
@@ -163,12 +166,12 @@ IN_PROC_BROWSER_TEST_F(AssistantBrowserTest, ShouldTurnUpVolume) {
 
   tester()->SendTextQuery("turn up volume");
 
-  tester()->ExpectResult(true, base::BindRepeating(
-                                   [](chromeos::CrasAudioHandler* cras) {
-                                     return cras->GetOutputVolumePercent() >
-                                            kStartVolumePercent;
-                                   },
-                                   cras));
+  ExpectResult(true, base::BindRepeating(
+                         [](chromeos::CrasAudioHandler* cras) {
+                           return cras->GetOutputVolumePercent() >
+                                  kStartVolumePercent;
+                         },
+                         cras));
 }
 
 IN_PROC_BROWSER_TEST_F(AssistantBrowserTest, ShouldTurnDownVolume) {
@@ -185,12 +188,12 @@ IN_PROC_BROWSER_TEST_F(AssistantBrowserTest, ShouldTurnDownVolume) {
 
   tester()->SendTextQuery("turn down volume");
 
-  tester()->ExpectResult(true, base::BindRepeating(
-                                   [](chromeos::CrasAudioHandler* cras) {
-                                     return cras->GetOutputVolumePercent() <
-                                            kStartVolumePercent;
-                                   },
-                                   cras));
+  ExpectResult(true, base::BindRepeating(
+                         [](chromeos::CrasAudioHandler* cras) {
+                           return cras->GetOutputVolumePercent() <
+                                  kStartVolumePercent;
+                         },
+                         cras));
 }
 
 IN_PROC_BROWSER_TEST_F(AssistantBrowserTest, ShouldTurnUpBrightness) {

chrome/browser/ui/ash/assistant/assistant_test_mixin.cc

@@ -260,23 +260,6 @@ class TypedExpectedResponseWaiter : public ExpectedResponseWaiter {
   const std::string class_name_;
 };
 
-template <typename T>
-void CheckResult(base::OnceClosure quit,
-                 T expected_value,
-                 base::RepeatingCallback<T()> value_callback) {
-  if (expected_value == value_callback.Run()) {
-    std::move(quit).Run();
-    return;
-  }
-
-  // Check again in the future
-  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(CheckResult<T>, std::move(quit), expected_value,
-                     value_callback),
-      base::TimeDelta::FromMilliseconds(10));
-}
-
 // Calls a callback when the view hierarchy changes.
 class CallbackViewHierarchyChangedObserver : views::ViewObserver {
  public:
@@ -454,27 +437,6 @@ void AssistantTestMixin::SendTextQuery(const std::string& query) {
   test_api_->SendTextQuery(query);
 }
 
-template <typename T>
-void AssistantTestMixin::ExpectResult(
-    T expected_value,
-    base::RepeatingCallback<T()> value_callback) {
-  const base::test::ScopedRunLoopTimeout run_timeout(FROM_HERE,
-                                                     kDefaultWaitTimeout);
-
-  // Wait until we're ready or we hit the timeout.
-  base::RunLoop run_loop;
-  CheckResult(run_loop.QuitClosure(), expected_value, value_callback);
-
-  EXPECT_NO_FATAL_FAILURE(run_loop.Run())
-      << "Failed waiting for expected result.\n"
-      << "Expected \"" << expected_value << "\"\n"
-      << "Got \"" << value_callback.Run() << "\"";
-}
-
-template void AssistantTestMixin::ExpectResult<bool>(
-    bool expected_value,
-    base::RepeatingCallback<bool()> value_callback);
-
 template <typename T>
 T AssistantTestMixin::SyncCall(
     base::OnceCallback<void(base::OnceCallback<void(T)>)> func) {

chrome/browser/ui/ash/assistant/assistant_test_mixin.h

@@ -74,17 +74,6 @@ class AssistantTestMixin : public InProcessBrowserTestMixin {
   // displaying the query input text field.
   void SendTextQuery(const std::string& query);
 
-  // Check if the |expected_value| is equal to the result of running
-  // |value_callback|.  This method will block and continuously try the
-  // comparison above until it succeeds, or timeout.
-  //
-  // NOTE: This is a template method. If you need to use it with a new type,
-  // you may see a link error. You will need to manually instantiate for the
-  // new type.  Please see .cc file for examples.
-  template <typename T>
-  void ExpectResult(T expected_value,
-                    base::RepeatingCallback<T()> value_callback);
-
   // Synchronize an async method call to make testing simpler. |func| is the
   // async method to be invoked, the inner callback is the result callback. The
   // result with type |T| will be the return value.

chrome/test/BUILD.gn

@@ -2888,6 +2888,7 @@ if (!is_android) {
         deps += [
           "//chrome/browser/ui/ash/assistant/test_support",
           "//chromeos/assistant/internal:internal",
+          "//chromeos/assistant/test_support",
         ]
 
         data += [ "//chromeos/assistant/internal/test_data/" ]

chromeos/assistant/test_support/BUILD.gn

+static_library("test_support") {
+  testonly = true
+  sources = [ "expect_utils.h" ]
+  deps = [
+    "//base",
+    "//base/test:test_support",
+    "//testing/gmock",
+    "//testing/gtest",
+  ]
+}

chromeos/assistant/test_support/expect_utils.h

+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_ASSISTANT_TEST_SUPPORT_EXPECT_UTILS_H_
+#define CHROMEOS_ASSISTANT_TEST_SUPPORT_EXPECT_UTILS_H_
+
+#include "base/callback_forward.h"
+#include "base/run_loop.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace assistant {
+namespace test {
+
+namespace {
+
+template <typename T>
+void CheckResult(base::OnceClosure quit,
+                 T expected_value,
+                 base::RepeatingCallback<T()> value_callback) {
+  if (expected_value == value_callback.Run()) {
+    std::move(quit).Run();
+    return;
+  }
+
+  // Check again in the future
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(CheckResult<T>, std::move(quit), expected_value,
+                     value_callback),
+      base::TimeDelta::FromMilliseconds(10));
+}
+
+}  // namespace
+
+// Check if the |expected_value| is equal to the result of running
+// |value_callback|.  This method will block and continuously try the
+// comparison above until it succeeds, or timeout.
+template <typename T>
+void ExpectResult(T expected_value,
+                  base::RepeatingCallback<T()> value_callback,
+                  const std::string& tag = std::string()) {
+  // Wait until we're ready or we hit the default task environment timeout.
+  base::RunLoop run_loop;
+  CheckResult(run_loop.QuitClosure(), expected_value, value_callback);
+
+  EXPECT_NO_FATAL_FAILURE(run_loop.Run())
+      << (tag.empty() ? tag : tag + ": ")
+      << "Failed waiting for expected result.\n"
+      << "Expected \"" << expected_value << "\"\n"
+      << "Got \"" << value_callback.Run() << "\"";
+}
+
+#define WAIT_FOR_CALL(mock_obj, call)                                        \
+  {                                                                          \
+    base::RunLoop run_loop;                                                  \
+    EXPECT_CALL(mock_obj, call).WillOnce([quit = run_loop.QuitClosure()]() { \
+      std::move(quit).Run();                                                 \
+    });                                                                      \
+    EXPECT_NO_FATAL_FAILURE(run_loop.Run())                                  \
+        << #mock_obj << "::" << #call << " is NOT called.";                  \
+  }
+
+}  // namespace test
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_ASSISTANT_TEST_SUPPORT_EXPECT_UTILS_H_

chromeos/services/assistant/BUILD.gn

@@ -207,6 +207,7 @@ source_set("tests") {
       "//chromeos/assistant/internal:test_support",
       "//chromeos/assistant/internal:tests",
       "//chromeos/assistant/internal/proto/google3",
+      "//chromeos/assistant/test_support",
       "//chromeos/dbus",
       "//chromeos/services/assistant/proxy",
       "//chromeos/services/assistant/public/cpp/migration",

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -451,6 +451,7 @@ void AssistantManagerServiceImpl::UpdateInternalMediaPlayerStatus(
 void AssistantManagerServiceImpl::StartVoiceInteraction() {
   DCHECK(assistant_manager());
   DVLOG(1) << __func__;
+  MaybeStopPreviousInteraction();
 
   audio_input_host_->SetMicState(true);
   assistant_manager()->StartAssistantInteraction();
@@ -465,8 +466,27 @@ void AssistantManagerServiceImpl::StopActiveInteraction(
     VLOG(1) << "Stopping interaction without assistant manager.";
     return;
   }
-  assistant_manager_internal()->StopAssistantInteractionInternal(
-      cancel_conversation);
+
+  // We do not stop the interaction immediately, but instead we give
+  // Libassistant a bit of time to stop on its own accord. This improves
+  // stability as Libassistant might misbehave when it's forcefully stopped.
+  auto stop_callback = [](base::WeakPtr<AssistantManagerServiceImpl> weak_this,
+                          bool cancel_conversation) {
+    if (!weak_this || !weak_this->assistant_manager_internal()) {
+      return;
+    }
+    VLOG(1) << "Stopping interaction.";
+    weak_this->assistant_manager_internal()->StopAssistantInteractionInternal(
+        cancel_conversation);
+  };
+
+  stop_interaction_closure_ =
+      std::make_unique<base::CancelableOnceClosure>(base::BindOnce(
+          stop_callback, weak_factory_.GetWeakPtr(), cancel_conversation));
+
+  main_task_runner()->PostDelayedTask(FROM_HERE,
+                                      stop_interaction_closure_->callback(),
+                                      stop_interactioin_delay_);
 }
 
 void AssistantManagerServiceImpl::StartEditReminderInteraction(
@@ -508,6 +528,8 @@ void AssistantManagerServiceImpl::StartTextInteraction(
     bool allow_tts) {
   DVLOG(1) << __func__;
 
+  MaybeStopPreviousInteraction();
+
   conversation_controller_proxy().SendTextQuery(
       query, allow_tts,
       NewPendingInteraction(AssistantInteractionType::kText, source, query));
@@ -569,6 +591,8 @@ void AssistantManagerServiceImpl::OnConversationTurnStartedInternal(
       &AssistantManagerServiceImpl::OnConversationTurnStartedInternal,
       metadata);
 
+  stop_interaction_closure_.reset();
+
   audio_input_host_->OnConversationTurnStarted();
 
   // Retrieve the cached interaction metadata associated with this conversation
@@ -594,6 +618,8 @@ void AssistantManagerServiceImpl::OnConversationTurnFinished(
   ENSURE_MAIN_THREAD(&AssistantManagerServiceImpl::OnConversationTurnFinished,
                      resolution);
 
+  stop_interaction_closure_.reset();
+
   // TODO(updowndota): Find a better way to handle the edge cases.
   if (resolution != Resolution::NORMAL_WITH_FOLLOW_ON &&
       resolution != Resolution::CANCELLED &&
@@ -1229,6 +1255,14 @@ void AssistantManagerServiceImpl::SendVoicelessInteraction(
       interaction, description, voiceless_options, [](auto) {});
 }
 
+void AssistantManagerServiceImpl::MaybeStopPreviousInteraction() {
+  if (!stop_interaction_closure_ || stop_interaction_closure_->IsCancelled()) {
+    return;
+  }
+
+  stop_interaction_closure_->callback().Run();
+}
+
 std::string AssistantManagerServiceImpl::GetLastSearchSource() {
   return ConsumeLastTriggerSource();
 }

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "ash/public/cpp/assistant/controller/assistant_screen_context_controller.h"
+#include "base/cancelable_callback.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/observer_list.h"
 #include "base/optional.h"
@@ -293,6 +294,8 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
                                 const std::string& description,
                                 bool is_user_initiated);
 
+  void MaybeStopPreviousInteraction();
+
   ash::AssistantAlarmTimerController* assistant_alarm_timer_controller();
   ash::AssistantNotificationController* assistant_notification_controller();
   ash::AssistantScreenContextController* assistant_screen_context_controller();
@@ -305,6 +308,9 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   ServiceControllerProxy& service_controller();
   const ServiceControllerProxy& service_controller() const;
   base::Thread& background_thread();
+  void set_stop_interaction_delay_for_testing(base::TimeDelta delay) {
+    stop_interactioin_delay_ = delay;
+  }
 
   void SetStateAndInformObservers(State new_state);
 
@@ -357,6 +363,10 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   // Configuration passed to libassistant.
   std::string libassistant_config_;
 
+  base::TimeDelta stop_interactioin_delay_ =
+      base::TimeDelta::FromMilliseconds(500);
+  std::unique_ptr<base::CancelableOnceClosure> stop_interaction_closure_;
+
   base::ScopedObservation<DeviceActions,
                           AppListEventSubscriber,
                           &DeviceActions::AddAndFireAppListEventSubscriber,