Bug 22046 – Crash when looking at JS parse error in Web Inspector

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Timothy Churchward]

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)

[Timothy Churchward]

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)]

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)]

<rdar://problem/6341828>

[Simon Fraser (smfr)]

May be related to 22052, which has a patch requesting review.

[Simon Fraser (smfr)]

I can't reproduce the crash in r38371

[Chris Marrin]

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]

Created attachment 25137 [details]
Patch to fix bug

[Darin Adler]

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]

Created attachment 25148 [details]
replacement patch incorporating changes from Darin

[Darin Adler]

Comment on attachment 25148 [details]
replacement patch incorporating changes from Darin

r=me

[Simon Fraser (smfr)]

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]

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]

I still get the crash that was originally reported.

[Simon Fraser (smfr)]

I can't reproduce the crash. Darin, do you have a stack trace?

[Darin Adler]

Created attachment 26723 [details]
backtrace of crash

Yes, here is the crash backtrace.

[Simon Fraser (smfr)]

This doesn't seem to be animation-related; it's crashing in
WebCore::Document::dispatchImageLoadEventsNow()

[Darin Adler]

(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]

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]

OK. Lets reclose this then and I'll open a new bug.