Bug 148442 – Fix crash due to animationDidEnd called on deallocated RemoteLayerTreeHost

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Wenson Hsieh]

WebKit may crash when calling -[PlatformCAAnimationRemote animationDidStop] if the PlatformCAAnimationRemote's layer tree host has been deallocated.

[Wenson Hsieh]

<rdar://problem/21609257>

[Wenson Hsieh]

Reproducible nearly 100% of the time -- visit http://whsieh.github.io/examples/layerhost-crash.html and click the link. Works best when running an iOS simulator on a debug build where you can see the stack trace upon the crash.

[Wenson Hsieh]

Created attachment 259975 [details]
Test case that reproduces the bug

Click the link to open a new tab. The new tab will crash while exiting.

[Wenson Hsieh]

Created attachment 259977 [details]
Patch

[Tim Horton]

Comment on attachment 259977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259977&action=review

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm:145
>      m_layers.remove(layerID);

1) You can do the hash table lookup only once.

2) Should you also cover the case in clearLayers()?

[Wenson Hsieh]

Comment on attachment 259977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259977&action=review

>> Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm:145
>>      m_layers.remove(layerID);
> 
> 1) You can do the hash table lookup only once.
> 
> 2) Should you also cover the case in clearLayers()?

1. Good point. Fixed.

2. clearLayers is only invoked in the destructor of RemoteLayerTreeHost, which ensures that all animation delegates are first invalidated. It might be better to lift the invalidation logic from ~RemoteLayerTreeHost to clearLayers() instead though.

[Tim Horton]

(In reply to comment #6)
> Comment on attachment 259977 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259977&action=review
> 
> >> Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm:145
> >>      m_layers.remove(layerID);
> > 
> > 1) You can do the hash table lookup only once.
> > 
> > 2) Should you also cover the case in clearLayers()?
> 
> 1. Good point. Fixed.
> 
> 2. clearLayers is only invoked in the destructor of RemoteLayerTreeHost,
> which ensures that all animation delegates are first invalidated. It might
> be better to lift the invalidation logic from ~RemoteLayerTreeHost to
> clearLayers() instead though.

Ah! No, that's probably fine. OK.

[Wenson Hsieh]

Created attachment 259980 [details]
Patch

[Tim Horton]

Please consider making your test a layout test (it seems like a pretty easy one).

[WebKit Commit Bot]

Comment on attachment 259980 [details]
Patch

Clearing flags on attachment: 259980

Committed r188991: <http://trac.webkit.org/changeset/188991>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.