Bug 65724 - Need to handle kCACFContextNeedsFlushNotification notifications that arrive after the AVFWrapper has been disposed
Summary: Need to handle kCACFContextNeedsFlushNotification notifications that arrive a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Jeff Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-08-04 15:12 PDT by Jeff Miller
Modified: 2011-08-12 11:42 PDT (History)
2 users (show)

See Also:


Attachments
Patch (20.47 KB, patch)
2011-08-11 17:39 PDT, Jeff Miller
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff Miller 2011-08-04 15:12:16 PDT
Unlike AVCFPlayerItem notifications, the kCACFContextNeedsFlushNotification is associated with a CACFContext, not a AVCFPlayerItem. As a result, even though we clean up the AVFWrapper on the same dispatch queue used for AVCFPlayerItem notifications, this won't prevent additional kCACFContextNeedsFlushNotifications from being received. We need to stop using the AVFWrapper* as the observer for these notifications, and identify the AVFWrapper by id instead so we can determine when to ignore a notification for a stale AVFWrapper.

Currently, we're working around this by leaking the ACFWrapper object (while cleaning up its ivars).
Comment 1 Jeff Miller 2011-08-04 15:12:58 PDT
<rdar://problem/9900100>
Comment 2 Jeff Miller 2011-08-11 17:39:22 PDT
Created attachment 103713 [details]
Patch
Comment 3 Eric Carlson 2011-08-12 09:01:28 PDT
Comment on attachment 103713 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:124
> +    inline void* callbackContext() const { return reinterpret_cast<void*>(m_objectID); }

It looks like this is only used within this class, can it be made private?

Nit: Is there any reason to call it "callbackContext" instead of "objectID"?


> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:131
> +    static Mutex& avfWrapperMapLock();
> +    static HashMap<uintptr_t, AVFWrapper*>& avfWrapperMap();
> +    static AVFWrapper* avfWrapperForCallbackContext(void*);
> +    void addToAVFWrapperMap();
> +    void removeFromAVFWrapperMap() const;

Nit: These are all private to the AVFWrapper class, so I don't think having "AVFWrapper" in the names is helpful.


> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:865
> +    // HashMap doesn't like a key of 0.
> +    if (!m_objectID)
> +        m_objectID = s_nextAVFWrapperObjectID++;
> +    

This *could* reuse the ID of an existing wrapper. It isn't likely but it is definitely possible - imagine a page that has a static <video> element but also uses one-shot <audio> elements for sound effects. You might as well look up the "new" ID in the map to make sure it isn't already in use.


> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:927
> +    // Assumes caller has locked avfWrapperMapLock().    
> +    HashMap<uintptr_t, AVFWrapper*>::iterator it = avfWrapperMap().find(reinterpret_cast<uintptr_t>(context));
> +    if (it == avfWrapperMap().end())
> +        return 0;
> +
> +    return it->second;

You might as well ASSERT that the map is locked in a debug build.


> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1028
> +    MutexLocker locker(avfWrapperMapLock());
> +    AVFWrapper* self = avfWrapperForCallbackContext(context);
> +    if (!self) {
> +        LOG(Media, "AVFWrapper::periodicTimeObserverCallback invoked for deleted AVFWrapper %d", reinterpret_cast<uintptr_t>(context));
>          return;
> +    }
>  
>      double time = std::max(0.0, CMTimeGetSeconds(cmTime)); // Clamp to zero, negative values are sometimes reported.
>      self->m_owner->scheduleMainThreadNotification(MediaPlayerPrivateAVFoundation::Notification::PlayerTimeChanged, time);

Is it safe/necessary to hold the lock when calling outside of the class?


> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1038
> +    MutexLocker locker(avfWrapperMapLock());
> +    AVFWrapper* self = avfWrapperForCallbackContext(observer);
>      
> -    if (!self->m_owner)
> +    if (!self) {
> +        LOG(Media, "AVFWrapper::notificationCallback invoked for deleted AVFWrapper %d", reinterpret_cast<uintptr_t>(observer));
>          return;

Ditto.


> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1082
> +    MutexLocker locker(avfWrapperMapLock());
> +    AVFWrapper* self = avfWrapperForCallbackContext(context);
> +    if (!self) {
> +        LOG(Media, "AVFWrapper::loadPlayableCompletionCallback invoked for deleted AVFWrapper %d", reinterpret_cast<uintptr_t>(context));
>          return;
> +    }
> +
> +    LOG(Media, "AVFWrapper::loadPlayableCompletionCallback(%p)", self);
>      self->m_owner->scheduleMainThreadNotification(MediaPlayerPrivateAVFoundation::Notification::AssetPlayabilityKnown);

Ditto.


> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1110
> +    MutexLocker locker(avfWrapperMapLock());
> +    AVFWrapper* self = avfWrapperForCallbackContext(context);
> +    if (!self) {
> +        LOG(Media, "AVFWrapper::loadMetadataCompletionCallback invoked for deleted AVFWrapper %d", reinterpret_cast<uintptr_t>(context));
>          return;
> +    }
> +
> +    LOG(Media, "AVFWrapper::loadMetadataCompletionCallback(%p)", self);
>      self->m_owner->scheduleMainThreadNotification(MediaPlayerPrivateAVFoundation::Notification::AssetMetadataLoaded);

Ditto.


> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1130
> +    MutexLocker locker(avfWrapperMapLock());
> +    AVFWrapper* self = avfWrapperForCallbackContext(context);
> +    if (!self) {
> +        LOG(Media, "AVFWrapper::seekCompletedCallback invoked for deleted AVFWrapper %d", reinterpret_cast<uintptr_t>(context));
>          return;
> +    }
>  
> +    LOG(Media, "AVFWrapper::seekCompletedCallback(%p)", self);
>      self->m_owner->scheduleMainThreadNotification(MediaPlayerPrivateAVFoundation::Notification::SeekCompleted, static_cast<bool>(finished));

Ditto.
Comment 4 Jeff Miller 2011-08-12 10:21:43 PDT
Comment on attachment 103713 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:124
>> +    inline void* callbackContext() const { return reinterpret_cast<void*>(m_objectID); }
> 
> It looks like this is only used within this class, can it be made private?
> 
> Nit: Is there any reason to call it "callbackContext" instead of "objectID"?

I'll make it private.  I chose callbackContext() because it makes sense in all the call sites that use it, since they don't need to know that it's actually the objectID. Plus, m_objectID is a uintptr_t, and we need a void* for the callback context.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:131
>> +    void removeFromAVFWrapperMap() const;
> 
> Nit: These are all private to the AVFWrapper class, so I don't think having "AVFWrapper" in the names is helpful.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:865
>> +    
> 
> This *could* reuse the ID of an existing wrapper. It isn't likely but it is definitely possible - imagine a page that has a static <video> element but also uses one-shot <audio> elements for sound effects. You might as well look up the "new" ID in the map to make sure it isn't already in use.

I don't know enough about media elements to understand the one-shot <audio> element example. I can add code to check for the ID already being in use, but it seems like if we did hit the case where we wrapped around and encountered an ID that was still in use, it would be equally likely that all the other IDs are also in use, in which case there's not much we could do.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:927
>> +    return it->second;
> 
> You might as well ASSERT that the map is locked in a debug build.

Good point, will do.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1028
>>      self->m_owner->scheduleMainThreadNotification(MediaPlayerPrivateAVFoundation::Notification::PlayerTimeChanged, time);
> 
> Is it safe/necessary to hold the lock when calling outside of the class?

Yes, we need to make sure the MediaPlayerPrivateAVFoundationCF object in m_owner remains valid during the call to its scheduleMainThreadNotification() method. Since the lock prevents the AVFWrapper from being torn down, which is done during the MediaPlayerPrivateAVFoundationCF destructor, holding it also prevents m_owner from being deleted.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1038
>>          return;
> 
> Ditto.

See my comment above.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1082
>>      self->m_owner->scheduleMainThreadNotification(MediaPlayerPrivateAVFoundation::Notification::AssetPlayabilityKnown);
> 
> Ditto.

See my comment above.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1110
>>      self->m_owner->scheduleMainThreadNotification(MediaPlayerPrivateAVFoundation::Notification::AssetMetadataLoaded);
> 
> Ditto.

See my comment above.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1130
>>      self->m_owner->scheduleMainThreadNotification(MediaPlayerPrivateAVFoundation::Notification::SeekCompleted, static_cast<bool>(finished));
> 
> Ditto.

See my comment above.
Comment 5 Jeff Miller 2011-08-12 10:25:32 PDT
Comment on attachment 103713 [details]
Patch

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

>>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:927
>>> +    return it->second;
>> 
>> You might as well ASSERT that the map is locked in a debug build.
> 
> Good point, will do.

Actually, there isn't a way to test if a Mutex is locked by the current thread, so I can't ASSERT here.
Comment 6 Jeff Miller 2011-08-12 10:33:30 PDT
Comment on attachment 103713 [details]
Patch

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

>>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:865
>>> +    
>> 
>> This *could* reuse the ID of an existing wrapper. It isn't likely but it is definitely possible - imagine a page that has a static <video> element but also uses one-shot <audio> elements for sound effects. You might as well look up the "new" ID in the map to make sure it isn't already in use.
> 
> I don't know enough about media elements to understand the one-shot <audio> element example. I can add code to check for the ID already being in use, but it seems like if we did hit the case where we wrapped around and encountered an ID that was still in use, it would be equally likely that all the other IDs are also in use, in which case there's not much we could do.

I think that for now, I'll just assert in the function that adds to the map that the ID isn't already in use.
Comment 7 Jeff Miller 2011-08-12 11:28:45 PDT
Comment on attachment 103713 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:865
>>>> +    
>>> 
>>> This *could* reuse the ID of an existing wrapper. It isn't likely but it is definitely possible - imagine a page that has a static <video> element but also uses one-shot <audio> elements for sound effects. You might as well look up the "new" ID in the map to make sure it isn't already in use.
>> 
>> I don't know enough about media elements to understand the one-shot <audio> element example. I can add code to check for the ID already being in use, but it seems like if we did hit the case where we wrapped around and encountered an ID that was still in use, it would be equally likely that all the other IDs are also in use, in which case there's not much we could do.
> 
> I think that for now, I'll just assert in the function that adds to the map that the ID isn't already in use.

I talked to Eric some more, and I now understand the use case he was describing.  A page could have a long-lived media element and also create and tear down many short-lived one-shot <audio> elements. The ID space wouldn't be exhausted, but eventually a one-shot element could try to re-use the same ID as the long-lived media element.  I agree with Eric, we should make sure we handle this case.
Comment 8 Jeff Miller 2011-08-12 11:42:30 PDT
Committed r92977: <http://trac.webkit.org/changeset/92977>