[webkit-reviews] review granted: [Bug 112977] Crash in WebCore::MediaPlayer::cachedResourceLoader + 4 : [Attachment 194388] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted  review:
Bug 112977: Crash in WebCore::MediaPlayer::cachedResourceLoader + 4
https://bugs.webkit.org/show_bug.cgi?id=112977

Attachment 194388: Patch
https://bugs.webkit.org/attachment.cgi?id=194388&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=194388&action=review


> Source/WebCore/ChangeLog:13
> +	   been deleted, and the delegate's m_callback pointer is not pointing
at freed memory.

Typo: should be "now".

> Source/WebCore/ChangeLog:17
> +	   m_callback ivar, to avoid calling into freed memory for already
in-process delegate callbacks.

Instead of saying "already in-process" here, let's say "already queued". If the
callbacks were truly in-process, this fix wouldn't work.

>>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:238
>> +	[m_loaderDelegate.get() setCallback:0];
> 
> If there is a race condition between A and B, by definition there is a race
condition between A and every unsynchronized subset of B. Therefore, you can't
solve a race condition between AVAssetResourceLoader and
~MediaPlayerPrivateAVFoundationObjC() by adding a line of code to
~MediaPlayerPrivateAVFoundationObjC(), unless that line of code locks a mutex.
> 
> Also, setting m_loaderDelegate's callback to 0 right before deallocating it
is dubious. This is only meaningful if you rely on the value stored to that
memory after that memory is freed.

I take it back: This works because the delegate queue is the main queue we're
running on, and that acts as synchronization.