Skip to content

Commit

Permalink
[Cocoa] Thread-unsafe use of Vector in WebAVSampleBufferErrorListener
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=264217
rdar://117950166

Reviewed by Eric Carlson.

WebAVSampleBufferErrorListener contains two Vectors (_layers and _renderers) that are accessed from
the main thread as well as whichever thread AVFoundation uses to post notifications and key-value
observation callbacks; this is unsafe.

Addressed this by ensuring both Vectors are accessed on the main thread. Also did the following:
- Renamed WebAVSampleBufferErrorListener to WebAVSampleBufferListener since it observes more than
just errors.
- Made use of context pointers in the key-value observer to make WebAVSampleBufferListener safe for
subclassing.
- Fixed some style issues.

* Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.h:
* Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:
(-[WebAVSampleBufferListener initWithParent:]):
(-[WebAVSampleBufferListener invalidate]):
(-[WebAVSampleBufferListener beginObservingLayer:]):
(-[WebAVSampleBufferListener stopObservingLayer:]):
(-[WebAVSampleBufferListener beginObservingRenderer:]):
(-[WebAVSampleBufferListener stopObservingRenderer:]):
(-[WebAVSampleBufferListener observeValueForKeyPath:ofObject:change:context:]):
(-[WebAVSampleBufferListener layerFailedToDecode:]):
(-[WebAVSampleBufferListener layerRequiresFlushToResumeDecodingChanged:]):
(-[WebAVSampleBufferListener audioRendererWasAutomaticallyFlushed:]):
(WebCore::SourceBufferPrivateAVFObjC::SourceBufferPrivateAVFObjC):
(WebCore::SourceBufferPrivateAVFObjC::destroyRenderers):
(WebCore::SourceBufferPrivateAVFObjC::trackDidChangeEnabled):
(WebCore::SourceBufferPrivateAVFObjC::setVideoLayer):
(-[WebAVSampleBufferErrorListener initWithParent:]): Deleted.
(-[WebAVSampleBufferErrorListener dealloc]): Deleted.
(-[WebAVSampleBufferErrorListener invalidate]): Deleted.
(-[WebAVSampleBufferErrorListener beginObservingLayer:]): Deleted.
(-[WebAVSampleBufferErrorListener stopObservingLayer:]): Deleted.
(-[WebAVSampleBufferErrorListener beginObservingRenderer:]): Deleted.
(-[WebAVSampleBufferErrorListener stopObservingRenderer:]): Deleted.
(-[WebAVSampleBufferErrorListener observeValueForKeyPath:ofObject:change:context:]): Deleted.
(-[WebAVSampleBufferErrorListener layerFailedToDecode:]): Deleted.
(-[WebAVSampleBufferErrorListener layerRequiresFlushToResumeDecodingChanged:]): Deleted.
(-[WebAVSampleBufferErrorListener layerReadyForDisplayChanged:]): Deleted.
(-[WebAVSampleBufferErrorListener audioRendererWasAutomaticallyFlushed:]): Deleted.

Canonical link: https://commits.webkit.org/270237@main
  • Loading branch information
aestes committed Nov 5, 2023
1 parent d7fa425 commit 66793be
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 85 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ OBJC_CLASS AVSampleBufferDisplayLayer;
OBJC_CLASS NSData;
OBJC_CLASS NSError;
OBJC_CLASS NSObject;
OBJC_CLASS WebAVSampleBufferErrorListener;
OBJC_CLASS WebAVSampleBufferListener;

typedef struct opaqueCMSampleBuffer *CMSampleBufferRef;
typedef const struct opaqueCMFormatDescription *CMFormatDescriptionRef;
Expand Down Expand Up @@ -219,7 +219,7 @@ ALLOW_NEW_API_WITHOUT_GUARDS_END
ALLOW_NEW_API_WITHOUT_GUARDS_BEGIN
HashMap<uint64_t, RetainPtr<AVSampleBufferAudioRenderer>, DefaultHash<uint64_t>, WTF::UnsignedWithZeroKeyHashTraits<uint64_t>> m_audioRenderers;
ALLOW_NEW_API_WITHOUT_GUARDS_END
RetainPtr<WebAVSampleBufferErrorListener> m_errorListener;
RetainPtr<WebAVSampleBufferListener> m_listener;
#if PLATFORM(IOS_FAMILY)
bool m_displayLayerWasInterrupted { false };
#endif
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@
#import <pal/cocoa/AVFoundationSoftLink.h>

