Skip to content

GitLab

T
tangled
Commit 6848457e authored by David Black's avatar David Black Committed by Commit Bot
Browse files
Options

Fix crash in Assistant response processing. 

The chain of ownership between AssistantResponseProcessor and UI
elements that were being processed was overly complicated. Previously,
it was possible for callbacks to fire when the underlying UI element
had already been destroyed.

Now, we refactor processing logic so that processors are bound to the
response/element that they are processing. As such, lifecycle
complications are drastically simplified.

Bug: b:119674543
Change-Id: I04a08f29ffaf0178d409c7bc0e5d84292993d1ae
Reviewed-on: https://chromium-review.googlesource.com/c/1350961
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: default avatarXiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611997}
parent c3e66455
Hide whitespace changes
Inline Side-by-side
Showing with 219 additions and 331 deletions
ash/BUILD.gn
View file @ 6848457e
......@@ -168,8 +168,6 @@ component("ash") {
"assistant/assistant_interaction_controller.h",
"assistant/assistant_notification_controller.cc",
"assistant/assistant_notification_controller.h",
"assistant/assistant_response_processor.cc",
"assistant/assistant_response_processor.h",
"assistant/assistant_screen_context_controller.cc",
"assistant/assistant_screen_context_controller.h",
"assistant/assistant_setup_controller.cc",
......
ash/assistant/assistant_interaction_controller.cc
View file @ 6848457e
......@@ -47,7 +47,6 @@ AssistantInteractionController::AssistantInteractionController(
AssistantController* assistant_controller)
: assistant_controller_(assistant_controller),
assistant_interaction_subscriber_binding_(this),
assistant_response_processor_(assistant_controller),
weak_factory_(this) {
AddModelObserver(this);
assistant_controller_->AddObserver(this);
......@@ -543,9 +542,15 @@ void AssistantInteractionController::OnProcessPendingResponse() {
return;
}
// Bind an interface to a navigable contents factory that is needed for
// processing card elements.
content::mojom::NavigableContentsFactoryPtr contents_factory;
assistant_controller_->GetNavigableContentsFactory(
mojo::MakeRequest(&contents_factory));
// Start processing.
assistant_response_processor_.Process(
*model_.pending_response(),
model_.pending_response()->Process(
std::move(contents_factory),
base::BindOnce(
&AssistantInteractionController::OnPendingResponseProcessed,
weak_factory_.GetWeakPtr()));
......
ash/assistant/assistant_interaction_controller.h
View file @ 6848457e
......@@ -11,7 +11,6 @@
#include <vector>
#include "ash/assistant/assistant_controller_observer.h"
#include "ash/assistant/assistant_response_processor.h"
#include "ash/assistant/model/assistant_interaction_model.h"
#include "ash/assistant/model/assistant_interaction_model_observer.h"
#include "ash/assistant/model/assistant_ui_model_observer.h"
......@@ -26,7 +25,6 @@ namespace ash {
class AssistantController;
class AssistantInteractionModelObserver;
class AssistantResponseProcessor;
class AssistantInteractionController
: public chromeos::assistant::mojom::AssistantInteractionSubscriber,
......@@ -131,8 +129,6 @@ class AssistantInteractionController
mojo::Binding<chromeos::assistant::mojom::AssistantInteractionSubscriber>
assistant_interaction_subscriber_binding_;
AssistantResponseProcessor assistant_response_processor_;
AssistantInteractionModel model_;
base::WeakPtrFactory<AssistantInteractionController> weak_factory_;
......
ash/assistant/assistant_response_processor.cc deleted 100644 → 0
View file @ c3e66455
// Copyright 2018 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.
#include "ash/assistant/assistant_response_processor.h"
#include <algorithm>
#include "ash/assistant/assistant_controller.h"
#include "ash/assistant/model/assistant_response.h"
#include "ash/assistant/model/assistant_ui_element.h"
#include "ash/assistant/ui/assistant_ui_constants.h"
#include "base/base64.h"
#include "base/stl_util.h"
namespace ash {
namespace {
// WebContents.
constexpr char kDataUriPrefix[] = "data:text/html;base64,";
} // namespace
// AssistantCardProcessor ------------------------------------------------------
AssistantCardProcessor::AssistantCardProcessor(
AssistantController* assistant_controller,
AssistantResponseProcessor* assistant_response_processor,
AssistantCardElement* assistant_card_element)
: assistant_controller_(assistant_controller),
assistant_response_processor_(assistant_response_processor),
assistant_card_element_(assistant_card_element) {}
AssistantCardProcessor::~AssistantCardProcessor() {
if (contents_)
contents_->RemoveObserver(this);
}
void AssistantCardProcessor::DidStopLoading() {
contents_->RemoveObserver(this);
// Transfer ownership of |contents_| to the card element and notify the
// response processor that we've finished processing.
assistant_card_element_->set_contents(std::move(contents_));
assistant_response_processor_->DidFinishProcessing(this);
}
void AssistantCardProcessor::Process() {
assistant_controller_->GetNavigableContentsFactory(
mojo::MakeRequest(&contents_factory_));
// TODO(dmblack): Find a better way of determining desired card size.
const int width_dip = kPreferredWidthDip - 2 * kUiElementHorizontalMarginDip;
// Configure parameters for the card.
auto contents_params = content::mojom::NavigableContentsParams::New();
contents_params->enable_view_auto_resize = true;
contents_params->auto_resize_min_size = gfx::Size(width_dip, 1);
contents_params->auto_resize_max_size = gfx::Size(width_dip, INT_MAX);
contents_params->suppress_navigations = true;
contents_ = std::make_unique<content::NavigableContents>(
contents_factory_.get(), std::move(contents_params));
// Observe |contents_| so that we are notified when loading is complete.
contents_->AddObserver(this);
// Navigate to the data URL which represents the card.
std::string encoded_html;
base::Base64Encode(assistant_card_element_->html(), &encoded_html);
contents_->Navigate(GURL(kDataUriPrefix + encoded_html));
}
// AssistantResponseProcessor::Task --------------------------------------------
AssistantResponseProcessor::Task::Task(AssistantResponse& response,
ProcessCallback callback)
: response(response.GetWeakPtr()), callback(std::move(callback)) {}
AssistantResponseProcessor::Task::~Task() = default;
// AssistantResponseProcessor --------------------------------------------------
AssistantResponseProcessor::AssistantResponseProcessor(
AssistantController* assistant_controller)
: assistant_controller_(assistant_controller), weak_factory_(this) {}
AssistantResponseProcessor::~AssistantResponseProcessor() = default;
void AssistantResponseProcessor::Process(AssistantResponse& response,
ProcessCallback callback) {
// We should only attempt to process responses that are unprocessed.
DCHECK_EQ(AssistantResponse::ProcessingState::kUnprocessed,
response.processing_state());
// Update processing state.
response.set_processing_state(
AssistantResponse::ProcessingState::kProcessing);
// We only support processing a single task at a time. As such, we should
// abort any task in progress before creating and starting a new one.
TryAbortingTask();
// Create a task.
task_.emplace(/*response=*/response,
/*callback=*/std::move(callback));
// Start processing UI elements.
for (const auto& ui_element : response.GetUiElements()) {
switch (ui_element->GetType()) {
case AssistantUiElementType::kCard:
// Create and start an element processor to process the card element.
task_->element_processors.push_back(
std::make_unique<AssistantCardProcessor>(
assistant_controller_, this,
static_cast<AssistantCardElement*>(ui_element.get())));
task_->element_processors.back()->Process();
break;
case AssistantUiElementType::kText:
// No processing necessary.
break;
}
}
// Try finishing. This will no-op if there are still UI elements being
// processed asynchronously.
TryFinishingTask();
}
void AssistantResponseProcessor::DidFinishProcessing(
const AssistantCardProcessor* card_processor) {
// If the response has been invalidated we should abort early.
if (!task_->response) {
TryAbortingTask();
return;
}
// Remove the finished element processor to indicate its completion.
base::EraseIf(task_->element_processors,
[card_processor](
const std::unique_ptr<AssistantCardProcessor>& candidate) {
return candidate.get() == card_processor;
});
// Try finishing. This will no-op if there are still UI elements being
// processed asynchronously.
TryFinishingTask();
}
void AssistantResponseProcessor::TryAbortingTask() {
if (!task_)
return;
// Invalidate weak pointers to prevent processing callbacks from running.
// Otherwise we might continue receiving card events for the aborted task.
weak_factory_.InvalidateWeakPtrs();
// Notify our callback and clean up any task related resources.
std::move(task_->callback).Run(/*success=*/false);
task_.reset();
}
void AssistantResponseProcessor::TryFinishingTask() {
// This method is a no-op if we are still processing.
if (!task_->element_processors.empty())
return;
// Update processing state.
task_->response->set_processing_state(
AssistantResponse::ProcessingState::kProcessed);
// Notify our callback and clean up any task related resources.
std::move(task_->callback).Run(/*success=*/true);
task_.reset();
}
} // namespace ash
ash/assistant/assistant_response_processor.h deleted 100644 → 0
View file @ c3e66455
// Copyright 2018 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 ASH_ASSISTANT_ASSISTANT_RESPONSE_PROCESSOR_H_
#define ASH_ASSISTANT_ASSISTANT_RESPONSE_PROCESSOR_H_
#include <memory>
#include <string>
#include <vector>
#include "base/callback.h"
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "base/unguessable_token.h"
#include "services/content/public/cpp/navigable_contents.h"
#include "ui/gfx/geometry/size.h"
namespace ash {
class AssistantController;
class AssistantCardElement;
class AssistantResponse;
class AssistantResponseProcessor;
enum class AssistantUiElementType;
// AssistantCardProcessor ------------------------------------------------------
// A UI element processor associated with a single card element that is
// responsible for processing an Assistant card on behalf of an
// AssistantResponseProcessor.
class AssistantCardProcessor : public content::NavigableContentsObserver {
public:
AssistantCardProcessor(
AssistantController* assistant_controller,
AssistantResponseProcessor* assistant_response_processor,
AssistantCardElement* assistant_card_element);
~AssistantCardProcessor() override;
// content::NavigableContentsObserver:
void DidStopLoading() override;
// Starts processing the associated card element. Upon completion, this
// processor will call DidFinishProcessing on |assistant_response_processor_|.
void Process();
private:
AssistantController* const assistant_controller_;
AssistantResponseProcessor* const assistant_response_processor_;
AssistantCardElement* const assistant_card_element_;
content::mojom::NavigableContentsFactoryPtr contents_factory_;
std::unique_ptr<content::NavigableContents> contents_;
DISALLOW_COPY_AND_ASSIGN(AssistantCardProcessor);
};
// AssistantResponseProcessor --------------------------------------------------
// The AssistantResponseProcessor is responsible for performing any processing
// steps necessary on an Assistant response before it is ready for presentation.
class AssistantResponseProcessor {
public:
using ProcessCallback = base::OnceCallback<void(bool)>;
explicit AssistantResponseProcessor(
AssistantController* assistant_controller);
~AssistantResponseProcessor();
// Performs processing of the specified Assistant |response|. Upon completion
// of processing, |callback| is run indicating success or failure. Note that
// only one Assistant response may be processed at a time. Calling this method
// while another response is being processed will abort the previous task.
void Process(AssistantResponse& response, ProcessCallback callback);
// Invoked when the specified |card_processor| has finished processing.
void DidFinishProcessing(const AssistantCardProcessor* card_processor);
private:
// Encapsulates a processing task for a given Assistant response. Upon task
// abort/completion, the associated callback should be run.
struct Task {
public:
Task(AssistantResponse& response, ProcessCallback callback);
~Task();
// Weak pointer to the response being processed.
base::WeakPtr<AssistantResponse> response;
// Callback to be run on task abort/completion.
ProcessCallback callback;
// Vector of element processors that are processing the UI elements
// contained in |response|. When |element_processors| is empty, response
// processing is complete.
std::vector<std::unique_ptr<AssistantCardProcessor>> element_processors;
};
// Processes a card element as a part of the task identified by |task_id|.
void ProcessCardElement(AssistantCardElement* card_element);
// Invoked when a card element has completed processing. The event is
// associated with the task identified by |task_id| for the specified
// |card_element|.
void OnCardElementProcessed(
AssistantCardElement* card_element,
const base::Optional<base::UnguessableToken>& embed_token);
// Checks if the |task_| exists. If so, processing is aborted, the callback
// associated with the task is run and the task is cleaned up. Otherwise this
// is a no-op.
void TryAbortingTask();
// Checks if the |task_| is finished processing. If so, the callback
// associated with the task is run and the task is cleaned up. Otherwise this
// is a no-op.
void TryFinishingTask();
AssistantController* const assistant_controller_; // Owned by Shell.
base::Optional<Task> task_;
base::WeakPtrFactory<AssistantResponseProcessor> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(AssistantResponseProcessor);
};
} // namespace ash
#endif // ASH_ASSISTANT_ASSISTANT_RESPONSE_PROCESSOR_H_
ash/assistant/model/assistant_response.cc
View file @ 6848457e
......@@ -8,7 +8,9 @@
namespace ash {
AssistantResponse::AssistantResponse() : weak_factory_(this) {}
// AssistantResponse -----------------------------------------------------------
AssistantResponse::AssistantResponse() = default;
AssistantResponse::~AssistantResponse() = default;
......@@ -50,8 +52,71 @@ AssistantResponse::GetSuggestions() const {
return suggestions;
}
base::WeakPtr<AssistantResponse> AssistantResponse::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
void AssistantResponse::Process(
content::mojom::NavigableContentsFactoryPtr contents_factory,
ProcessingCallback callback) {
processor_ = std::make_unique<Processor>(*this, std::move(contents_factory),
std::move(callback));
processor_->Process();
}
// AssistantResponse::Processor ------------------------------------------------
AssistantResponse::Processor::Processor(
AssistantResponse& response,
content::mojom::NavigableContentsFactoryPtr contents_factory,
ProcessingCallback callback)
: response_(response),
contents_factory_(std::move(contents_factory)),
callback_(std::move(callback)) {}
AssistantResponse::Processor::~Processor() {
if (callback_)
std::move(callback_).Run(/*success=*/false);
}
void AssistantResponse::Processor::Process() {
// Responses should only be processed once.
DCHECK_EQ(ProcessingState::kUnprocessed, response_.processing_state());
response_.set_processing_state(ProcessingState::kProcessing);
for (const auto& ui_element : response_.GetUiElements()) {
switch (ui_element->GetType()) {
case AssistantUiElementType::kCard:
++processing_count_;
// Start asynchronous processing of the card element.
static_cast<AssistantCardElement*>(ui_element.get())
->Process(contents_factory_.get(),
base::BindOnce(
&AssistantResponse::Processor::OnFinishedProcessing,
base::Unretained(this)));
break;
case AssistantUiElementType::kText:
// No processing necessary.
break;
}
}
// If any elements are processing asynchronously this will no-op.
TryFinishing();
}
void AssistantResponse::Processor::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.
--processing_count_;
TryFinishing();
}
void AssistantResponse::Processor::TryFinishing() {
// No-op if we are already finished or if elements are still processing.
if (!callback_ || processing_count_ > 0)
return;
// Notify processing success.
response_.set_processing_state(ProcessingState::kProcessed);
std::move(callback_).Run(/*success=*/true);
}
} // namespace ash
\ No newline at end of file
ash/assistant/model/assistant_response.h
View file @ 6848457e
......@@ -10,8 +10,8 @@
#include <vector>
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
#include "services/content/public/cpp/navigable_contents.h"
namespace ash {
......@@ -20,16 +20,18 @@ class AssistantUiElement;
// Models a renderable Assistant response.
class AssistantResponse {
public:
using AssistantSuggestion = chromeos::assistant::mojom::AssistantSuggestion;
using AssistantSuggestionPtr =
chromeos::assistant::mojom::AssistantSuggestionPtr;
using ProcessingCallback = base::OnceCallback<void(bool)>;
enum class ProcessingState {
kUnprocessed, // Response has not yet been processed.
kProcessing, // Response is currently being processed.
kProcessed, // Response has finished processing.
};
using AssistantSuggestion = chromeos::assistant::mojom::AssistantSuggestion;
using AssistantSuggestionPtr =
chromeos::assistant::mojom::AssistantSuggestionPtr;
AssistantResponse();
~AssistantResponse();
......@@ -61,16 +63,48 @@ class AssistantResponse {
bool has_tts() const { return has_tts_; }
void set_has_tts(bool has_tts) { has_tts_ = has_tts; }
// Returns a weak pointer to this instance.
base::WeakPtr<AssistantResponse> GetWeakPtr();
// Invoke to begin processing the response. Upon completion, |callback| will
// be run to indicate success or failure.
void Process(content::mojom::NavigableContentsFactoryPtr contents_factory,
ProcessingCallback callback);
private:
// Handles processing for an AssistantResponse.
class Processor {
public:
Processor(AssistantResponse& response,
content::mojom::NavigableContentsFactoryPtr contents_factory,
ProcessingCallback callback);
~Processor();
// Invoke to begin processing.
void Process();
private:
// Event fired upon completion of a UI element's asynchronous processing.
// Once all asynchronous processing of UI elements has completed, the
// response itself has finished processing.
void OnFinishedProcessing(bool success);
// Attempts to successfully complete response processing. This will no-op
// if we have already finished or if elements are still processing.
void TryFinishing();
AssistantResponse& response_;
content::mojom::NavigableContentsFactoryPtr contents_factory_;
ProcessingCallback callback_;
int processing_count_ = 0;
DISALLOW_COPY_AND_ASSIGN(Processor);
};
std::vector<std::unique_ptr<AssistantUiElement>> ui_elements_;
std::vector<AssistantSuggestionPtr> suggestions_;
ProcessingState processing_state_ = ProcessingState::kUnprocessed;
bool has_tts_ = false;
base::WeakPtrFactory<AssistantResponse> weak_factory_;
std::unique_ptr<Processor> processor_;
DISALLOW_COPY_AND_ASSIGN(AssistantResponse);
};
......
ash/assistant/model/assistant_ui_element.cc
View file @ 6848457e
......@@ -4,8 +4,18 @@
#include "ash/assistant/model/assistant_ui_element.h"
#include "ash/assistant/ui/assistant_ui_constants.h"
#include "base/base64.h"
namespace ash {
namespace {
// Navigable contents.
constexpr char kDataUriPrefix[] = "data:text/html;base64,";
} // namespace
// AssistantCardElement --------------------------------------------------------
AssistantCardElement::AssistantCardElement(const std::string& html,
......@@ -16,4 +26,62 @@ AssistantCardElement::AssistantCardElement(const std::string& html,
AssistantCardElement::~AssistantCardElement() = default;
void AssistantCardElement::Process(
content::mojom::NavigableContentsFactory* contents_factory,
ProcessingCallback callback) {
processor_ =
std::make_unique<Processor>(*this, contents_factory, std::move(callback));
processor_->Process();
}
// AssistantCardElement::Processor ---------------------------------------------
AssistantCardElement::Processor::Processor(
AssistantCardElement& card_element,
content::mojom::NavigableContentsFactory* contents_factory,
ProcessingCallback callback)
: card_element_(card_element),
contents_factory_(contents_factory),
callback_(std::move(callback)) {}
AssistantCardElement::Processor::~Processor() {
if (contents_)
contents_->RemoveObserver(this);
if (callback_)
std::move(callback_).Run(/*success=*/false);
}
void AssistantCardElement::Processor::Process() {
// TODO(dmblack): Find a better way of determining desired card size.
const int width_dip = kPreferredWidthDip - 2 * kUiElementHorizontalMarginDip;
// Configure parameters for the card.
auto contents_params = content::mojom::NavigableContentsParams::New();
contents_params->enable_view_auto_resize = true;
contents_params->auto_resize_min_size = gfx::Size(width_dip, 1);
contents_params->auto_resize_max_size = gfx::Size(width_dip, INT_MAX);
contents_params->suppress_navigations = true;
contents_ = std::make_unique<content::NavigableContents>(
contents_factory_, std::move(contents_params));
// Observe |contents_| so that we are notified when loading is complete.
contents_->AddObserver(this);
// Navigate to the data URL which represents the card.
std::string encoded_html;
base::Base64Encode(card_element_.html(), &encoded_html);
contents_->Navigate(GURL(kDataUriPrefix + encoded_html));
}
void AssistantCardElement::Processor::DidStopLoading() {
contents_->RemoveObserver(this);
// Pass ownership of |contents_| to the card element that was being processed
// and notify our |callback_| of success.
card_element_.set_contents(std::move(contents_));
std::move(callback_).Run(/*success=*/true);
}
} // namespace ash
ash/assistant/model/assistant_ui_element.h
View file @ 6848457e
......@@ -44,6 +44,8 @@ class AssistantUiElement {
// An Assistant UI element that will be rendered as an HTML card.
class AssistantCardElement : public AssistantUiElement {
public:
using ProcessingCallback = base::OnceCallback<void(bool)>;
explicit AssistantCardElement(const std::string& html,
const std::string& fallback);
~AssistantCardElement() override;
......@@ -59,12 +61,42 @@ class AssistantCardElement : public AssistantUiElement {
contents_ = std::move(contents);
}
// Invoke to begin processing the card element. Upon completion, the specified
// |callback| will be run to indicate success or failure.
void Process(content::mojom::NavigableContentsFactory* contents_factory,
ProcessingCallback callback);
private:
// Handles processing of an AssistantCardElement.
class Processor : public content::NavigableContentsObserver {
public:
Processor(AssistantCardElement& card_element,
content::mojom::NavigableContentsFactory* contents_factory,
ProcessingCallback callback);
~Processor() override;
// Invoke to begin processing.
void Process();
// content::NavigableContentsObserver:
void DidStopLoading() override;
private:
AssistantCardElement& card_element_;
content::mojom::NavigableContentsFactory* const contents_factory_;
ProcessingCallback callback_;
std::unique_ptr<content::NavigableContents> contents_;
DISALLOW_COPY_AND_ASSIGN(Processor);
};
const std::string html_;
const std::string fallback_;
std::unique_ptr<content::NavigableContents> contents_;
std::unique_ptr<Processor> processor_;
DISALLOW_COPY_AND_ASSIGN(AssistantCardElement);
};
......
ash/assistant/ui/main_stage/ui_element_container_view.h
View file @ 6848457e
......@@ -10,7 +10,6 @@
#include <utility>
#include <vector>
#include "ash/assistant/assistant_response_processor.h"
#include "ash/assistant/model/assistant_interaction_model_observer.h"
#include "ash/assistant/ui/base/assistant_scroll_view.h"
#include "base/macros.h"
......
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