GitLab

The completion block added to FullscreenAnimator updates the
FullscreenModel and cleans up state in the FullscreenMediator.
FullscreenMediator calls StopAnimating() in its Disconnect() function,
which is guaranteed to be executed before deallocation.  This calls
into FullscreenAnimator's |-stopAnimation:| with a |withoutFinishing|
value of YES, which according to the UIViewAnimating protocol, should
immediately stop the animation.  However, UIViewPropertyAnimator's
implementation does not guarantee that the completion blocks are
called synchronously, so there is a possibility that the completion is
executed after the mediator's destruction.

The previous implementation of AnimateWithStyle() used a pointer to
the FullscreenModel to attempt to prevent crashes here.  However, this
is incorrect, as the raw pointer value is used. When the mediator is
disconnected, it resets the |model_| ivar to nullptr, so when the
pointer value is checked in the completion block, it was a use-after-
free error.  It only worked because the ivar was reset, but this
behavior was non-deterministic, as the memory can be reallocated
before the completion block is executed.

Instead, this CL updates FullscreenMediator to support weak pointers,
and uses a weak pointer from within the completion and animation blocks
to prevent use-after-free issues.  Additionally, this CL cleans up
FullscreenMediator::AnimateWithStyle() to early return before creating
the animator if there is no fullscreen progress update, preventing the
animation observer callbacks from being executed unnecessarily.

Bug: 1071777
Change-Id: I75aa1a54899d99befcdba140a2fefd846d785bc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2155071
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Auto-Submit: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760576}