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()
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).
Created attachment 86246 [details] Patch
Comment on attachment 86246 [details] Patch Withdrawing. Doesn't yet fix something I hoped it would. I now have a nice testcase!
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.
(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
Created attachment 86253 [details] Patch
Now with a test case that exercises the new code.
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?
Created attachment 86366 [details] Patch
(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 on attachment 86366 [details] Patch Looks good
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.
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
http://trac.webkit.org/changeset/81613 might have broken Chromium Mac Release
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