Logs & Screenshot: https://pantheon.corp.google.com/storage/browser/chromiumos-test-logs/bugfiles/cr/712422/

pgangishetty@, does it repro with different user account(s)? Guest user?

1. Yes, it is reproducible with another user account.
2. Guest user scenario is not applicable here, as there is no log out option.  

It is a lock/unlock scenario and not logout/login

Does it always reproduce no matter what you do before locking.
Can you check if it reproduces
- with other boards
- when no audio or video are played
- with no browser window is open

Yes, it is reproducible:
- with other board(s) (tried on Squawks)
- when no audio or video is played
- with no browser window is open

crash/6a26e2f690000000

0x00007f7fa2ad75d7	(chrome -bind_helpers.h:274 )	Run
0x00007f7fa6375f5a	(chrome + 0x047bdf5a )	OnLayerAnimationAborted
0x00007f7fa573b773	(chrome -layer_animation_sequence.cc:279 )	NotifyEnded
0x00007f7fa573f30c	(chrome -layer_animator.cc:447 )	FinishAnimation
0x00007f7fa573fa9b	(chrome -layer_animator.cc:478 )	Step
0x00007f7fa573de85	(chrome -layer_animator.cc:866 )	StartSequenceImmediately
0x00007f7fa573ec4b	(chrome -layer_animator.cc:807 )	ProcessQueue
0x00007f7fa573f31b	(chrome -layer_animator.cc:597 )	FinishAnimation
0x00007f7fa573fa9b	(chrome -layer_animator.cc:478 )	Step
0x00007f7fa573de85	(chrome -layer_animator.cc:866 )	StartSequenceImmediately
0x00007f7fa573ec4b	(chrome -layer_animator.cc:807 )	ProcessQueue
0x00007f7fa573f31b	(chrome -layer_animator.cc:597 )	FinishAnimation
0x00007f7fa573fa9b	(chrome -layer_animator.cc:478 )	Step
0x00007f7fa5741660	(chrome -layer_animator_collection.cc:54 )	OnAnimationStep
0x00007f7fa5734923	(chrome -compositor.cc:463 )	BeginMainFrame
0x00007f7fa55b9b9b	(chrome -single_thread_proxy.cc:691 )	DoBeginMainFrame
0x00007f7fa55ba35e	(chrome -single_thread_proxy.cc:661 )	BeginMainFrame

Observed the same on Kevin with ToT build version: 59.0.3070.0/9469.0.0

This started happening after CL: https://codereview.chromium.org/2801333002 landed.

The problem is the following:

- When we have multiple displays, we create a CallbackAnimationObserver per each LOCK_SCREEN_CONTAINER in each display. [1]

- The CallbackAnimationObserver copies the callback to invoke it later when animation ends. [2]

- The problem is with this auto-generated-code mojo callback. It doesn't expect to be invoked more than once. (The code is not yet on CodeSearch, so I'll paste it here):


  static SessionController::RunUnlockAnimationCallback CreateCallback(
      uint64_t request_id,
      bool is_sync,
      std::unique_ptr<mojo::MessageReceiverWithStatus> responder) {
    std::unique_ptr<SessionController_RunUnlockAnimation_ProxyToResponder> proxy(
        new SessionController_RunUnlockAnimation_ProxyToResponder(
            request_id, is_sync, std::move(responder)));
    return base::Bind(&SessionController_RunUnlockAnimation_ProxyToResponder::Run,
                      base::Passed(&proxy));
  }

......

      SessionController::RunUnlockAnimationCallback callback =
          SessionController_RunUnlockAnimation_ProxyToResponder::CreateCallback(
              message->request_id(),
              message->has_flag(mojo::Message::kFlagIsSync),
              std::move(responder));
      // A null |impl| means no implementation was bound.
      assert(impl);
      TRACE_EVENT0("mojom", "SessionController::RunUnlockAnimation");
      mojo::internal::MessageDispatchContext context(message);
      impl->RunUnlockAnimation(std::move(callback));


That base::Passed(&proxy) is the problem. The second time we invoke the callback, we fail the below CHECK():


[36461:36461:0418/171429.771683:FATAL:bind_helpers.h(274)] Check failed: is_valid_. 
#0 0x2af2c5d79efb base::debug::StackTrace::StackTrace()
#1 0x2af2c5d78c3c base::debug::StackTrace::StackTrace()
#2 0x2af2c5de523c logging::LogMessage::~LogMessage()
#3 0x2af2d683c487 base::internal::PassedWrapper<>::Take()
#4 0x2af2d683c3cc base::BindUnwrapTraits<>::Unwrap()
#5 0x2af2d683c22f _ZN4base8internal6UnwrapIRKNS0_13PassedWrapperISt10unique_ptrIN3ash5mojom53SessionController_RunUnlockAnimation_ProxyToResponderESt14default_deleteIS6_EEEEEEDTclsr9UnwrapperIT_EE6Unwrapclsr3stdE7forwardISD_Efp_EEEOSD_
#6 0x2af2d683c17a _ZN4base8internal7InvokerINS0_9BindStateIMN3ash5mojom53SessionController_RunUnlockAnimation_ProxyToResponderEFvvEJNS0_13PassedWrapperISt10unique_ptrIS5_St14default_deleteIS5_EEEEEEEFvvEE7RunImplIRKS7_RKSt5tupleIJSD_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#7 0x2af2d683c0cc _ZN4base8internal7InvokerINS0_9BindStateIMN3ash5mojom53SessionController_RunUnlockAnimation_ProxyToResponderEFvvEJNS0_13PassedWrapperISt10unique_ptrIS5_St14default_deleteIS5_EEEEEEEFvvEE3RunEPNS0_13BindStateBaseE
#8 0x2af2d47c9d0d _ZNKR4base8CallbackIFvvELNS_8internal8CopyModeE1ELNS2_10RepeatModeE1EE3RunEv
#9 0x2af2d4b4dc55 ash::(anonymous namespace)::CallbackAnimationObserver::OnLayerAnimationEnded()
#10 0x2af2d08a26a4 ui::LayerAnimationSequence::NotifyEnded()
#11 0x2af2d08a29ad ui::LayerAnimationSequence::ProgressToEnd()
#12 0x2af2d08adc4f ui::LayerAnimator::ProgressAnimationToEnd()
#13 0x2af2d08ad5d0 ui::LayerAnimator::FinishAnimation()
#14 0x2af2d08ae018 ui::LayerAnimator::Step()
#15 0x2af2d08abd56 ui::LayerAnimator::StartSequenceImmediately()
#16 0x2af2d08ac9ce ui::LayerAnimator::ProcessQueue()
#17 0x2af2d08ad5f8 ui::LayerAnimator::FinishAnimation()
#18 0x2af2d08ae018 ui::LayerAnimator::Step()
#19 0x2af2d08abd56 ui::LayerAnimator::StartSequenceImmediately()
#20 0x2af2d08ac9ce ui::LayerAnimator::ProcessQueue()
#21 0x2af2d08ad5f8 ui::LayerAnimator::FinishAnimation()
#22 0x2af2d08ae018 ui::LayerAnimator::Step()
#23 0x2af2d08b86d4 ui::LayerAnimatorCollection::OnAnimationStep()
#24 0x2af2d085e0ae ui::Compositor::BeginMainFrame()
#25 0x2af2cfa9ba16 cc::LayerTreeHost::BeginMainFrame()
#26 0x2af2cfb711a0 cc::SingleThreadProxy::DoBeginMainFrame()
#27 0x2af2cfb71f18 cc::SingleThreadProxy::BeginMainFrame()



I'm not very familiar with mojo, so how can we tweak this auto generated code, or work around it?


[1] https://cs.chromium.org/chromium/src/ash/wm/session_state_animator_impl.cc?q=SessionStateAnimatorImpl::StartAnimationWithCallback+package:%5Echromium$&l=554

[2] https://cs.chromium.org/chromium/src/ash/wm/session_state_animator_impl.cc?q=CallbackAnimationObserver&l=298

If we don't expect to create a ScreenLocker per each lock_screen_container in each display, then we probably don't need to create an animation observer for each container that will trigger ScreenLocker::ScheduleDeletion() on each animation ended.

My bad. Mojo callback is not expected to be called multiple times. There should be only one ScreenLocker even with multiple displays. But we would have multiple lock containers, one on each display.

If you have not started a fix, I could take this since I am still working the code around the lock screen.

I confirmed that your CL fixes the issue, so assigning to you! :)

Thanks afakhry@ for the detailed analysis and try out the CL.

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

commit 52558bf8b2796a8e97c73a2351e23a1f8c45e423
Author: xiyuan <xiyuan@chromium.org>
Date: Wed Apr 19 18:09:19 2017

cros: Fix unlock crash with multi-display

SessionController::RunUnlockAnimation expects the callback to be
invoked only once. SessionStateAnimatorImpl uses the callback per
animation per container. Mulit-display would have multiple containers
involved hence multiple animation and callbacks. The multiple
callback would cause a mojo CHECK failure. SessionStateAnimator
should only run the callback once after all animations finish.
The CL use a BarrierClosure to combine the animation callbacks.

BUG= 712422 
TEST=SessionStateAnimatiorImplContainersTest.AnimationCallbackOnMultiDisplay

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

[modify] https://crrev.com/52558bf8b2796a8e97c73a2351e23a1f8c45e423/ash/wm/session_state_animator.h
[modify] https://crrev.com/52558bf8b2796a8e97c73a2351e23a1f8c45e423/ash/wm/session_state_animator_impl.cc
[modify] https://crrev.com/52558bf8b2796a8e97c73a2351e23a1f8c45e423/ash/wm/session_state_animator_impl_unittest.cc

Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit b2a474304aa38cbdf585d72f1600eabd82010914
Author: Xiyuan Xia <xiyuan@chromium.org>
Date: Thu Apr 20 18:26:24 2017

Merge "cros: Fix unlock crash with multi-display"

> SessionController::RunUnlockAnimation expects the callback to be
> invoked only once. SessionStateAnimatorImpl uses the callback per
> animation per container. Mulit-display would have multiple containers
> involved hence multiple animation and callbacks. The multiple
> callback would cause a mojo CHECK failure. SessionStateAnimator
> should only run the callback once after all animations finish.
> The CL use a BarrierClosure to combine the animation callbacks.
>
> BUG= 712422 
> TEST=SessionStateAnimatiorImplContainersTest.AnimationCallbackOnMultiDisplay
>
> Review-Url: https://codereview.chromium.org/2823343006
> Cr-Commit-Position: refs/heads/master@{#465670}
> (cherry picked from commit 52558bf8b2796a8e97c73a2351e23a1f8c45e423)

Review-Url: https://codereview.chromium.org/2831703003 .
Cr-Commit-Position: refs/branch-heads/3071@{#91}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/b2a474304aa38cbdf585d72f1600eabd82010914/ash/wm/session_state_animator.h
[modify] https://crrev.com/b2a474304aa38cbdf585d72f1600eabd82010914/ash/wm/session_state_animator_impl.cc
[modify] https://crrev.com/b2a474304aa38cbdf585d72f1600eabd82010914/ash/wm/session_state_animator_impl_unittest.cc

Verified with version 59.0.3071.25/9460.11.0