Bug 56325

Summary: ASSERTION FAILED: paused() in AnimationBase::updateStateMachine()
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: Layout and RenderingAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarrin, dino, eric, levin, pfeldman, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 56242    
Attachments:
Description Flags
Patch
none
Patch
none
Patch cmarrin: review+

Description Tony Gentilcore 2011-03-14 12:51:06 PDT
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()
Comment 1 Tony Gentilcore 2011-03-14 13:02:39 PDT
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).
Comment 2 Dean Jackson 2011-03-18 17:20:04 PDT
Created attachment 86246 [details]
Patch
Comment 3 Dean Jackson 2011-03-18 17:31:04 PDT
Comment on attachment 86246 [details]
Patch

Withdrawing. Doesn't yet fix something I hoped it would. I now have a nice testcase!
Comment 4 Simon Fraser (smfr) 2011-03-18 17:31:41 PDT
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.
Comment 5 Dean Jackson 2011-03-18 18:51:31 PDT
(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
Comment 6 Dean Jackson 2011-03-18 18:52:30 PDT
Created attachment 86253 [details]
Patch
Comment 7 Dean Jackson 2011-03-18 18:53:11 PDT
Now with a test case that exercises the new code.
Comment 8 Chris Marrin 2011-03-21 07:24:23 PDT
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?
Comment 9 Dean Jackson 2011-03-21 14:36:09 PDT
Created attachment 86366 [details]
Patch
Comment 10 Dean Jackson 2011-03-21 14:43:29 PDT
(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.
Comment 11 Chris Marrin 2011-03-21 14:46:28 PDT
Comment on attachment 86366 [details]
Patch

Looks good
Comment 12 Simon Fraser (smfr) 2011-03-21 14:47:34 PDT
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.
Comment 13 Dean Jackson 2011-03-21 15:06:09 PDT
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
Comment 14 WebKit Review Bot 2011-03-21 15:59:04 PDT
http://trac.webkit.org/changeset/81613 might have broken Chromium Mac Release
Comment 15 Pavel Feldman 2011-03-23 12:12:48 PDT
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