Bug 258022

Summary:	REGRESSION(264515@main): [GStreamer] m_player WeakPtr is accessed from non-main threads
Product:	WebKit	Reporter:	Alicia Boya García <aboya>
Component:	Web Template Framework	Assignee:	Jean-Yves Avenard [:jya] <jean-yves.avenard>
Status:	RESOLVED FIXED
Severity:	Normal	CC:	bugs-noreply, emilio, jean-yves.avenard, mcatanzaro, webkit-bug-importer
Priority:	P2	Keywords:	InRadar
Version:	WebKit Nightly Build
Hardware:	Unspecified
OS:	Unspecified
See Also:	https://bugs.webkit.org/show_bug.cgi?id=254954 https://bugs.webkit.org/show_bug.cgi?id=258129

Description Alicia Boya García 2023-06-13 06:31:54 PDT

Here is a crash backtrace from a JSC thread garbage collecting an HTMLMediaElement.

(gdb) bt
#0  0x00007fe237cbe1fe in WTFCrash() () at /app/webkit/Source/WTF/wtf/Assertions.cpp:327
#1  0x00007fe23f81378b in WTFCrashWithInfo(int, char const*, char const*, int) () at WTF/Headers/wtf/Assertions.h:762
#2  0x00007fe241524f4a in WTF::WeakPtr<WebCore::MediaPlayer, WTF::DefaultWeakPtrImpl>::operator->() const (this=0x7fe22344e0c0) at WTF/Headers/wtf/WeakPtr.h:139
#3  0x00007fe2415091d4 in WebCore::MediaPlayerPrivateGStreamer::gstreamerPositionFromSinks() const (this=0x7fe22344e080) at /app/webkit/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1297
#4  0x00007fe241504377 in WebCore::MediaPlayerPrivateGStreamer::playbackPosition() const (this=0x7fe22344e080) at /app/webkit/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1328
#5  0x00007fe24150662b in WebCore::MediaPlayerPrivateGStreamer::currentMediaTime() const (this=0x7fe22344e080) at /app/webkit/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:623
#6  0x00007fe2453bcb68 in WebCore::MediaPlayer::currentTime() const (this=0x7fe02225cbc0) at /app/webkit/Source/WebCore/platform/graphics/MediaPlayer.cpp:779
#7  0x00007fe2446bd1de in WebCore::HTMLMediaElement::refreshCachedTime() const (this=0x7fe1c504a440) at /app/webkit/Source/WebCore/html/HTMLMediaElement.cpp:3648
#8  0x00007fe2446bbec8 in WebCore::HTMLMediaElement::currentMediaTime() const (this=0x7fe1c504a440) at /app/webkit/Source/WebCore/html/HTMLMediaElement.cpp:3733
#9  0x00007fe2446cae76 in WebCore::HTMLMediaElement::endedPlayback() const (this=0x7fe1c504a440) at /app/webkit/Source/WebCore/html/HTMLMediaElement.cpp:5874
#10 0x00007fe2446cadb1 in WebCore::HTMLMediaElement::ended() const (this=0x7fe1c504a440) at /app/webkit/Source/WebCore/html/HTMLMediaElement.cpp:3930
#11 0x00007fe2446d322b in WebCore::HTMLMediaElement::hasLiveSource() const (this=0x7fe1c504a440) at /app/webkit/Source/WebCore/html/HTMLMediaElement.cpp:6372
#12 0x00007fe2446d32e2 in WebCore::HTMLMediaElement::virtualHasPendingActivity() const (this=0x7fe1c504a440) at /app/webkit/Source/WebCore/html/HTMLMediaElement.cpp:6379
#13 0x00007fe2418600ce in WebCore::ActiveDOMObject::hasPendingActivity() const (this=0x7fe1c504a4c0) at /app/webkit/Source/WebCore/dom/ActiveDOMObject.h:60
#14 0x00007fe242085836 in WebCore::JSHTMLAudioElementOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::AbstractSlotVisitor&, char const**) (this=0x7fe246c85258 <WebCore::wrapperOwner(WebCore::DOMWrapperWorld&, WebCore::HTMLAudioElement*)::owner>, handle=..., visitor=..., reason=0x0) at WebCore/DerivedSources/JSHTMLAudioElement.cpp:237
#15 0x00007fe236c396dd in JSC::WeakBlock::specializedVisit<JSC::PreciseAllocation, JSC::SlotVisitor>(JSC::PreciseAllocation&, JSC::SlotVisitor&) (this=0x7fe1d298b000, container=..., visitor=...) at /app/webkit/Source/JavaScriptCore/heap/WeakBlock.cpp:126
#16 0x00007fe236c2de3f in JSC::WeakBlock::visitImpl<JSC::SlotVisitor>(JSC::SlotVisitor&) (this=0x7fe1d298b000, visitor=...) at /app/webkit/Source/JavaScriptCore/heap/WeakBlock.cpp:149
#17 0x00007fe236c2b4ad in JSC::WeakBlock::visit(JSC::SlotVisitor&) (this=0x7fe1d298b000, visitor=...) at /app/webkit/Source/JavaScriptCore/heap/WeakBlock.cpp:155
#18 0x00007fe236bbf3e9 in JSC::MarkedSpace::forEachWeakInParallel<JSC::SlotVisitor&>()::Task::run(JSC::SlotVisitor&) (this=0x7fe2233a5cc0, visitor=...) at /app/webkit/Source/JavaScriptCore/heap/MarkedSpaceInlines.h:130
#19 0x00007fe236c07e40 in JSC::MarkingConstraint::doParallelWork(JSC::SlotVisitor&, WTF::SharedTask<void (JSC::SlotVisitor&)>&) (this=0x7fe22301c600, visitor=..., task=...) at /app/webkit/Source/JavaScriptCore/heap/MarkingConstraint.cpp:95
#20 0x00007fe236c08ffd in JSC::MarkingConstraintSolver::runExecutionThread(JSC::SlotVisitor&, JSC::MarkingConstraintSolver::SchedulerPreference, WTF::ScopedLambda<std::optional<unsigned int> ()>) (this=0x7ffeff3b8320, visitor=..., preference=JSC::MarkingConstraintSolver::NextConstraintFirst, pickNext=...) at /app/webkit/Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:230
#21 0x00007fe236c10ec1 in JSC::MarkingConstraintSolver::execute(JSC::MarkingConstraintSolver::SchedulerPreference, WTF::ScopedLambda<std::optional<unsigned int> ()>)::$_30::operator()(JSC::SlotVisitor&) const (this=0x7fe02232f5e0, visitor=...) at /app/webkit/Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:67
#22 0x00007fe236c10e71 in WTF::SharedTaskFunctor<void (JSC::SlotVisitor&), JSC::MarkingConstraintSolver::execute(JSC::MarkingConstraintSolver::SchedulerPreference, WTF::ScopedLambda<std::optional<unsigned int> ()>)::$_30>::run(JSC::SlotVisitor&) (this=0x7fe02232f5d0, arguments=...) at WTF/Headers/wtf/SharedTask.h:91
#23 0x00007fe236c20b31 in JSC::SlotVisitor::drainFromShared(JSC::SlotVisitor::SharedDrainMode, WTF::MonotonicTime) (this=0x7fe2230a8900, sharedDrainMode=JSC::SlotVisitor::HelperDrain, timeout=...) at /app/webkit/Source/JavaScriptCore/heap/SlotVisitor.cpp:681
#24 0x00007fe236b6d6d1 in JSC::Heap::runBeginPhase(JSC::GCConductor)::$_18::operator()() const (this=0x7fe2237c3190) at /app/webkit/Source/JavaScriptCore/heap/Heap.cpp:1421
#25 0x00007fe236b6d5f9 in WTF::SharedTaskFunctor<void (), JSC::Heap::runBeginPhase(JSC::GCConductor)::$_18>::run() (this=0x7fe2237c3180) at WTF/Headers/wtf/SharedTask.h:91
#26 0x00007fe237f48d69 in WTF::ParallelHelperClient::runTask(WTF::RefPtr<WTF::SharedTask<void ()>, WTF::RawPtrTraits<WTF::SharedTask<void ()> >, WTF::DefaultRefDerefTraits<WTF::SharedTask<void ()> > > const&) (this=0x7fe1d2400418, task=...) at /app/webkit/Source/WTF/wtf/ParallelHelperPool.cpp:110
#27 0x00007fe237f49ba1 in WTF::ParallelHelperPool::Thread::work() (this=0x7fe2232838d0) at /app/webkit/Source/WTF/wtf/ParallelHelperPool.cpp:201
#28 0x00007fe237cc1c22 in WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0::operator()() const (this=0x7fe2234fed28) at /app/webkit/Source/WTF/wtf/AutomaticThread.cpp:229
#29 0x00007fe237cc1969 in WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0, void>::call() (this=0x7fe2234fed20) at /app/webkit/Source/WTF/wtf/Function.h:53
#30 0x00007fe2366ae1b2 in WTF::Function<void ()>::operator()() const (this=0x7fe1977fdba0) at WTF/Headers/wtf/Function.h:82
#31 0x00007fe237f66788 in WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) (newThreadContext=0x7fe2233b3470) at /app/webkit/Source/WTF/wtf/Threading.cpp:250
#32 0x00007fe23801de05 in WTF::wtfThreadEntryPoint(void*) (context=0x7fe2233b3470) at /app/webkit/Source/WTF/wtf/posix/ThreadingPOSIX.cpp:242
#33 0x00007fe23148f1da in start_thread (arg=<optimized out>) at pthread_create.c:442
#34 0x00007fe231517f44 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:100

