258022 2023-06-13 06:31:54 -0700 REGRESSION(264515@main): [GStreamer] m_player WeakPtr is accessed from non-main threads 2023-06-16 01:09:03 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=254954 https://bugs.webkit.org/show_bug.cgi?id=258129 InRadar P2 Normal --- 1 aboya jean-yves.avenard bugs-noreply emilio jean-yves.avenard mcatanzaro webkit-bug-importer oldest_to_newest 1961313 0 aboya 2023-06-13 06:31:54 -0700 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. 1961374 1 mcatanzaro 2023-06-13 11:29:26 -0700 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 1961709 2 jean-yves.avenard 2023-06-14 20:12:05 -0700 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 1961715 3 webkit-bug-importer 2023-06-14 21:21:25 -0700 <rdar://problem/110817538> 1961744 4 jean-yves.avenard 2023-06-15 00:05:30 -0700 Pull request: https://github.com/WebKit/WebKit/pull/14994 1962031 5 ews-feeder 2023-06-16 01:08:58 -0700 Committed 265233@main (c234d2219797): <https://commits.webkit.org/265233@main> Reviewed commits have been landed. Closing PR #14994 and removing active labels.