[webkit-reviews] review granted: [Bug 65724] Need to handle kCACFContextNeedsFlushNotification notifications that arrive after the AVFWrapper has been disposed : [Attachment 103713] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jeff Miller
<jeffm at apple.com>'s request for review:
Bug 65724: Need to handle kCACFContextNeedsFlushNotification notifications that
arrive after the AVFWrapper has been disposed
https://bugs.webkit.org/show_bug.cgi?id=65724

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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::N
otification::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::N
otification::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::N
otification::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::N
otification::SeekCompleted, static_cast<bool>(finished));

Ditto.