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

Issue 660889 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Crash in FullscreenNotificationBlocker::ShouldShowNotificationAsPopup on R55

Project Member Reported by bhthompson@google.com, Oct 31 2016

Issue description

Chrome Version: 55.0.2883.29
Chrome OS Version: 8872.27.0
Chrome OS Platform: Cyan 

We are seeing new crashes in 55.0.2883.29 over the prior dev channel push.

Report ID1f52e04700000000
Product, versionChrome_ChromeOS, 55.0.2883.29
Process typebrowser
Magic SignatureFullscreenNotificationBlocker::ShouldShowNotificationAsPopupedit bugs&comments
Stable SignatureFullscreenNotificationBlocker::ShouldShowNotificationAsPopup-7606ec4aedit bugs&comments
Report TimeMon, 31 Oct 2016 11:55:07 GMT
Uptime8465194 ms
Client IDfbd223f8b7cc4b8fab797fb45fffbf2e
Filesminidumpchrome.txti915_error_state.log.xz
In shutdownfalse
Device Modelcyan-signed-mp-v2keys
Thread 0 CRASHED [SIGSEGV @ 0x00000000 ] MAGIC SIGNATURE THREAD
0x00007fe7ef38dd4f	(chrome -fullscreen_notification_blocker.cc:75 )	FullscreenNotificationBlocker::ShouldShowNotificationAsPopup
0x00007fe7f0491aac	(chrome -notification_list.cc:28 )	message_center::NotificationList::HasPopupNotifications
0x00007fe7f0490741	(chrome -message_center_tray.cc:258 )	message_center::MessageCenterTray::OnMessageCenterChanged
0x00007fe7f048ecfc	(chrome -message_center_impl.cc:550 )	message_center::MessageCenterImpl::AddNotificationImmediately
0x00007fe7f048ee30	(chrome -message_center_impl.cc:531 )	message_center::MessageCenterImpl::AddNotification
0x00007fe7f0b93f1d	(chrome -battery_notification.cc:95 )	ash::BatteryNotification::BatteryNotification
0x00007fe7f0b44601	(chrome -tray_power.cc:208 )	ash::TrayPower::OnPowerStatusChanged
0x00007fe7f0ac9b13	(chrome -power_status.cc:514 )	ash::PowerStatus::PowerChanged
0x00007fe7efbf449b	(chrome -power_manager_client.cc:479 )	chromeos::PowerManagerClientImpl::HandlePowerSupplyProperties
0x00007fe7efbf4db2	(chrome -power_manager_client.cc:434 )	chromeos::PowerManagerClientImpl::PowerSupplyPollReceived
0x00007fe7efc29023	(chrome -callback.h:64 )	dbus::ObjectProxy::RunMethod
0x00007fe7efc29f02	(chrome -bind_internal.h:214 )	base::internal::Invoker<base::internal::BindState<void (dbus::ObjectProxy::*)(base::TimeTicks, std::vector<base::Callback<void(dbus::Signal*), (base::internal::CopyMode)1, (base::internal::RepeatMode)1>, std::allocator<base::Callback<void(dbus::Signal*), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> > >, dbus::Signal*), scoped_refptr<dbus::ObjectProxy>, base::TimeTicks, std::vector<base::Callback<void(dbus::Signal*), (base::internal::CopyMode)1, (base::internal::RepeatMode)1>, std::allocator<base::Callback<void(dbus::Signal*), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> > >, dbus::Signal*>, void()>::Run
0x00007fe7edbb854c	(chrome -callback.h:64 )	base::debug::TaskAnnotator::RunTask
0x00007fe7edba7616	(chrome -message_loop.cc:405 )	base::MessageLoop::DoWork
0x00007fe7edba7eb2	(chrome -message_pump_libevent.cc:217 )	base::MessagePumpLibevent::Run
0x00007fe7ef40ae07	(chrome -run_loop.cc:35 )	base::RunLoop::Run
0x00007fe7ef0f5a24	(chrome -chrome_browser_main.cc:2116 )	ChromeBrowserMainParts::MainMessageLoopRun
0x00007fe7ee7ca34a	(chrome -browser_main_loop.cc:981 )	content::BrowserMainLoop::RunMainMessageLoopParts
0x00007fe7ee7cc044	(chrome -browser_main_runner.cc:155 )	content::BrowserMainRunnerImpl::Run
0x00007fe7ee7c6c3b	(chrome -browser_main.cc:46 )	content::BrowserMain
0x00007fe7ef098050	(chrome -content_main_runner.cc:779 )	content::ContentMainRunnerImpl::Run
0x00007fe7ef096bea	(chrome -content_main.cc:20 )	content::ContentMain
0x00007fe7ede1d195	(chrome -chrome_main.cc:97 )	ChromeMain
0x00007fe7eb476fb5	(libc-2.19.so -libc-start.c:292 )	__libc_start_main
0x00007fe7ede1cfe4	(chrome + 0x011c6fe4 )	_start
0x00007fff9a3e58f7	

