Skip to content

Commit

Permalink
[ macOS EWS ] media/video-remove-insert-repaints.html is a flaky crash
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=260663
rdar://114387091

Reviewed by Jer Noble.

If VideoFullscreenManager::removeContext is called with a contextId that doesn't exist in
m_contextMap then a new model would be created with a null video element, leading to a crash when
attempting to remove the video element from m_videoElements. Addressed this by returning early in
VideoFullscreenManager::removeContext if no model/interface pair exists for the given contextId.
Assert that this does not occur to help us track down the underlying issue in Debug builds
(removeContext should not be called for a non-existent contextId).

* Source/WebKit/WebProcess/cocoa/PlaybackSessionManager.mm:
(WebKit::PlaybackSessionManager::removeContext):
* Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:
(WebKit::VideoFullscreenManager::removeContext):

Canonical link: https://commits.webkit.org/267257@main
  • Loading branch information
aestes committed Aug 25, 2023
1 parent 3d24de6 commit 8f95bb7
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 10 deletions.
18 changes: 13 additions & 5 deletions Source/WebKit/WebProcess/cocoa/PlaybackSessionManager.mm
Original file line number Diff line number Diff line change
Expand Up @@ -231,15 +231,23 @@

void PlaybackSessionManager::removeContext(PlaybackSessionContextIdentifier contextId)
{
auto& [model, interface] = ensureModelAndInterface(contextId);
auto [model, interface] = m_contextMap.get(contextId);
ASSERT(model);
ASSERT(interface);
if (!model || !interface)
return;

RefPtr<HTMLMediaElement> mediaElement = model->mediaElement();
model->setMediaElement(nullptr);
model->removeClient(*interface);
interface->invalidate();
if (mediaElement)
m_mediaElements.remove(*mediaElement);
m_contextMap.remove(contextId);

RefPtr mediaElement = model->mediaElement();
ASSERT(mediaElement);
if (!mediaElement)
return;

model->setMediaElement(nullptr);
m_mediaElements.remove(*mediaElement);
}

void PlaybackSessionManager::addClientForContext(PlaybackSessionContextIdentifier contextId)
Expand Down
19 changes: 14 additions & 5 deletions Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm
Original file line number Diff line number Diff line change
Expand Up @@ -206,16 +206,25 @@ static FloatRect inlineVideoFrame(HTMLVideoElement& element)

void VideoFullscreenManager::removeContext(PlaybackSessionContextIdentifier contextId)
{
auto [model, interface] = ensureModelAndInterface(contextId);

m_playbackSessionManager->removeClientForContext(contextId);

RefPtr<HTMLVideoElement> videoElement = model->videoElement();
model->setVideoElement(nullptr);
auto [model, interface] = m_contextMap.get(contextId);
ASSERT(model);
ASSERT(interface);
if (!model || !interface)
return;

model->removeClient(*interface);
interface->invalidate();
m_videoElements.remove(*videoElement);
m_contextMap.remove(contextId);

RefPtr videoElement = model->videoElement();
ASSERT(videoElement);
if (!videoElement)
return;

model->setVideoElement(nullptr);
m_videoElements.remove(*videoElement);
}

void VideoFullscreenManager::addClientForContext(PlaybackSessionContextIdentifier contextId)
Expand Down

0 comments on commit 8f95bb7

Please sign in to comment.