Skip to content

ASSERT under HTMLMediaElement::virtualHasPendingActivity#64228

Merged
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
eric-carlson:virtual-has-pending-activity-should-not-ref
May 5, 2026
Merged

ASSERT under HTMLMediaElement::virtualHasPendingActivity#64228
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
eric-carlson:virtual-has-pending-activity-should-not-ref

Conversation

@eric-carlson
Copy link
Copy Markdown
Contributor

@eric-carlson eric-carlson commented May 5, 2026

640e6ff

ASSERT under HTMLMediaElement::virtualHasPendingActivity
https://bugs.webkit.org/show_bug.cgi?id=314034
rdar://176143872

Reviewed by Jean-Yves Avenard and Ryosuke Niwa.

Many MediaPlayerPrivateMediaSourceAVFObjC member variables are a main-thread-only but
it can be access from the worker thread by the MediaSource/Private, so its destructor
should always run on the main thread.

While investigation this it was observed that HTMLMediaElement::virtualHasPendingActivity
may be called from the GC thread but it calls HTMLMediaElement::canProduceAudio() which
calls MediaPlayer::hasAudio() and creates a temporary RefPtr to the MediaPlayerPrivate.
This is unnecessary as every MediaPlayer notifies HTMLMediaElement when its ability to
produce audio changes, so fix this by caching `canProduceAudio` in a std::atomic which is
updated only on the main thread whenever relevant state changes. canProduceAudio()
now just returns the cached value and is safe to call from any thread.

* Source/WebCore/html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::canProduceAudio const):
(WebCore::HTMLMediaElement::computeCanProduceAudio const):
(WebCore::HTMLMediaElement::canProduceAudioChanged):
* Source/WebCore/html/HTMLMediaElement.h:
* Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.h:

Canonical link: https://commits.webkit.org/312598@main

be78faa

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe 🛠 win ⏳ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 🧪 win-tests ⏳ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ⏳ 🛠 vision-apple
🧪 ios-wk2-wpt 🧪 api-mac-debug ✅ 🛠 gtk3-libwebrtc
🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 ios-safer-cpp ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🛠 playstation
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@eric-carlson eric-carlson requested a review from cdumez as a code owner May 5, 2026 00:16
@eric-carlson eric-carlson self-assigned this May 5, 2026
@eric-carlson eric-carlson requested a review from rniwa as a code owner May 5, 2026 00:16
@eric-carlson eric-carlson added the Media Bugs related to the HTML 5 Media elements. label May 5, 2026
@rniwa
Copy link
Copy Markdown
Member

rniwa commented May 5, 2026

The explanation doesn't make much sense. The main thread is paused while HTMLMediaElement::virtualHasPendingActivity is being called.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 5, 2026
@eric-carlson eric-carlson removed the merging-blocked Applied to prevent a change from being merged label May 5, 2026
@eric-carlson eric-carlson force-pushed the virtual-has-pending-activity-should-not-ref branch from b1ac587 to be78faa Compare May 5, 2026 02:47
@jyavenard jyavenard added the merge-queue Applied to send a pull request to merge-queue label May 5, 2026
https://bugs.webkit.org/show_bug.cgi?id=314034
rdar://176143872

Reviewed by Jean-Yves Avenard and Ryosuke Niwa.

Many MediaPlayerPrivateMediaSourceAVFObjC member variables are a main-thread-only but
it can be access from the worker thread by the MediaSource/Private, so its destructor
should always run on the main thread.

While investigation this it was observed that HTMLMediaElement::virtualHasPendingActivity
may be called from the GC thread but it calls HTMLMediaElement::canProduceAudio() which
calls MediaPlayer::hasAudio() and creates a temporary RefPtr to the MediaPlayerPrivate.
This is unnecessary as every MediaPlayer notifies HTMLMediaElement when its ability to
produce audio changes, so fix this by caching `canProduceAudio` in a std::atomic which is
updated only on the main thread whenever relevant state changes. canProduceAudio()
now just returns the cached value and is safe to call from any thread.

* Source/WebCore/html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::canProduceAudio const):
(WebCore::HTMLMediaElement::computeCanProduceAudio const):
(WebCore::HTMLMediaElement::canProduceAudioChanged):
* Source/WebCore/html/HTMLMediaElement.h:
* Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.h:

Canonical link: https://commits.webkit.org/312598@main
@webkit-commit-queue webkit-commit-queue force-pushed the virtual-has-pending-activity-should-not-ref branch from be78faa to 640e6ff Compare May 5, 2026 07:46
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 312598@main (640e6ff): https://commits.webkit.org/312598@main

Reviewed commits have been landed. Closing PR #64228 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 640e6ff into WebKit:main May 5, 2026
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Media Bugs related to the HTML 5 Media elements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants