Changeset 247331 in webkit


Ignore:
Timestamp:
Jul 10, 2019 4:08:27 PM (5 years ago)
Author:
Chris Dumez
Message:

Stop using GenericTaskQueue from multiple threads
https://bugs.webkit.org/show_bug.cgi?id=199652

Reviewed by Eric Carlson and Geoffrey Garen.

WebCoreAVFLoaderDelegate was calling GenericTaskQueue::enqueueTask() from a background thread,
which is not safe because the implementation of enqueueTask() calls makeWeakPtr() on the
GenericTaskQueue (a main thread object).

Update WebCoreAVFLoaderDelegate to make sure it is on the main thread before it calls
GenericTaskQueue::enqueueTask().

  • platform/GenericTaskQueue.h:

Remove last template parameter which was used exclusively by WebCoreAVFLoaderDelegate to try and
make GenericTaskQueue thread-safe.

  • platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:

(-[WebCoreAVFMovieObserver metadataLoaded]):
(-[WebCoreAVFMovieObserver didEnd:]):
(-[WebCoreAVFMovieObserver observeValueForKeyPath:ofObject:change:context:]):
(-[WebCoreAVFMovieObserver legibleOutput:didOutputAttributedStrings:nativeSampleBuffers:forItemTime:]):
(-[WebCoreAVFMovieObserver outputSequenceWasFlushed:]):
(-[WebCoreAVFLoaderDelegate resourceLoader:shouldWaitForLoadingOfRequestedResource:]):
(-[WebCoreAVFLoaderDelegate resourceLoader:didCancelLoadingRequest:]):

