diff --git a/include/cppgc/internal/pointer-policies.h b/include/cppgc/internal/pointer-policies.h index 1ef17a2082..e09b86199f 100644 --- a/include/cppgc/internal/pointer-policies.h +++ b/include/cppgc/internal/pointer-policies.h @@ -17,6 +17,7 @@ namespace cppgc { namespace internal { +class HeapBase; class PersistentRegion; class CrossThreadPersistentRegion; @@ -76,7 +77,7 @@ class V8_EXPORT EnabledCheckingPolicy { } }; - void* state_ = nullptr; + const HeapBase* heap_ = nullptr; }; class DisabledCheckingPolicy { diff --git a/src/heap/cppgc/heap-base.h b/src/heap/cppgc/heap-base.h index b09e87f51f..8136541718 100644 --- a/src/heap/cppgc/heap-base.h +++ b/src/heap/cppgc/heap-base.h @@ -19,6 +19,7 @@ #include "src/heap/cppgc/metric-recorder.h" #include "src/heap/cppgc/object-allocator.h" #include "src/heap/cppgc/process-heap-statistics.h" +#include "src/heap/cppgc/process-heap.h" #include "src/heap/cppgc/raw-heap.h" #include "src/heap/cppgc/sweeper.h" #include "v8config.h" // NOLINT(build/include_directory) @@ -212,6 +213,8 @@ class V8_EXPORT_PRIVATE HeapBase : public cppgc::HeapHandle { std::unique_ptr lsan_page_allocator_; #endif // LEAK_SANITIZER + HeapRegistry::Subscription heap_registry_subscription_{*this}; + #if defined(CPPGC_CAGED_HEAP) CagedHeap caged_heap_; #endif // CPPGC_CAGED_HEAP diff --git a/src/heap/cppgc/pointer-policies.cc b/src/heap/cppgc/pointer-policies.cc index 89fe5d5cbb..23ad552c7a 100644 --- a/src/heap/cppgc/pointer-policies.cc +++ b/src/heap/cppgc/pointer-policies.cc @@ -12,6 +12,8 @@ #include "src/heap/cppgc/heap-object-header.h" #include "src/heap/cppgc/heap-page.h" #include "src/heap/cppgc/heap.h" +#include "src/heap/cppgc/page-memory.h" +#include "src/heap/cppgc/process-heap.h" namespace cppgc { namespace internal { @@ -36,28 +38,26 @@ void EnabledCheckingPolicy::CheckPointerImpl(const void* ptr, // valid for large objects. DCHECK_IMPLIES(base_page->is_large(), points_to_payload); - if (!state_) { - state_ = base_page->heap(); - // Member references are used from within objects that cannot change their - // heap association which means that state is immutable once it is set. - // - // TODO(chromium:1056170): Binding state late allows for getting the initial - // state wrong which requires a check that `this` is contained in heap that - // is itself expensive. Investigate options on non-caged builds to improve - // coverage. + // References cannot change their heap association which means that state is + // immutable once it is set. + if (!heap_) { + heap_ = base_page->heap(); + if (!heap_->page_backend()->Lookup(reinterpret_cast
(this))) { + // If `this` is not contained within the heap of `ptr`, we must deal with + // an on-stack or off-heap reference. For both cases there should be no + // heap registered. + CHECK(!HeapRegistry::TryFromManagedPointer(this)); + } } - HeapBase* heap = static_cast(state_); - if (!heap) return; - // Member references should never mix heaps. - DCHECK_EQ(heap, base_page->heap()); + DCHECK_EQ(heap_, base_page->heap()); // Header checks. const HeapObjectHeader* header = nullptr; if (points_to_payload) { header = &HeapObjectHeader::FromObject(ptr); - } else if (!heap->sweeper().IsSweepingInProgress()) { + } else if (!heap_->sweeper().IsSweepingInProgress()) { // Mixin case. header = &base_page->ObjectHeaderFromInnerAddress(ptr); DCHECK_LE(header->ObjectStart(), ptr); diff --git a/src/heap/cppgc/process-heap.cc b/src/heap/cppgc/process-heap.cc index e084ea1264..1ad26f4009 100644 --- a/src/heap/cppgc/process-heap.cc +++ b/src/heap/cppgc/process-heap.cc @@ -4,10 +4,64 @@ #include "src/heap/cppgc/process-heap.h" +#include +#include + +#include "src/base/lazy-instance.h" +#include "src/base/platform/mutex.h" +#include "src/heap/cppgc/heap-base.h" +#include "src/heap/cppgc/page-memory.h" + namespace cppgc { namespace internal { v8::base::LazyMutex g_process_mutex = LAZY_MUTEX_INITIALIZER; +namespace { + +HeapRegistry::Storage& GetHeapRegistryStorage() { + static v8::base::LazyInstance::type heap_registry = + LAZY_INSTANCE_INITIALIZER; + return *heap_registry.Pointer(); +} + +} // namespace + +// static +void HeapRegistry::RegisterHeap(HeapBase& heap) { + v8::base::MutexGuard guard(g_process_mutex.Pointer()); + + auto& storage = GetHeapRegistryStorage(); + DCHECK_EQ(storage.end(), std::find(storage.begin(), storage.end(), &heap)); + storage.push_back(&heap); +} + +// static +void HeapRegistry::UnregisterHeap(HeapBase& heap) { + v8::base::MutexGuard guard(g_process_mutex.Pointer()); + + auto& storage = GetHeapRegistryStorage(); + const auto pos = std::find(storage.begin(), storage.end(), &heap); + DCHECK_NE(storage.end(), pos); + storage.erase(pos); +} + +// static +HeapBase* HeapRegistry::TryFromManagedPointer(const void* needle) { + v8::base::MutexGuard guard(g_process_mutex.Pointer()); + + for (auto* heap : GetHeapRegistryStorage()) { + const auto address = + heap->page_backend()->Lookup(reinterpret_cast(needle)); + if (address) return heap; + } + return nullptr; +} + +// static +const HeapRegistry::Storage& HeapRegistry::GetRegisteredHeapsForTesting() { + return GetHeapRegistryStorage(); +} + } // namespace internal } // namespace cppgc diff --git a/src/heap/cppgc/process-heap.h b/src/heap/cppgc/process-heap.h index 8afc7c88eb..c581bad29c 100644 --- a/src/heap/cppgc/process-heap.h +++ b/src/heap/cppgc/process-heap.h @@ -5,13 +5,48 @@ #ifndef V8_HEAP_CPPGC_PROCESS_HEAP_H_ #define V8_HEAP_CPPGC_PROCESS_HEAP_H_ +#include + +#include "src/base/macros.h" #include "src/base/platform/mutex.h" namespace cppgc { namespace internal { +class HeapBase; + extern v8::base::LazyMutex g_process_mutex; +class V8_EXPORT_PRIVATE HeapRegistry final { + public: + using Storage = std::vector; + + class Subscription final { + public: + inline explicit Subscription(HeapBase&); + inline ~Subscription(); + + private: + HeapBase& heap_; + }; + + static HeapBase* TryFromManagedPointer(const void* needle); + + static const Storage& GetRegisteredHeapsForTesting(); + + private: + static void RegisterHeap(HeapBase&); + static void UnregisterHeap(HeapBase&); +}; + +HeapRegistry::Subscription::Subscription(HeapBase& heap) : heap_(heap) { + HeapRegistry::RegisterHeap(heap_); +} + +HeapRegistry::Subscription::~Subscription() { + HeapRegistry::UnregisterHeap(heap_); +} + } // namespace internal } // namespace cppgc diff --git a/test/unittests/BUILD.gn b/test/unittests/BUILD.gn index 507940fc0a..f47b499413 100644 --- a/test/unittests/BUILD.gn +++ b/test/unittests/BUILD.gn @@ -102,6 +102,7 @@ v8_source_set("cppgc_unittests_sources") { "heap/cppgc/heap-growing-unittest.cc", "heap/cppgc/heap-object-header-unittest.cc", "heap/cppgc/heap-page-unittest.cc", + "heap/cppgc/heap-registry-unittest.cc", "heap/cppgc/heap-statistics-collector-unittest.cc", "heap/cppgc/heap-unittest.cc", "heap/cppgc/incremental-marking-schedule-unittest.cc", diff --git a/test/unittests/heap/cppgc/heap-registry-unittest.cc b/test/unittests/heap/cppgc/heap-registry-unittest.cc new file mode 100644 index 0000000000..3726eab946 --- /dev/null +++ b/test/unittests/heap/cppgc/heap-registry-unittest.cc @@ -0,0 +1,88 @@ +// Copyright 2020 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include + +#include "include/cppgc/allocation.h" +#include "include/cppgc/heap.h" +#include "src/heap/cppgc/heap-base.h" +#include "src/heap/cppgc/process-heap.h" +#include "test/unittests/heap/cppgc/tests.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace cppgc { +namespace internal { + +class HeapRegistryTest : public testing::TestWithPlatform {}; + +TEST_F(HeapRegistryTest, Empty) { + EXPECT_EQ(0u, HeapRegistry::GetRegisteredHeapsForTesting().size()); +} + +namespace { + +bool Contains(const HeapRegistry::Storage& storage, const cppgc::Heap* needle) { + return storage.end() != + std::find(storage.begin(), storage.end(), + &cppgc::internal::Heap::From(needle)->AsBase()); +} + +} // namespace + +TEST_F(HeapRegistryTest, RegisterUnregisterHeaps) { + const auto& storage = HeapRegistry::GetRegisteredHeapsForTesting(); + EXPECT_EQ(0u, storage.size()); + { + const auto heap1 = Heap::Create(platform_); + EXPECT_TRUE(Contains(storage, heap1.get())); + EXPECT_EQ(1u, storage.size()); + { + const auto heap2 = Heap::Create(platform_); + EXPECT_TRUE(Contains(storage, heap1.get())); + EXPECT_TRUE(Contains(storage, heap2.get())); + EXPECT_EQ(2u, storage.size()); + } + EXPECT_TRUE(Contains(storage, heap1.get())); + EXPECT_EQ(1u, storage.size()); + } + EXPECT_EQ(0u, storage.size()); +} + +TEST_F(HeapRegistryTest, DoesNotFindNullptr) { + const auto heap = Heap::Create(platform_); + EXPECT_EQ(nullptr, HeapRegistry::TryFromManagedPointer(nullptr)); +} + +TEST_F(HeapRegistryTest, DoesNotFindStackAddress) { + const auto heap = Heap::Create(platform_); + EXPECT_EQ(nullptr, HeapRegistry::TryFromManagedPointer(&heap)); +} + +TEST_F(HeapRegistryTest, DoesNotFindOffHeap) { + const auto heap = Heap::Create(platform_); + auto dummy = std::make_unique(); + EXPECT_EQ(nullptr, HeapRegistry::TryFromManagedPointer(dummy.get())); +} + +namespace { + +class GCed final : public GarbageCollected { + public: + void Trace(Visitor*) const {} +}; + +} // namespace + +TEST_F(HeapRegistryTest, FindsRightHeapForOnHeapAddress) { + const auto heap1 = Heap::Create(platform_); + const auto heap2 = Heap::Create(platform_); + auto* o = MakeGarbageCollected(heap1->GetAllocationHandle()); + EXPECT_EQ(&cppgc::internal::Heap::From(heap1.get())->AsBase(), + HeapRegistry::TryFromManagedPointer(o)); + EXPECT_NE(&cppgc::internal::Heap::From(heap2.get())->AsBase(), + HeapRegistry::TryFromManagedPointer(o)); +} + +} // namespace internal +} // namespace cppgc diff --git a/test/unittests/heap/cppgc/member-unittest.cc b/test/unittests/heap/cppgc/member-unittest.cc index cbebb719dc..ff9ee73ceb 100644 --- a/test/unittests/heap/cppgc/member-unittest.cc +++ b/test/unittests/heap/cppgc/member-unittest.cc @@ -524,14 +524,41 @@ class LinkedNode final : public GarbageCollected { } // namespace -TEST_F(MemberHeapDeathTest, AssignDifferentHeapValues) { - auto* o1 = MakeGarbageCollected(GetAllocationHandle(), nullptr); - auto* o2 = MakeGarbageCollected(GetAllocationHandle(), o1); +TEST_F(MemberHeapDeathTest, CheckForOffHeapMemberCrashesOnReassignment) { + std::vector> off_heap_member; + // Verification state is constructed on first assignment. + off_heap_member.emplace_back( + MakeGarbageCollected(GetAllocationHandle(), nullptr)); { auto tmp_heap = cppgc::Heap::Create(platform_); - auto* o3 = MakeGarbageCollected(tmp_heap->GetAllocationHandle(), - nullptr); - EXPECT_DEATH_IF_SUPPORTED(o2->SetNext(o3), ""); + auto* tmp_obj = MakeGarbageCollected( + tmp_heap->GetAllocationHandle(), nullptr); + EXPECT_DEATH_IF_SUPPORTED(off_heap_member[0] = tmp_obj, ""); + } +} + +TEST_F(MemberHeapDeathTest, CheckForOnStackMemberCrashesOnReassignment) { + Member stack_member; + // Verification state is constructed on first assignment. + stack_member = + MakeGarbageCollected(GetAllocationHandle(), nullptr); + { + auto tmp_heap = cppgc::Heap::Create(platform_); + auto* tmp_obj = MakeGarbageCollected( + tmp_heap->GetAllocationHandle(), nullptr); + EXPECT_DEATH_IF_SUPPORTED(stack_member = tmp_obj, ""); + } +} + +TEST_F(MemberHeapDeathTest, CheckForOnHeapMemberCrashesOnInitialAssignment) { + auto* obj = MakeGarbageCollected(GetAllocationHandle(), nullptr); + { + auto tmp_heap = cppgc::Heap::Create(platform_); + EXPECT_DEATH_IF_SUPPORTED( + // For regular on-heap Member references the verification state is + // constructed eagerly on creating the reference. + MakeGarbageCollected(tmp_heap->GetAllocationHandle(), obj), + ""); } }