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

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Jer Noble <jer.noble at apple.com>'s
request for 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


I don't think this can be right.

>
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.