New issue
Advanced search Search tips

Issue 700255 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Clean up StatusAreaWidget::Shutdown() to avoid manually resetting tray pointers

Project Member Reported by jamescook@chromium.org, Mar 10 2017

Issue description

It manually deletes and nulls out each foo_tray_ pointer with a comment about tests:

  // Destroy the trays early, causing them to be removed from the view
  // hierarchy. Do not used scoped pointers since we don't want to destroy them
  // in the destructor if Shutdown() is not called (e.g. in tests).

However, if you miss one you end up with layout-related crashes at shutdown. I've added a DCHECK to ensure we clean them up, but a better thing to do would be to fix the tests, fix the layout problems, and switch to unique_ptr<>.

See issue 700122

 
Status: Started (was: Assigned)
There are other ordering dependencies, unfortunately, but switching to unique_ptr does make things clearer, and I've eliminated the separate Shutdown() pass.

https://chromium-review.googlesource.com/c/chromium/src/+/902819
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2240672cc3ce761d75a3d77baf692af1c9993e40

commit 2240672cc3ce761d75a3d77baf692af1c9993e40
Author: James Cook <jamescook@chromium.org>
Date: Tue Feb 06 01:48:20 2018

cros: Clean up StatusAreaWidget teardown

ShelfWidget and StatusAreaWidget have a complex shutdown dance.
Eliminate the StatusAreaWidget::Shutdown() pass. Make StatusAreaWidget
and its child views explicitly owned to make the lifetime management
more clear.

multiple monitors, shutdown with system tray bubble open

Bug:  700255 
Test: ash_unittests, manually test add/remove monitor, shutdown with
Change-Id: Ib218cdafe27799945760056d7bac8ce06f795f12
Reviewed-on: https://chromium-review.googlesource.com/902819
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534579}
[modify] https://crrev.com/2240672cc3ce761d75a3d77baf692af1c9993e40/ash/shelf/shelf_widget.cc
[modify] https://crrev.com/2240672cc3ce761d75a3d77baf692af1c9993e40/ash/shelf/shelf_widget.h
[modify] https://crrev.com/2240672cc3ce761d75a3d77baf692af1c9993e40/ash/system/status_area_layout_manager.cc
[modify] https://crrev.com/2240672cc3ce761d75a3d77baf692af1c9993e40/ash/system/status_area_widget.cc
[modify] https://crrev.com/2240672cc3ce761d75a3d77baf692af1c9993e40/ash/system/status_area_widget.h
[modify] https://crrev.com/2240672cc3ce761d75a3d77baf692af1c9993e40/ash/system/status_area_widget_delegate.cc
[modify] https://crrev.com/2240672cc3ce761d75a3d77baf692af1c9993e40/ui/views/bubble/tray_bubble_view.cc

Status: Fixed (was: Started)

Sign in to add a comment