Bug 173302 – REGRESSION (r217997): mint.com header renders incorrectly when initially loaded

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Jon Lee]

Load mint.com by typing it into the location field and pressing enter.

The header is white instead of being transparent. Hover over "Log In".

Then reload the page. It renders correctly.

[Radar WebKit Bug Importer]

<rdar://problem/32731747>

[Chris Dumez]

It is likely that a CSS transition animation is getting suspended but not resumed properly when the page realizes it is visible.

[Chris Dumez]

Animations logging shows:
Created ImplicitAnimation 0x11fec1870 on renderer 0x11fea9840 for property background-color duration 0.40 delay 0.00
Created ImplicitAnimation 0x120810000 on renderer 0x11fea9840 for property border-bottom-color duration 0.40 delay 0.00
Created ImplicitAnimation 0x120810630 on renderer 0x11fe61420 for property color duration 0.25 delay 0.00

[Chris Dumez]

Better logging:
Created ImplicitAnimation 0x1139bcc60 on renderer 0x112d09528 for property background-color duration 0.40 delay 0.00
Created ImplicitAnimation 0x1139bccf0 on renderer 0x112d09528 for property border-bottom-color duration 0.40 delay 0.00
0x1139bcc60 AnimationState New -> AnimationState::PausedNew
0x1139bccf0 AnimationState New -> AnimationState::PausedNew
Created ImplicitAnimation 0x1139bc360 on renderer 0x112f68738 for property color duration 0.25 delay 0.00
0x1139bc360 AnimationState New -> AnimationState::PausedNew
0x1139bcc60 AnimationState PausedNew -> PausedWaitResponse
0x1139bccf0 AnimationState PausedNew -> PausedWaitResponse
0x1139bc360 AnimationState PausedNew -> PausedWaitResponse
CompositeAnimation::resumeAnimations() 2 transitions
-> Transition 0x1139bcc60, hasStyle? 1
0x1139bcc60 - AnimationBase::updatePlayState(0) returns early, pause: 0, paused(): 0, isNew: 0
-> Transition 0x1139bccf0, hasStyle? 1
0x1139bccf0 - AnimationBase::updatePlayState(0) returns early, pause: 0, paused(): 0, isNew: 0
CompositeAnimation::resumeAnimations() 1 transitions
-> Transition 0x1139bc360, hasStyle? 1
0x1139bc360 - AnimationBase::updatePlayState(0) returns early, pause: 0, paused(): 0, isNew: 0


It looks like something at some point caused those ImplicitAnimations to switch from state PausedNew to PausedWaitResponse. As a result, when we later resume animations and call AnimationBase::updatePlayState(AnimPlayStatePlaying), that method returns early and fails to start the animation because of this check in updatePlayState():

    if (pause == paused() && !isNew()) {
        WTFLogAlways("%p - AnimationBase::updatePlayState(%d) returns early, pause: %d, paused(): %d, isNew: %d", this, static_cast<int>(playState), pause, paused(), isNew());
        return;
    }

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> Better logging:
> Created ImplicitAnimation 0x1139bcc60 on renderer 0x112d09528 for property
> background-color duration 0.40 delay 0.00
> Created ImplicitAnimation 0x1139bccf0 on renderer 0x112d09528 for property
> border-bottom-color duration 0.40 delay 0.00
> 0x1139bcc60 AnimationState New -> AnimationState::PausedNew
> 0x1139bccf0 AnimationState New -> AnimationState::PausedNew
> Created ImplicitAnimation 0x1139bc360 on renderer 0x112f68738 for property
> color duration 0.25 delay 0.00
> 0x1139bc360 AnimationState New -> AnimationState::PausedNew
> 0x1139bcc60 AnimationState PausedNew -> PausedWaitResponse
> 0x1139bccf0 AnimationState PausedNew -> PausedWaitResponse
> 0x1139bc360 AnimationState PausedNew -> PausedWaitResponse
> CompositeAnimation::resumeAnimations() 2 transitions
> -> Transition 0x1139bcc60, hasStyle? 1
> 0x1139bcc60 - AnimationBase::updatePlayState(0) returns early, pause: 0,
> paused(): 0, isNew: 0
> -> Transition 0x1139bccf0, hasStyle? 1
> 0x1139bccf0 - AnimationBase::updatePlayState(0) returns early, pause: 0,
> paused(): 0, isNew: 0
> CompositeAnimation::resumeAnimations() 1 transitions
> -> Transition 0x1139bc360, hasStyle? 1
> 0x1139bc360 - AnimationBase::updatePlayState(0) returns early, pause: 0,
> paused(): 0, isNew: 0
> 
> 
> It looks like something at some point caused those ImplicitAnimations to
> switch from state PausedNew to PausedWaitResponse. As a result, when we
> later resume animations and call
> AnimationBase::updatePlayState(AnimPlayStatePlaying), that method returns
> early and fails to start the animation because of this check in
> updatePlayState():
> 
>     if (pause == paused() && !isNew()) {
>         WTFLogAlways("%p - AnimationBase::updatePlayState(%d) returns early,
> pause: %d, paused(): %d, isNew: %d", this, static_cast<int>(playState),
> pause, paused(), isNew());
>         return;
>     }

