Fix tast test constant failure due to chrome crash.

Part of our Assistant integration tests keep failing because of a crash
happened during Assistant response processing.

Previously it was possible that the |OnFinishedProcessing| callback gets
fired after the response and its processor has been destroyed. To fix
this, we use the WeakPtr in the callback because a specified destruction
order is needed to indicate the failure of completion when the response
processing can possibly get interrupted under some cases.

Misc: fix linter warnings.

Bug: b/148677894
Test: local compile and run tast tests.
Change-Id: I87049a1d324a78c8bdc8b1487ea8dd5ce289b7de
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2040734
Commit-Queue: Meilin Wang <meilinw@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742419}

ash/assistant/assistant_interaction_controller.cc

@@ -732,8 +732,13 @@ void AssistantInteractionController::OnProcessPendingResponse() {
       weak_factory_.GetWeakPtr()));
 }
 
-void AssistantInteractionController::OnPendingResponseProcessed(bool success) {
-  if (!success)
+void AssistantInteractionController::OnPendingResponseProcessed(
+    bool is_completed) {
+  // If the response processing has been interrupted and not completed, we will
+  // ignore it and don't flush to the UI. This can happen if two queries were
+  // sent close enough, and the interaction started by the second query arrived
+  // before the first query's response even finished processing.
+  if (!is_completed)
     return;
 
   // Once the pending response has been processed it is safe to flush to the UI.

ash/assistant/assistant_interaction_controller.h

@@ -132,7 +132,7 @@ class AssistantInteractionController
   bool HasActiveInteraction() const;
 
   void OnProcessPendingResponse();
-  void OnPendingResponseProcessed(bool success);
+  void OnPendingResponseProcessed(bool is_completed);
 
   void OnUiVisible(AssistantEntryPoint entry_point);
   bool ShouldAttemptWarmerWelcome(AssistantEntryPoint entry_point) const;

ash/assistant/model/assistant_response.cc

@@ -4,8 +4,11 @@
 
 #include "ash/assistant/model/assistant_response.h"
 
+#include <utility>
+
 #include "ash/assistant/model/ui/assistant_ui_element.h"
 #include "base/bind.h"
+#include "base/memory/weak_ptr.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
 
 namespace ash {
@@ -14,7 +17,7 @@ namespace ash {
 
 class AssistantResponse::Processor {
  public:
-  Processor(AssistantResponse& response, ProcessingCallback callback)
+  Processor(AssistantResponse* response, ProcessingCallback callback)
       : response_(response), callback_(std::move(callback)) {}
 
   Processor(const Processor& copy) = delete;
@@ -22,36 +25,41 @@ class AssistantResponse::Processor {
 
   ~Processor() {
     if (callback_)
-      std::move(callback_).Run(/*success=*/false);
+      std::move(callback_).Run(/*is_completed=*/false);
   }
 
   void Process() {
     // Responses should only be processed once.
-    DCHECK_EQ(ProcessingState::kUnprocessed, response_.processing_state());
-    response_.set_processing_state(ProcessingState::kProcessing);
+    DCHECK_EQ(ProcessingState::kUnprocessed, response_->processing_state());
+    response_->set_processing_state(ProcessingState::kProcessing);
 
     // Completion of |response_| processing is indicated by |processing_count_|
     // reaching zero. This value is decremented as each UI element is processed.
-    processing_count_ = response_.GetUiElements().size();
+    processing_count_ = response_->GetUiElements().size();
+
+    // Try finishing directly if there are no UI elements to be processed.
+    if (processing_count_ == 0) {
+      TryFinishing();
+      return;
+    }
 
-    for (const auto& ui_element : response_.GetUiElements()) {
+    for (const auto& ui_element : response_->GetUiElements()) {
       // Start asynchronous processing of the UI element. Note that if the UI
       // element does not require any pre-rendering processing the callback may
-      // be run synchronously.
+      // be run synchronously. Also we must use WeakPtr here because |this| will
+      // destroy before |ui_element| by design.
       ui_element->Process(
           base::BindOnce(&AssistantResponse::Processor::OnFinishedProcessing,
-                         base::Unretained(this)));
+                         weak_ptr_factory_.GetWeakPtr()));
     }
-
-    // If any elements are processing asynchronously this will no-op.
-    TryFinishing();
   }
 
  private:
-  void OnFinishedProcessing(bool success) {
-    // We handle success/failure cases the same because failures will skipped in
-    // view handling. We decrement our |processing_count_| and attempt to finish
-    // response processing. This will no-op if elements are still processing.
+  void OnFinishedProcessing() {
+    // We handle success/failure cases the same because failures will be skipped
+    // in view handling. We decrement our |processing_count_| and attempt to
+    // finish response processing. This will no-op if elements are still
+    // processing.
     --processing_count_;
     TryFinishing();
   }
@@ -61,22 +69,30 @@ class AssistantResponse::Processor {
     if (!callback_ || processing_count_ > 0)
       return;
 
-    // Notify processing success.
-    response_.set_processing_state(ProcessingState::kProcessed);
-    std::move(callback_).Run(/*success=*/true);
+    // Notify processing completion.
+    response_->set_processing_state(ProcessingState::kProcessed);
+    std::move(callback_).Run(/*is_completed=*/true);
   }
 
-  AssistantResponse& response_;
+  // |response_| should outlive the Processor.
+  AssistantResponse* const response_;
   ProcessingCallback callback_;
 
   int processing_count_ = 0;
+  base::WeakPtrFactory<AssistantResponse::Processor> weak_ptr_factory_{this};
 };
 
 // AssistantResponse -----------------------------------------------------------
 
 AssistantResponse::AssistantResponse() = default;
 
-AssistantResponse::~AssistantResponse() = default;
+AssistantResponse::~AssistantResponse() {
+  // Reset |processor_| explicitly in the destructor to guarantee the correct
+  // lifecycle where |this| should outlive the |processor_|. This can also force
+  // |processor_| to be destroyed before |ui_elements_| as we want regardless of
+  // the declaration order.
+  processor_.reset();
+}
 
 void AssistantResponse::AddUiElement(
     std::unique_ptr<AssistantUiElement> ui_element) {
@@ -117,7 +133,7 @@ AssistantResponse::GetSuggestions() const {
 }
 
 void AssistantResponse::Process(ProcessingCallback callback) {
-  processor_ = std::make_unique<Processor>(*this, std::move(callback));
+  processor_ = std::make_unique<Processor>(this, std::move(callback));
   processor_->Process();
 }
 

ash/assistant/model/assistant_response.h

@@ -67,8 +67,9 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantResponse
   bool has_tts() const { return has_tts_; }
   void set_has_tts(bool has_tts) { has_tts_ = has_tts; }
 
-  // Invoke to begin processing the response. Upon completion, |callback| will
-  // be run to indicate success or failure.
+  // Invoke to begin processing the response. The specified |callback| will be
+  // run to indicate whether or not the processor has completed processing of
+  // all UI elements in the response.
   void Process(ProcessingCallback callback);
 
  private:
@@ -77,11 +78,16 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantResponse
   friend class base::RefCounted<AssistantResponse>;
   ~AssistantResponse();
 
-  std::vector<std::unique_ptr<AssistantUiElement>> ui_elements_;
   std::vector<AssistantSuggestionPtr> suggestions_;
   ProcessingState processing_state_ = ProcessingState::kUnprocessed;
   bool has_tts_ = false;
 
+  // We specify the declaration order below as intended because we want
+  // |processor_| to be destroyed before |ui_elements_| (we also forced this
+  // order in the destructor), so that when the response processing got
+  // interrupted, the |ProcessingCallback| can have a chance to return false
+  // during the destruction to indicate the failure of completion.
+  std::vector<std::unique_ptr<AssistantUiElement>> ui_elements_;
   std::unique_ptr<Processor> processor_;
 
   DISALLOW_COPY_AND_ASSIGN(AssistantResponse);

ash/assistant/model/ui/assistant_card_element.cc

@@ -4,6 +4,8 @@
 
 #include "ash/assistant/model/ui/assistant_card_element.h"
 
+#include <utility>
+
 #include "ash/assistant/ui/assistant_ui_constants.h"
 #include "ash/public/cpp/assistant/assistant_web_view_factory.h"
 #include "base/base64.h"
@@ -14,7 +16,7 @@ namespace ash {
 
 class AssistantCardElement::Processor : public AssistantWebView::Observer {
  public:
-  Processor(AssistantCardElement& card_element, ProcessingCallback callback)
+  Processor(AssistantCardElement* card_element, ProcessingCallback callback)
       : card_element_(card_element), callback_(std::move(callback)) {}
 
   Processor(const Processor& copy) = delete;
@@ -25,7 +27,7 @@ class AssistantCardElement::Processor : public AssistantWebView::Observer {
       contents_view_->RemoveObserver(this);
 
     if (callback_)
-      std::move(callback_).Run(/*success=*/false);
+      std::move(callback_).Run();
   }
 
   void Process() {
@@ -51,7 +53,7 @@ class AssistantCardElement::Processor : public AssistantWebView::Observer {
 
     // Encode the html string to be URL-safe.
     std::string encoded_html;
-    base::Base64Encode(card_element_.html(), &encoded_html);
+    base::Base64Encode(card_element_->html(), &encoded_html);
 
     // Navigate to the data URL which represents the card.
     constexpr char kDataUriPrefix[] = "data:text/html;base64,";
@@ -64,12 +66,13 @@ class AssistantCardElement::Processor : public AssistantWebView::Observer {
     contents_view_->RemoveObserver(this);
 
     // Pass ownership of |contents_view_| to the card element that was being
-    // processed and notify our |callback_| of success.
-    card_element_.set_contents_view(std::move(contents_view_));
-    std::move(callback_).Run(/*success=*/true);
+    // processed and notify our |callback_| of the completion.
+    card_element_->set_contents_view(std::move(contents_view_));
+    std::move(callback_).Run();
   }
 
-  AssistantCardElement& card_element_;
+  // |card_element_| should outlive the Processor.
+  AssistantCardElement* const card_element_;
   ProcessingCallback callback_;
 
   std::unique_ptr<AssistantWebView> contents_view_;
@@ -83,10 +86,13 @@ AssistantCardElement::AssistantCardElement(const std::string& html,
       html_(html),
       fallback_(fallback) {}
 
-AssistantCardElement::~AssistantCardElement() = default;
+AssistantCardElement::~AssistantCardElement() {
+  // |processor_| should be destroyed before |this| has been deleted.
+  processor_.reset();
+}
 
 void AssistantCardElement::Process(ProcessingCallback callback) {
-  processor_ = std::make_unique<Processor>(*this, std::move(callback));
+  processor_ = std::make_unique<Processor>(this, std::move(callback));
   processor_->Process();
 }
 

ash/assistant/model/ui/assistant_ui_element.cc

@@ -4,6 +4,8 @@
 
 #include "ash/assistant/model/ui/assistant_ui_element.h"
 
+#include <utility>
+
 namespace ash {
 
 AssistantUiElement::AssistantUiElement(AssistantUiElementType type)
@@ -13,7 +15,7 @@ AssistantUiElement::~AssistantUiElement() = default;
 
 void AssistantUiElement::Process(ProcessingCallback callback) {
   // By default, Assistant UI elements do not require pre-rendering processing.
-  std::move(callback).Run(/*success=*/true);
+  std::move(callback).Run();
 }
 
 }  // namespace ash

ash/assistant/model/ui/assistant_ui_element.h

@@ -28,9 +28,12 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantUiElement {
 
   AssistantUiElementType type() const { return type_; }
 
-  // Invoke to being processing the UI element for rendering. Upon completion,
-  // the specified |callback| will be run to indicate success or failure.
-  using ProcessingCallback = base::OnceCallback<void(bool)>;
+  // Invoke to being processing the UI element for rendering. The specified
+  // |callback| will be run upon completion. Note that we don't include the
+  // processing result in the callback, as in |AssistantResponse| we handle
+  // success/failure cases the same because failures will be skipped in view
+  // handling.
+  using ProcessingCallback = base::OnceCallback<void()>;
   virtual void Process(ProcessingCallback callback);
 
  protected: