Skip to content

Commit ebc0414

Browse files
Benjamin-DobellBenjamin-Dobell
committed
fix: ObjectDB object removal reference race
1 parent f4b3ebf commit ebc0414

5 files changed

Lines changed: 121 additions & 32 deletions

File tree

‎.changeset/fuzzy-mammals-grin.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@godot-js/editor": patch
3+
---
4+
5+
Fixed ObjectDB object removal race/deadlock/crash

‎bridge/jsb_environment.cpp‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -948,17 +948,21 @@ namespace jsb
948948

949949
// avoid crash in the situation that `InstanceBindingCallbacks::free_callback` is called before JS object gc callback is called,
950950
// which makes the pointer already erased in `object_gc_callback`
951-
if (jsb_unlikely(!object_handle))
952-
{
953-
return;
954-
}
955-
956-
#if JSB_DEBUG
957-
jsb_check(object_handle->pointer == p_pointer);
958-
#endif
959-
const NativeClassID class_id = object_handle->class_id;
960-
// hold it in a local variable to avoid gc too early
961-
v8::Global<v8::Object> obj_ref = std::move(object_handle->ref_);
951+
if (jsb_unlikely(!object_handle))
952+
{
953+
return;
954+
}
955+
956+
#if JSB_DEBUG
957+
jsb_check(object_handle->pointer == p_pointer);
958+
#endif
959+
const NativeClassID class_id = object_handle->class_id;
960+
// hold it in a local variable to avoid gc too early
961+
v8::Global<v8::Object> obj_ref = std::move(object_handle->ref_);
962+
963+
// erase from ObjectDB before clearing the ref to avoid exposing a transient state
964+
// with an empty `ref_` in the ObjectDB, which can race with reference callbacks.
965+
object_db_.remove_object(object_handle, p_pointer);
962966

963967
// TODO: Look into if we ought to be calling obj->free_instance_binding(this)
964968

@@ -971,8 +975,6 @@ namespace jsb
971975
// clear_internal_field(isolate_, obj_ref);
972976
// }
973977

974-
object_handle = nullptr;
975-
object_db_.remove_object(p_pointer);
976978
obj_ref.Reset();
977979

978980
if (p_finalize != FinalizationType::None)

‎bridge/jsb_object_db.h‎

Lines changed: 34 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,20 +7,34 @@
77

88
namespace jsb
99
{
10+
class ObjectDB;
11+
1012
#if JSB_THREADING
1113
# define JSB_OBJECT_DB_HANDLE(Type, Ptr) Type(&lock_, Ptr)
1214
# define JSB_OBJECT_DB_STATEMENT(Statement) Statement
15+
# define JSB_OBJECT_DB_PREPARE_FOR_REMOVAL(Handle) Handle.prepare_for_removal()
16+
#else
17+
# define JSB_OBJECT_DB_HANDLE(Type, Ptr) (sizeof(Type), Ptr)
18+
# define JSB_OBJECT_DB_STATEMENT(Statement) (void) 0
19+
# define JSB_OBJECT_DB_PREPARE_FOR_REMOVAL(Handle) Handle = nullptr
20+
#endif
1321

22+
#if JSB_THREADING
1423
struct ObjectHandlePtr
1524
{
1625
private:
26+
friend class ObjectDB;
27+
1728
RWLock* lock_;
1829
internal::SArray<ObjectHandle, NativeObjectID>::Pointer ptr_;
1930

31+
// Release slot address scope while preserving ObjectDB write lock.
32+
jsb_force_inline void prepare_for_removal() { ptr_ = nullptr; }
33+
2034
public:
21-
ObjectHandlePtr(const ObjectHandlePtr& ) = delete;
35+
ObjectHandlePtr(const ObjectHandlePtr&) = delete;
2236

23-
ObjectHandlePtr(): lock_(nullptr) {}
37+
ObjectHandlePtr() : lock_(nullptr) {}
2438
ObjectHandlePtr(RWLock* p_lock, internal::SArray<ObjectHandle, NativeObjectID>::Pointer&& p_ptr)
2539
: lock_(p_lock), ptr_(std::move(p_ptr))
2640
{
@@ -68,9 +82,9 @@ namespace jsb
6882
internal::SArray<ObjectHandle, NativeObjectID>::ConstPointer ptr_;
6983

7084
public:
71-
ObjectHandleConstPtr(const ObjectHandleConstPtr& ) = delete;
85+
ObjectHandleConstPtr(const ObjectHandleConstPtr&) = delete;
7286

73-
ObjectHandleConstPtr(): lock_(nullptr) {}
87+
ObjectHandleConstPtr() : lock_(nullptr) {}
7488
ObjectHandleConstPtr(const RWLock* p_lock, internal::SArray<ObjectHandle, NativeObjectID>::ConstPointer&& p_ptr)
7589
: lock_(p_lock), ptr_(std::move(p_ptr))
7690
{
@@ -110,9 +124,6 @@ namespace jsb
110124
}
111125
};
112126
#else
113-
# define JSB_OBJECT_DB_HANDLE(Type, Ptr) (sizeof(Type), Ptr)
114-
# define JSB_OBJECT_DB_STATEMENT(Statement) (void) 0
115-
116127
typedef internal::SArray<ObjectHandle, NativeObjectID>::Pointer ObjectHandlePtr;
117128
typedef internal::SArray<ObjectHandle, NativeObjectID>::ConstPointer ObjectHandleConstPtr;
118129
#endif
@@ -131,6 +142,15 @@ namespace jsb
131142
RWLock lock_;
132143
#endif
133144

145+
// Remove object entry while caller already holds ObjectDB write lock.
146+
jsb_force_inline void remove_object_internal(void* p_pointer)
147+
{
148+
const NativeObjectID* entry = objects_index_.getptr(p_pointer);
149+
jsb_check(entry);
150+
objects_.remove_at_checked(*entry);
151+
objects_index_.erase(p_pointer);
152+
}
153+
134154
public:
135155
ObjectDB(int p_capacity)
136156
{
@@ -222,17 +242,16 @@ namespace jsb
222242
}
223243

224244
// [MUTABLE]
225-
void remove_object(void* p_pointer)
245+
jsb_force_inline void remove_object(ObjectHandlePtr& p_handle, void* p_pointer)
226246
{
227-
JSB_OBJECT_DB_STATEMENT(lock_.write_lock());
228-
const NativeObjectID* entry = objects_index_.getptr(p_pointer);
229-
jsb_check(entry);
230-
objects_.remove_at_checked(*entry);
231-
objects_index_.erase(p_pointer);
232-
JSB_OBJECT_DB_STATEMENT(lock_.write_unlock());
247+
#if JSB_DEBUG
248+
jsb_check(p_handle->pointer == p_pointer);
249+
#endif
250+
JSB_OBJECT_DB_PREPARE_FOR_REMOVAL(p_handle);
251+
remove_object_internal(p_pointer);
252+
p_handle = nullptr;
233253
}
234254
};
235255
}
236256

237257
#endif
238-

‎internal/jsb_sarray.h‎

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,9 @@ namespace jsb::internal
4848
int _free_index = -1;
4949
int _first_index = -1;
5050
int _last_index = -1;
51+
#if JSB_WITH_CHECK
5152
int _address_locked = 0;
53+
#endif
5254
AllocatorType allocator;
5355

