| Summary: | Crash when looking at JS parse error in Web Inspector | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Timothy Churchward <WebKitBugzilla> | ||||||||||
| Component: | WebCore Misc. | Assignee: | Darin Adler <darin> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Major | CC: | darin, simon.fraser | ||||||||||
| Priority: | P2 | Keywords: | InRadar, NeedsReduction, Regression | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Mac | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| URL: | http://www.formyownamusement.com/WebKit/classCrashTest.html | ||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 24857 [details]
Crash Test File
The attached file produces the crash in WebKit when viewing the parse error with the Web Inspector
Exception Type: EXC_BAD_ACCESS (SIGBUS) Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000003 Crashed Thread: 0 Thread 0 Crashed: 0 ??? 0x00000003 0 + 3 1 com.apple.WebCore 0x04b3dd82 WebCore::AnimationTimerCallback::timerFired(WebCore::Timer<WebCore::AnimationTimerBase>*) + 34 2 com.apple.WebCore 0x04b40bbb WebCore::Timer<WebCore::AnimationTimerBase>::fired() + 43 3 com.apple.WebCore 0x04a155c5 WebCore::TimerBase::fireTimers(double, WTF::Vector<WebCore::TimerBase*, 0ul> const&) + 133 4 com.apple.WebCore 0x04a15892 WebCore::TimerBase::sharedTimerFired() + 162 5 com.apple.WebCore 0x049fbbf4 __ZN7WebCoreL10timerFiredEP16__CFRunLoopTimerPv + 68 May be related to 22052, which has a patch requesting review. This only reproduces on the release build, perhaps because it has different timing or different GC behavior. I understand the bug and am making a patch Created attachment 25137 [details]
Patch to fix bug
Comment on attachment 25137 [details] Patch to fix bug > void AnimationBase::animationTimerCallbackFired(const AtomicString& eventType, double elapsedTime) > { > + // We have to make sure to keep a ref to the this pointer, because it could get destroyed > + // during an animation callback that might get called. > + RefPtr<AnimationBase> retainer(this); > + > ASSERT(m_object->document() && !m_object->document()->inPageCache()); We typically call these "protector". I suspect the "retain" term may be familiar to ObjC programmers and not others. I hate to have any of these -- it's so tricky to know when you should or should not use one, but I think it's fine to have this one here. Can we construct a test case to demonstrate the failure case here? It seems like we should be able to regression-test this. > // Set start time on all animations waiting for it > AnimationNameMap::const_iterator end = m_keyframeAnimations.end(); > for (AnimationNameMap::const_iterator it = m_keyframeAnimations.begin(); it != end; ++it) { > - KeyframeAnimation* anim = it->second.get(); > - if (anim && anim->waitingForStartTime()) > - anim->updateStateMachine(AnimationBase::AnimationStateInputStartTimeSet, t); > + // We have to make sure to keep a ref to the animation pointer, because it could get destroyed > + // during an animation callback that might get called. > + RefPtr<KeyframeAnimation> animation = it->second; > + if (animation && animation->waitingForStartTime()) > + animation->updateStateMachine(AnimationBase::AnimationStateInputStartTimeSet, t); > } > } I see two possibilities: 1) There's no way the m_keyframeAnimations map could be modified inside the updateStateMachine function. In this case, there's no need for a local RefPtr because the value in the map is already a RefPtr. 2) The m_keyframeAnimations map could be modified by an animation callback. In this case we have a potentially-crashing bug, because HashMap does not support iterating it while it's modified and we could end up with an infinite loop, accessing deallocated memory, or some other equally bad problem. Either way, putting the animation into a RefPtr is not the fix. > // Set the start time for given property transition > CSSPropertyTransitionsMap::const_iterator end = m_transitions.end(); > for (CSSPropertyTransitionsMap::const_iterator it = m_transitions.begin(); it != end; ++it) { > - ImplicitAnimation* anim = it->second.get(); > - if (anim && anim->waitingForStartTime() && anim->animatingProperty() == property) > - anim->updateStateMachine(AnimationBase::AnimationStateInputStartTimeSet, t); > + // We have to make sure to keep a ref to the animation pointer, because it could get destroyed > + // during an animation callback that might get called. > + RefPtr<ImplicitAnimation> animation = it->second; > + if (animation && animation->waitingForStartTime() && animation->animatingProperty() == property) > + animation->updateStateMachine(AnimationBase::AnimationStateInputStartTimeSet, t); > } > } Same comment as for m_keyframeAnimations above. > for (size_t i = 0; i < animations.size(); ++i) { > - KeyframeAnimation* anim = animations[i].get(); > - if (anim && anim->waitingForStyleAvailable()) > - anim->updateStateMachine(AnimationBase::AnimationStateInputStyleAvailable, -1); > + // We have to make sure to keep a ref to the animation pointer, because it could get destroyed > + // during an animation callback that might get called. > + RefPtr<KeyframeAnimation> animation = animations[i]; > + if (animation && animation->waitingForStyleAvailable()) > + animation->updateStateMachine(AnimationBase::AnimationStateInputStyleAvailable, -1); > } I don't see the value of using a RefPtr here. The animations vector is already a local vector of RefPtr, so we're holding a ref to all of the animations until that vector is destroyed. > CSSPropertyTransitionsMap::const_iterator end = m_transitions.end(); > for (CSSPropertyTransitionsMap::const_iterator it = m_transitions.begin(); it != end; ++it) { > - ImplicitAnimation* anim = it->second.get(); > - if (anim && anim->waitingForStyleAvailable()) > - anim->updateStateMachine(AnimationBase::AnimationStateInputStyleAvailable, -1); > + // We have to make sure to keep a ref to the animation pointer, because it could get destroyed > + // during an animation callback that might get called. > + RefPtr<ImplicitAnimation> animation = it->second; > + if (animation && animation->waitingForStyleAvailable()) > + animation->updateStateMachine(AnimationBase::AnimationStateInputStyleAvailable, -1); > } Same comment as for m_keyframeAnimations above. review- because only the first change seems obviously correct Created attachment 25148 [details]
replacement patch incorporating changes from Darin
Comment on attachment 25148 [details]
replacement patch incorporating changes from Darin
r=me
Committed r38768 M WebCore/ChangeLog M WebCore/page/animation/AnimationBase.h M WebCore/page/animation/CompositeAnimation.cpp M WebCore/page/animation/AnimationController.cpp M WebCore/page/animation/CompositeAnimation.h M WebCore/page/animation/AnimationBase.cpp M LayoutTests/ChangeLog A LayoutTests/animations/transform-animation-event-destroy-element.html A LayoutTests/animations/transform-animation-event-destroy-element-expected.txt A LayoutTests/transitions/transform-transition-event-destroy-element-expected.txt A LayoutTests/transitions/transform-transition-event-destroy-element.html r38768 = 7c14de362f15d6dc75bbd7914f4b8db76e2c1430 (trunk) I added a couple more tests. The previous checkin was for the wrong patch (22052). This is the correct patch Sending WebCore/ChangeLog Sending WebCore/page/animation/AnimationBase.cpp Sending WebCore/page/animation/ImplicitAnimation.cpp Sending WebCore/page/animation/KeyframeAnimation.cpp Transmitting file data .... Committed revision 38876. I still get the crash that was originally reported. I can't reproduce the crash. Darin, do you have a stack trace? Created attachment 26723 [details]
backtrace of crash
Yes, here is the crash backtrace.
This doesn't seem to be animation-related; it's crashing in WebCore::Document::dispatchImageLoadEventsNow() (In reply to comment #16) > This doesn't seem to be animation-related; it's crashing in > WebCore::Document::dispatchImageLoadEventsNow() I agree. Sorry, I wasn't thinking about animation because I was looking at the title of the bug, original description, and test case. Maybe the animation bug was always only part of the bug, or maybe the bug was fixed and this is some new problem. This is a different stack trace and so I think it is an unrelated case. I have been trying to reproduce the crash for a while on both the release and debug builds to no avail. OK. Lets reclose this then and I'll open a new bug. |
The Web Inspector Console reports the use of the `class` keyword as a JavaScript parse error. Safari/WebKit crashes shortly after reporting this error. To reproduce please see the attached URL. It has a very simple JavaScript <script type="text/javascript" charset="utf-8"> var class = "not so nice to call one's self a class when it's not"; </script> Next bring up the Web Inspector and click through to see the parse error. WebKit crashes shortly after. I have tried this on several nightly builds of WebKit with the same result. (sorry I can't remember which builds exactly) My current test was on r38068 (3rd November 2008)