The crash occurs because as part of the winding down code, currentTime is queried from that thread. The code for getting the currentTime inside MediaPlayerPrivateGStreamer dereferences m_player (of type WeakPtr<MediaPlayer>), but WeakPtr are not supposed to be used from threads different than the threads they were created, and as such, fails in this assertion:

#2  0x00007fe241524f4a in WTF::WeakPtr<WebCore::MediaPlayer, WTF::DefaultWeakPtrImpl>::operator-> (this=0x7fe22344e0c0) at WTF/Headers/wtf/WeakPtr.h:139
139	        ASSERT(!m_impl || !m_shouldEnableAssertions || m_impl->wasConstructedOnMainThread() == isMainThread());

Either currentTime should not be queried from non-main threads, or m_player should not be dereferenced from non-main threads.

I don't know if querying currentTime from non-main threads is even valid. MediaPlayerPrivateInterface.h has no notes about it.

Comment 1 Michael Catanzaro 2023-06-13 11:29:26 PDT

I'm pretty sure it's not safe even with only one thread in the mix, see: https://bugs.webkit.org/show_bug.cgi?id=254954#c3

Comment 2 Jean-Yves Avenard [:jya] 2023-06-14 20:12:05 PDT

When the GC thread runs, the main thread is suspended. So it's not unsafe here.

The issue at hand, is that WeakPtr::get() allows for it to be called on the GC threads [1], but not the operator->() [2]
[1] https://searchfox.org/wubkat/rev/49a9b51cd3f2e1941aa86570e9948012b4c1cbfa/Source/WTF/wtf/WeakPtr.h#123-128
[2] https://searchfox.org/wubkat/rev/49a9b51cd3f2e1941aa86570e9948012b4c1cbfa/Source/WTF/wtf/WeakPtr.h#137-141

Comment 3 Radar WebKit Bug Importer 2023-06-14 21:21:25 PDT

<rdar://problem/110817538>

Comment 4 Jean-Yves Avenard [:jya] 2023-06-15 00:05:30 PDT

Pull request: https://github.com/WebKit/WebKit/pull/14994

Comment 5 EWS 2023-06-16 01:08:58 PDT

Committed 265233@main (c234d2219797): <https://commits.webkit.org/265233@main>

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