Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2918763002:
Fix crash on updating notification before attaching (Closed)
|
Created:
3 years, 6 months ago by yoshiki Modified:
3 years, 6 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org, victorhsieh+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix crash on updating notification before attaching
- Do not update the pinned states when control_buttons_view_ is null
- Do not update the visibility of buttons when floating_control_buttons_widget_ is null
- Create the floating buttons widget just after attaching
BUG=728480
TEST=open message center during fuzz (step:10000 and key:60) without crash
Review-Url: https://codereview.chromium.org/2918763002
Cr-Commit-Position: refs/heads/master@{#477587}
Committed: https://chromium.googlesource.com/chromium/src/+/a23721b5ffdf529bc575ee5b278534a54b06c582
Patch Set 1 #Patch Set 2 : . #
Total comments: 8
Patch Set 3 : addressed comment #Messages
Total messages: 25 (17 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...
Description was changed from ========== wip BUG= ========== to ========== Fixing crash on updating notification before attaching - Do not update the pinned states when control_buttons_view_ is null - Do not update the visibility of buttons when floating_control_buttons_widget_ is null - Create the floating buttons widget just after attaching BUG=728480 ==========
yoshiki@chromium.org changed reviewers: + edcourtney@chromium.org
Description was changed from ========== Fixing crash on updating notification before attaching - Do not update the pinned states when control_buttons_view_ is null - Do not update the visibility of buttons when floating_control_buttons_widget_ is null - Create the floating buttons widget just after attaching BUG=728480 ========== to ========== Fixing crash on updating notification before attaching - Do not update the pinned states when control_buttons_view_ is null - Do not update the visibility of buttons when floating_control_buttons_widget_ is null - Create the floating buttons widget just after attaching BUG=728480 TEST=manual (crash doesn't happen) ==========
Description was changed from ========== Fixing crash on updating notification before attaching - Do not update the pinned states when control_buttons_view_ is null - Do not update the visibility of buttons when floating_control_buttons_widget_ is null - Create the floating buttons widget just after attaching BUG=728480 TEST=manual (crash doesn't happen) ========== to ========== Fix crash on updating notification before attaching - Do not update the pinned states when control_buttons_view_ is null - Do not update the visibility of buttons when floating_control_buttons_widget_ is null - Create the floating buttons widget just after attaching BUG=728480 TEST=manual (crash doesn't happen) ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix crash on updating notification before attaching - Do not update the pinned states when control_buttons_view_ is null - Do not update the visibility of buttons when floating_control_buttons_widget_ is null - Create the floating buttons widget just after attaching BUG=728480 TEST=manual (crash doesn't happen) ========== to ========== Fix crash on updating notification before attaching - Do not update the pinned states when control_buttons_view_ is null - Do not update the visibility of buttons when floating_control_buttons_widget_ is null - Create the floating buttons widget just after attaching BUG=728480 TEST=open message center during fuzz (step:10000 and key:60) without crash ==========
Eliot, PTAL. Thanks.
https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_content_view.cc (left): https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view.cc:400: if (!floating_control_buttons_widget_) { I think removing this code will cause a regression (crbug.com/709862). WDYT? If we don't remove it though it may potentially continue causing memory corruption - not sure if this memory corruption is actually triggered though. https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_content_view.cc (right): https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view.cc:406: if (!control_buttons_view_) After removing the call to UpdatePinnedState from AttachSurface, this is only called from OnItemUpdated AFAICT, which should mean item_ is non-null, but, if a call to this is added later that doesn't guarantee item_ is non-null it could crash. Or, if there is a regression of a previous bug where item_ can become null while OnItemUpdated is executing it could be bad. So, it might be better to leave the item_ check in https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view.cc:447: if (item_) I think item_ is checked inside MaybeCreateFloatingControl buttons, so this line is now not necessary https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view.cc:448: MaybeCreateFloatingControlButtons(); Doesn't this mean the pinned state potentially isn't updated? I'm not sure if we are guaranteed that OnItemUpdated will be called after this has executed.
https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_content_view.cc (left): https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view.cc:400: if (!floating_control_buttons_widget_) { On 2017/06/01 07:37:39, Eliot Courtney wrote: > I think removing this code will cause a regression (crbug.com/709862). WDYT? > > If we don't remove it though it may potentially continue causing memory > corruption - not sure if this memory corruption is actually triggered though. As I checked, the regression is not happened. (although I found an issue of close button which is not related with this: crbug.com/728915). If surface_ == null, MaybeCreateFloatingControlButtons() does nothing. https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_content_view.cc (right): https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view.cc:406: if (!control_buttons_view_) On 2017/06/01 07:37:39, Eliot Courtney wrote: > After removing the call to UpdatePinnedState from AttachSurface, this is only > called from OnItemUpdated AFAICT, which should mean item_ is non-null, but, if a > call to this is added later that doesn't guarantee item_ is non-null it could > crash. Or, if there is a regression of a previous bug where item_ can become > null while OnItemUpdated is executing it could be bad. So, it might be better to > leave the item_ check in Done. https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view.cc:447: if (item_) On 2017/06/01 07:37:39, Eliot Courtney wrote: > I think item_ is checked inside MaybeCreateFloatingControl buttons, so this line > is now not necessary Done. https://codereview.chromium.org/2918763002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_content_view.cc:448: MaybeCreateFloatingControlButtons(); On 2017/06/01 07:37:39, Eliot Courtney wrote: > Doesn't this mean the pinned state potentially isn't updated? I'm not sure if we > are guaranteed that OnItemUpdated will be called after this has executed. The pinned state is updated by this method because the floating widget is recreated. If the notification is pinned, the close button is not added to the recreated widget.
Thanks~! lgtm. I think you will need approval from a committer by the way.
yoshiki@chromium.org changed reviewers: + yhanada@chromium.org
Hanada-san, PTAL. Thanks.
lgtm!
The CQ bit was checked by yoshiki@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": 1496824088529610,
"parent_rev": "870292d41a98aac99aca42e5665376151c9eb1ad", "commit_rev":
"a23721b5ffdf529bc575ee5b278534a54b06c582"}
Message was sent while issue was closed.
Description was changed from ========== Fix crash on updating notification before attaching - Do not update the pinned states when control_buttons_view_ is null - Do not update the visibility of buttons when floating_control_buttons_widget_ is null - Create the floating buttons widget just after attaching BUG=728480 TEST=open message center during fuzz (step:10000 and key:60) without crash ========== to ========== Fix crash on updating notification before attaching - Do not update the pinned states when control_buttons_view_ is null - Do not update the visibility of buttons when floating_control_buttons_widget_ is null - Create the floating buttons widget just after attaching BUG=728480 TEST=open message center during fuzz (step:10000 and key:60) without crash Review-Url: https://codereview.chromium.org/2918763002 Cr-Commit-Position: refs/heads/master@{#477587} Committed: https://chromium.googlesource.com/chromium/src/+/a23721b5ffdf529bc575ee5b2785... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a23721b5ffdf529bc575ee5b2785... |