@interface AVSampleBufferDisplayLayer (WebCoreAVSampleBufferDisplayLayerQueueManagementPrivate)
- (void)expectMinimumUpcomingSampleBufferPresentationTime: (CMTime)minimumUpcomingPresentationTime;
- (void)expectMinimumUpcomingSampleBufferPresentationTime:(CMTime)minimumUpcomingPresentationTime;
- (void)resetUpcomingSampleBufferPresentationTimeExpectations;
@end

Expand All @@ -89,15 +89,23 @@ @interface AVSampleBufferDisplayLayer (Staging_113656776)
@end
#endif

@interface WebAVSampleBufferErrorListener : NSObject {
static NSString * const errorKeyPath = @"error";
static NSString * const outputObscuredDueToInsufficientExternalProtectionKeyPath = @"outputObscuredDueToInsufficientExternalProtection";

static void* errorContext = &errorContext;
static void* outputObscuredDueToInsufficientExternalProtectionContext = &outputObscuredDueToInsufficientExternalProtectionContext;

@interface WebAVSampleBufferListener : NSObject {
WeakPtr<WebCore::SourceBufferPrivateAVFObjC> _parent;
Vector<RetainPtr<AVSampleBufferDisplayLayer>> _layers;
ALLOW_NEW_API_WITHOUT_GUARDS_BEGIN
Vector<RetainPtr<AVSampleBufferAudioRenderer>> _renderers;
ALLOW_NEW_API_WITHOUT_GUARDS_END
}

- (id)initWithParent:(WeakPtr<WebCore::SourceBufferPrivateAVFObjC>&&)parent;
+ (instancetype)new NS_UNAVAILABLE;
- (instancetype)init NS_UNAVAILABLE;
- (instancetype)initWithParent:(WebCore::SourceBufferPrivateAVFObjC&)parent NS_DESIGNATED_INITIALIZER;
- (void)invalidate;
- (void)beginObservingLayer:(AVSampleBufferDisplayLayer *)layer;
- (void)stopObservingLayer:(AVSampleBufferDisplayLayer *)layer;
Expand All @@ -107,14 +115,14 @@ - (void)stopObservingRenderer:(AVSampleBufferAudioRenderer *)renderer;
ALLOW_NEW_API_WITHOUT_GUARDS_END
@end

@implementation WebAVSampleBufferErrorListener
@implementation WebAVSampleBufferListener

- (id)initWithParent:(WeakPtr<WebCore::SourceBufferPrivateAVFObjC>&&)parent
- (instancetype)initWithParent:(WebCore::SourceBufferPrivateAVFObjC&)parent
{
if (!(self = [super init]))
return nil;

_parent = WTFMove(parent);
_parent = parent;
return self;
}

Expand All @@ -126,32 +134,34 @@ - (void)dealloc

- (void)invalidate
{
ASSERT(isMainThread());
if (!_parent && !_layers.size() && !_renderers.size())
return;

for (auto& layer : _layers) {
[layer removeObserver:self forKeyPath:@"error"];
[layer removeObserver:self forKeyPath:@"outputObscuredDueToInsufficientExternalProtection"];
[layer removeObserver:self forKeyPath:errorKeyPath];
[layer removeObserver:self forKeyPath:outputObscuredDueToInsufficientExternalProtectionKeyPath];
}
_layers.clear();

for (auto& renderer : _renderers)
[renderer removeObserver:self forKeyPath:@"error"];
[renderer removeObserver:self forKeyPath:errorKeyPath];
_renderers.clear();

[[NSNotificationCenter defaultCenter] removeObserver:self];
[NSNotificationCenter.defaultCenter removeObserver:self];

_parent = nullptr;
}

- (void)beginObservingLayer:(AVSampleBufferDisplayLayer *)layer
{
ASSERT(isMainThread());
ASSERT(_parent);
ASSERT(!_layers.contains(layer));

_layers.append(layer);
[layer addObserver:self forKeyPath:@"error" options:NSKeyValueObservingOptionNew context:nullptr];
[layer addObserver:self forKeyPath:@"outputObscuredDueToInsufficientExternalProtection" options:NSKeyValueObservingOptionNew context:nullptr];
[layer addObserver:self forKeyPath:errorKeyPath options:NSKeyValueObservingOptionNew context:errorContext];
[layer addObserver:self forKeyPath:outputObscuredDueToInsufficientExternalProtectionKeyPath options:NSKeyValueObservingOptionNew context:outputObscuredDueToInsufficientExternalProtectionContext];
[NSNotificationCenter.defaultCenter addObserver:self selector:@selector(layerFailedToDecode:) name:AVSampleBufferDisplayLayerFailedToDecodeNotification object:layer];
[NSNotificationCenter.defaultCenter addObserver:self selector:@selector(layerRequiresFlushToResumeDecodingChanged:) name:AVSampleBufferDisplayLayerRequiresFlushToResumeDecodingDidChangeNotification object:layer];
#if HAVE(AVSAMPLEBUFFERDISPLAYLAYER_READYFORDISPLAY)
Expand All @@ -163,11 +173,12 @@ - (void)beginObservingLayer:(AVSampleBufferDisplayLayer *)layer

- (void)stopObservingLayer:(AVSampleBufferDisplayLayer *)layer
{
ASSERT(isMainThread());
ASSERT(_parent);
ASSERT(_layers.contains(layer));

[layer removeObserver:self forKeyPath:@"error"];
[layer removeObserver:self forKeyPath:@"outputObscuredDueToInsufficientExternalProtection"];
[layer removeObserver:self forKeyPath:errorKeyPath];
[layer removeObserver:self forKeyPath:outputObscuredDueToInsufficientExternalProtectionKeyPath];
_layers.remove(_layers.find(layer));

[NSNotificationCenter.defaultCenter removeObserver:self name:AVSampleBufferDisplayLayerFailedToDecodeNotification object:layer];
Expand All @@ -180,99 +191,107 @@ - (void)stopObservingLayer:(AVSampleBufferDisplayLayer *)layer
}

ALLOW_NEW_API_WITHOUT_GUARDS_BEGIN
- (void)beginObservingRenderer:(AVSampleBufferAudioRenderer*)renderer
{
- (void)beginObservingRenderer:(AVSampleBufferAudioRenderer *)renderer
ALLOW_NEW_API_WITHOUT_GUARDS_END
{
ASSERT(isMainThread());
ASSERT(_parent);
ASSERT(!_renderers.contains(renderer));

_renderers.append(renderer);
[renderer addObserver:self forKeyPath:@"error" options:NSKeyValueObservingOptionNew context:nullptr];
[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(audioRendererWasAutomaticallyFlushed:) name:AVSampleBufferAudioRendererWasFlushedAutomaticallyNotification object:renderer];
[renderer addObserver:self forKeyPath:errorKeyPath options:NSKeyValueObservingOptionNew context:errorContext];
[NSNotificationCenter.defaultCenter addObserver:self selector:@selector(audioRendererWasAutomaticallyFlushed:) name:AVSampleBufferAudioRendererWasFlushedAutomaticallyNotification object:renderer];
}

ALLOW_NEW_API_WITHOUT_GUARDS_BEGIN
- (void)stopObservingRenderer:(AVSampleBufferAudioRenderer*)renderer
- (void)stopObservingRenderer:(AVSampleBufferAudioRenderer *)renderer
ALLOW_NEW_API_WITHOUT_GUARDS_END
{
ASSERT(isMainThread());
ASSERT(_parent);
ASSERT(_renderers.contains(renderer));

[renderer removeObserver:self forKeyPath:@"error"];
[[NSNotificationCenter defaultCenter] removeObserver:self name:AVSampleBufferAudioRendererWasFlushedAutomaticallyNotification object:renderer];
[renderer removeObserver:self forKeyPath:errorKeyPath];
[NSNotificationCenter.defaultCenter removeObserver:self name:AVSampleBufferAudioRendererWasFlushedAutomaticallyNotification object:renderer];

_renderers.remove(_renderers.find(renderer));
}

- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context
- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void*)context
{
UNUSED_PARAM(context);
UNUSED_PARAM(keyPath);
ASSERT(_parent);

if ([object isKindOfClass:PAL::getAVSampleBufferDisplayLayerClass()]) {
RetainPtr<AVSampleBufferDisplayLayer> layer = (AVSampleBufferDisplayLayer *)object;
ASSERT(_layers.contains(layer.get()));
if (context == errorContext) {
RetainPtr error = dynamic_objc_cast<NSError>([change valueForKey:NSKeyValueChangeNewKey]);
if (!error)
return;

if ([keyPath isEqualToString:@"error"]) {
RetainPtr<NSError> error = [change valueForKey:NSKeyValueChangeNewKey];
if ([error isKindOfClass:[NSNull class]])
return;
if ([object isKindOfClass:PAL::getAVSampleBufferDisplayLayerClass()]) {
RetainPtr layer = (AVSampleBufferDisplayLayer *)object;

callOnMainThread([parent = _parent, layer = WTFMove(layer), error = WTFMove(error)] {
if (auto strongParent = RefPtr { parent.get() })
strongParent->layerDidReceiveError(layer.get(), error.get());
});
} else if ([keyPath isEqualToString:@"outputObscuredDueToInsufficientExternalProtection"]) {
callOnMainThread([parent = _parent, obscured = [[change valueForKey:NSKeyValueChangeNewKey] boolValue]] {
if (auto strongParent = RefPtr { parent.get() })
strongParent->outputObscuredDueToInsufficientExternalProtectionChanged(obscured);
ensureOnMainThread([self, protectedSelf = RetainPtr { self }, layer = WTFMove(layer), error = WTFMove(error)] {
ASSERT(_layers.contains(layer.get()));
if (RefPtr parent = _parent.get())
parent->layerDidReceiveError(layer.get(), error.get());
});
} else
ASSERT_NOT_REACHED();
return;
}

} else if ([object isKindOfClass:PAL::getAVSampleBufferAudioRendererClass()]) {
if ([object isKindOfClass:PAL::getAVSampleBufferAudioRendererClass()]) {
ALLOW_NEW_API_WITHOUT_GUARDS_BEGIN
RetainPtr<AVSampleBufferAudioRenderer> renderer = (AVSampleBufferAudioRenderer *)object;
RetainPtr renderer = (AVSampleBufferAudioRenderer *)object;
ALLOW_NEW_API_WITHOUT_GUARDS_END
RetainPtr<NSError> error = [change valueForKey:NSKeyValueChangeNewKey];
if ([error isKindOfClass:[NSNull class]])

ensureOnMainThread([self, protectedSelf = RetainPtr { self }, renderer = WTFMove(renderer), error = WTFMove(error)] {
ASSERT(_renderers.contains(renderer.get()));
if (RefPtr parent = _parent.get())
parent->rendererDidReceiveError(renderer.get(), error.get());
});
return;
}

ASSERT(_renderers.contains(renderer.get()));
ASSERT([keyPath isEqualToString:@"error"]);
ASSERT_NOT_REACHED_WITH_MESSAGE("Unexpected kind of object while observing errorContext");
return;
}

if (context == outputObscuredDueToInsufficientExternalProtectionContext) {
RetainPtr layer = dynamic_objc_cast<AVSampleBufferDisplayLayer>(object, PAL::getAVSampleBufferDisplayLayerClass());
BOOL isObscured = [[change valueForKey:NSKeyValueChangeNewKey] boolValue];

callOnMainThread([parent = _parent, renderer = WTFMove(renderer), error = WTFMove(error)] {
if (auto strongParent = RefPtr { parent.get() })
strongParent->rendererDidReceiveError(renderer.get(), error.get());
ensureOnMainThread([self, protectedSelf = RetainPtr { self }, layer = WTFMove(layer), isObscured] {
ASSERT(_layers.contains(layer.get()));
if (RefPtr parent = _parent.get())
parent->outputObscuredDueToInsufficientExternalProtectionChanged(isObscured);
});
} else
ASSERT_NOT_REACHED();
return;
}

[super observeValueForKeyPath:keyPath ofObject:object change:change context:context];
}

- (void)layerFailedToDecode:(NSNotification*)note
- (void)layerFailedToDecode:(NSNotification *)notification
{
RetainPtr<AVSampleBufferDisplayLayer> layer = (AVSampleBufferDisplayLayer *)[note object];
if (!_layers.contains(layer.get()))
return;
RetainPtr layer = dynamic_objc_cast<AVSampleBufferDisplayLayer>(notification.object, PAL::getAVSampleBufferDisplayLayerClass());
RetainPtr error = dynamic_objc_cast<NSError>([notification.userInfo valueForKey:AVSampleBufferDisplayLayerFailedToDecodeNotificationErrorKey]);

callOnMainThread([parent = _parent, layer = WTFMove(layer), error = retainPtr([[note userInfo] valueForKey:AVSampleBufferDisplayLayerFailedToDecodeNotificationErrorKey])] {
if (auto strongParent = RefPtr { parent.get() })
strongParent->layerDidReceiveError(layer.get(), error.get());
ensureOnMainThread([self, protectedSelf = RetainPtr { self }, layer = WTFMove(layer), error = WTFMove(error)] {
if (!_layers.contains(layer.get()))
return;
if (RefPtr parent = _parent.get())
parent->layerDidReceiveError(layer.get(), error.get());
});
}

- (void)layerRequiresFlushToResumeDecodingChanged:(NSNotification *)note
- (void)layerRequiresFlushToResumeDecodingChanged:(NSNotification *)notification
{
RetainPtr<AVSampleBufferDisplayLayer> layer = (AVSampleBufferDisplayLayer *)[note object];
if (!_layers.contains(layer.get()))
return;

bool requiresFlush = [layer requiresFlushToResumeDecoding];
RetainPtr layer = dynamic_objc_cast<AVSampleBufferDisplayLayer>(notification.object, PAL::getAVSampleBufferDisplayLayerClass());
BOOL requiresFlush = [layer requiresFlushToResumeDecoding];

callOnMainThread([parent = _parent, layer = WTFMove(layer), requiresFlush] {
if (auto strongParent = RefPtr { parent.get() })
strongParent->layerRequiresFlushToResumeDecodingChanged(layer.get(), requiresFlush);
ensureOnMainThread([self, protectedSelf = RetainPtr { self }, layer = WTFMove(layer), requiresFlush] {
if (!_layers.contains(layer.get()))
return;
if (RefPtr parent = _parent.get())
parent->layerRequiresFlushToResumeDecodingChanged(layer.get(), requiresFlush);
});
}

Expand All @@ -291,17 +310,19 @@ - (void)layerReadyForDisplayChanged:(NSNotification *)notification
}
#endif

- (void)audioRendererWasAutomaticallyFlushed:(NSNotification*)note
- (void)audioRendererWasAutomaticallyFlushed:(NSNotification *)notification
{
RetainPtr<AVSampleBufferAudioRenderer> renderer = (AVSampleBufferAudioRenderer *)[note object];
if (!_renderers.contains(renderer.get()))
return;
RetainPtr renderer = dynamic_objc_cast<AVSampleBufferAudioRenderer>(notification.object, PAL::getAVSampleBufferAudioRendererClass());
CMTime flushTime = [[notification.userInfo valueForKey:AVSampleBufferAudioRendererFlushTimeKey] CMTimeValue];

callOnMainThread([parent = _parent, renderer = WTFMove(renderer), flushTime = [[[note userInfo] valueForKey:AVSampleBufferAudioRendererFlushTimeKey] CMTimeValue]] {
if (auto strongParent = RefPtr { parent.get() })
strongParent->rendererWasAutomaticallyFlushed(renderer.get(), flushTime);
ensureOnMainThread([self, protectedSelf = RetainPtr { self }, renderer = WTFMove(renderer), flushTime] {
if (!_renderers.contains(renderer.get()))
return;
if (RefPtr parent = _parent.get())
parent->rendererWasAutomaticallyFlushed(renderer.get(), flushTime);
});
}

@end

namespace WebCore {
Expand Down Expand Up @@ -343,7 +364,7 @@ static bool sampleBufferRenderersSupportKeySession()

SourceBufferPrivateAVFObjC::SourceBufferPrivateAVFObjC(MediaSourcePrivateAVFObjC* parent, Ref<SourceBufferParser>&& parser)
: m_parser(WTFMove(parser))
, m_errorListener(adoptNS([[WebAVSampleBufferErrorListener alloc] initWithParent:*this]))
, m_listener(adoptNS([[WebAVSampleBufferListener alloc] initWithParent:*this]))
, m_appendQueue(WorkQueue::create("SourceBufferPrivateAVFObjC data parser queue"))
, m_mediaSource(parent)
#if ENABLE(ENCRYPTED_MEDIA) && HAVE(AVCONTENTKEYSESSION)
Expand Down Expand Up @@ -777,16 +798,16 @@ static bool sampleBufferRenderersSupportKeySession()
player->removeAudioRenderer(renderer.get());
[renderer flush];
[renderer stopRequestingMediaData];
[m_errorListener stopObservingRenderer:renderer.get()];
[m_listener stopObservingRenderer:renderer.get()];

#if ENABLE(ENCRYPTED_MEDIA) && HAVE(AVCONTENTKEYSESSION)
if (m_cdmInstance && sampleBufferRenderersSupportKeySession())
[m_cdmInstance->contentKeySession() removeContentKeyRecipient:renderer.get()];
#endif
}

[m_errorListener invalidate];
m_errorListener = nullptr;
[m_listener invalidate];
m_listener = nullptr;

m_audioRenderers.clear();
}
Expand Down Expand Up @@ -911,7 +932,7 @@ static bool sampleBufferRenderersSupportKeySession()
weakThis->didBecomeReadyForMoreSamples(trackID);
}];
m_audioRenderers.set(trackID, renderer);
[m_errorListener beginObservingRenderer:renderer.get()];
[m_listener beginObservingRenderer:renderer.get()];
} else
renderer = m_audioRenderers.get(trackID);

Expand Down Expand Up @@ -1561,7 +1582,7 @@ static bool sampleBufferRenderersSupportKeySession()
if (m_displayLayer) {
[m_displayLayer flush];
[m_displayLayer stopRequestingMediaData];
[m_errorListener stopObservingLayer:m_displayLayer.get()];
[m_listener stopObservingLayer:m_displayLayer.get()];

#if ENABLE(ENCRYPTED_MEDIA) && HAVE(AVCONTENTKEYSESSION)
if (m_cdmInstance && sampleBufferRenderersSupportKeySession())
Expand All @@ -1582,7 +1603,7 @@ static bool sampleBufferRenderersSupportKeySession()
if (weakThis)
weakThis->didBecomeReadyForMoreSamples(m_enabledVideoTrackID);
}];
[m_errorListener beginObservingLayer:m_displayLayer.get()];
[m_listener beginObservingLayer:m_displayLayer.get()];
reenqueSamples(AtomString::number(m_enabledVideoTrackID));
}
}
Expand Down Expand Up @@ -1628,6 +1649,6 @@ static bool sampleBufferRenderersSupportKeySession()
}
#endif

}
} // namespace WebCore

#endif // ENABLE(MEDIA_SOURCE) && USE(AVFOUNDATION)

0 comments on commit 66793be

Please sign in to comment.