Skip to content

Commit

Permalink
cros:Speculative fix for crash during popup close
Browse files Browse the repository at this point in the history
Suspected crash scenario:
A popup notification is marked visible, then closed when
the UnifiedMessageCenter is shown. During the widget close[1] (note the
widget is closed but the ptr is still kept stored in popup_items_),
an animation callback is called which eventually attempts to get the visible popup which was just closed[2]. MessagePopupCollection handles removing popups by first closing the widget, then once iterating through all owned popups, remove the items from the list.

The issue is that the popup->Close() will delete the child views,
triggering an animation callback which then grabs the closed widget[2]
and accesses a ptr to a deleted view[3].

Check whether the widget is closing, and don't return the popup
in that scenario. This is a speculative fix, and I have not figured out how to trigger this scenario manually, but I am pretty sure this is the cause of the crash.

[1] https://source.chromium.org/chromium/chromium/src/+/main:ui/message_center/views/message_popup_collection.cc;l=658;drc=c3306e8bb47c6468c90736db08c39f6aed9c9fe2

[2]
https://source.chromium.org/chromium/chromium/src/+/main:ash/system/message_center/notification_grouping_controller.cc;l=271-272;drc=13dced01cce80364299ad7663479ad65069db73d

[3]
https://source.chromium.org/chromium/chromium/src/+/main:ui/message_center/views/message_popup_collection.cc;l=152-153;drc=13dced01cce80364299ad7663479ad65069db73d

Bug: 1360618
Change-Id: I4216cfe75e0d2d4c04311149b73739e4d982f293
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3880503
Commit-Queue: Alex Newcomer <newcomer@chromium.org>
Reviewed-by: Andre Le <leandre@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1044285}
  • Loading branch information
Alex Newcomer authored and Chromium LUCI CQ committed Sep 8, 2022
1 parent 184a820 commit aa8f4c3
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 1 deletion.
7 changes: 6 additions & 1 deletion ash/system/message_center/ash_notification_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,12 @@ void AshNotificationView::AnimateSingleToGroup(
auto* parent_notification =
message_center::MessageCenter::Get()->FindNotificationById(
parent_id);
if (!parent_notification)
auto* child_notification =
message_center::MessageCenter::Get()->FindNotificationById(
notification_id);
// The child and parent notifications are not guaranteed to exist. If
// they were deleted avoid the animation cleanup.
if (!parent_notification || !child_notification)
return;

grouping_controller->ConvertFromSingleToGroupNotificationAfterAnimation(
Expand Down
5 changes: 5 additions & 0 deletions ui/message_center/views/message_popup_collection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,11 @@ MessageView* MessagePopupCollection::GetMessageViewForNotificationId(
const std::string& notification_id) {
auto it = std::find_if(
popup_items_.begin(), popup_items_.end(), [&](const auto& child) {
auto* widget = child.popup->GetWidget();
// Do not return popups that are in the process of closing, but have not
// yet been removed from `popup_items_`.
if (!widget || widget->IsClosed())
return false;
return child.popup->message_view()->notification_id() ==
notification_id;
});
Expand Down

0 comments on commit aa8f4c3

Please sign in to comment.