New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 875031 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 23
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Shutdown crash in ~ArcNotificationManager

Project Member Reported by jamescook@chromium.org, Aug 16

Issue description

M69 manually built and deployed to caroline:

* Shortly after sign-in, click on system tray, then tap the shutdown icon with your finger
* Shutdown crash:

#0 0x59c0d693695c base::debug::StackTrace::StackTrace()
#1 0x59c0d69364c1 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x736fde7512e0 <unknown>
#3 0x59c0d8b54da8 ash::ArcNotificationManager::~ArcNotificationManager()
#4 0x59c0d8b54fce ash::ArcNotificationManager::~ArcNotificationManager()
#5 0x59c0d8a787a2 ash::MessageCenterController::~MessageCenterController()
#6 0x59c0d8a788be ash::MessageCenterController::~MessageCenterController()
#7 0x59c0d89959c4 ash::Shell::~Shell()
#8 0x59c0d8996e2e ash::Shell::~Shell()
#9 0x59c0d8d689fa ChromeBrowserMainExtraPartsAsh::PostMainMessageLoopRun()
#10 0x59c0d659a25a ChromeBrowserMainParts::PostMainMessageLoopRun()
#11 0x59c0d589a3f6 chromeos::ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun()
#12 0x59c0d4e45354 content::BrowserMainLoop::ShutdownThreadsAndCleanUp()
#13 0x59c0d4e47c03 content::BrowserMainRunnerImpl::Shutdown()
#14 0x59c0d4e415ef content::BrowserMain()
#15 0x59c0d6589952 content::ContentMainRunnerImpl::Run()
#16 0x59c0d65908dc service_manager::Main()
#17 0x59c0d6587921 content::ContentMain()
#18 0x59c0d4308b3f ChromeMain

I might have had open notifications at the time.

I suspect the problem is that ArcNotificationManager holds a raw pointer to the message center.

MessageCenterController::~MessageCenterController() {
  all_popup_blocker_.reset();
  session_state_notification_blocker_.reset();
  inactive_user_notification_blocker_.reset();
  fullscreen_notification_blocker_.reset();
  // *** should delete arc_notification_manager_ here ***

  message_center::MessageCenter::Shutdown();
  // *** actually deleted here ***
}

estade's CL might have caused this, https://chromium-review.googlesource.com/c/chromium/src/+/940186

 
The suggested fix sgtm.

I should not have cached the raw pointer. :( It is misleading. The crash happens after https://chromium-review.googlesource.com/c/chromium/src/+/1111495 that do the message_center_->RemoveObserver in ArcNotificationManager dtor.
Owner: est...@chromium.org
Status: Started (was: Untriaged)
I think the proposed fix looks correct. Thanks for report/analysis.

It looks like we're getting crash reports for this but just not very many: https://crash.corp.google.com/browse?q=stable_signature%3D%27ash%3A%3AArcNotificationManager%3A%3A~ArcNotificationManager-15e55251%27 

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 17

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

commit d4c2f4e729bb4fb26571f681d4b38e839fd37c29
Author: Evan Stade <estade@chromium.org>
Date: Fri Aug 17 16:18:49 2018

Destroy ArcNotificationManager before shutting down MessageCenter.

credit to jamescook@ for the fix

Bug:  875031 
Change-Id: I4f1f0c10e45d768da0515400d0666a08937c004b
Reviewed-on: https://chromium-review.googlesource.com/1178935
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584094}
[modify] https://crrev.com/d4c2f4e729bb4fb26571f681d4b38e839fd37c29/ash/message_center/message_center_controller.cc

Labels: Merge-Request-69
Project Member

Comment 5 by sheriffbot@chromium.org, Aug 18

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-69 Merge-Approved-69
Merge approved, M69.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 20

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e5dc53a5f477086a20717c5a359e17ca0b0bb20f

commit e5dc53a5f477086a20717c5a359e17ca0b0bb20f
Author: Evan Stade <estade@chromium.org>
Date: Mon Aug 20 17:09:08 2018

Destroy ArcNotificationManager before shutting down MessageCenter.

credit to jamescook@ for the fix

Bug:  875031 
Change-Id: I4f1f0c10e45d768da0515400d0666a08937c004b
Reviewed-on: https://chromium-review.googlesource.com/1178935
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#584094}(cherry picked from commit d4c2f4e729bb4fb26571f681d4b38e839fd37c29)
Reviewed-on: https://chromium-review.googlesource.com/1181583
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#715}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/e5dc53a5f477086a20717c5a359e17ca0b0bb20f/ash/message_center/message_center_controller.cc

Status: Fixed (was: Started)

Sign in to add a comment