New issue
Advanced search Search tips

Issue 829931 link

Starred by 1 user
Status: Fixed
Owner:
jamescook@chromium.org
Closed: Apr 2018
Cc:
xiy...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

ScopedSessionObserver should not check Shell::HasInstance

Project Member Reported by jamescook@chromium.org, Apr 6 2018 
If you initialize a ScopedSessionObserver too early during ash::Shell initialization (e.g. before the Shell instance is set) then it will silently fail to observe session changes. This is easy to do if you add a new controller object to ash and try to construct in the Shell member initializer list.

This is because of a Shell::HasInstance() test in the observer. It should probably be a DCHECK().

Changing this will require fixing these tests:

    LogoutConfirmationControllerTest.DurationExpired (../../ash/system/session/logout_confirmation_controller_unittest.cc:67)
    LogoutConfirmationControllerTest.DurationExpiredAfterDeniedRequest (../../ash/system/session/logout_confirmation_controller_unittest.cc:144)
    LogoutConfirmationControllerTest.DurationExtended (../../ash/system/session/logout_confirmation_controller_unittest.cc:97)
    LogoutConfirmationControllerTest.DurationShortened (../../ash/system/session/logout_confirmation_controller_unittest.cc:80)
    LogoutConfirmationControllerTest.Lock (../../ash/system/session/logout_confirmation_controller_unittest.cc:111)
    LogoutConfirmationControllerTest.UserAccepted (../../ash/system/session/logout_confirmation_controller_unittest.cc:122)
    LogoutConfirmationControllerTest.UserDenied (../../ash/system/session/logout_confirmation_controller_unittest.cc:132)
    LogoutConfirmationControllerTest.ZeroDuration (../../ash/system/session/logout_confirmation_controller_unittest.cc:59)
    ShelfBackgroundAnimatorTest.AnimatorDestroyedWhenChangingBackgroundImmediately (../../ash/shelf/shelf_background_animator_unittest.cc:276)
    ShelfBackgroundAnimatorTest.AnimatorIsDetroyedWhenCompletingSuccessfully (../../ash/shelf/shelf_background_animator_unittest.cc:268)
    ShelfBackgroundAnimatorTest.BackgroundTypesWhenAnimatingToSameTarget (../../ash/shelf/shelf_background_animator_unittest.cc:176)
    ShelfBackgroundAnimatorTest.DefaultBackground (../../ash/shelf/shelf_background_animator_unittest.cc:223)
    ShelfBackgroundAnimatorTest.ExistingAnimatorIsReusedWhenAnimatingToPreviousState (../../ash/shelf/shelf_background_animator_unittest.cc:286)
    ShelfBackgroundAnimatorTest.ExistingAnimatorNotReusedWhenTargetBackgroundNotPreviousBackground (../../ash/shelf/shelf_background_animator_unittest.cc:301)
    ShelfBackgroundAnimatorTest.FullscreenAppListBackground (../../ash/shelf/shelf_background_animator_unittest.cc:259)
    ShelfBackgroundAnimatorTest.MaximizedBackground (../../ash/shelf/shelf_background_animator_unittest.cc:241)
    ShelfBackgroundAnimatorTest.MultipleAnimateCallsToSameTargetAreIgnored (../../ash/shelf/shelf_background_animator_unittest.cc:192)
    ShelfBackgroundAnimatorTest.ObserversAreNotifiedWhenSnappingToSameTargetBackground (../../ash/shelf/shelf_background_animator_unittest.cc:316)
    ShelfBackgroundAnimatorTest.ObserversUpdatedWhenAdded (../../ash/shelf/shelf_background_animator_unittest.cc:212)
    ShelfBackgroundAnimatorTest.OverlapBackground (../../ash/shelf/shelf_background_animator_unittest.cc:232)
    ShelfBackgroundAnimatorTest.SplitViewBackground (../../ash/shelf/shelf_background_animator_unittest.cc:250)

Comment 1 by jamescook@chromium.org, Apr 19 2018

Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 20 2018 

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

commit 934ba81c6c8f34a52edf9d6ffdd376895cc8a566
Author: James Cook <jamescook@chromium.org>
Date: Fri Apr 20 14:17:35 2018

cros: DCHECK ash::Shell::HasInstance in ash::ScopedSessionObserver

If you initialize a ScopedSessionObserver too early during ash::Shell
initialization (e.g. before the Shell instance is set) then it will
silently fail to observe session changes. This is easy to do if you add
a new controller object to ash and try to construct in the Shell member
initializer list.

Change the HasInstance() test to a DCHECK.

Bug:  829931 
Test: ash_unittests, unit_tests, browser_tests
Change-Id: Ibb31638c8f2b4868b8de56b90e6f6a97200c4c5c
Reviewed-on: https://chromium-review.googlesource.com/1020462
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552325}
[modify] https://crrev.com/934ba81c6c8f34a52edf9d6ffdd376895cc8a566/ash/session/session_observer.cc
[modify] https://crrev.com/934ba81c6c8f34a52edf9d6ffdd376895cc8a566/ash/session/session_observer.h
[modify] https://crrev.com/934ba81c6c8f34a52edf9d6ffdd376895cc8a566/ash/shelf/shelf_background_animator.cc
[modify] https://crrev.com/934ba81c6c8f34a52edf9d6ffdd376895cc8a566/ash/shelf/shelf_background_animator.h
[modify] https://crrev.com/934ba81c6c8f34a52edf9d6ffdd376895cc8a566/ash/shell.cc
[modify] https://crrev.com/934ba81c6c8f34a52edf9d6ffdd376895cc8a566/ash/system/session/logout_confirmation_controller.cc
[modify] https://crrev.com/934ba81c6c8f34a52edf9d6ffdd376895cc8a566/ash/system/session/logout_confirmation_controller.h

Comment 3 by jamescook@chromium.org, Apr 20 2018

Status: Fixed (was: Started)
►

Sign in to add a comment