Location:
trunk/Source/WebCore
Files:
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r247330 r247331  
     12019-07-10  Chris Dumez  <cdumez@apple.com>
     2
     3        Stop using GenericTaskQueue from multiple threads
     4        https://bugs.webkit.org/show_bug.cgi?id=199652
     5
     6        Reviewed by Eric Carlson and Geoffrey Garen.
     7
     8        WebCoreAVFLoaderDelegate was calling GenericTaskQueue::enqueueTask() from a background thread,
     9        which is not safe because the implementation of enqueueTask() calls makeWeakPtr() on the
     10        GenericTaskQueue (a main thread object).
     11
     12        Update WebCoreAVFLoaderDelegate to make sure it is on the main thread before it calls
     13        GenericTaskQueue::enqueueTask().
     14
     15        * platform/GenericTaskQueue.h:
     16        Remove last template parameter which was used exclusively by WebCoreAVFLoaderDelegate to try and
     17        make GenericTaskQueue thread-safe.
     18
     19        * platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:
     20        (-[WebCoreAVFMovieObserver metadataLoaded]):
     21        (-[WebCoreAVFMovieObserver didEnd:]):
     22        (-[WebCoreAVFMovieObserver observeValueForKeyPath:ofObject:change:context:]):
     23        (-[WebCoreAVFMovieObserver legibleOutput:didOutputAttributedStrings:nativeSampleBuffers:forItemTime:]):
     24        (-[WebCoreAVFMovieObserver outputSequenceWasFlushed:]):
     25        (-[WebCoreAVFLoaderDelegate resourceLoader:shouldWaitForLoadingOfRequestedResource:]):
     26        (-[WebCoreAVFLoaderDelegate resourceLoader:didCancelLoadingRequest:]):
     27
    1282019-07-10  Chris Fleizach  <cfleizach@apple.com>
    229
  • trunk/Source/WebCore/platform/GenericTaskQueue.h

    r247323 r247331  
    7272};
    7373
    74 template <typename T, typename C = unsigned>
    75 class GenericTaskQueue : public CanMakeWeakPtr<GenericTaskQueue<T, C>> {
     74template <typename T>
     75class GenericTaskQueue : public CanMakeWeakPtr<GenericTaskQueue<T>> {
    7676public:
    7777    GenericTaskQueue()
     
    125125private:
    126126    TaskDispatcher<T> m_dispatcher;
    127     C m_pendingTasks { 0 };
     127    unsigned m_pendingTasks { 0 };
    128128    bool m_isClosed { false };
    129129};
  • trunk/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm

    r247323 r247331  
    167167};
    168168
     169static void ensureOnMainThread(Function<void()>&& f)
     170{
     171    if (isMainThread())
     172        f();
     173    else
     174        callOnMainThread(WTFMove(f));
     175}
     176
    169177@interface WebCoreAVFMovieObserver : NSObject <AVPlayerItemLegibleOutputPushDelegate>
    170178{
    171179    WeakPtr<MediaPlayerPrivateAVFoundationObjC> m_player;
    172     GenericTaskQueue<Timer, std::atomic<unsigned>> m_taskQueue;
     180    GenericTaskQueue<Timer> m_taskQueue;
    173181    int m_delayCallbacks;
    174182}
     
    185193@interface WebCoreAVFLoaderDelegate : NSObject<AVAssetResourceLoaderDelegate> {
    186194    WeakPtr<MediaPlayerPrivateAVFoundationObjC> m_player;
    187     GenericTaskQueue<Timer, std::atomic<unsigned>> m_taskQueue;
     195    GenericTaskQueue<Timer> m_taskQueue;
    188196}
    189197- (id)initWithPlayer:(WeakPtr<MediaPlayerPrivateAVFoundationObjC>&&)player;
     
    32883296- (void)metadataLoaded
    32893297{
    3290     m_taskQueue.enqueueTask([player = m_player] {
    3291         if (player)
    3292             player->metadataLoaded();
     3298    ensureOnMainThread([self, strongSelf = retainPtr(self)] {
     3299        m_taskQueue.enqueueTask([player = m_player] {
     3300            if (player)
     3301                player->metadataLoaded();
     3302        });
    32933303    });
    32943304}
     
    32973307{
    32983308    UNUSED_PARAM(unusedNotification);
    3299     m_taskQueue.enqueueTask([player = m_player] {
    3300         if (player)
    3301             player->didEnd();
     3309    ensureOnMainThread([self, strongSelf = retainPtr(self)] {
     3310        m_taskQueue.enqueueTask([player = m_player] {
     3311            if (player)
     3312                player->didEnd();
     3313        });
    33023314    });
    33033315}
     
    33053317- (void)observeValueForKeyPath:keyPath ofObject:(id)object change:(NSDictionary *)change context:(MediaPlayerAVFoundationObservationContext)context
    33063318{
    3307     m_taskQueue.enqueueTask([player = m_player, keyPath = retainPtr(keyPath), change = retainPtr(change), object = retainPtr(object), context] {
    3308         if (!player)
    3309             return;
    3310         id newValue = [change valueForKey:NSKeyValueChangeNewKey];
    3311         bool willChange = [[change valueForKey:NSKeyValueChangeNotificationIsPriorKey] boolValue];
    3312         bool shouldLogValue = !willChange;
    3313 
    3314         if (context == MediaPlayerAVFoundationObservationContextAVPlayerLayer) {
    3315             if ([keyPath isEqualToString:@"readyForDisplay"])
    3316                 player->firstFrameAvailableDidChange([newValue boolValue]);
    3317         }
    3318 
    3319         if (context == MediaPlayerAVFoundationObservationContextPlayerItemTrack) {
    3320             if ([keyPath isEqualToString:@"enabled"])
    3321                 player->trackEnabledDidChange([newValue boolValue]);
    3322         }
    3323 
    3324         if (context == MediaPlayerAVFoundationObservationContextPlayerItem && willChange) {
    3325             if ([keyPath isEqualToString:@"playbackLikelyToKeepUp"])
    3326                 player->playbackLikelyToKeepUpWillChange();
    3327             else if ([keyPath isEqualToString:@"playbackBufferEmpty"])
    3328                 player->playbackBufferEmptyWillChange();
    3329             else if ([keyPath isEqualToString:@"playbackBufferFull"])
    3330                 player->playbackBufferFullWillChange();
    3331         }
    3332 
    3333         if (context == MediaPlayerAVFoundationObservationContextPlayerItem && !willChange) {
    3334             // A value changed for an AVPlayerItem
    3335             if ([keyPath isEqualToString:@"status"])
    3336                 player->playerItemStatusDidChange([newValue intValue]);
    3337             else if ([keyPath isEqualToString:@"playbackLikelyToKeepUp"])
    3338                 player->playbackLikelyToKeepUpDidChange([newValue boolValue]);
    3339             else if ([keyPath isEqualToString:@"playbackBufferEmpty"])
    3340                 player->playbackBufferEmptyDidChange([newValue boolValue]);
    3341             else if ([keyPath isEqualToString:@"playbackBufferFull"])
    3342                 player->playbackBufferFullDidChange([newValue boolValue]);
    3343             else if ([keyPath isEqualToString:@"asset"]) {
    3344                 player->setAsset(RetainPtr<id>(newValue));
    3345                 shouldLogValue = false;
    3346             } else if ([keyPath isEqualToString:@"loadedTimeRanges"])
    3347                 player->loadedTimeRangesDidChange(RetainPtr<NSArray>(newValue));
    3348             else if ([keyPath isEqualToString:@"seekableTimeRanges"])
    3349                 player->seekableTimeRangesDidChange(RetainPtr<NSArray>(newValue));
    3350             else if ([keyPath isEqualToString:@"tracks"]) {
    3351                 player->tracksDidChange(RetainPtr<NSArray>(newValue));
    3352                 shouldLogValue = false;
    3353             } else if ([keyPath isEqualToString:@"hasEnabledAudio"])
    3354                 player->hasEnabledAudioDidChange([newValue boolValue]);
    3355             else if ([keyPath isEqualToString:@"presentationSize"])
    3356                 player->presentationSizeDidChange(FloatSize([newValue sizeValue]));
    3357             else if ([keyPath isEqualToString:@"duration"])
    3358                 player->durationDidChange(PAL::toMediaTime([newValue CMTimeValue]));
    3359             else if ([keyPath isEqualToString:@"timedMetadata"] && newValue) {
    3360                 MediaTime now;
    3361                 CMTime itemTime = [(AVPlayerItem *)object.get() currentTime];
    3362                 if (CMTIME_IS_NUMERIC(itemTime))
    3363                     now = std::max(PAL::toMediaTime(itemTime), MediaTime::zeroTime());
    3364                 player->metadataDidArrive(RetainPtr<NSArray>(newValue), now);
    3365                 shouldLogValue = false;
    3366             } else if ([keyPath isEqualToString:@"canPlayFastReverse"])
    3367                 player->canPlayFastReverseDidChange([newValue boolValue]);
    3368             else if ([keyPath isEqualToString:@"canPlayFastForward"])
    3369                 player->canPlayFastForwardDidChange([newValue boolValue]);
    3370         }
    3371 
    3372         if (context == MediaPlayerAVFoundationObservationContextPlayer && !willChange) {
    3373             // A value changed for an AVPlayer.
    3374             if ([keyPath isEqualToString:@"rate"])
    3375                 player->rateDidChange([newValue doubleValue]);
    3376             else if ([keyPath isEqualToString:@"timeControlStatus"])
    3377                 player->timeControlStatusDidChange([newValue intValue]);
     3319    ensureOnMainThread([self, strongSelf = retainPtr(self), keyPath = retainPtr(keyPath), change = retainPtr(change), object = retainPtr(object), context]() mutable {
     3320        m_taskQueue.enqueueTask([player = m_player, keyPath = WTFMove(keyPath), change = WTFMove(change), object = WTFMove(object), context] {
     3321            if (!player)
     3322                return;
     3323            id newValue = [change valueForKey:NSKeyValueChangeNewKey];
     3324            bool willChange = [[change valueForKey:NSKeyValueChangeNotificationIsPriorKey] boolValue];
     3325            bool shouldLogValue = !willChange;
     3326
     3327            if (context == MediaPlayerAVFoundationObservationContextAVPlayerLayer) {
     3328                if ([keyPath isEqualToString:@"readyForDisplay"])
     3329                    player->firstFrameAvailableDidChange([newValue boolValue]);
     3330            }
     3331
     3332            if (context == MediaPlayerAVFoundationObservationContextPlayerItemTrack) {
     3333                if ([keyPath isEqualToString:@"enabled"])
     3334                    player->trackEnabledDidChange([newValue boolValue]);
     3335            }
     3336
     3337            if (context == MediaPlayerAVFoundationObservationContextPlayerItem && willChange) {
     3338                if ([keyPath isEqualToString:@"playbackLikelyToKeepUp"])
     3339                    player->playbackLikelyToKeepUpWillChange();
     3340                else if ([keyPath isEqualToString:@"playbackBufferEmpty"])
     3341                    player->playbackBufferEmptyWillChange();
     3342                else if ([keyPath isEqualToString:@"playbackBufferFull"])
     3343                    player->playbackBufferFullWillChange();
     3344            }
     3345
     3346            if (context == MediaPlayerAVFoundationObservationContextPlayerItem && !willChange) {
     3347                // A value changed for an AVPlayerItem
     3348                if ([keyPath isEqualToString:@"status"])
     3349                    player->playerItemStatusDidChange([newValue intValue]);
     3350                else if ([keyPath isEqualToString:@"playbackLikelyToKeepUp"])
     3351                    player->playbackLikelyToKeepUpDidChange([newValue boolValue]);
     3352                else if ([keyPath isEqualToString:@"playbackBufferEmpty"])
     3353                    player->playbackBufferEmptyDidChange([newValue boolValue]);
     3354                else if ([keyPath isEqualToString:@"playbackBufferFull"])
     3355                    player->playbackBufferFullDidChange([newValue boolValue]);
     3356                else if ([keyPath isEqualToString:@"asset"]) {
     3357                    player->setAsset(RetainPtr<id>(newValue));
     3358                    shouldLogValue = false;
     3359                } else if ([keyPath isEqualToString:@"loadedTimeRanges"])
     3360                    player->loadedTimeRangesDidChange(RetainPtr<NSArray>(newValue));
     3361                else if ([keyPath isEqualToString:@"seekableTimeRanges"])
     3362                    player->seekableTimeRangesDidChange(RetainPtr<NSArray>(newValue));
     3363                else if ([keyPath isEqualToString:@"tracks"]) {
     3364                    player->tracksDidChange(RetainPtr<NSArray>(newValue));
     3365                    shouldLogValue = false;
     3366                } else if ([keyPath isEqualToString:@"hasEnabledAudio"])
     3367                    player->hasEnabledAudioDidChange([newValue boolValue]);
     3368                else if ([keyPath isEqualToString:@"presentationSize"])
     3369                    player->presentationSizeDidChange(FloatSize([newValue sizeValue]));
     3370                else if ([keyPath isEqualToString:@"duration"])
     3371                    player->durationDidChange(PAL::toMediaTime([newValue CMTimeValue]));
     3372                else if ([keyPath isEqualToString:@"timedMetadata"] && newValue) {
     3373                    MediaTime now;
     3374                    CMTime itemTime = [(AVPlayerItem *)object.get() currentTime];
     3375                    if (CMTIME_IS_NUMERIC(itemTime))
     3376                        now = std::max(PAL::toMediaTime(itemTime), MediaTime::zeroTime());
     3377                    player->metadataDidArrive(RetainPtr<NSArray>(newValue), now);
     3378                    shouldLogValue = false;
     3379                } else if ([keyPath isEqualToString:@"canPlayFastReverse"])
     3380                    player->canPlayFastReverseDidChange([newValue boolValue]);
     3381                else if ([keyPath isEqualToString:@"canPlayFastForward"])
     3382                    player->canPlayFastForwardDidChange([newValue boolValue]);
     3383            }
     3384
     3385            if (context == MediaPlayerAVFoundationObservationContextPlayer && !willChange) {
     3386                // A value changed for an AVPlayer.
     3387                if ([keyPath isEqualToString:@"rate"])
     3388                    player->rateDidChange([newValue doubleValue]);
     3389                else if ([keyPath isEqualToString:@"timeControlStatus"])
     3390                    player->timeControlStatusDidChange([newValue intValue]);
    33783391#if ENABLE(WIRELESS_PLAYBACK_TARGET)
    3379             else if ([keyPath isEqualToString:@"externalPlaybackActive"] || [keyPath isEqualToString:@"allowsExternalPlayback"])
    3380                 player->playbackTargetIsWirelessDidChange();
     3392                else if ([keyPath isEqualToString:@"externalPlaybackActive"] || [keyPath isEqualToString:@"allowsExternalPlayback"])
     3393                    player->playbackTargetIsWirelessDidChange();
    33813394#endif
    33823395#if ENABLE(LEGACY_ENCRYPTED_MEDIA) || ENABLE(ENCRYPTED_MEDIA)
    3383             else if ([keyPath isEqualToString:@"outputObscuredDueToInsufficientExternalProtection"])
    3384                 player->outputObscuredDueToInsufficientExternalProtectionChanged([newValue boolValue]);
    3385 #endif
    3386         }
     3396                else if ([keyPath isEqualToString:@"outputObscuredDueToInsufficientExternalProtection"])
     3397                    player->outputObscuredDueToInsufficientExternalProtectionChanged([newValue boolValue]);
     3398#endif
     3399            }
    33873400
    33883401#if !RELEASE_LOG_DISABLED
    3389         if (player->logger().willLog(player->logChannel(), WTFLogLevel::Debug) && !([keyPath isEqualToString:@"loadedTimeRanges"] || [keyPath isEqualToString:@"seekableTimeRanges"])) {
    3390             auto identifier = Logger::LogSiteIdentifier("MediaPlayerPrivateAVFoundation", "observeValueForKeyPath", player->logIdentifier());
    3391 
    3392             if (shouldLogValue) {
    3393                 if ([keyPath isEqualToString:@"duration"])
    3394                     player->logger().debug(player->logChannel(), identifier, "did change '", [keyPath UTF8String], "' to ", PAL::toMediaTime([newValue CMTimeValue]));
    3395                 else {
    3396                     RetainPtr<NSString> valueString = adoptNS([[NSString alloc] initWithFormat:@"%@", newValue]);
    3397                     player->logger().debug(player->logChannel(), identifier, "did change '", [keyPath UTF8String], "' to ", [valueString.get() UTF8String]);
    3398                 }
    3399             } else
    3400                 player->logger().debug(player->logChannel(), identifier, willChange ? "will" : "did", " change '", [keyPath UTF8String], "'");
    3401         }
    3402 #endif
     3402            if (player->logger().willLog(player->logChannel(), WTFLogLevel::Debug) && !([keyPath isEqualToString:@"loadedTimeRanges"] || [keyPath isEqualToString:@"seekableTimeRanges"])) {
     3403                auto identifier = Logger::LogSiteIdentifier("MediaPlayerPrivateAVFoundation", "observeValueForKeyPath", player->logIdentifier());
     3404
     3405                if (shouldLogValue) {
     3406                    if ([keyPath isEqualToString:@"duration"])
     3407                        player->logger().debug(player->logChannel(), identifier, "did change '", [keyPath UTF8String], "' to ", PAL::toMediaTime([newValue CMTimeValue]));
     3408                    else {
     3409                        RetainPtr<NSString> valueString = adoptNS([[NSString alloc] initWithFormat:@"%@", newValue]);
     3410                        player->logger().debug(player->logChannel(), identifier, "did change '", [keyPath UTF8String], "' to ", [valueString.get() UTF8String]);
     3411                    }
     3412                } else
     3413                    player->logger().debug(player->logChannel(), identifier, willChange ? "will" : "did", " change '", [keyPath UTF8String], "'");
     3414            }
     3415#endif
     3416        });
    34033417    });
    34043418}
     
    34083422    UNUSED_PARAM(output);
    34093423
    3410     m_taskQueue.enqueueTask([player = m_player, strings = retainPtr(strings), nativeSamples = retainPtr(nativeSamples), itemTime] {
    3411         if (!player)
    3412             return;
    3413         MediaTime time = std::max(PAL::toMediaTime(itemTime), MediaTime::zeroTime());
    3414         player->processCue(strings.get(), nativeSamples.get(), time);
     3424    ensureOnMainThread([self, strongSelf = retainPtr(self), strings = retainPtr(strings), nativeSamples = retainPtr(nativeSamples), itemTime]() mutable {
     3425        m_taskQueue.enqueueTask([player = m_player, strings = WTFMove(strings), nativeSamples = WTFMove(nativeSamples), itemTime] {
     3426            if (!player)
     3427                return;
     3428            MediaTime time = std::max(PAL::toMediaTime(itemTime), MediaTime::zeroTime());
     3429            player->processCue(strings.get(), nativeSamples.get(), time);
     3430        });
    34153431    });
    34163432}
     
    34203436    UNUSED_PARAM(output);
    34213437
    3422     m_taskQueue.enqueueTask([player = m_player] {
    3423         if (player)
    3424             player->flushCues();
     3438    ensureOnMainThread([self, strongSelf = retainPtr(self)] {
     3439        m_taskQueue.enqueueTask([player = m_player] {
     3440            if (player)
     3441                player->flushCues();
     3442        });
    34253443    });
    34263444}
     
    34473465        return NO;
    34483466
    3449     m_taskQueue.enqueueTask([player = m_player, loadingRequest = retainPtr(loadingRequest)] {
    3450         if (!player) {
    3451             [loadingRequest finishLoadingWithError:nil];
    3452             return;
    3453         }
    3454 
    3455         if (!player->shouldWaitForLoadingOfResource(loadingRequest.get()))
    3456             [loadingRequest finishLoadingWithError:nil];
     3467    ensureOnMainThread([self, strongSelf = retainPtr(self), loadingRequest = retainPtr(loadingRequest)]() mutable {
     3468        m_taskQueue.enqueueTask([player = m_player, loadingRequest = WTFMove(loadingRequest)] {
     3469            if (!player) {
     3470                [loadingRequest finishLoadingWithError:nil];
     3471                return;
     3472            }
     3473
     3474            if (!player->shouldWaitForLoadingOfResource(loadingRequest.get()))
     3475                [loadingRequest finishLoadingWithError:nil];
     3476        });
    34573477    });
    34583478
     
    34713491{
    34723492    UNUSED_PARAM(resourceLoader);
    3473     m_taskQueue.enqueueTask([player = m_player, loadingRequest = retainPtr(loadingRequest)] {
    3474         if (player)
    3475             player->didCancelLoadingRequest(loadingRequest.get());
     3493    ensureOnMainThread([self, strongSelf = retainPtr(self), loadingRequest = retainPtr(loadingRequest)]() mutable {
     3494        m_taskQueue.enqueueTask([player = m_player, loadingRequest = WTFMove(loadingRequest)] {
     3495            if (player)
     3496                player->didCancelLoadingRequest(loadingRequest.get());
     3497        });
    34763498    });
    34773499}
Note: See TracChangeset for help on using the changeset viewer.