JSC Slotvisitor::drain crash

[modeveci]

Sometimes with 2.38, we are observing crashes around this call stack, to be able to address we need to know whether changes around this call stack between versions 2.46-2.38 can improve these occurrences.

JSC::SlotVisitor::drainFromShared
WTF::ParallelHelperClient::runTask
WTF::ParallelHelperPool::Thread::work
call
WTF::Thread::entryPoint
wtfThreadEntryPoint

It is not easy to reproduce but we want to understand whether these changes on GC can improve and prevent this crash. In particular, whether there is difference regarding a race condition in GC between marking and sweeping.

wpe-2.38

WPEWebKit/Source/JavaScriptCore/heap/MarkedBlock.cpp

Line 196 in a78b2a8

void MarkedBlock::aboutToMarkSlow(HeapVersion markingVersion)

wpe-2.46

WPEWebKit/Source/JavaScriptCore/heap/MarkedBlock.cpp

Line 251 in 99499dc

void MarkedBlock::aboutToMarkSlow(HeapVersion markingVersion, HeapCell* cell)

[modeveci]

@magomez , can Justin look at this ticket? Thank you!

[magomez]

@magomez , can Justin look at this ticket? Thank you!

Sure, I'll ask him

[aoikonomopoulos]

@modeveci The differences in aboutToMarkSlow come from https://commits.webkit.org/284376@main, later modified by https://commits.webkit.org/286689@main.

Both of those changes add code to dump the state of a MarkedBlock when a handle is unexpectedly NULL. If that's where you've been observing the crash in 2.38 then these changes won't fix it; they can only produce some extra diagnostic output that may help track down the source.

That said, if the problem you're running into is the NULL handle, collecting the diagnostic output in the rare occurrences when this does happen, would help track down this bug.

[GlebNovodranPE]

@aoikonomopoulos
FYI. Here is the more detailed stack trace:

JSC::MarkedBlock::aboutToMarkSlow
/usr/src/debug/lib32-wpe-webkit/2.38.8+gitAUTOINC+680d0afd32-r7/build/../git/Source/JavaScriptCore/heap/MarkedBlock.h:469

JSC::Structure::visitChildren
/usr/src/debug/lib32-wpe-webkit/2.38.8+gitAUTOINC+680d0afd32-r7/build/../git/Source/JavaScriptCore/heap/MarkedBlock.h:576

operator
/usr/src/debug/lib32-wpe-webkit/2.38.8+gitAUTOINC+680d0afd32-r7/build/../git/Source/JavaScriptCore/runtime/ClassInfo.h:118

JSC::SlotVisitor::drain
/usr/src/debug/lib32-wpe-webkit/2.38.8+gitAUTOINC+680d0afd32-r7/build/../git/Source/JavaScriptCore/heap/SlotVisitorInlines.h:184

JSC::SlotVisitor::drainFromShared
/usr/src/debug/lib32-wpe-webkit/2.38.8+gitAUTOINC+680d0afd32-r7/build/../git/Source/JavaScriptCore/heap/SlotVisitor.cpp:694

operator
/usr/src/debug/lib32-wpe-webkit/2.38.8+gitAUTOINC+680d0afd32-r7/build/../git/Source/JavaScriptCore/heap/Heap.cpp:1393

WTF::ParallelHelperClient::runTask
/usr/src/debug/lib32-wpe-webkit/2.38.8+gitAUTOINC+680d0afd32-r7/build/../git/Source/WTF/wtf/ParallelHelperPool.cpp:110

WTF::ParallelHelperPool::Thread::work
/usr/src/debug/lib32-wpe-webkit/2.38.8+gitAUTOINC+680d0afd32-r7/build/../git/Source/WTF/wtf/ParallelHelperPool.cpp:201

call
/usr/src/debug/lib32-wpe-webkit/2.38.8+gitAUTOINC+680d0afd32-r7/build/../git/Source/WTF/wtf/AutomaticThread.cpp:229

WTF::Thread::entryPoint
/usr/src/debug/lib32-wpe-webkit/2.38.8+gitAUTOINC+680d0afd32-r7/build/../git/Source/WTF/wtf/Function.h:82

wtfThreadEntryPoint
/usr/src/debug/lib32-wpe-webkit/2.38.8+gitAUTOINC+680d0afd32-r7/build/../git/Source/WTF/wtf/posix/ThreadingPOSIX.cpp:242

start_thread
/usr/src/debug/lib32-glibc/2.35-r0/git/nptl/pthread_create.c:442

clone
:

[GlebNovodranPE]

It looks like it crashes upon dereferencing a null pointer:
BlockDirectory* directory = handle().directory();

WPEWebKit/Source/JavaScriptCore/heap/MarkedBlock.h

Line 469 in 7609a21

return m_directory;

Looks like the wpe-2.46 adds the check and dump of the additional info.

WPEWebKit/Source/JavaScriptCore/heap/MarkedBlock.cpp

Line 262 in 9e67942

if (UNLIKELY(!handle))

This probably means that the problem per se was not solved, but acknowledged in the upstream.

[aoikonomopoulos]

@GlebNovodranPE Thanks. That is exactly the issue that the new instrumentation code is trying to catch.

[modeveci]

Thank you both @aoikonomopoulos and @GlebNovodranPE.

@aoikonomopoulos do you think it is useful to backport those changes to 2/38 and try to catch crashes with this new instrumental impl.?

[aoikonomopoulos]

Yes, but for it to be useful we need to make sure the diagnostic output gets picked up by your telemetry. Currently, nothing is logged on non-Cocoa platforms.