Title: [264673] trunk/Source/_javascript_Core
- Revision
- 264673
- Author
- keith_mil...@apple.com
- Date
- 2020-07-21 12:36:56 -0700 (Tue, 21 Jul 2020)
Log Message
Fix FinalizationRegistry GC finalizer interation
https://bugs.webkit.org/show_bug.cgi?id=214586
<rdar://65854264>
Reviewed by Mark Lam and Yusuke Suzuki.
Turns out when you remove the ith element from a Vector and you
increment the index anyway you skip things... Use the helper
functions instead. This fixes an ASAN crash on our
FinalizationRegistry tests.
* heap/Heap.cpp:
(JSC::Heap::finalizeUnconditionalFinalizers):
* runtime/DeferredWorkTimer.cpp:
(JSC::DeferredWorkTimer::addPendingWork):
(JSC::DeferredWorkTimer::hasPendingWork):
(JSC::DeferredWorkTimer::hasDependancyInPendingWork):
(JSC::DeferredWorkTimer::cancelPendingWork):
* runtime/JSFinalizationRegistry.cpp:
(JSC::JSFinalizationRegistry::finalizeUnconditionally):
(JSC::JSFinalizationRegistry::takeDeadHoldingsValue):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (264672 => 264673)
--- trunk/Source/_javascript_Core/ChangeLog 2020-07-21 19:29:00 UTC (rev 264672)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-07-21 19:36:56 UTC (rev 264673)
@@ -1,3 +1,27 @@
+2020-07-21 Keith Miller <keith_mil...@apple.com>
+
+ Fix FinalizationRegistry GC finalizer interation
+ https://bugs.webkit.org/show_bug.cgi?id=214586
+ <rdar://65854264>
+
+ Reviewed by Mark Lam and Yusuke Suzuki.
+
+ Turns out when you remove the ith element from a Vector and you
+ increment the index anyway you skip things... Use the helper
+ functions instead. This fixes an ASAN crash on our
+ FinalizationRegistry tests.
+
+ * heap/Heap.cpp:
+ (JSC::Heap::finalizeUnconditionalFinalizers):
+ * runtime/DeferredWorkTimer.cpp:
+ (JSC::DeferredWorkTimer::addPendingWork):
+ (JSC::DeferredWorkTimer::hasPendingWork):
+ (JSC::DeferredWorkTimer::hasDependancyInPendingWork):
+ (JSC::DeferredWorkTimer::cancelPendingWork):
+ * runtime/JSFinalizationRegistry.cpp:
+ (JSC::JSFinalizationRegistry::finalizeUnconditionally):
+ (JSC::JSFinalizationRegistry::takeDeadHoldingsValue):
+
2020-07-21 Saam Barati <sbar...@apple.com>
Disable NO_SMT by default
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (264672 => 264673)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2020-07-21 19:29:00 UTC (rev 264672)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2020-07-21 19:36:56 UTC (rev 264673)
@@ -612,6 +612,8 @@
finalizeMarkedUnconditionalFinalizers<JSWeakObjectRef>(*vm().m_weakObjectRefSpace);
if (vm().m_errorInstanceSpace)
finalizeMarkedUnconditionalFinalizers<ErrorInstance>(*vm().m_errorInstanceSpace);
+
+ // FinalizationRegistries currently rely on serial finalization because they can post tasks to the deferredWorkTimer, which normally expects tasks to only be posted by the API lock holder.
if (vm().m_finalizationRegistrySpace)
finalizeMarkedUnconditionalFinalizers<JSFinalizationRegistry>(*vm().m_finalizationRegistrySpace);
Modified: trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.cpp (264672 => 264673)
--- trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.cpp 2020-07-21 19:29:00 UTC (rev 264672)
+++ trunk/Source/_javascript_Core/runtime/DeferredWorkTimer.cpp 2020-07-21 19:36:56 UTC (rev 264673)
@@ -110,7 +110,7 @@
void DeferredWorkTimer::addPendingWork(VM& vm, Ticket ticket, Vector<Strong<JSCell>>&& dependencies)
{
- ASSERT(vm.currentThreadIsHoldingAPILock());
+ ASSERT(vm.currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && vm.heap.worldIsStopped()));
for (unsigned i = 0; i < dependencies.size(); ++i)
ASSERT(dependencies[i].get() != ticket);
@@ -127,13 +127,13 @@
bool DeferredWorkTimer::hasPendingWork(Ticket ticket)
{
- ASSERT(ticket->vm().currentThreadIsHoldingAPILock());
+ ASSERT(ticket->vm().currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && ticket->vm().heap.worldIsStopped()));
return m_pendingTickets.contains(ticket);
}
bool DeferredWorkTimer::hasDependancyInPendingWork(Ticket ticket, JSCell* dependency)
{
- ASSERT(ticket->vm().currentThreadIsHoldingAPILock());
+ ASSERT(ticket->vm().currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && ticket->vm().heap.worldIsStopped()));
ASSERT(m_pendingTickets.contains(ticket));
auto result = m_pendingTickets.get(ticket);
@@ -142,7 +142,7 @@
bool DeferredWorkTimer::cancelPendingWork(Ticket ticket)
{
- ASSERT(ticket->vm().currentThreadIsHoldingAPILock());
+ ASSERT(ticket->vm().currentThreadIsHoldingAPILock() || (Thread::mayBeGCThread() && ticket->vm().heap.worldIsStopped()));
bool result = m_pendingTickets.remove(ticket);
if (result)
Modified: trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.cpp (264672 => 264673)
--- trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.cpp 2020-07-21 19:29:00 UTC (rev 264672)
+++ trunk/Source/_javascript_Core/runtime/JSFinalizationRegistry.cpp 2020-07-21 19:36:56 UTC (rev 264673)
@@ -92,39 +92,52 @@
{
auto locker = holdLock(cellLock());
- auto removeFromVector = [] (auto& vector, size_t position) {
- std::swap(vector[position], vector.last());
- return vector.takeLast();
- };
+#if ASSERT_ENABLED
+ for (const auto& iter : m_deadRegistrations)
+ RELEASE_ASSERT(iter.value.size());
+#endif
bool readiedCell = false;
- for (unsigned i = 0; i < m_noUnregistrationLive.size(); ++i) {
- ASSERT(!m_noUnregistrationLive[i].holdings.get().isCell() || vm.heap.isMarked(m_noUnregistrationLive[i].holdings.get().asCell()));
- if (!vm.heap.isMarked(m_noUnregistrationLive[i].target)) {
- m_noUnregistrationDead.append(removeFromVector(m_noUnregistrationLive, i).holdings);
+ m_noUnregistrationLive.removeAllMatching([&] (const Registration& reg) {
+ ASSERT(!reg.holdings.get().isCell() || vm.heap.isMarked(reg.holdings.get().asCell()));
+ if (!vm.heap.isMarked(reg.target)) {
+ m_noUnregistrationDead.append(reg.holdings);
readiedCell = true;
+ return true;
}
- }
+ return false;
+ });
m_liveRegistrations.removeIf([&] (auto& bucket) -> bool {
ASSERT(bucket.value.size());
bool keyIsDead = !vm.heap.isMarked(bucket.key);
+ DeadRegistrations* deadList = nullptr;
+ auto getDeadList = [&] () -> DeadRegistrations& {
+ if (UNLIKELY(!deadList))
+ deadList = &m_deadRegistrations.add(bucket.key, DeadRegistrations()).iterator->value;
+ return *deadList;
+ };
- for (size_t i = 0; i < bucket.value.size(); i++) {
- ASSERT(!bucket.value[i].holdings.get().isCell() || vm.heap.isMarked(bucket.value[i].holdings.get().asCell()));
- if (!vm.heap.isMarked(bucket.value[i].target)) {
+ bucket.value.removeAllMatching([&] (const Registration& reg) {
+ ASSERT(!reg.holdings.get().isCell() || vm.heap.isMarked(reg.holdings.get().asCell()));
+ if (!vm.heap.isMarked(reg.target)) {
if (keyIsDead)
- m_noUnregistrationDead.append(removeFromVector(bucket.value, i).holdings);
- else {
- auto deadList = m_deadRegistrations.add(bucket.key, DeadRegistrations());
- deadList.iterator->value.append(removeFromVector(bucket.value, i).holdings);
- }
+ m_noUnregistrationDead.append(reg.holdings);
+ else
+ getDeadList().append(reg.holdings);
readiedCell = true;
- } else if (keyIsDead)
- m_noUnregistrationLive.append(removeFromVector(bucket.value, i));
- }
+ return true;
+ }
+ if (keyIsDead) {
+ m_noUnregistrationLive.append(reg);
+ return true;
+ }
+
+ return false;
+ });
+
return !bucket.value.size();
});
@@ -169,10 +182,10 @@
m_deadRegistrations.remove(iter);
}
- if constexpr (ASSERT_ENABLED) {
- for (const auto& iter : m_deadRegistrations)
- RELEASE_ASSERT(iter.value.size());
- }
+#if ASSERT_ENABLED
+ for (const auto& iter : m_deadRegistrations)
+ RELEASE_ASSERT(iter.value.size());
+#endif
return result;
}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
