Bug 22046 - Crash when looking at JS parse error in Web Inspector
Summary: Crash when looking at JS parse error in Web Inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Major
Assignee: Darin Adler
URL: http://www.formyownamusement.com/WebK...
Keywords: InRadar, NeedsReduction, Regression
Depends on:
Blocks:
 
Reported: 2008-11-03 03:10 PST by Timothy Churchward
Modified: 2009-01-20 12:17 PST (History)
2 users (show)

See Also:


Attachments
Crash Test File (789 bytes, text/html)
2008-11-03 03:12 PST, Timothy Churchward
no flags Details
Patch to fix bug (8.03 KB, patch)
2008-11-13 14:32 PST, Chris Marrin
darin: review-
Details | Formatted Diff | Diff
replacement patch incorporating changes from Darin (4.12 KB, patch)
2008-11-13 17:18 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
backtrace of crash (1.58 KB, text/plain)
2009-01-14 11:46 PST, Darin Adler
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Churchward 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)
Comment 1 Timothy Churchward 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
Comment 2 Mark Rowe (bdash) 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

Comment 3 Mark Rowe (bdash) 2008-11-03 19:33:32 PST
<rdar://problem/6341828>
Comment 4 Simon Fraser (smfr) 2008-11-13 10:03:02 PST
May be related to 22052, which has a patch requesting review.
Comment 5 Simon Fraser (smfr) 2008-11-13 10:34:11 PST
I can't reproduce the crash in r38371
Comment 6 Chris Marrin 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
Comment 7 Chris Marrin 2008-11-13 14:32:44 PST
Created attachment 25137 [details]
Patch to fix bug
Comment 8 Darin Adler 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
Comment 9 Chris Marrin 2008-11-13 17:18:51 PST
Created attachment 25148 [details]
replacement patch incorporating changes from Darin
Comment 10 Darin Adler 2008-11-13 17:37:30 PST
Comment on attachment 25148 [details]
replacement patch incorporating changes from Darin

r=me
Comment 11 Simon Fraser (smfr) 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.
Comment 12 Chris Marrin 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.

Comment 13 Darin Adler 2009-01-14 10:46:40 PST
I still get the crash that was originally reported.
Comment 14 Simon Fraser (smfr) 2009-01-14 11:12:01 PST
I can't reproduce the crash. Darin, do you have a stack trace?
Comment 15 Darin Adler 2009-01-14 11:46:35 PST
Created attachment 26723 [details]
backtrace of crash

Yes, here is the crash backtrace.
Comment 16 Simon Fraser (smfr) 2009-01-14 11:55:46 PST
This doesn't seem to be animation-related; it's crashing in
WebCore::Document::dispatchImageLoadEventsNow()
Comment 17 Darin Adler 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.
Comment 18 Chris Marrin 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.
Comment 19 Darin Adler 2009-01-20 12:17:03 PST
OK. Lets reclose this then and I'll open a new bug.