Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2836023002:
Fix use-after-free in MessageListView. (Closed)
DescriptionFix use-after-free in MessageListView.
This is caused by calling RemoveNotification() while
'Clear All' operation is in progress. A MessageView
could be deleted twice.
BUG=713983
Review-Url: https://codereview.chromium.org/2836023002
Cr-Commit-Position: refs/heads/master@{#467248}
Committed: https://chromium.googlesource.com/chromium/src/+/4518695792a1cad0afdb80bbe0e3ea1850f310c1
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address the comments #Patch Set 3 : rebase #
Messages
Total messages: 24 (16 generated)
yhanada@chromium.org changed reviewers: + edcourtney@chromium.org, yoshiki@chromium.org
PTAL.
The CQ bit was checked by yhanada@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2836023002/diff/1/ui/message_center/views/mes... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2836023002/diff/1/ui/message_center/views/mes... ui/message_center/views/message_list_view.cc:141: Should we skip updating if the view is in clearing_all_views_? https://codereview.chromium.org/2836023002/diff/1/ui/message_center/views/mes... ui/message_center/views/message_list_view.cc:274: observer.OnAllNotificationsCleared(); Could you rename is_clearing_ in MessageCenterView class? This property is no longer the flag of just "clear" but "clear-all". btw, this is not related this CL, I think we shouldn't call RemoveAllNotifications in MessageCenterView::OnAllNotificationsCleared(), since there are no notifications at that time. WDTY? https://codereview.chromium.org/2836023002/diff/1/ui/message_center/views/mes... ui/message_center/views/message_list_view.cc:324: deleting_views_.end() && Should we check clearing_all_views_ here as well?
https://codereview.chromium.org/2836023002/diff/1/ui/message_center/views/mes... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2836023002/diff/1/ui/message_center/views/mes... ui/message_center/views/message_list_view.cc:274: observer.OnAllNotificationsCleared(); On 2017/04/25 03:59:11, yoshiki wrote: > Could you rename is_clearing_ in MessageCenterView class? This property is no > longer the flag of just "clear" but "clear-all". > > btw, this is not related this CL, I think we shouldn't call > RemoveAllNotifications in MessageCenterView::OnAllNotificationsCleared(), since > there are no notifications at that time. WDTY? My understanding of this was that without the call to RemoveAllNotifications the notifications won't be removed. ClearAllClosableNotifications adds views to clearing_all_views_, which are then animated, and when OnBoundsAnimatorDone is called it calls OnAllNotificationsCleared which actually clears the notifications. If so we can't remove it. WDYT?
https://codereview.chromium.org/2836023002/diff/1/ui/message_center/views/mes... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2836023002/diff/1/ui/message_center/views/mes... ui/message_center/views/message_list_view.cc:141: On 2017/04/25 03:59:11, yoshiki wrote: > Should we skip updating if the view is in clearing_all_views_? Done. https://codereview.chromium.org/2836023002/diff/1/ui/message_center/views/mes... ui/message_center/views/message_list_view.cc:274: observer.OnAllNotificationsCleared(); On 2017/04/25 04:08:47, Eliot Courtney wrote: > On 2017/04/25 03:59:11, yoshiki wrote: > > Could you rename is_clearing_ in MessageCenterView class? This property is no > > longer the flag of just "clear" but "clear-all". Renamed to is_clearing_all_notifications_. > > btw, this is not related this CL, I think we shouldn't call > > RemoveAllNotifications in MessageCenterView::OnAllNotificationsCleared(), > since > > there are no notifications at that time. WDTY? > > My understanding of this was that without the call to RemoveAllNotifications the > notifications won't be removed. ClearAllClosableNotifications adds views to > clearing_all_views_, which are then animated, and when OnBoundsAnimatorDone is > called it calls OnAllNotificationsCleared which actually clears the > notifications. If so we can't remove it. WDYT? A MessageView of a notification should be deleted before deleting a notification in MessageCenter as Eliot said. https://codereview.chromium.org/2836023002/diff/1/ui/message_center/views/mes... ui/message_center/views/message_list_view.cc:324: deleting_views_.end() && On 2017/04/25 03:59:11, yoshiki wrote: > Should we check clearing_all_views_ here as well? Done.
The CQ bit was checked by yhanada@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhanada@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...
Please take another look. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by yhanada@chromium.org
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": 1493185501077720,
"parent_rev": "1baa747be827c77f3fad895f072eac6dee1cf18c", "commit_rev":
"4518695792a1cad0afdb80bbe0e3ea1850f310c1"}
Message was sent while issue was closed.
Description was changed from ========== Fix use-after-free in MessageListView. This is caused by calling RemoveNotification() while 'Clear All' operation is in progress. A MessageView could be deleted twice. BUG=713983 ========== to ========== Fix use-after-free in MessageListView. This is caused by calling RemoveNotification() while 'Clear All' operation is in progress. A MessageView could be deleted twice. BUG=713983 Review-Url: https://codereview.chromium.org/2836023002 Cr-Commit-Position: refs/heads/master@{#467248} Committed: https://chromium.googlesource.com/chromium/src/+/4518695792a1cad0afdb80bbe0e3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4518695792a1cad0afdb80bbe0e3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
