From 0c18a3a577353d8ee2ae644dc026c7c5fe5b4e38 Mon Sep 17 00:00:00 2001 From: Darius M Date: Wed, 30 Nov 2022 10:52:54 +0100 Subject: [PATCH] Fix dead lock with transition array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The main thread can enter TransitionsAccessor::PutPrototypeTransition, lock the full_transition_array_access mutex, and then try to allocate, which can trigger a GC. It would then wait for all threads to be in a Safepoint. However, if a background thread tried to acquired a shared lock on full_transition_array_access (this happens from JsNativeContextSpecialization::ReducePropertyAccess, which eventually calls TransitionsAccessor::SearchTransition, which takes tries to take the shared lock) outside of a safepoint, we have a deadlock: the main thread waits for the background thread to be in a safepoint, but the background thread waits for the main thread to release the lock on full_transition_array_access. This CL fixes the issue by releasing the shared lock from the main thread during the "allocation" part of the growing of the transition array, so that the background thread can take the shared lock and then reach a safepoint; the main thread would then do the allocation and trigger the GC, and only then take the exclusive lock to replace the old transition array by the new one (but this wouldn't allocate). Bug: chromium:1393904 Change-Id: Ia3b94be575a0266c41c2fe6445090e551673b617 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4062039 Reviewed-by: Dominik Inführ Reviewed-by: Patrick Thier Commit-Queue: Darius Mercadier Cr-Commit-Position: refs/heads/main@{#84565} --- src/objects/transitions.cc | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/src/objects/transitions.cc b/src/objects/transitions.cc index 12190cf9ad..22a7a741da 100644 --- a/src/objects/transitions.cc +++ b/src/objects/transitions.cc @@ -371,13 +371,14 @@ Handle TransitionArray::GrowPrototypeTransitionArray( new_capacity = std::min({kMaxCachedPrototypeTransitions, new_capacity}); DCHECK_GT(new_capacity, capacity); int grow_by = new_capacity - capacity; - array = isolate->factory()->CopyWeakFixedArrayAndGrow(array, grow_by); + Handle new_array = + isolate->factory()->CopyWeakFixedArrayAndGrow(array, grow_by); if (capacity < 0) { // There was no prototype transitions array before, so the size // couldn't be copied. Initialize it explicitly. - SetNumberOfPrototypeTransitions(*array, 0); + SetNumberOfPrototypeTransitions(*new_array, 0); } - return array; + return new_array; } // static @@ -397,15 +398,33 @@ void TransitionsAccessor::PutPrototypeTransition(Isolate* isolate, int capacity = cache->length() - header; int transitions = TransitionArray::NumberOfPrototypeTransitions(*cache) + 1; - base::SharedMutexGuard scope( - isolate->full_transition_array_access()); + // We're not using a MutexGuard for {full_transition_array_access}, because + // we'll need to release it before growing the transition array (if needed), + // in order to avoid deadlock if a background thread is waiting for the shared + // mutex outside of a safepoint. And after growing the array, we'll need to + // re-lock it. + base::SharedMutex* transition_array_mutex = + isolate->full_transition_array_access(); + transition_array_mutex->LockExclusive(); if (transitions > capacity) { // Grow the array if compacting it doesn't free space. if (!TransitionArray::CompactPrototypeTransitionArray(isolate, *cache)) { + transition_array_mutex->UnlockExclusive(); if (capacity == TransitionArray::kMaxCachedPrototypeTransitions) return; + + // GrowPrototypeTransitionArray can allocate, so it shouldn't hold the + // exclusive lock on {full_transition_array_access} mutex, since + // background threads could be waiting for the shared lock (outside of a + // safe point). This is not an issue, because GrowPrototypeTransitionArray + // doesn't actually modify in place the array, but instead return a new + // array. + transition_array_mutex->LockShared(); cache = TransitionArray::GrowPrototypeTransitionArray( cache, 2 * transitions, isolate); + transition_array_mutex->UnlockShared(); + + transition_array_mutex->LockExclusive(); SetPrototypeTransitions(isolate, map, cache); } } @@ -416,6 +435,8 @@ void TransitionsAccessor::PutPrototypeTransition(Isolate* isolate, cache->Set(entry, HeapObjectReference::Weak(*target_map)); TransitionArray::SetNumberOfPrototypeTransitions(*cache, last + 1); + + transition_array_mutex->UnlockExclusive(); } // static