RESOLVED FIXED 22046
Crash when looking at JS parse error in Web Inspector
https://bugs.webkit.org/show_bug.cgi?id=22046
Summary Crash when looking at JS parse error in Web Inspector
Timothy Churchward
Reported 2008-11-03 03:10:18 PST
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)
Attachments
Crash Test File (789 bytes, text/html)
2008-11-03 03:12 PST, Timothy Churchward
no flags
Patch to fix bug (8.03 KB, patch)
2008-11-13 14:32 PST, Chris Marrin
darin: review-
replacement patch incorporating changes from Darin (4.12 KB, patch)
2008-11-13 17:18 PST, Chris Marrin
no flags
backtrace of crash (1.58 KB, text/plain)
2009-01-14 11:46 PST, Darin Adler
no flags
Timothy Churchward
Comment 1 2008-11-03 03:12:05 PST
Created attachment 24857 [details] Crash Test File The attached file produces the crash in WebKit when viewing the parse error with the Web Inspector
Mark Rowe (bdash)
Comment 2 2008-11-03 19:31:52 PST
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
Mark Rowe (bdash)
Comment 3 2008-11-03 19:33:32 PST
Simon Fraser (smfr)
Comment 4 2008-11-13 10:03:02 PST
May be related to 22052, which has a patch requesting review.
Simon Fraser (smfr)
Comment 5 2008-11-13 10:34:11 PST
I can't reproduce the crash in r38371
Chris Marrin
Comment 6 2008-11-13 13:56:24 PST
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
Chris Marrin
Comment 7 2008-11-13 14:32:44 PST
Created attachment 25137 [details] Patch to fix bug
Darin Adler
Comment 8 2008-11-13 16:21:29 PST
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
Chris Marrin
Comment 9 2008-11-13 17:18:51 PST
Created attachment 25148 [details] replacement patch incorporating changes from Darin
Darin Adler
Comment 10 2008-11-13 17:37:30 PST
Comment on attachment 25148 [details] replacement patch incorporating changes from Darin r=me
Simon Fraser (smfr)
Comment 11 2008-11-25 16:18:21 PST
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.
Chris Marrin
Comment 12 2008-12-01 14:51:08 PST
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.
Darin Adler
Comment 13 2009-01-14 10:46:40 PST
I still get the crash that was originally reported.
Simon Fraser (smfr)
Comment 14 2009-01-14 11:12:01 PST
I can't reproduce the crash. Darin, do you have a stack trace?
Darin Adler
Comment 15 2009-01-14 11:46:35 PST
Created attachment 26723 [details] backtrace of crash Yes, here is the crash backtrace.
Simon Fraser (smfr)
Comment 16 2009-01-14 11:55:46 PST
This doesn't seem to be animation-related; it's crashing in WebCore::Document::dispatchImageLoadEventsNow()
Darin Adler
Comment 17 2009-01-14 11:58:56 PST
(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.
Chris Marrin
Comment 18 2009-01-20 11:29:44 PST
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.
Darin Adler
Comment 19 2009-01-20 12:17:03 PST
OK. Lets reclose this then and I'll open a new bug.
Note You need to log in before you can comment on or make changes to this bug.