1886445 - Assertion failure: CurrentThreadCanAccessRuntime(runtime()), at js/src/gc/Nursery.cpp:1646

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P3)

Description

[lukas.bernhard]

Steps to reproduce:

On git commit 6a2a2a52d7e544a2fd5678d04991a7e78b694f22 I found an assertion violation. Unfortunately, the sample does not reproduce but maybe the backtrace is helpful. The shell was running with flags --fast-warmup --ion-check-range-analysis --ion-extra-checks --fuzzing-safe --disable-oom-functions --enable-new-set-methods.
If the fuzzer finds a (flakey) reproducer I'll update the ticket.

Assertion failure: CurrentThreadCanAccessRuntime(runtime()), at js/src/gc/Nursery.cpp:1646
#01 0x1c73cd8 => obj-lto/dist/include/mozilla/Assertions.h:43
#02 0x27c9c78 => js/src/gc/StoreBuffer.h:504
#03 0x255816d => js/src/gc/StoreBuffer.h:563
#04 0x28925a4 => js/src/gc/Barrier.h:402
#05 0x3114c71 => js/src/gc/WeakMap-inl.h:283
#06 0x205e504 => js/src/gc/WeakMap.cpp:152
#07 0x1c59724 => js/src/gc/SweepingAPI.h:33
#08 0x26eea61 => js/src/gc/GCInternals.h:188
#09 0x1eda4ed => js/src/gc/GCParallelTask.cpp:201
#10 0x1eda8e9 => js/src/gc/GCParallelTask.cpp:183
#11 0x23d83d6 => js/src/vm/HelperThreadState.h:441
#12 0x23d7433 => js/src/vm/HelperThreads.cpp:894
#13 0x204dae5 => js/src/vm/InternalThreadPool.cpp:282

Comment 1

[Jan de Mooij [:jandem]]

Based on the stack, it looks like a barrier is trying to access the store buffer from a helper thread.

Comment 2

[Steven DeTar [:sdetar]]

Jonco, could you take a look at this newer bug related to the nursery when you have time

Comment 3

[Jon Coppeard (:jonco)]

I'm pretty sure this is related to the changes to allow symbols as weakmap keys and should be fixed by the patch in bug 1885775. Specifically it's triggering a barrier when accessing weakmap keys during sweeping because we haven't finished marking the atoms zone yet.

The description doesn't mention enabling the feature, but I'm assuming it got enabled during fuzzing.

Comment 4

[lukas.bernhard]

The target runs with ./js --fast-warmup --ion-check-range-analysis --ion-extra-checks --fuzzing-safe --disable-oom-functions --enable-new-set-methods.

Compiled with:

ac_add_options --disable-bootstrap
ac_add_options --enable-application=js

ac_add_options --enable-fuzzing

ac_add_options --enable-js-fuzzilli
ac_add_options --disable-tests

ac_add_options --disable-shared-js
ac_add_options --enable-linker=lld
ac_add_options --enable-gczeal

ac_add_options --enable-debug
ac_add_options --enable-optimize

ac_add_options --with-temporal-api

Comment 5

[Daniel Veditz [:dveditz]]

Jon: I don't see --enable-symbols-as-weakmap-keys as in bug 1885775. Still think this might be a dupe?

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Daniel Veditz [:dveditz] from comment #5)
You're right and I don't actually see how the feature can get enabled after startup, but that's what the stack looks like to me. It is definitely weakmap-related, and the timing is close to the other bug. Impossible to say for sure without a testcase though.

Comment 7

[lukas.bernhard]

I'll re-deploy the fuzzers with the fix for bug 1885775, lets see whether the asserts surfaces again (hopefully with a reliable reproducer).

Comment 8

[lukas.bernhard]

Unfortunately, the issue persists in a more recent commit (git 28cc363411d2029aed04c969c8f98785cae110db). On the upside, I did manage to capture a pernos.co trace: https://pernos.co/debug/jHYPG-N8wYz0RXxHzbpaIA/index.html
The corresponding input is quite messy (multiple .js send via the reprl interface) and difficult to clean, as the crash manifests only in <1% of attempts. If the trace is insufficient in figuring out the root cause I'll attempt to minimize anyways.

Backtrace:

js/src/gc/Nursery.cpp:1800
js/src/gc/StoreBuffer.h:505
js/src/gc/StoreBuffer.h:564
js/src/gc/Barrier.h:402
obj-lto/dist/include/mozilla/HashTable.h:1879
obj-lto/dist/include/mozilla/HashTable.h:1496
js/src/gc/WeakMap.cpp:152
js/src/gc/Sweeping.cpp:1271
js/src/gc/GCInternals.h:188
js/src/gc/GCParallelTask.cpp:201
js/src/gc/GCParallelTask.cpp:183
js/src/vm/HelperThreads.cpp:1728
js/src/vm/HelperThreads.cpp:1697
js/src/vm/InternalThreadPool.cpp:282
js/src/vm/InternalThreadPool.cpp:225
js/src/threading/Thread.h:228

Comment 9

[Jon Coppeard (:jonco)]

Thanks for the pernosco trace. The problem is that sweeping the hashtable used for a weak map is resizing the table and firing post barriers for nursery objects, which is filling up the value store buffer and requesting a minor GC. Nursery::requestMinorGC doesn't expect to be called off the main thread and asserts.

This has likely come up because the changes for symbols as weakmap keys changed the hashtable used for JS weakmaps to use Values for the key type so changing the store buffer used.

Comment 10

[Jon Coppeard (:jonco)]

Attachment: Bug 1886445 - Relax assertion in Nursery::requestMinorGC to permit sweeping to trigger minor GC r?sfink

I couldn't easily come up with a test case for this. Possibly we want to make
the store buffer size configurable. I did manage to provoke this failure
manaully though and verified that this patch fixed it.

Comment 11

[Jon Coppeard (:jonco)]

This is an over eager assertion and is not security sensitive.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76b5f0dab023
Relax assertion in Nursery::requestMinorGC to permit sweeping to trigger minor GC r=sfink

Comment 13

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/76b5f0dab023