Updating that if condition to use:
    if (pause == inPausedState() && !isNew()) {

fixes the issue. Simon, do you know if this is the best way to fix this? All these various paused states on AnimationBase are very confusing for me :/

[Chris Dumez]

0x1138af630 AnimationState PausedNew -> PausedWaitResponse (a)
1   0x1057ad9ab WebCore::ImplicitAnimation::reset(WebCore::RenderStyle const&, WebCore::CompositeAnimation&)
2   0x1057ad7fd WebCore::ImplicitAnimation::animate(WebCore::CompositeAnimation&, WebCore::RenderElement*, WebCore::RenderStyle const*, WebCore::RenderStyle const&, std::__1::unique_ptr<WebCore::RenderStyle, std::__1::default_delete<WebCore::RenderStyle> >&, bool&)
3   0x1052cfe0c WebCore::CompositeAnimation::animate(WebCore::RenderElement&, WebCore::RenderStyle const*, WebCore::RenderStyle const&, std::__1::unique_ptr<WebCore::RenderStyle, std::__1::default_delete<WebCore::RenderStyle> >&)
4   0x1053590e8 WebCore::CSSAnimationController::updateAnimations(WebCore::RenderElement&, WebCore::RenderStyle const&, std::__1::unique_ptr<WebCore::RenderStyle, std::__1::default_delete<WebCore::RenderStyle> >&)
5   0x1062312f7 WebCore::Style::TreeResolver::createAnimatedElementUpdate(std::__1::unique_ptr<WebCore::RenderStyle, std::__1::default_delete<WebCore::RenderStyle> >, WebCore::Element&, WebCore::Style::Change)
6   0x106231041 WebCore::Style::TreeResolver::resolveElement(WebCore::Element&)
7   0x1062318c7 WebCore::Style::TreeResolver::resolveComposedTree()
8   0x1062323e2 WebCore::Style::TreeResolver::resolve()
9   0x1054635c7 WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType)
10  0x10560e4c3 WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive()
11  0x105573d5b WebCore::FocusController::setActiveInternal(bool)
12  0x105573cf4 WebCore::FocusController::setActivityState(unsigned int)
13  0x105e5405e WebCore::Page::setActivityState(unsigned int)
14  0x103340a39 WebKit::WebPage::setActivityState(unsigned int, bool, WTF::Vector<unsigned long long, 0ul, WTF::CrashOnOverflow, 16ul> const&)
15  0x103361ccb void IPC::handleMessage<Messages::WebPage::SetActivityState, WebKit::WebPage, void (WebKit::WebPage::*)(unsigned int, bool, WTF::Vector<unsigned long long, 0ul, WTF::CrashOnOverflow, 16ul> const&)>(IPC::Decoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned int, bool, WTF::Vector<unsigned long long, 0ul, WTF::CrashOnOverflow, 16ul> const&))
16  0x1031c7ddd IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&)
17  0x1033c88c0 WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
18  0x1031872d1 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
19  0x103189e24 IPC::Connection::dispatchOneMessage()
20  0x108ac15ed WTF::RunLoop::performWork()
21  0x108ac1802 WTF::RunLoop::performWork(void*)
22  0x7fff9fd950b1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
23  0x7fff9fd7839f __CFRunLoopDoSources0
24  0x7fff9fd7794f __CFRunLoopRun
25  0x7fff9fd772b9 CFRunLoopRunSpecific
26  0x7fff9f0be93b RunCurrentEventLoopInMode
27  0x7fff9f0be6de ReceiveNextEventCommon
28  0x7fff9f0be45c _BlockUntilNextEventMatchingListInModeWithFilter
29  0x7fff9d3a0e2b _DPSNextEvent
30  0x7fff9db28d7e -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
31  0x7fff9d395cc6 -[NSApplication run]

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> 0x1138af630 AnimationState PausedNew -> PausedWaitResponse (a)
> 1   0x1057ad9ab WebCore::ImplicitAnimation::reset(WebCore::RenderStyle
> const&, WebCore::CompositeAnimation&)
> 2   0x1057ad7fd
> WebCore::ImplicitAnimation::animate(WebCore::CompositeAnimation&,
> WebCore::RenderElement*, WebCore::RenderStyle const*, WebCore::RenderStyle
> const&, std::__1::unique_ptr<WebCore::RenderStyle,
> std::__1::default_delete<WebCore::RenderStyle> >&, bool&)
> 3   0x1052cfe0c
> WebCore::CompositeAnimation::animate(WebCore::RenderElement&,
> WebCore::RenderStyle const*, WebCore::RenderStyle const&,
> std::__1::unique_ptr<WebCore::RenderStyle,
> std::__1::default_delete<WebCore::RenderStyle> >&)
> 4   0x1053590e8
> WebCore::CSSAnimationController::updateAnimations(WebCore::RenderElement&,
> WebCore::RenderStyle const&, std::__1::unique_ptr<WebCore::RenderStyle,
> std::__1::default_delete<WebCore::RenderStyle> >&)
> 5   0x1062312f7
> WebCore::Style::TreeResolver::createAnimatedElementUpdate(std::__1::
> unique_ptr<WebCore::RenderStyle,
> std::__1::default_delete<WebCore::RenderStyle> >, WebCore::Element&,
> WebCore::Style::Change)
> 6   0x106231041
> WebCore::Style::TreeResolver::resolveElement(WebCore::Element&)
> 7   0x1062318c7 WebCore::Style::TreeResolver::resolveComposedTree()
> 8   0x1062323e2 WebCore::Style::TreeResolver::resolve()
> 9   0x1054635c7
> WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType)
> 10  0x10560e4c3 WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive()
> 11  0x105573d5b WebCore::FocusController::setActiveInternal(bool)
> 12  0x105573cf4 WebCore::FocusController::setActivityState(unsigned int)
> 13  0x105e5405e WebCore::Page::setActivityState(unsigned int)
> 14  0x103340a39 WebKit::WebPage::setActivityState(unsigned int, bool,
> WTF::Vector<unsigned long long, 0ul, WTF::CrashOnOverflow, 16ul> const&)
> 15  0x103361ccb void IPC::handleMessage<Messages::WebPage::SetActivityState,
> WebKit::WebPage, void (WebKit::WebPage::*)(unsigned int, bool,
> WTF::Vector<unsigned long long, 0ul, WTF::CrashOnOverflow, 16ul>
> const&)>(IPC::Decoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned
> int, bool, WTF::Vector<unsigned long long, 0ul, WTF::CrashOnOverflow, 16ul>
> const&))
> 16  0x1031c7ddd IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&,
> IPC::Decoder&)
> 17  0x1033c88c0 WebKit::WebProcess::didReceiveMessage(IPC::Connection&,
> IPC::Decoder&)
> 18  0x1031872d1
> IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder,
> std::__1::default_delete<IPC::Decoder> >)
> 19  0x103189e24 IPC::Connection::dispatchOneMessage()
> 20  0x108ac15ed WTF::RunLoop::performWork()
> 21  0x108ac1802 WTF::RunLoop::performWork(void*)
> 22  0x7fff9fd950b1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
> 23  0x7fff9fd7839f __CFRunLoopDoSources0
> 24  0x7fff9fd7794f __CFRunLoopRun
> 25  0x7fff9fd772b9 CFRunLoopRunSpecific
> 26  0x7fff9f0be93b RunCurrentEventLoopInMode
> 27  0x7fff9f0be6de ReceiveNextEventCommon
> 28  0x7fff9f0be45c _BlockUntilNextEventMatchingListInModeWithFilter
> 29  0x7fff9d3a0e2b _DPSNextEvent
> 30  0x7fff9db28d7e -[NSApplication(NSEvent)
> _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
> 31  0x7fff9d395cc6 -[NSApplication run]

Issue seems to be because we sometimes call AnimationBase::updateStateMachine(AnimationStateInput::PlayStatePaused) on an animation that is already in AnimationState::PausedNew paused.

This seems to cause the state to become PausedWaitResponse :/

[Simon Fraser (smfr)]

> fixes the issue. Simon, do you know if this is the best way to fix this? All
> these various paused states on AnimationBase are very confusing for me :/

Me too, hence my caveat on your other patch. I can't easily reason about whether your suggested change is correct.

I think pausedness should not be a set of parallel states, but a separate flag on the Animation, but that's too big to do now.

[Chris Dumez]

(In reply to Simon Fraser (smfr) from comment #8)
> > fixes the issue. Simon, do you know if this is the best way to fix this? All
> > these various paused states on AnimationBase are very confusing for me :/
> 
> Me too, hence my caveat on your other patch. I can't easily reason about
> whether your suggested change is correct.
> 
> I think pausedness should not be a set of parallel states, but a separate
> flag on the Animation, but that's too big to do now.

The following more limited change also fixes the issue:
@@ -209,8 +209,10 @@ void ImplicitAnimation::reset(const RenderStyle& to, CompositeAnimation& composi
         Style::loadPendingResources(*m_toStyle, m_object->element()->document(), m_object->element());
 
     // Restart the transition
-    if (m_fromStyle && m_toStyle)
-        updateStateMachine(compositeAnimation.isSuspended() ? AnimationStateInput::PlayStatePaused : AnimationStateInput::RestartAnimation, -1);
+    if (m_fromStyle && m_toStyle) {
+        if (!compositeAnimation.isSuspended())
+            updateStateMachine(AnimationStateInput::RestartAnimation, -1);
+    }

[Chris Dumez]

Created attachment 312800 [details]
WIP Patch

[Build Bot]

Attachment 312800 [details] did not pass style-queue:


ERROR: Source/WebCore/page/animation/ImplicitAnimation.cpp:212:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 312832 [details]
Patch

[Chris Dumez]

Comment on attachment 312832 [details]
Patch

Clearing flags on attachment: 312832

Committed r218257: <http://trac.webkit.org/changeset/218257>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.