[webkit-reviews] review denied: [Bug 22046] Crash when looking at JS parse error in Web Inspector : [Attachment 25137] Patch to fix bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Chris Marrin <cmarrin at apple.com>'s
request for review:
Bug 22046: Crash when looking at JS parse error in Web Inspector
https://bugs.webkit.org/show_bug.cgi?id=22046

Attachment 25137: Patch to fix bug
https://bugs.webkit.org/attachment.cgi?id=25137&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>  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