Justin, could you take a look at this? It looks like the code around here most recently changed by your CL https://chromium.googlesource.com/chromium/src.git/+/a29730328a155605b4ab7c3def12e8830397a9c2
 

Comment 1 by peter@chromium.org, Oct 31 2016

Cc: bmalcolm@chromium.org
+bmalcolm
I see... The problem is that the battery notification is creating a notification with a NULL delegate, but ShouldShowNotificationAsPopup assumes that the delegate is non-null.
Status: Started (was: Unconfirmed)
https://codereview.chromium.org/2459263004 is in progress
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 31 2016

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

commit b58f87810fac6a4d5ef606032a35ed8e8b179bd2
Author: dewittj <dewittj@chromium.org>
Date: Mon Oct 31 21:36:44 2016

Fullscreen Notifications: Fix crash if delegate is NULL

Notification Delegates should never be NULL but it is not enforced with
a DCHECK, so of course there is a delegate that is null (for the ChromeOS
Battery notification).  So fix the logic that never checks the delegate's
validity in FullscreenNotificationBlocker.

R=bmalcolm@chromium.org
BUG= 660889 
TEST=have a fullscreen app window and drain the battery
     below the warning level.

Review-Url: https://codereview.chromium.org/2459263004
Cr-Commit-Position: refs/heads/master@{#428825}

[modify] https://crrev.com/b58f87810fac6a4d5ef606032a35ed8e8b179bd2/chrome/browser/notifications/fullscreen_notification_blocker.cc

Labels: Merge-Request-55
Status: Fixed (was: Started)
Labels: -Merge-Request-55 Merge-Approved-55
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 1 2016

Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3bd6317c8d9449690991c07ff4f1957449d70fee

commit 3bd6317c8d9449690991c07ff4f1957449d70fee
Author: Justin DeWitt <dewittj@chromium.org>
Date: Tue Nov 01 15:43:30 2016

Fullscreen Notifications: Fix crash if delegate is NULL

Notification Delegates should never be NULL but it is not enforced with
a DCHECK, so of course there is a delegate that is null (for the ChromeOS
Battery notification).  So fix the logic that never checks the delegate's
validity in FullscreenNotificationBlocker.

R=bmalcolm@chromium.org
BUG= 660889 
TEST=have a fullscreen app window and drain the battery
     below the warning level.

Review-Url: https://codereview.chromium.org/2459263004
Cr-Commit-Position: refs/heads/master@{#428825}
(cherry picked from commit b58f87810fac6a4d5ef606032a35ed8e8b179bd2)

Review URL: https://codereview.chromium.org/2463383002 .

Cr-Commit-Position: refs/branch-heads/2883@{#403}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/3bd6317c8d9449690991c07ff4f1957449d70fee/chrome/browser/notifications/fullscreen_notification_blocker.cc

Issue 661303 has been merged into this issue.
Status: Verified (was: Fixed)
55.0.2883.54/8872.54.0

Sign in to add a comment