Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557593009:
Fix Chrome crashes on calling OnTrayVisibilityChange (Closed)
DescriptionFix Chrome crashes on calling OnTrayVisibilityChange
When status area widget is initialized in Chrome Book with stylus
pen, StatusAreaWidget::CreateTrayViews will call AddPaletteTray()
to initialize a |palette_tray_| (which controls the stylus pen) and
update this tray's visibility. The problem is that the
|logout_button_tray_| is not initialized yet (it's initialized after
the |palette_tray_|) and is used in the visibility update function
in |palette_tray_|. As a result, Chrome OS crashes on machine with
a stylus pen only.
This crash happened because the order of the tray initialization is
changed in cl: https://codereview.chromium.org/2427313003
BUG=671293
Committed: https://crrev.com/e422d9c3ef94d97c426160937f9b7422987c7d18
Cr-Commit-Position: refs/heads/master@{#438840}
Patch Set 1 #
Total comments: 10
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : address comments #
Total comments: 2
Patch Set 4 : address comments #
Total comments: 3
Patch Set 5 : new approach #
Messages
Total messages: 35 (18 generated)
Description was changed from ========== fix crash fix crash BUG=671293 ========== to ========== Fix Chrome crashes on calling OnTrayVisibilityChange When an external display is connected to the Chromebook, both functions AddRemoveDisplay (which will re-initialize each tray in StatusAreaWidget and updates each tray's visibility) and OnMessageCenterTrayChanged (Upon showing new notifications, visibility of notification center may be changed) are called asynchronously. So OnTrayVisibilityChange may access uninitialized tray item in its function. A new variable |is_initialized_| is introduced to keep track the state of tray items in SatusAreaWidget class. TEST=SystemTrayTest.SystemTrayInitialization BUG=671293 ==========
The CQ bit was checked by yiyix@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.
yiyix@chromium.org changed reviewers: + sadrul@chromium.org, varkha@chromium.org
@varkha, sadrul, could you please take a look at this change? Thank you.
https://codereview.chromium.org/2557593009/diff/1/ash/common/system/status_ar... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2557593009/diff/1/ash/common/system/status_ar... ash/common/system/status_area_widget.cc:157: // added to prevent to access |tray| before its initialization. nit: I think this early return is self-explanatory. If you want to add a comment I would put something like "StatusAreaWidget may be |Shutdown| but still be accessed before it gets reinitialized. Avoid accessing uninitialized trays." https://codereview.chromium.org/2557593009/diff/1/ash/common/system/tray/syst... File ash/common/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2557593009/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_unittest.cc:68: bool AreTraysInitialized(StatusAreaWidget* widget) { nit: I think this could be IsWidgetInitialized(). https://codereview.chromium.org/2557593009/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_unittest.cc:679: // Calling OnVisibilityChange on uninitialized |tray| caused crashes, see nit: s/uninitialized |tray|/StatusAreaWidget that has been Shutdown https://codereview.chromium.org/2557593009/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_unittest.cc:681: TEST_F(SystemTrayTest, SystemTrayInitialization) { Thanks for putting this together. https://codereview.chromium.org/2557593009/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_unittest.cc:685: ->web_notification_tray(); nit: can move this down close to the place you use it, i.e. before line 689.
@varkha, sadrul, Could you take another look at this change? https://codereview.chromium.org/2557593009/diff/1/ash/common/system/status_ar... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2557593009/diff/1/ash/common/system/status_ar... ash/common/system/status_area_widget.cc:157: // added to prevent to access |tray| before its initialization. On 2016/12/09 00:59:03, varkha wrote: > nit: I think this early return is self-explanatory. If you want to add a comment > I would put something like "StatusAreaWidget may be |Shutdown| but still be > accessed before it gets reinitialized. Avoid accessing uninitialized trays." True, i kinda of want to give some background about the bug. It is a good general comment without the knowledge of this specific bug. https://codereview.chromium.org/2557593009/diff/1/ash/common/system/tray/syst... File ash/common/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2557593009/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_unittest.cc:68: bool AreTraysInitialized(StatusAreaWidget* widget) { On 2016/12/09 00:59:03, varkha wrote: > nit: I think this could be IsWidgetInitialized(). Done. https://codereview.chromium.org/2557593009/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_unittest.cc:679: // Calling OnVisibilityChange on uninitialized |tray| caused crashes, see On 2016/12/09 00:59:03, varkha wrote: > nit: s/uninitialized |tray|/StatusAreaWidget that has been Shutdown I was not sure how to describe StatusAreaWidget that have trays uninitialized, I like your wording. https://codereview.chromium.org/2557593009/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_unittest.cc:681: TEST_F(SystemTrayTest, SystemTrayInitialization) { On 2016/12/09 00:59:03, varkha wrote: > Thanks for putting this together. :D https://codereview.chromium.org/2557593009/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_unittest.cc:685: ->web_notification_tray(); On 2016/12/09 00:59:03, varkha wrote: > nit: can move this down close to the place you use it, i.e. before line 689. Done.
https://codereview.chromium.org/2557593009/diff/20001/ash/common/system/statu... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2557593009/diff/20001/ash/common/system/statu... ash/common/system/status_area_widget.cc:154: // StatusAreaWidget may be |Shutdown| but still be accessed before it gets What is accessing this widget after Shutdown() is called? What happens if this widget is accessed after calling 'Shutdown()'?
Description was changed from ========== Fix Chrome crashes on calling OnTrayVisibilityChange When an external display is connected to the Chromebook, both functions AddRemoveDisplay (which will re-initialize each tray in StatusAreaWidget and updates each tray's visibility) and OnMessageCenterTrayChanged (Upon showing new notifications, visibility of notification center may be changed) are called asynchronously. So OnTrayVisibilityChange may access uninitialized tray item in its function. A new variable |is_initialized_| is introduced to keep track the state of tray items in SatusAreaWidget class. TEST=SystemTrayTest.SystemTrayInitialization BUG=671293 ========== to ========== Fix Chrome crashes on calling OnTrayVisibilityChange When an external display is connected to the Chromebook, both functions DisplayChangeObserver::OnDisplayModeChanged (which will re-initialize each tray in StatusAreaWidget and updates each tray's visibility) and OnMessageCenterTrayChanged (Upon showing new notifications, visibility of notification center may be changed) are called asynchronously. So OnTrayVisibilityChange may access uninitialized tray item in its function. A new variable |is_initialized_| is introduced to keep track the state of tray items in SatusAreaWidget class. TEST=SystemTrayTest.SystemTrayInitialization BUG=671293 ==========
lgtm with the nits https://codereview.chromium.org/2557593009/diff/40001/ash/common/system/statu... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2557593009/diff/40001/ash/common/system/statu... ash/common/system/status_area_widget.cc:285: if (!logout_button_tray_) { Don't need {} https://codereview.chromium.org/2557593009/diff/40001/ash/common/system/statu... ash/common/system/status_area_widget.cc:291: DCHECK_NE(-1, logout_button_index); As discussed offline, this DCHECK can probably go now (although we may want to make sure that GetIndexOf() does return a valid index when logout_button_tray_ is non-null?
@sadrul, varkha, could you please take another look at this change? @dcastagna, feel free to test if it mitigate your issues. https://codereview.chromium.org/2557593009/diff/20001/ash/common/system/statu... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2557593009/diff/20001/ash/common/system/statu... ash/common/system/status_area_widget.cc:154: // StatusAreaWidget may be |Shutdown| but still be accessed before it gets On 2016/12/12 19:34:48, sadrul wrote: > What is accessing this widget after Shutdown() is called? What happens if this > widget is accessed after calling 'Shutdown()'? We have discussed this problem offline. The main problem this CL is trying to fix is calling logout_button_tray_->IsVisible() when the |logout_button_tray_| is still not initialized. After discussion, we agreed that this check should be inside of the function IsNextVisibleTrayToLogout where the uninitialized tray is used. Assuming callers would always check the tray's initialization is unreliable.
dcastagna@chromium.org changed reviewers: + dcastagna@chromium.org
In the CL description you mention "A new variable |is_initialized_|" but it probably got removed with the latest patch. https://codereview.chromium.org/2557593009/diff/60001/ash/common/system/statu... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2557593009/diff/60001/ash/common/system/statu... ash/common/system/status_area_widget.cc:285: if (!logout_button_tray_) Is whoever needs this going to call IsNextVisibleTrayToLogout again after it fails the first time?
https://codereview.chromium.org/2557593009/diff/60001/ash/common/system/tray/... File ash/common/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2557593009/diff/60001/ash/common/system/tray/... ash/common/system/tray/system_tray_unittest.cc:68: void ShutDownLogoutButtonTray(StatusAreaWidget* widget) { nit: I think we usually prefer Shutdown over ShutDown. https://codereview.chromium.org/2557593009/diff/60001/ash/common/system/tray/... ash/common/system/tray/system_tray_unittest.cc:683: ShutDownLogoutButtonTray(widget); I am still confused if the bad scenario is a result of a call to visibility change callback while we are inside StatusAreaWidget::CreateTrayViews() but haven't initialized the |logout_button_tray_| yet (in which case you are rightly testing it with resetting a single data member) or alternatively if the scenario is a result of a race between StatusAreaWidget::CreateTrayViews() and StatusAreaWidget::IsNextVisibleTrayToLogout() where IsNextVisibleTrayToLogout() can be called before CreateTrayViews() (in which case I would prefer the test to just invoke StatusAreaWidget::Shutdown() instead to match the real life scenario.
On 2016/12/13 at 17:10:38, yiyix wrote: > @sadrul, varkha, could you please take another look at this change? > > @dcastagna, feel free to test if it mitigate your issues. > Just tested it, I'm not hitting the DCHECK anymore (since it's gone). Is there anything visual I should check to make sure we're not regressing something else? > https://codereview.chromium.org/2557593009/diff/20001/ash/common/system/statu... > File ash/common/system/status_area_widget.cc (right): > > https://codereview.chromium.org/2557593009/diff/20001/ash/common/system/statu... > ash/common/system/status_area_widget.cc:154: // StatusAreaWidget may be |Shutdown| but still be accessed before it gets > On 2016/12/12 19:34:48, sadrul wrote: > > What is accessing this widget after Shutdown() is called? What happens if this > > widget is accessed after calling 'Shutdown()'? > > We have discussed this problem offline. The main problem this CL is trying to fix is calling logout_button_tray_->IsVisible() when the |logout_button_tray_| is still not initialized. After discussion, we agreed that this check should be inside of the function IsNextVisibleTrayToLogout where the uninitialized tray is used. Assuming callers would always check the tray's initialization is unreliable.
Description was changed from ========== Fix Chrome crashes on calling OnTrayVisibilityChange When an external display is connected to the Chromebook, both functions DisplayChangeObserver::OnDisplayModeChanged (which will re-initialize each tray in StatusAreaWidget and updates each tray's visibility) and OnMessageCenterTrayChanged (Upon showing new notifications, visibility of notification center may be changed) are called asynchronously. So OnTrayVisibilityChange may access uninitialized tray item in its function. A new variable |is_initialized_| is introduced to keep track the state of tray items in SatusAreaWidget class. TEST=SystemTrayTest.SystemTrayInitialization BUG=671293 ========== to ========== Fix Chrome crashes on calling OnTrayVisibilityChange When status area widget is initialized in Chrome Book with stylus pen, StatusAreaWidget::CreateTrayViews will call AddPaletteTray() to initialize a |palette_tray_| (which controls the stylus pen) and update this tray's visibility. The problem is that the |logout_button_tray_| is not initialized yet (it's initialized after the |palette_tray_|) and is used in the visibility update function in |palette_tray_|. As a result, Chrome OS crashes on machine with a stylus pen only. BUG=671293 ==========
Description was changed from ========== Fix Chrome crashes on calling OnTrayVisibilityChange When status area widget is initialized in Chrome Book with stylus pen, StatusAreaWidget::CreateTrayViews will call AddPaletteTray() to initialize a |palette_tray_| (which controls the stylus pen) and update this tray's visibility. The problem is that the |logout_button_tray_| is not initialized yet (it's initialized after the |palette_tray_|) and is used in the visibility update function in |palette_tray_|. As a result, Chrome OS crashes on machine with a stylus pen only. BUG=671293 ========== to ========== Fix Chrome crashes on calling OnTrayVisibilityChange When status area widget is initialized in Chrome Book with stylus pen, StatusAreaWidget::CreateTrayViews will call AddPaletteTray() to initialize a |palette_tray_| (which controls the stylus pen) and update this tray's visibility. The problem is that the |logout_button_tray_| is not initialized yet (it's initialized after the |palette_tray_|) and is used in the visibility update function in |palette_tray_|. As a result, Chrome OS crashes on machine with a stylus pen only. This crash happened because the order of the tray initialization is changed in cl: https://codereview.chromium.org/2427313003 BUG=671293 ==========
As I am finally able to reproduce and diagnose the issue. I changed my approach. Could you please take another look at this change? By the way, as the |logout_button_tray_| is updated to be the left most tray in the status area widget, the logic that i have on deciding whether to show the separator is no longer needed. The dependency of a |tray| to |logout_button_tray_| will be removed. I will clean up the logic in the next cl.
The CQ bit was checked by yiyix@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.
lgtm
@dcastagna, thank you for testing this patch on a chrome book with stylus pen! It stops hitting the break point. @sadrul, could you please review this change?
I think we should have a test. Maybe you can look at that after this lands. lgtm
The CQ bit was checked by yiyix@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": 80001, "attempt_start_ts": 1481818009040490,
"parent_rev": "50b56164482f50fe49b5c7f6c6ebe5a4110201ee", "commit_rev":
"3242846a49559dd6871075f4952d1689f866e859"}
Message was sent while issue was closed.
Description was changed from ========== Fix Chrome crashes on calling OnTrayVisibilityChange When status area widget is initialized in Chrome Book with stylus pen, StatusAreaWidget::CreateTrayViews will call AddPaletteTray() to initialize a |palette_tray_| (which controls the stylus pen) and update this tray's visibility. The problem is that the |logout_button_tray_| is not initialized yet (it's initialized after the |palette_tray_|) and is used in the visibility update function in |palette_tray_|. As a result, Chrome OS crashes on machine with a stylus pen only. This crash happened because the order of the tray initialization is changed in cl: https://codereview.chromium.org/2427313003 BUG=671293 ========== to ========== Fix Chrome crashes on calling OnTrayVisibilityChange When status area widget is initialized in Chrome Book with stylus pen, StatusAreaWidget::CreateTrayViews will call AddPaletteTray() to initialize a |palette_tray_| (which controls the stylus pen) and update this tray's visibility. The problem is that the |logout_button_tray_| is not initialized yet (it's initialized after the |palette_tray_|) and is used in the visibility update function in |palette_tray_|. As a result, Chrome OS crashes on machine with a stylus pen only. This crash happened because the order of the tray initialization is changed in cl: https://codereview.chromium.org/2427313003 BUG=671293 Review-Url: https://codereview.chromium.org/2557593009 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix Chrome crashes on calling OnTrayVisibilityChange When status area widget is initialized in Chrome Book with stylus pen, StatusAreaWidget::CreateTrayViews will call AddPaletteTray() to initialize a |palette_tray_| (which controls the stylus pen) and update this tray's visibility. The problem is that the |logout_button_tray_| is not initialized yet (it's initialized after the |palette_tray_|) and is used in the visibility update function in |palette_tray_|. As a result, Chrome OS crashes on machine with a stylus pen only. This crash happened because the order of the tray initialization is changed in cl: https://codereview.chromium.org/2427313003 BUG=671293 Review-Url: https://codereview.chromium.org/2557593009 ========== to ========== Fix Chrome crashes on calling OnTrayVisibilityChange When status area widget is initialized in Chrome Book with stylus pen, StatusAreaWidget::CreateTrayViews will call AddPaletteTray() to initialize a |palette_tray_| (which controls the stylus pen) and update this tray's visibility. The problem is that the |logout_button_tray_| is not initialized yet (it's initialized after the |palette_tray_|) and is used in the visibility update function in |palette_tray_|. As a result, Chrome OS crashes on machine with a stylus pen only. This crash happened because the order of the tray initialization is changed in cl: https://codereview.chromium.org/2427313003 BUG=671293 Committed: https://crrev.com/e422d9c3ef94d97c426160937f9b7422987c7d18 Cr-Commit-Position: refs/heads/master@{#438840} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e422d9c3ef94d97c426160937f9b7422987c7d18 Cr-Commit-Position: refs/heads/master@{#438840} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