5456
Slot* get_data() const
@@ -61,8 +63,18 @@ namespace jsb::internal
6163
{
6264
SArray* container_;
6365

64-
AddressScope(SArray* p_container) : container_(p_container) { container_->lock_address(); }
65-
~AddressScope() { if (container_) container_->unlock_address(); }
66+
AddressScope(SArray* p_container) : container_(p_container)
67+
{
68+
#if JSB_WITH_CHECK
69+
container_->lock_address();
70+
#endif
71+
}
72+
~AddressScope()
73+
{
74+
#if JSB_WITH_CHECK
75+
if (container_) container_->unlock_address();
76+
#endif
77+
}
6678

6779
AddressScope(const AddressScope&) = delete;
6880
AddressScope& operator=(const AddressScope&) = delete;
@@ -73,7 +85,9 @@ namespace jsb::internal
7385
{
7486
if (this != &p_other)
7587
{
88+
#if JSB_WITH_CHECK
7689
if (container_) container_->unlock_address();
90+
#endif
7791
container_ = p_other.container_;
7892
p_other.container_ = nullptr;
7993
}
@@ -93,33 +107,48 @@ namespace jsb::internal
93107
TScopedPointer(std::nullptr_t) : container_(nullptr), ptr_(nullptr) { }
94108
TScopedPointer(SArray* p_container, S* p_ptr) : container_(p_container), ptr_(p_ptr)
95109
{
110+
#if JSB_WITH_CHECK
96111
if (container_)
97112
{
98113
container_->lock_address();
99114
}
115+
#endif
116+
}
117+
~TScopedPointer()
118+
{
119+
#if JSB_WITH_CHECK
120+
if (container_) container_->unlock_address();
121+
#endif
100122
}
101-
~TScopedPointer() { if (container_) container_->unlock_address(); }
102123

103124
TScopedPointer(const TScopedPointer& p_other): container_(p_other.container_), ptr_(p_other.ptr_)
104125
{
126+
#if JSB_WITH_CHECK
105127
container_->lock_address();
128+
#endif
106129
}
107130

108131
TScopedPointer& operator=(const TScopedPointer& p_other)
109132
{
110133
if (this != &p_other)
111134
{
135+
#if JSB_WITH_CHECK
112136
if (container_) container_->unlock_address();
137+
#endif
113138
container_ = p_other.container_;
114139
ptr_ = p_other.ptr_;
140+
#if JSB_WITH_CHECK
115141
if (container_) container_->lock_address();
142+
#endif
116143
}
117144
return *this;
118145
}
119146

120147
TScopedPointer& operator=(std::nullptr_t)
121148
{
149+
#if JSB_WITH_CHECK
122150
if (container_) container_->unlock_address();
151+
#endif
123152
container_ = nullptr;
124153
ptr_ = nullptr;
125154
return *this;
@@ -134,7 +163,9 @@ namespace jsb::internal
134163
ptr_ = nullptr;
135164
if (container_)
136165
{
166+
#if JSB_WITH_CHECK
137167
container_->unlock_address();
168+
#endif
138169
container_ = nullptr;
139170
}
140171
return ptr;
@@ -153,7 +184,9 @@ namespace jsb::internal
153184
{
154185
if (this != &p_other)
155186
{
187+
#if JSB_WITH_CHECK
156188
if (container_) container_->unlock_address();
189+
#endif
157190
container_ = p_other.container_;
158191
ptr_ = p_other.ptr_;
159192
p_other.container_ = nullptr;
@@ -952,8 +985,10 @@ namespace jsb::internal
952985
}
953986

954987
private:
988+
#if JSB_WITH_CHECK
955989
jsb_force_inline void lock_address() { ++_address_locked; }
956990
jsb_force_inline void unlock_address() { jsb_check(_address_locked > 0); --_address_locked; }
991+
#endif
957992

958993
#if JSB_SARRAY_CONSISTENCY_CHECK
959994
bool is_consistent() const

‎tests/test_jsb_any_runtime.h‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
#include "jsb_test_helpers.h"
55
#include "../bridge/jsb_essentials.h"
6+
#include "../bridge/jsb_object_db.h"
67
#include "../bridge/jsb_type_convert.h"
78

89
#define JSB_TESTS_OPTION_ENABLED(OptionName) kOption_##OptionName
@@ -116,6 +117,34 @@ namespace jsb::tests
116117
CHECK(ctx.counter == 12);
117118
}
118119

120+
TEST_CASE("[jsb] ObjectDB remove while handle alive")
121+
{
122+
ObjectDB object_db(4);
123+
int native_object = 0;
124+
void* native_pointer = &native_object;
125+
126+
ObjectHandlePtr object_handle;
127+
const NativeObjectID object_id = object_db.add_object(native_pointer, &object_handle);
128+
129+
REQUIRE(object_handle);
130+
#if JSB_DEBUG
131+
object_handle->pointer = native_pointer;
132+
#endif
133+
object_handle = nullptr;
134+
135+
CHECK(object_db.has_object(native_pointer));
136+
CHECK(object_db.has_object(object_id));
137+
138+
object_handle = object_db.try_get_object(native_pointer);
139+
REQUIRE(object_handle);
140+
141+
object_db.remove_object(object_handle, native_pointer);
142+
143+
CHECK(!object_handle);
144+
CHECK(!object_db.has_object(native_pointer));
145+
CHECK(!object_db.has_object(object_id));
146+
}
147+
119148
TEST_CASE("[jsb] raw isolate essential tests")
120149
{
121150
impl::GlobalInitialize::init();
@@ -511,4 +540,3 @@ file = undefined;
511540
}
512541

513542
#endif
514-

0 commit comments

Comments
 (0)