Commit 853ed5fe authored by Jeroen Dhollander's avatar Jeroen Dhollander Committed by Chromium LUCI CQ

Fix Crash when shutting down after starting Assistant

This could lead to the following chain of events:
    - (Browser thread) Starts Assistant, and calls mojom method
      |ServiceController::Start()|.
    - (Browser thread) Stops Assistant, and calls mojom method
      |ServiceController::Stop()|.
    - (Mojom thread) ServiceController::Start()
           - Creates |AssistantManager|
           - Publishes it through the singleton |LibassistantV1Api|.
           - Informs browser thread through mojom method
             |StateObserver::OnStateChanged(started)|.
    - Here we get an interwoven sequence of events between the mojom
      thread and the browser thread:
      - (Mojom thread) ServiceController::Stop()
             - Informs browser thread through mojom method
               |StateObserver::OnStateChanged(stopped)|.
             - (Here we are 'interrupted' by the browser thread)
      - (Browser thread) StateObserver::OnStateChanged(started) calls
        AssistantManagerServiceImpl::PostInitAssistant() which
        accesses the |LibassistantV1Api| which is not null yet.
      - (Mojom thread) now destroys |LibassistantV1Api| and
        |AssistantManager|.
      - (Browser thread) dereferences the (now deleted)
        |AssistantManager| object and crashes.

Bug: 1162136
Test: Compiled and deployed
Change-Id: I86b30c76c544a926e46e7817540f17f0910da192
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612402
Commit-Queue: Jeroen Dhollander <jeroendh@chromium.org>
Reviewed-by: default avatarXiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840883}
parent 7439386a
......@@ -212,7 +212,6 @@ void ServiceControllerProxy::OnStateChanged(ServiceState new_state) {
switch (new_state) {
case ServiceState::kStarted:
DCHECK_EQ(state_, State::kStarting);
FinishCreatingAssistant();
break;
case ServiceState::kRunning:
......@@ -237,6 +236,16 @@ ServiceControllerProxy::assistant_manager_internal() {
}
void ServiceControllerProxy::FinishCreatingAssistant() {
if (state_ == State::kStopped) {
// We can come here if the system went into shutdown while the mojom
// service was busy starting Libassistant.
// This means the |AssistantManager| could be destroyed at any second,
// so we simply clean up and bail out.
on_start_done_callback_.reset();
pending_display_connection_ = nullptr;
return;
}
DCHECK(on_start_done_callback_.has_value());
DCHECK_NE(pending_display_connection_, nullptr);
......
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