Bug 112977 – Crash in WebCore::MediaPlayer::cachedResourceLoader + 4

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Crash in WebCore::MediaPlayer::cachedResourceLoader + 4

[Jer Noble]

Created attachment 194388 [details]
Patch

[Jer Noble]

<rdar://problem/13435161>

[Geoffrey Garen]

Comment on attachment 194388 [details]
Patch

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

[Geoffrey Garen]

Comment on attachment 194388 [details]
Patch

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

[Jer Noble]

(In reply to comment #3)
> (From update of attachment 194388 [details])
> 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/MediaPlayerPrivateAVFoundationObjC.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.

The synchronization happens due to AVAssetResourceLoader calling dispatch_sync() to the main thread.  m_loaderDelegate is retained in CoreMedia's thread, and not released until that dispatch_sync() returns. So while m_loaderDelegate's retain count is modified from both threads, m_loaderDelegate->m_callback is only accessed from the main thread (from within the dispatch_sync), which is why it is safe to modify its value without a lock.

Previously, MediaPlayerPrivateAVFoundationObjC was destroyed in one runloop, and the object pointed to by m_loaderDelegate was accessed (by CoreMedia) in the next runloop.  With this change, that will still happen, but m_loaderDelegate will no longer have a stale pointer to a (destroyed) MediaPlayerPrivateAVFoundationObjC.

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

Not really.  Your statement would be correct if AVAssetResourceLoader did not retain m_loaderDelegate.  Since it does retain m_loaderDelegate, it will be accessing valid memory when it calls into -resourceLoader: shouldWaitForLoadingOfRequestedResource:.

[Jer Noble]

(In reply to comment #4)
> (From update of attachment 194388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194388&action=review
> 
> I take it back: This works because the delegate queue is the main queue we're running on, and that acts as synchronization.

Whew!

> Typo: should be "now".

Fixed.

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

Sure thing.

Thanks!

[Jer Noble]

Committed r146563: <http://trac.webkit.org/changeset/146563>