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.

[justinmichaud]

This looks like it is going to be hard to debug.

Just confirming:

1. This reproduces on 2.38
2. This reproduces on 2.46, albeit with a slightly different signature

Can we focus on testing on 2.46?

BlockDirectory* directory = handle().directory();

I will propose some release asserts to add to rule out some things.

Then:

1. Let's start by testing on 2.46 and turning on this logging for all platforms (@aoikonomopoulos has already started a thread about that).

2. Do you have a history of the crash rate? Was there a 2.38 release that did not have this issue? Maybe a recent patch introduced this, and we can bisect.

3. I will try to reproduce this on my end. This will probably be hard / take a while, I will privately ping you for more details.

Next, what happens to the repro rate under the following situations (on 2.46)? I can test these myself if I can reproduce it.

First, let me confirm we are on the latest 2.46 with no additional JSC options being set. Please let me know if any default options have been changed.

1. collectContinuously=1 (might make the crash happen much sooner)
2. useZombieMode=1 (this will tell us if we have a race or a uaf)
3. useConcurrentGC=0
4. useGenerationalGC=0
5. useConcurrentJIT=0
6. forceCodeBlockLiveness=1 (this will tell us if we have a code lifecycle issue)
7. arm64 wpe (this will improve testing rate, since I can test on my workstation)
8. if it reproduces on arm64 wpe, then I can try to repro on arm64 ToT. That would rule out missing backported patches

The more of these options we can turn on while still crashing, the easier it will be to debug.

[justinmichaud]

The line numbers in your symbolicated stack trace don't match up with the symbol names. For example, JSC::Structure::visitChildren is not MarkedBlock.h:576.

Nothing quite lines up (maybe due to optimization), but I have made a few guesses to make things make sense. We need to double check the back trace though.

We know (based on what you said above):

MarkedBlock::Handle* handle = header().handlePointerForNullCheck();

handle is 0 (please confirm that the trap value is 0, not just unmapped)

The path to get here:

SlotVisitor::drain
forEachMarkStack callback body, m_collectorStack
visitChildren()
SlotVisitor::visitChildren
(via cell->methodTable()->visitChildren) MethodTable::visitChildren
(via a pointer in the method table) JSC::Structure::visitChildren
<speculation>
visitor.append, appendUnbarriered (it would be so freaking helpful to know which one)
aboutToMark
</speculation>
MarkedBlock::aboutToMarkSlow

This cell was added to the collector stack by:

some visit children function || 
appendSlow | appendHiddenSlow ||
SlotVisitor::appendHiddenSlowImpl || SlotVisitor::append(ConservativeRoots)
setMarkedAndAppendToMarkStack || appendJSCellOrAuxiliary
SlotVisitor::appendToMarkStack

Then:

1. header() is garbage

cell is garbage, interpreted as a structure. Then, cell->markedBlock() (which is just masking off some bits of cell) is garbage.

ENABLE_GC_VALIDATION might be able to help us trace down where/why.

If we can collect many crashes, but every time we see a different visitChildren method being called, this case seems much more likely.

2. header() is valid, header().m_handle is 0

Then either:

a) Handle::didRemoveFromDirectory was called

Then BlockDirectory::removeBlock(WillDeleteBlock::No) was called, so Handle::removeFromDirectory was called, so tryAllocateWithoutCollecting (with Options::stealEmptyBlocksFromOtherAllocators()) was called).

This gives us a super easy test: disable JSC_stealEmptyBlocksFromOtherAllocators!

b) Handle::didAddToDirectory got null

There is already a RELEASE_ASSERT that would fail if that was the case, but maybe it got compiled out. I'll add more asserts here just in case, but this seems unlikely.

---

1. Do we only ever see Structure::visitChildren?

2. Does this crash reproduce with JSC_stealEmptyBlocksFromOtherAllocators=0?

The answer to this can already rule out a bunch of options today, without needing a new build.

[GlebNovodranPE]

1. Let's start by testing on 2.46 and turning on this logging for all platforms ([@aoikonomopoulos](https://github.com/aoikonomopoulos) has already started a thread about that).

@justinmichaud , @modeveci ,
We do not have 2.46 based devices in the field at this point.

2. Do you have a history of the crash rate? Was there a 2.38 release that did not have this issue? Maybe a recent patch introduced this, and we can bisect.

The crash is a long standing issue it was present even before 2.38 I think, with a somewhat different stack trace.

3. I will try to reproduce this on my end. This will probably be hard / take a while, I will privately ping you for more details.

I am not sure if attempting to reproduce it locally is not going to be a waste of time. Would you please talk to @modeveci regarding the reproduction approaches discussed with him.

Next, what happens to the repro rate under the following situations (on 2.46)? I can test these myself if I can reproduce it.

As I mentioned previously it is impossible to conclude anything about reproducibility in the field with 2.46.

First, let me confirm we are on the latest 2.46 with no additional JSC options being set. Please let me know if any default options have been changed.

1. collectContinuously=1 (might make the crash happen much sooner)

2. useZombieMode=1 (this will tell us if we have a race or a uaf)

3. useConcurrentGC=0

4. useGenerationalGC=0

5. useConcurrentJIT=0

6. forceCodeBlockLiveness=1 (this will tell us if we have a code lifecycle issue)

7. arm64 wpe (this will improve testing rate, since I can test on my workstation)

8. if it reproduces on arm64 wpe, then I can try to repro on arm64 ToT. That would rule out missing backported patches

I don't think anything above was tweaked, but let me clarify this with the colleagues.

[GlebNovodranPE]

@justinmichaud ,

handle is 0 (please confirm that the trap value is 0, not just unmapped

The crash information we have comes from Breakpad minidumps and the binaries upon which the dumps were created are stripped, so it is difficult to conclude about the local variable values.

Do we only ever see Structure::visitChildren?

I would say it constitutes a bigger part of SEGV crashes in JSC::MarkedBlock::aboutToMarkSlow we observe. I am attaching sample staktraces for the cases when it dos not originate from Structure::visitChildren.

JSFinalObject.txt
UnlinkedCodeBlock.txt
visitAggregate.txt

Does this crash reproduce with JSC_stealEmptyBlocksFromOtherAllocators=0?

We don't know at the moment and it would be hard to conclude if it helps, since it is difficult to reproduce it in a lab even without that tweak. All the data we have come from the filed where it is enabled by default.