Bug 112977 - Crash in WebCore::MediaPlayer::cachedResourceLoader + 4
Summary: Crash in WebCore::MediaPlayer::cachedResourceLoader + 4
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-21 16:30 PDT by Jer Noble
Modified: 2013-03-21 22:54 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.96 KB, patch)
2013-03-21 16:56 PDT, Jer Noble
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2013-03-21 16:30:11 PDT
Crash in WebCore::MediaPlayer::cachedResourceLoader + 4
Comment 1 Jer Noble 2013-03-21 16:56:39 PDT
Created attachment 194388 [details]
Patch
Comment 2 Jer Noble 2013-03-21 16:57:28 PDT
<rdar://problem/13435161>
Comment 3 Geoffrey Garen 2013-03-21 22:01:34 PDT
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.
Comment 4 Geoffrey Garen 2013-03-21 22:18:21 PDT
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.
Comment 5 Jer Noble 2013-03-21 22:27:21 PDT
(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:.
Comment 6 Jer Noble 2013-03-21 22:30:02 PDT
(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!
Comment 7 Jer Noble 2013-03-21 22:54:17 PDT
Committed r146563: <http://trac.webkit.org/changeset/146563>