Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2833403002:
[Notifications] Fix crash in AddNotification. (Closed)
|
Created:
3 years, 8 months ago by Eliot Courtney Modified:
3 years, 8 months ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Notifications] Fix crash in AddNotification.
Calling ResetRepositionSession during UpdateNotificationSize can end up
deleting notification items. This caused two crashes. One in
AddNotification, which calls onItemUpdated, which, without this change,
can set item_ to null. The other is a crash in
ShouldUpdateControlButtonsColor which does not check if item_ is null
since it is called from a context where item_ should not be null.
BUG=714493
BUG=714032
Review-Url: https://codereview.chromium.org/2833403002
Cr-Commit-Position: refs/heads/master@{#466914}
Committed: https://chromium.googlesource.com/chromium/src/+/d331195379094f2cf000746e21ad047d5e1fb6d0
Patch Set 1 #Patch Set 2 : Add check back. #
Total comments: 6
Patch Set 3 : Add TODO #
Messages
Total messages: 20 (11 generated)
edcourtney@chromium.org changed reviewers: + xiyuan@chromium.org, yhanada@chromium.org, yoshiki@chromium.org
This changes the order of setting the reposition session and looking up in notification_views_ whether to return early. I couldn't see why this would be a problem, but PTAL~! Thanks~
lgtm
The CQ bit was checked by edcourtney@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by edcourtney@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
lgtm https://codereview.chromium.org/2833403002/diff/20001/ui/message_center/views... File ui/message_center/views/message_center_view.cc (right): https://codereview.chromium.org/2833403002/diff/20001/ui/message_center/views... ui/message_center/views/message_center_view.cc:411: // checks it anyway. How about adding NOTREACHED() before return? https://codereview.chromium.org/2833403002/diff/20001/ui/message_center/views... ui/message_center/views/message_center_view.cc:665: return; This should also not be happened. How about adding NOTREACHED()?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2833403002/diff/20001/ui/message_center/views... File ui/message_center/views/message_center_view.cc (right): https://codereview.chromium.org/2833403002/diff/20001/ui/message_center/views... ui/message_center/views/message_center_view.cc:411: // checks it anyway. On 2017/04/25 04:58:00, yoshiki wrote: > How about adding NOTREACHED() before return? This can still be reached, but it's possible we don't need to check it since UpdateNotification checks it anyway. We can't remove the check from UpdateNotification because ResetRepositionSession can trigger notifications to be removed, which seems like it will change notification_views_ (meaning that the original order of checking this before ResetRepositionSession had a bug?). https://codereview.chromium.org/2833403002/diff/20001/ui/message_center/views... ui/message_center/views/message_center_view.cc:665: return; On 2017/04/25 04:58:01, yoshiki wrote: > This should also not be happened. How about adding NOTREACHED()? I believe this can happen when ResetRepositionSession triggers notifications to be removed - OnNotificationRemoved in this class may be called. The flow is: ResetRepositionSession => BoundsAnimator::Cancel => MessageListView::OnBoundsAnimatorDone => OnAllNotificationsCleared => MessageCenterView::OnAllNotificationsCleared => MessageCenterImpl::RemoveAllNotifications => NotificationDelegate::Close() => OnNotificationRemoved
btw, could you add a test for this change? If it's urgent, doing in separated CL is ok.
Sure, I will add a test in a separate CL. Thanks for the reviews~! https://codereview.chromium.org/2833403002/diff/20001/ui/message_center/views... File ui/message_center/views/message_center_view.cc (right): https://codereview.chromium.org/2833403002/diff/20001/ui/message_center/views... ui/message_center/views/message_center_view.cc:411: // checks it anyway. On 2017/04/25 05:06:52, Eliot Courtney wrote: > On 2017/04/25 04:58:00, yoshiki wrote: > > How about adding NOTREACHED() before return? > > This can still be reached, but it's possible we don't need to check it since > UpdateNotification checks it anyway. We can't remove the check from > UpdateNotification because ResetRepositionSession can trigger notifications to > be removed, which seems like it will change notification_views_ (meaning that > the original order of checking this before ResetRepositionSession had a bug?). Discussed offline - added a TODO below to investigate if these checks are really needed. https://codereview.chromium.org/2833403002/diff/20001/ui/message_center/views... ui/message_center/views/message_center_view.cc:665: return; On 2017/04/25 05:06:52, Eliot Courtney wrote: > On 2017/04/25 04:58:01, yoshiki wrote: > > This should also not be happened. How about adding NOTREACHED()? > > I believe this can happen when ResetRepositionSession triggers notifications to > be removed - OnNotificationRemoved in this class may be called. The flow is: > > ResetRepositionSession => BoundsAnimator::Cancel => > MessageListView::OnBoundsAnimatorDone => OnAllNotificationsCleared => > MessageCenterView::OnAllNotificationsCleared => > MessageCenterImpl::RemoveAllNotifications => NotificationDelegate::Close() => > OnNotificationRemoved I investigated a bit, and it seems that when close all is pressed on the message center, it unregisters itself as an observer on MessageCenterImpl, so it never gets the OnNotificationRemoved calls that would cause this statement to fire. Discussed offline, and I've added a TODO to investigate whether or not we can remove this check.
The CQ bit was checked by edcourtney@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, yoshiki@chromium.org, yhanada@chromium.org Link to the patchset: https://codereview.chromium.org/2833403002/#ps40001 (title: "Add TODO")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493101786292590,
"parent_rev": "b95f5bc3e4433d6e28d44850506f47efcf8e4fff", "commit_rev":
"d331195379094f2cf000746e21ad047d5e1fb6d0"}
Message was sent while issue was closed.
Description was changed from ========== [Notifications] Fix crash in AddNotification. Calling ResetRepositionSession during UpdateNotificationSize can end up deleting notification items. This caused two crashes. One in AddNotification, which calls onItemUpdated, which, without this change, can set item_ to null. The other is a crash in ShouldUpdateControlButtonsColor which does not check if item_ is null since it is called from a context where item_ should not be null. BUG=714493 BUG=714032 ========== to ========== [Notifications] Fix crash in AddNotification. Calling ResetRepositionSession during UpdateNotificationSize can end up deleting notification items. This caused two crashes. One in AddNotification, which calls onItemUpdated, which, without this change, can set item_ to null. The other is a crash in ShouldUpdateControlButtonsColor which does not check if item_ is null since it is called from a context where item_ should not be null. BUG=714493 BUG=714032 Review-Url: https://codereview.chromium.org/2833403002 Cr-Commit-Position: refs/heads/master@{#466914} Committed: https://chromium.googlesource.com/chromium/src/+/d331195379094f2cf000746e21ad... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d331195379094f2cf000746e21ad... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
