Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2913993002:
Prevent crash on clear-all button in message center (Closed)
DescriptionPrevent crash on clear-all button in message center
Defer deleting view after calling OnAllNotificationsCleared(). Views are deleted before OnAllNotificationsCleared(), but it causes crash. This CL moves the deletion as a temporary solution. We need to refactor the code around more in near feature.
BUG=725299
TEST=none
Review-Url: https://codereview.chromium.org/2913993002
Cr-Commit-Position: refs/heads/master@{#476235}
Committed: https://chromium.googlesource.com/chromium/src/+/9374f06a7c08b17751ea465e6abcb68721db85b8
Patch Set 1 #Patch Set 2 #
Total comments: 2
Patch Set 3 : Revert back the unnecessary change #
Total comments: 4
Patch Set 4 : Addressed comments (updating comments) #Messages
Total messages: 39 (31 generated)
The CQ bit was checked by yoshiki@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 yoshiki@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 yoshiki@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 yoshiki@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...
Description was changed from ========== Prevent crash on clear-all button in message center BUG=725299 ========== to ========== Prevent crash on clear-all button in message center Defer deleting view after calling OnAllNotificationsCleared(). Views are deleted before OnAllNotificationsCleared(), but it causes crash. This CL moves the deletion as a temporary solution. We need to refactor the code around more in near feature. BUG=725299 TEST=none ==========
yoshiki@chromium.org changed reviewers: + edcourtney@chromium.org
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Eliot, PTAL. Thanks.
https://codereview.chromium.org/2913993002/diff/60001/ui/message_center/views... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2913993002/diff/60001/ui/message_center/views... ui/message_center/views/message_list_view.cc:312: clear_all_started_ = false; What effect does moving clear_all_started_ to after the observer calls have?
The CQ bit was checked by yoshiki@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...
Eliot, PTAL. I found the crash we saw with fuzzing is not caused by this change. I filed it separately: crbug.com/728480 https://codereview.chromium.org/2913993002/diff/60001/ui/message_center/views... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2913993002/diff/60001/ui/message_center/views... ui/message_center/views/message_list_view.cc:312: clear_all_started_ = false; On 2017/06/01 03:40:03, Eliot Courtney wrote: > What effect does moving clear_all_started_ to after the observer calls have? This change was used in previous CLs but now it isn't. Reverted.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits I'm not a committer yet, so I think you probably need approval from someone else? https://codereview.chromium.org/2913993002/diff/80001/ui/message_center/views... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2913993002/diff/80001/ui/message_center/views... ui/message_center/views/message_list_view.cc:314: if (!deleted_when_done_.empty()) Could you add a comment about why we need to call DoUpdateIfPossible() in this case? https://codereview.chromium.org/2913993002/diff/80001/ui/message_center/views... ui/message_center/views/message_list_view.cc:318: // Ensures that the views are not destroyed yet. Since this is a DCHECK, maybe "None of these views should be deleted already.", since it doesn't ensure anything.
The CQ bit was checked by yoshiki@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...
yoshiki@chromium.org changed reviewers: + fukino@chromium.org
Fukino-san, could you take a look? Thanks. https://codereview.chromium.org/2913993002/diff/80001/ui/message_center/views... File ui/message_center/views/message_list_view.cc (right): https://codereview.chromium.org/2913993002/diff/80001/ui/message_center/views... ui/message_center/views/message_list_view.cc:314: if (!deleted_when_done_.empty()) On 2017/06/01 07:53:16, Eliot Courtney wrote: > Could you add a comment about why we need to call DoUpdateIfPossible() in this > case? Done. https://codereview.chromium.org/2913993002/diff/80001/ui/message_center/views... ui/message_center/views/message_list_view.cc:318: // Ensures that the views are not destroyed yet. On 2017/06/01 07:53:16, Eliot Courtney wrote: > Since this is a DCHECK, maybe "None of these views should be deleted already.", > since it doesn't ensure anything. Done.
lgtm
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 yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from edcourtney@chromium.org Link to the patchset: https://codereview.chromium.org/2913993002/#ps100001 (title: "Addressed comments (updating comments)")
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": 100001, "attempt_start_ts": 1496309049002960,
"parent_rev": "3b6c0e0cda44ef86b480a3c9604417d78f6ac650", "commit_rev":
"9374f06a7c08b17751ea465e6abcb68721db85b8"}
Message was sent while issue was closed.
Description was changed from ========== Prevent crash on clear-all button in message center Defer deleting view after calling OnAllNotificationsCleared(). Views are deleted before OnAllNotificationsCleared(), but it causes crash. This CL moves the deletion as a temporary solution. We need to refactor the code around more in near feature. BUG=725299 TEST=none ========== to ========== Prevent crash on clear-all button in message center Defer deleting view after calling OnAllNotificationsCleared(). Views are deleted before OnAllNotificationsCleared(), but it causes crash. This CL moves the deletion as a temporary solution. We need to refactor the code around more in near feature. BUG=725299 TEST=none Review-Url: https://codereview.chromium.org/2913993002 Cr-Commit-Position: refs/heads/master@{#476235} Committed: https://chromium.googlesource.com/chromium/src/+/9374f06a7c08b17751ea465e6abc... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/9374f06a7c08b17751ea465e6abc... |
