Bug 56325 – ASSERTION FAILED: paused() in AnimationBase::updateStateMachine()

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Tony Gentilcore]

I'm hitting this assertion locally on transitions/mask-transitions.html. http://trac.webkit.org/changeset/80846 was in the area recently so I thought you might have more insight.

ASSERTION FAILED: paused()
/Volumes/work/WebKit/Source/WebCore/page/animation/AnimationBase.cpp(1149) : void WebCore::AnimationBase::updateStateMachine(WebCore::AnimationBase::AnimStateInput, double)
1   WebCore::AnimationBase::updateStateMachine(WebCore::AnimationBase::AnimStateInput, double)
2   WebCore::AnimationBase::onAnimationStartResponse(double)
3   WebCore::AnimationControllerPrivate::startTimeResponse(double)
4   WebCore::AnimationControllerPrivate::receivedStartTimeResponse(double)
5   WebCore::AnimationBase::freezeAtTime(double)
6   WebCore::CompositeAnimation::pauseTransitionAtTime(int, double)
7   WebCore::CompositeAnimation::pauseTransitionAtTime(int, double)
8   WebCore::AnimationControllerPrivate::pauseTransitionAtTime(WebCore::RenderObject*, WTF::String const&, double)
9   WebCore::AnimationController::pauseTransitionAtTime(WebCore::RenderObject*, WTF::String const&, double)
10  -[WebFrame(WebPrivate) _pauseTransitionOfProperty:onNode:atTime:]
11  LayoutTestController::pauseTransitionAtTimeOnElementWithId(OpaqueJSString*, double, OpaqueJSString*)
12  pauseTransitionAtTimeOnElementWithIdCallback(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**)
13  JSC::JSCallbackFunction::call(JSC::ExecState*)
14  cti_op_call_NotJSFunction
15  jscGeneratedNativeCode
16  JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*)
17  JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
18  JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
19  WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
20  WebCore::ScheduledAction::executeFunctionInContext(JSC::JSGlobalObject*, JSC::JSValue, WebCore::ScriptExecutionContext*)
21  WebCore::ScheduledAction::execute(WebCore::Document*)
22  WebCore::ScheduledAction::execute(WebCore::ScriptExecutionContext*)
23  WebCore::DOMTimer::fired()
24  WebCore::ThreadTimers::sharedTimerFiredInternal()
25  WebCore::ThreadTimers::sharedTimerFired()
26  WebCore::timerFired(__CFRunLoopTimer*, void*)
27  __CFRunLoopRun
28  CFRunLoopRunSpecific
29  -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
30  runTest(std::string const&)
31  runTestingServerLoop()

[Tony Gentilcore]

Commenting out the new "if (m_object)" guard in ~AnimationBase() fixes it for me.

Removing that also fixes transitions/interrupted-accelerated-transition.html (filed as bug 56242).

[Dean Jackson]

Created attachment 86246 [details]
Patch

[Dean Jackson]

Comment on attachment 86246 [details]
Patch

Withdrawing. Doesn't yet fix something I hoped it would. I now have a nice testcase!

[Simon Fraser (smfr)]

Comment on attachment 86246 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86246&action=review

> Source/WebCore/ChangeLog:13
> +        from the sets into AnimationController, where it belongs.

Into AnimationControllerPrivate, right?

> Source/WebCore/page/animation/AnimationController.cpp:372
> +    if (m_startTimeResponseWaiters.contains(animation))
> +        m_startTimeResponseWaiters.remove(animation);

This does two hash lookups. There's no point calling contains().

> Source/WebCore/page/animation/AnimationController.cpp:385
> +    HashSet<RefPtr<AnimationBase> >::const_iterator it = m_styleAvailableWaiters.begin();

You should add a typedef for HashSet<RefPtr<AnimationBase> >

> Source/WebCore/page/animation/AnimationController.cpp:388
> +        (*it)->styleAvailable();

Are you sure that this can never re-ender this method?

> Source/WebCore/page/animation/AnimationController.cpp:423
> +    if (m_startTimeResponseWaiters.size())

Use !isEmpty()

> Source/WebCore/page/animation/AnimationController.cpp:434
> +        (*it)->onAnimationStartResponse(time);

Can this ever re-enter this method?

> Source/WebCore/page/animation/AnimationController.cpp:437
> +    m_waitingForStartTimeResponse = false;

Do we need m_waitingForStartTimeResponse? Can we just check !m_startTimeResponseWaiters.isEmpty()?

> Source/WebCore/page/animation/AnimationControllerPrivate.h:128
> +    HashSet<RefPtr<AnimationBase> > m_styleAvailableWaiters;
>      
> -    AnimationBase* m_startTimeResponseWaiters;
> -    AnimationBase* m_lastStartTimeResponseWaiter;
> +    HashSet<RefPtr<AnimationBase> > m_startTimeResponseWaiters;

I was never a fan of the 'waiters' terminology. Maybe rename to 'animationsWaitingFor...'?

> Source/WebCore/page/animation/CompositeAnimation.cpp:180
> +            animationController()->removeFromStyleAvailableWaitList(anim);
> +            animationController()->removeFromStartTimeResponseWaitList(anim);

