[go: up one dir, main page]

[M76 Merge] Fix crash when pressing Assistant suggestion chip.

Previously a crash could occur because the Suggestion associated with
the suggestion chip may get destroyed during the OpenUrl sequence,
leaving us with a reference to a bad GURL.

To address this, we now post the OpenUrl to a task when handling
OnSuggestionChipPressed events. In addition to fixing the crash, this
also prevents any other observers of this event in the future from
receiving notification of the event with a bad pointer.

Note that there is potential for a similar crash when handling
notification clicks in the future, so handling that here as well. This
case does not require posting.

(cherry picked from commit c0f7696202f4172cf4683716b4338e86c5041924)

Bug: b:134773721
Change-Id: I0a178fc849c70b3015631ac2d9f7617395f616ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1646598
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#667766}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1653073
Cr-Commit-Position: refs/branch-heads/3809@{#232}
Cr-Branched-From: d82dec1a818f378c464ba307ddd9c92133eac355-refs/heads/master@{#665002}
diff --git a/ash/assistant/assistant_interaction_controller.cc b/ash/assistant/assistant_interaction_controller.cc
index 57c2ba4..5f7ebe1 100644
--- a/ash/assistant/assistant_interaction_controller.cc
+++ b/ash/assistant/assistant_interaction_controller.cc
@@ -478,7 +478,16 @@
   // If the suggestion contains a non-empty action url, we will handle the
   // suggestion chip pressed event by launching the action url in the browser.
   if (!suggestion->action_url.is_empty()) {
-    assistant_controller_->OpenUrl(suggestion->action_url);
+    // Note that we post a new task when opening the |action_url| associated
+    // with |sugggestion| as this will potentially cause Assistant UI to close
+    // and destroy |suggestion| in the process. Failure to post in this case
+    // would cause any subsequent observers of this suggestion chip event to
+    // receive a deleted pointer.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(&AssistantController::OpenUrl,
+                       assistant_controller_->GetWeakPtr(),
+                       suggestion->action_url, /*from_server=*/false));
     return;
   }
 
@@ -490,7 +499,7 @@
   // user's preceding voice interaction.
   StartTextInteraction(suggestion->text, /*allow_tts=*/model_.response() &&
                                              model_.response()->has_tts(),
-                       /*query_source*/ AssistantQuerySource::kSuggestionChip);
+                       /*query_source=*/AssistantQuerySource::kSuggestionChip);
 }
 
 void AssistantInteractionController::OnSuggestionsResponse(
diff --git a/ash/assistant/assistant_notification_controller.cc b/ash/assistant/assistant_notification_controller.cc
index 19dcc7a..9552486 100644
--- a/ash/assistant/assistant_notification_controller.cc
+++ b/ash/assistant/assistant_notification_controller.cc
@@ -266,7 +266,10 @@
 
   // Open the action url if it is valid.
   if (IsValidActionUrl(action_url)) {
-    assistant_controller_->OpenUrl(action_url);
+    // Note that we copy construct a new GURL as our |notification| may be
+    // destroyed during the |OpenUrl| sequence leaving |action_url| in a bad
+    // state.
+    assistant_controller_->OpenUrl(GURL(action_url));
     model_.RemoveNotificationById(id, /*from_server=*/false);
     return;
   }