Maybe wrap these two calls up in a AnimationControllerPrivate::animationWillBeRemoved() method.

[Dean Jackson]

(In reply to comment #4)
> (From update of attachment 86246 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86246&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        from the sets into AnimationController, where it belongs.
> 
> Into AnimationControllerPrivate, right?

Yes

> 
> > Source/WebCore/page/animation/AnimationController.cpp:372
> > +    if (m_startTimeResponseWaiters.contains(animation))
> > +        m_startTimeResponseWaiters.remove(animation);
> 
> This does two hash lookups. There's no point calling contains().

OK

> 
> > Source/WebCore/page/animation/AnimationController.cpp:385
> > +    HashSet<RefPtr<AnimationBase> >::const_iterator it = m_styleAvailableWaiters.begin();
> 
> You should add a typedef for HashSet<RefPtr<AnimationBase> >

OK

> 
> > Source/WebCore/page/animation/AnimationController.cpp:388
> > +        (*it)->styleAvailable();
> 
> Are you sure that this can never re-ender this method?

We couldn't re-enter it, but the previous code did some very subtle things (probably unintentionally) with moving things to/from the waiting lists as it was iterating here. This new code is pretty careful about keeping the lists clean at all times.

> 
> > Source/WebCore/page/animation/AnimationController.cpp:423
> > +    if (m_startTimeResponseWaiters.size())
> 
> Use !isEmpty()

OK

> 
> > Source/WebCore/page/animation/AnimationController.cpp:434
> > +        (*it)->onAnimationStartResponse(time);
> 
> Can this ever re-enter this method?

See above

> 
> > Source/WebCore/page/animation/AnimationController.cpp:437
> > +    m_waitingForStartTimeResponse = false;
> 
> Do we need m_waitingForStartTimeResponse? Can we just check !m_startTimeResponseWaiters.isEmpty()?

No, because the animation doesn't always have to wait for start time. The state machine makes that decision, and as soon as one animation is added that is flagged, then all the existing ones are too.

> 
> > Source/WebCore/page/animation/AnimationControllerPrivate.h:128
> > +    HashSet<RefPtr<AnimationBase> > m_styleAvailableWaiters;
> >      
> > -    AnimationBase* m_startTimeResponseWaiters;
> > -    AnimationBase* m_lastStartTimeResponseWaiter;
> > +    HashSet<RefPtr<AnimationBase> > m_startTimeResponseWaiters;
> 
> I was never a fan of the 'waiters' terminology. Maybe rename to 'animationsWaitingFor...'?

OK

> 
> > Source/WebCore/page/animation/CompositeAnimation.cpp:180
> > +            animationController()->removeFromStyleAvailableWaitList(anim);
> > +            animationController()->removeFromStartTimeResponseWaitList(anim);
> 
> Maybe wrap these two calls up in a AnimationControllerPrivate::animationWillBeRemoved() method.

OK

[Dean Jackson]

Created attachment 86253 [details]
Patch

[Dean Jackson]

Now with a test case that exercises the new code.

[Chris Marrin]

Comment on attachment 86253 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86253&action=review

All looks good and I like the earlier flushing of the lists. But I don't see how you can guarantee that the AnimationBase won't be in one of the lists when it gets destroyed.

> Source/WebCore/page/animation/AnimationBase.cpp:-819
> -

Why is this no longer necessary? Isn't it still possible for it to be destroyed when it is still in one of the lists?

[Dean Jackson]

Created attachment 86366 [details]
Patch

[Dean Jackson]

(In reply to comment #8)
> (From update of attachment 86253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86253&action=review
> 
> All looks good and I like the earlier flushing of the lists. But I don't see how you can guarantee that the AnimationBase won't be in one of the lists when it gets destroyed.
> 
> > Source/WebCore/page/animation/AnimationBase.cpp:-819
> > -
> 
> Why is this no longer necessary? Isn't it still possible for it to be destroyed when it is still in one of the lists?

Because an AnimationBase can only be destroyed after it is removed from the lists in CompositeAnimation - the code in updateKeyframeAnimations and updateTransitions. Before it is removed from the CompositeAnimation lists, I call  animationController()->animationWillBeRemoved().

The AnimationBase may still stick around after that, if JS has a reference via a WebKitAnimation, but it certainly should not be in the waiting lists.

[Chris Marrin]

Comment on attachment 86366 [details]
Patch

Looks good

[Simon Fraser (smfr)]

Comment on attachment 86366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86366&action=review

> LayoutTests/animations/body-removal-crash.html:35
> +<html>
> +<head>
> +  <style id="a" type="text/css" media="screen">
> +    #box {
> +      -webkit-animation-duration: 2s;
> +      -webkit-animation-timing-function: linear;
> +      -webkit-animation-name: anim;
> +      background-color: blue;
> +      width: 100px;
> +      height: 100px;
> +    }
> +    @-webkit-keyframes anim {
> +        from { -webkit-transform: rotate(0) scale(1,1); }
> +        to   { -webkit-transform: rotate(360deg) scale(2,4); }
> +    }
> +  </style>
> +</head>
> +<body>
> +<div id="box">
> +</div>
> +</body>
> +</html>
> +
> +<script> 
> +function crash() {
> +    // trigger style processing
> +    document.alinkColor = "aaa";
> +    // now remove the body and insert it in a different location
> +    var elem = document.body;
> +    elem.parentNode.removeChild(elem);
> +    document.getElementById("a").parentNode.insertBefore(elem, document.getElementById("a").nextSibling);
> +}
> +
> +crash();
> +</script>

This doesn't need to be a pixel test.

[Dean Jackson]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/animations/3d/transform-origin-vs-functions.html
	A	LayoutTests/animations/body-removal-crash-expected.txt
	A	LayoutTests/animations/body-removal-crash.html
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/page/animation/AnimationBase.cpp
	M	Source/WebCore/page/animation/AnimationBase.h
	M	Source/WebCore/page/animation/AnimationController.cpp
	M	Source/WebCore/page/animation/AnimationControllerPrivate.h
	M	Source/WebCore/page/animation/CompositeAnimation.cpp
Committed r81613

[WebKit Review Bot]

http://trac.webkit.org/changeset/81613 might have broken Chromium Mac Release

[Pavel Feldman]

We started seeing following crash on Chromium reliability bots somewhere around this land.

Stack trace:
chrome_25a0000!WebCore::ImplicitAnimation::~ImplicitAnimation+0x16 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\page\animation\implicitanimation.cpp @ 58]
chrome_25a0000!WebCore::ImplicitAnimation::`scalar deleting destructor'+0x8
chrome_25a0000!WTF::HashTable<WTF::RefPtr<WebCore::AnimationBase>,WTF::RefPtr<WebCore::AnimationBase>,WTF::IdentityExtractor<WTF::RefPtr<WebCore::AnimationBase> >,WTF::PtrHash<WTF::RefPtr<WebCore::AnimationBase> >,WTF::HashTraits<WTF::RefPtr<WebCore::AnimationBase> >,WTF::HashTraits<WTF::RefPtr<WebCore::AnimationBase> > >::deallocateTable+0x3a [c:\b\build\slave\win\build\src\third_party\webkit\source\javascriptcore\wtf\hashtable.h @ 891]
chrome_25a0000!WebCore::AnimationControllerPrivate::~AnimationControllerPrivate+0x29 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\page\animation\animationcontroller.cpp @ 64]
chrome_25a0000!WebCore::AnimationController::~AnimationController+0xe [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\page\animation\animationcontroller.cpp @ 462]
chrome_25a0000!WebCore::Frame::~Frame+0x140 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\page\frame.cpp @ 245]
chrome_25a0000!WebCore::Page::~Page+0x309 [c:\b\build\slave\win\build\src\third_party\webkit\source\webcore\page\page.cpp @ 215]
chrome_25a0000!WebKit::WebViewImpl::close+0x60 [c:\b\build\slave\win\build\src\third_party\webkit\source\webkit\chromium\src\webviewimpl.cpp @ 940]
chrome_25a0000!RenderWidget::Close+0x10 [c:\b\build\slave\win\build\src\content\renderer\render_widget.cc @ 824]
chrome_25a0000!RenderView::Close+0xc [c:\b\build\slave\win\build\src\content\renderer\render_view.cc @ 5056]
chrome_25a0000!MessageLoop::RunTask+0xf0 [c:\b\build\slave\win\build\src\base\message_loop.cc @ 368]
chrome_25a0000!MessageLoop::DoWork+0x176 [c:\b\build\slave\win\build\src\base\message_loop.cc @ 569]
chrome_25a0000!base::MessagePumpDefault::Run+0x122 [c:\b\build\slave\win\build\src\base\message_pump_default.cc @ 50]
chrome_25a0000!MessageLoop::RunInternal+0x9e [c:\b\build\slave\win\build\src\base\message_loop.cc @ 342]
chrome_25a0000!MessageLoop::Run+0x5b [c:\b\build\slave\win\build\src\base\message_loop.cc @ 240]
chrome_25a0000!RendererMain+0x391 [c:\b\build\slave\win\build\src\chrome\renderer\renderer_main.cc @ 335]
chrome_25a0000!`anonymous namespace'::RunNamedProcessTypeMain+0x10f [c:\b\build\slave\win\build\src\chrome\app\chrome_main.cc @ 476]
chrome_25a0000!ChromeMain+0x7e3 [c:\b\build\slave\win\build\src\chrome\app\chrome_main.cc @ 787]
chrome!MainDllLoader::Launch+0x194 [c:\b\build\slave\win\build\src\chrome\app\client_util.cc @ 289]
chrome!wWinMain+0xa1 [c:\b\build\slave\win\build\src\chrome\app\chrome_exe_main_win.cc @ 47]
chrome!__tmainCRTStartup+0x112 [f:\dd\vctools\crt_bld\self_x86\crt\src\crt0.c @ 263]
WARNING: Stack unwind information not available. Following frames may be wrong.
kernel32!RegisterWaitForInputIdle+0x49