[ffi/isolate_group_shared] Keep isolate alive for isolate group shared callback.

To fix encountered tsan race(patchset 3)  split Metadata into MetadataEntry as discussed on https://github.com/dart-lang/sdk/issues/60728#event-17760701621

TEST=ci
Fixes https://github.com/dart-lang/sdk/issues/60728

Change-Id: I9308d6fb6a9b819221f7fe4668aefb3a578fe8a8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/430122
Reviewed-by: Liam Appelbe <liama@google.com>
Commit-Queue: Alexander Aprelev <aam@google.com>

runtime/vm/ffi_callback_metadata.cc

@@ -59,10 +59,10 @@ void FfiCallbackMetadata::EnsureStubPageLocked() {
   original_metadata_page_ = VirtualMemory::AllocateAligned(
       MappingSize(), MappingAlignment(), /*is_executable=*/false,
       /*is_compressed=*/false, "FfiCallbackMetadata::TrampolinePage");
-  Metadata* metadata = reinterpret_cast<Metadata*>(
+  MetadataEntry* metadata_entry = reinterpret_cast<MetadataEntry*>(
       original_metadata_page_->start() + MetadataOffset());
   for (intptr_t i = 0; i < NumCallbackTrampolinesPerPage(); ++i) {
-    AddToFreeListLocked(&metadata[i]);
+    AddToFreeListLocked(&metadata_entry[i]);
   }
 #endif  // defined(DART_TARGET_OS_FUCHSIA)
 }
@@ -176,10 +176,10 @@ void FfiCallbackMetadata::EnsureFreeListNotEmptyLocked() {
 
   // Add all the trampolines to the free list.
   const intptr_t trampolines_per_page = NumCallbackTrampolinesPerPage();
-  Metadata* metadata =
-      reinterpret_cast<Metadata*>(new_page->start() + MetadataOffset());
+  MetadataEntry* metadata_entry =
+      reinterpret_cast<MetadataEntry*>(new_page->start() + MetadataOffset());
   for (intptr_t i = 0; i < trampolines_per_page; ++i) {
-    AddToFreeListLocked(&metadata[i]);
+    AddToFreeListLocked(&metadata_entry[i]);
   }
 }
 
@@ -189,63 +189,64 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateMetadataEntry(
     TrampolineType trampoline_type,
     uword target_entry_point,
     uint64_t context,
-    Metadata** list_head) {
+    MetadataEntry** list_head) {
   MutexLocker locker(&lock_);
   EnsureFreeListNotEmptyLocked();
   ASSERT(free_list_head_ != nullptr);
-  Metadata* entry = free_list_head_;
+  MetadataEntry* entry = free_list_head_;
   free_list_head_ = entry->free_list_next_;
   if (free_list_head_ == nullptr) {
     ASSERT(free_list_tail_ == entry);
     free_list_tail_ = nullptr;
   }
-  Metadata* next_entry = *list_head;
+  MetadataEntry* next_entry = *list_head;
   if (next_entry != nullptr) {
     ASSERT(next_entry->list_prev_ == nullptr);
     next_entry->list_prev_ = entry;
   }
   if (target_isolate != nullptr) {
-    *entry = Metadata(target_isolate, trampoline_type, target_entry_point,
+    *entry = MetadataEntry(target_isolate, trampoline_type, target_entry_point,
                            context, nullptr, next_entry);
   } else {
     ASSERT(target_isolate_group != nullptr);
-    *entry = Metadata(target_isolate_group, trampoline_type, target_entry_point,
-                      context, nullptr, next_entry);
+    *entry = MetadataEntry(target_isolate_group, trampoline_type,
+                           target_entry_point, context, nullptr, next_entry);
   }
   *list_head = entry;
-  return TrampolineOfMetadata(entry);
+  return TrampolineOfMetadataEntry(entry);
 }
 
-void FfiCallbackMetadata::AddToFreeListLocked(Metadata* entry) {
+void FfiCallbackMetadata::AddToFreeListLocked(MetadataEntry* entry) {
   ASSERT(lock_.IsOwnedByCurrentThread());
   if (free_list_tail_ == nullptr) {
     ASSERT(free_list_head_ == nullptr);
     free_list_head_ = free_list_tail_ = entry;
   } else {
     ASSERT(free_list_head_ != nullptr && free_list_tail_ != nullptr);
-    ASSERT(!free_list_tail_->IsLive());
+    ASSERT(!free_list_tail_->metadata()->IsLive());
     free_list_tail_->free_list_next_ = entry;
     free_list_tail_ = entry;
   }
-  entry->context_ = 0;
-  entry->target_isolate_ = nullptr;
+  entry->metadata()->context_ = 0;
+  entry->metadata()->target_isolate_ = nullptr;
   entry->free_list_next_ = nullptr;
 }
 
-void FfiCallbackMetadata::DeleteCallbackLocked(Metadata* entry) {
+void FfiCallbackMetadata::DeleteCallbackLocked(MetadataEntry* entry) {
   ASSERT(lock_.IsOwnedByCurrentThread());
-  if (entry->trampoline_type_ != TrampolineType::kAsync &&
-      entry->context_ != 0) {
-    ASSERT(entry->target_isolate_ != nullptr);
-    entry->api_state()->FreePersistentHandle(entry->closure_handle());
+  if (entry->metadata()->trampoline_type_ != TrampolineType::kAsync &&
+      entry->metadata()->context_ != 0) {
+    ASSERT(entry->metadata()->target_isolate_ != nullptr);
+    entry->metadata()->api_state()->FreePersistentHandle(
+        entry->metadata()->closure_handle());
   }
   AddToFreeListLocked(entry);
 }
 
-void FfiCallbackMetadata::DeleteAllCallbacks(Metadata** list_head) {
+void FfiCallbackMetadata::DeleteAllCallbacks(MetadataEntry** list_head) {
   MutexLocker locker(&lock_);
-  for (Metadata* entry = *list_head; entry != nullptr;) {
-    Metadata* next = entry->list_next();
+  for (MetadataEntry* entry = *list_head; entry != nullptr;) {
+    MetadataEntry* next = entry->list_next();
     DeleteCallbackLocked(entry);
     entry = next;
   }
@@ -253,10 +254,10 @@ void FfiCallbackMetadata::DeleteAllCallbacks(Metadata** list_head) {
 }
 
 void FfiCallbackMetadata::DeleteCallback(Trampoline trampoline,
-                                         Metadata** list_head) {
+                                         MetadataEntry** list_head) {
   MutexLocker locker(&lock_);
-  auto* entry = MetadataOfTrampoline(trampoline);
-  ASSERT(entry->IsLive());
+  auto* entry = MetadataEntryOfTrampoline(trampoline);
+  ASSERT(entry->metadata()->IsLive());
   auto* prev = entry->list_prev_;
   auto* next = entry->list_next_;
   if (prev != nullptr) {
@@ -295,7 +296,7 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateLocalFfiCallback(
     Zone* zone,
     const Function& function,
     const Closure& closure,
-    Metadata** list_head) {
+    MetadataEntry** list_head) {
   PersistentHandle* handle = nullptr;
   if (closure.IsNull()) {
     // If the closure is null, it means the target is a static function, so is
@@ -326,7 +327,7 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateSyncFfiCallbackImpl(
     Zone* zone,
     const Function& function,
     PersistentHandle* closure,
-    Metadata** list_head) {
+    MetadataEntry** list_head) {
   TrampolineType trampoline_type =
       isolate != nullptr ? TrampolineType::kSync
                          : TrampolineType::kSyncIsolateGroupShared;
@@ -355,7 +356,7 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateAsyncFfiCallback(
     Zone* zone,
     const Function& send_function,
     Dart_Port send_port,
-    Metadata** list_head) {
+    MetadataEntry** list_head) {
   ASSERT(send_function.GetFfiCallbackKind() == FfiCallbackKind::kAsyncCallback);
   return CreateMetadataEntry(isolate, /*isolate_group=*/nullptr,
                              TrampolineType::kAsync,
@@ -363,11 +364,12 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateAsyncFfiCallback(
                              static_cast<uint64_t>(send_port), list_head);
 }
 
-FfiCallbackMetadata::Trampoline FfiCallbackMetadata::TrampolineOfMetadata(
-    Metadata* metadata) const {
-  const uword start = MappingStart(reinterpret_cast<uword>(metadata));
-  Metadata* metadatas = reinterpret_cast<Metadata*>(start + MetadataOffset());
-  const uword index = metadata - metadatas;
+FfiCallbackMetadata::Trampoline FfiCallbackMetadata::TrampolineOfMetadataEntry(
+    MetadataEntry* metadata_entry) const {
+  const uword start = MappingStart(reinterpret_cast<uword>(metadata_entry));
+  MetadataEntry* metadata_entries =
+      reinterpret_cast<MetadataEntry*>(start + MetadataOffset());
+  const uword index = metadata_entry - metadata_entries;
 #if defined(SIMULATOR_FFI) && defined(HOST_ARCH_ARM64)
   return reinterpret_cast<uword>(SimulatorFfiCallbackTrampoline) +
          index * kNativeCallbackTrampolineSize;
@@ -380,8 +382,8 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::TrampolineOfMetadata(
 #endif
 }
 
-FfiCallbackMetadata::Metadata* FfiCallbackMetadata::MetadataOfTrampoline(
-    Trampoline trampoline) const {
+FfiCallbackMetadata::MetadataEntry*
+FfiCallbackMetadata::MetadataEntryOfTrampoline(Trampoline trampoline) const {
 #if defined(DART_TARGET_OS_FUCHSIA) ||                                         \
     (defined(SIMULATOR_FFI) && defined(HOST_ARCH_ARM64))
   // On Fuchsia the metadata page is separate to the trampoline page.
@@ -393,22 +395,23 @@ FfiCallbackMetadata::Metadata* FfiCallbackMetadata::MetadataOfTrampoline(
       (trampoline - offset_of_first_trampoline_in_page_ - page_start) /
       kNativeCallbackTrampolineSize;
   ASSERT(index < NumCallbackTrampolinesPerPage());
-  Metadata* metadata_table = reinterpret_cast<Metadata*>(
+  MetadataEntry* metadata_etnry_table = reinterpret_cast<MetadataEntry*>(
       original_metadata_page_->start() + MetadataOffset());
-  return metadata_table + index;
+  return metadata_etnry_table + index;
 #else
   const uword start = MappingStart(trampoline);
-  Metadata* metadatas = reinterpret_cast<Metadata*>(start + MetadataOffset());
+  MetadataEntry* metadata_entries =
+      reinterpret_cast<MetadataEntry*>(start + MetadataOffset());
   const uword index =
       (trampoline - start - offset_of_first_trampoline_in_page_) /
       kNativeCallbackTrampolineSize;
-  return &metadatas[index];
+  return &metadata_entries[index];
 #endif
 }
 
 FfiCallbackMetadata::Metadata FfiCallbackMetadata::LookupMetadataForTrampoline(
     Trampoline trampoline) const {
-  return *MetadataOfTrampoline(trampoline);
+  return *MetadataEntryOfTrampoline(trampoline)->metadata();
 }
 
 FfiCallbackMetadata* FfiCallbackMetadata::singleton_ = nullptr;

runtime/vm/ffi_callback_metadata.h

@@ -37,6 +37,7 @@ class PersistentHandle;
 class FfiCallbackMetadata {
  public:
   class Metadata;
+  class MetadataEntry;
 
   // The address of the allocated trampoline.
   using Trampoline = uword;
@@ -68,7 +69,7 @@ class FfiCallbackMetadata {
                                     Zone* zone,
                                     const Function& function,
                                     Dart_Port send_port,
-                                    Metadata** list_head);
+                                    MetadataEntry** list_head);
 
   // Creates an isolate- or isolategroup- local callback trampoline for
   // the given function.
@@ -77,13 +78,13 @@ class FfiCallbackMetadata {
                                     Zone* zone,
                                     const Function& function,
                                     const Closure& closure,
-                                    Metadata** list_head);
+                                    MetadataEntry** list_head);
 
   // Deletes a single trampoline.
-  void DeleteCallback(Trampoline trampoline, Metadata** list_head);
+  void DeleteCallback(Trampoline trampoline, MetadataEntry** list_head);
 
   // Deletes all the trampolines in the list.
-  void DeleteAllCallbacks(Metadata** list_head);
+  void DeleteAllCallbacks(MetadataEntry** list_head);
 
   // FFI callback metadata for any sync or async trampoline.
   class Metadata {
@@ -93,9 +94,6 @@ class FfiCallbackMetadata {
     };
     TrampolineType trampoline_type_;
 
-    union {
-      // IsLive()
-      struct {
     // Note: This is a pointer into an an Instructions object. This is only
     // safe because Instructions objects are never moved by the GC.
     uword target_entry_point_;
@@ -104,40 +102,23 @@ class FfiCallbackMetadata {
     // is a persistent handle to the callback's closure, or null.
     uint64_t context_;
 
-        // Links in the Isolate's list of callbacks.
-        Metadata* list_prev_;
-        Metadata* list_next_;
-      };
-
-      // !IsLive()
-      Metadata* free_list_next_;
-    };
-
     Metadata(Isolate* target_isolate,
              TrampolineType trampoline_type,
              uword target_entry_point,
-             uint64_t context,
-             Metadata* list_prev,
-             Metadata* list_next)
+             uint64_t context)
         : target_isolate_(target_isolate),
           trampoline_type_(trampoline_type),
           target_entry_point_(target_entry_point),
-          context_(context),
-          list_prev_(list_prev),
-          list_next_(list_next) {}
+          context_(context) {}
 
     Metadata(IsolateGroup* target_isolate_group,
              TrampolineType trampoline_type,
              uword target_entry_point,
-             uint64_t context,
-             Metadata* list_prev,
-             Metadata* list_next)
+             uint64_t context)
         : target_isolate_group_(target_isolate_group),
           trampoline_type_(trampoline_type),
           target_entry_point_(target_entry_point),
-          context_(context),
-          list_prev_(list_prev),
-          list_next_(list_next) {}
+          context_(context) {}
 
    public:
     friend class FfiCallbackMetadata;
@@ -208,24 +189,72 @@ class FfiCallbackMetadata {
       return static_cast<Dart_Port>(context_);
     }
 
-    // To efficiently delete all the callbacks for a isolate, they are stored in
-    // a linked list. Since we also need to delete async callbacks at arbitrary
-    // times, the list must be doubly linked.
-    Metadata* list_prev() {
-      ASSERT(IsLive());
-      return list_prev_;
-    }
-    Metadata* list_next() {
-      ASSERT(IsLive());
-      return list_next_;
-    }
-
     // Tells FfiCallbackTrampolineStub how to call into the entry point. Mostly
     // it's just a flag for whether this is a sync or async callback, but on
     // IA32 it also encodes whether there's a stack delta of 4 to deal with.
     TrampolineType trampoline_type() const { return trampoline_type_; }
   };
 
+  // Metadata linked into a double-linked list.
+  class MetadataEntry {
+    Metadata metadata_;
+
+    union {
+      // IsLive()
+      struct {
+        // Links in the Isolate's list of callbacks.
+        MetadataEntry* list_prev_;
+        MetadataEntry* list_next_;
+      };
+
+      // !IsLive()
+      MetadataEntry* free_list_next_;
+    };
+
+   public:
+    friend class Metadata;
+    friend class FfiCallbackMetadata;
+    MetadataEntry(Isolate* target_isolate,
+                  TrampolineType trampoline_type,
+                  uword target_entry_point,
+                  uint64_t context,
+                  MetadataEntry* list_prev,
+                  MetadataEntry* list_next)
+        : metadata_(target_isolate,
+                    trampoline_type,
+                    target_entry_point,
+                    context),
+          list_prev_(list_prev),
+          list_next_(list_next) {}
+
+    MetadataEntry(IsolateGroup* target_isolate_group,
+                  TrampolineType trampoline_type,
+                  uword target_entry_point,
+                  uint64_t context,
+                  MetadataEntry* list_prev,
+                  MetadataEntry* list_next)
+        : metadata_(target_isolate_group,
+                    trampoline_type,
+                    target_entry_point,
+                    context),
+          list_prev_(list_prev),
+          list_next_(list_next) {}
+
+    // To efficiently delete all the callbacks for a isolate, they are stored in
+    // a linked list. Since we also need to delete async callbacks at arbitrary
+    // times, the list must be doubly linked.
+    MetadataEntry* list_prev() {
+      ASSERT(metadata_.IsLive());
+      return list_prev_;
+    }
+    MetadataEntry* list_next() {
+      ASSERT(metadata_.IsLive());
+      return list_next_;
+    }
+
+    Metadata* metadata() { return &metadata_; }
+  };
+
   // Returns the Metadata object for the given trampoline.
   Metadata LookupMetadataForTrampoline(Trampoline trampoline) const;
 
@@ -264,12 +293,12 @@ class FfiCallbackMetadata {
   //   * [RX] 2 pages fully containing [StubCode::FfiCallbackTrampoline()]
   //   * [RW] pages sufficient to hold
   //      - `kNumRuntimeFunctions` x [uword] function pointers
-  //      - `NumCallbackTrampolinesPerPage()` x [Metadata] objects
+  //      - `NumCallbackTrampolinesPerPage()` x [MetadataEntry] objects
   static constexpr intptr_t RXMappingSize() { return 2 * kPageSize; }
   static constexpr intptr_t RWMappingSize() {
     return Utils::RoundUp(
         kNumRuntimeFunctions * compiler::target::kWordSize +
-            sizeof(Metadata) * NumCallbackTrampolinesPerPage(),
+            sizeof(MetadataEntry) * NumCallbackTrampolinesPerPage(),
         kPageSize);
   }
   static constexpr intptr_t MappingSize() {
@@ -318,15 +347,15 @@ class FfiCallbackMetadata {
 #endif
 
   // Visible for testing.
-  Metadata* MetadataOfTrampoline(Trampoline trampoline) const;
-  Trampoline TrampolineOfMetadata(Metadata* metadata) const;
+  MetadataEntry* MetadataEntryOfTrampoline(Trampoline trampoline) const;
+  Trampoline TrampolineOfMetadataEntry(MetadataEntry* metadata) const;
 
  private:
   FfiCallbackMetadata();
   ~FfiCallbackMetadata();
   void EnsureStubPageLocked();
-  void AddToFreeListLocked(Metadata* entry);
-  void DeleteCallbackLocked(Metadata* entry);
+  void AddToFreeListLocked(MetadataEntry* entry);
+  void DeleteCallbackLocked(MetadataEntry* entry);
   void FillRuntimeFunction(VirtualMemory* page, uword index, void* function);
   VirtualMemory* AllocateTrampolinePage();
   void EnsureFreeListNotEmptyLocked();
@@ -335,19 +364,13 @@ class FfiCallbackMetadata {
                                  TrampolineType trampoline_type,
                                  uword target_entry_point,
                                  uint64_t context,
-                                 Metadata** list_head);
+                                 MetadataEntry** list_head);
   Trampoline CreateSyncFfiCallbackImpl(Isolate* isolate,
                                        IsolateGroup* isolate_group,
                                        Zone* zone,
                                        const Function& function,
                                        PersistentHandle* closure,
-                                       Metadata** list_head);
-  Trampoline CreateIsolateGroupSharedFfiCallbackImpl(
-      IsolateGroup* isolate_group,
-      Zone* zone,
-      const Function& function,
-      PersistentHandle* closure,
-      Metadata** list_head);
+                                       MetadataEntry** list_head);
   Trampoline TryAllocateFromFreeListLocked();
   static uword GetEntryPoint(Zone* zone, const Function& function);
   static PersistentHandle* CreatePersistentHandle(IsolateGroup* isolate_group,
@@ -359,8 +382,8 @@ class FfiCallbackMetadata {
   VirtualMemory* stub_page_ = nullptr;
   MallocGrowableArray<VirtualMemory*> trampoline_pages_;
   uword offset_of_first_trampoline_in_page_ = 0;
-  Metadata* free_list_head_ = nullptr;
-  Metadata* free_list_tail_ = nullptr;
+  MetadataEntry* free_list_head_ = nullptr;
+  MetadataEntry* free_list_tail_ = nullptr;
 
 #if defined(DART_TARGET_OS_FUCHSIA) ||                                         \
     (defined(SIMULATOR_FFI) && defined(HOST_ARCH_ARM64))

runtime/vm/ffi_callback_metadata_test.cc

@@ -111,7 +111,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateSyncFfiCallback) {
                 static_cast<int>(FfiCallbackMetadata::TrampolineType::kSync));
 
       // head -> tramp1
-      auto* e1 = fcm->MetadataOfTrampoline(tramp1);
+      auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1);
       EXPECT_EQ(isolate->ffi_callback_list_head(), e1);
       EXPECT_EQ(e1->list_prev(), nullptr);
       EXPECT_EQ(e1->list_next(), nullptr);
@@ -135,8 +135,8 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateSyncFfiCallback) {
 
     {
       // head -> tramp2 -> tramp1
-      auto* e1 = fcm->MetadataOfTrampoline(tramp1);
-      auto* e2 = fcm->MetadataOfTrampoline(tramp2);
+      auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1);
+      auto* e2 = fcm->MetadataEntryOfTrampoline(tramp2);
       EXPECT_EQ(isolate->ffi_callback_list_head(), e2);
       EXPECT_EQ(e2->list_prev(), nullptr);
       EXPECT_EQ(e2->list_next(), e1);
@@ -151,7 +151,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateSyncFfiCallback) {
       EXPECT(!m1.IsLive());
 
       // head -> tramp2
-      auto* e2 = fcm->MetadataOfTrampoline(tramp2);
+      auto* e2 = fcm->MetadataEntryOfTrampoline(tramp2);
       EXPECT_EQ(isolate->ffi_callback_list_head(), e2);
       EXPECT_EQ(e2->list_prev(), nullptr);
       EXPECT_EQ(e2->list_next(), nullptr);
@@ -206,7 +206,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateAsyncFfiCallback) {
                 static_cast<int>(FfiCallbackMetadata::TrampolineType::kAsync));
 
       // head -> tramp1
-      auto* e1 = fcm->MetadataOfTrampoline(tramp1);
+      auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1);
       EXPECT_EQ(isolate->ffi_callback_list_head(), e1);
       EXPECT_EQ(e1->list_prev(), nullptr);
       EXPECT_EQ(e1->list_next(), nullptr);
@@ -230,8 +230,8 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateAsyncFfiCallback) {
 
     {
       // head -> tramp2 -> tramp1
-      auto* e1 = fcm->MetadataOfTrampoline(tramp1);
-      auto* e2 = fcm->MetadataOfTrampoline(tramp2);
+      auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1);
+      auto* e2 = fcm->MetadataEntryOfTrampoline(tramp2);
       EXPECT_EQ(isolate->ffi_callback_list_head(), e2);
       EXPECT_EQ(e2->list_prev(), nullptr);
       EXPECT_EQ(e2->list_next(), e1);
@@ -246,7 +246,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateAsyncFfiCallback) {
       EXPECT(!m2.IsLive());
 
       // head -> tramp1
-      auto* e1 = fcm->MetadataOfTrampoline(tramp1);
+      auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1);
       EXPECT_EQ(isolate->ffi_callback_list_head(), e1);
       EXPECT_EQ(e1->list_prev(), nullptr);
       EXPECT_EQ(e1->list_next(), nullptr);
@@ -308,7 +308,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateIsolateLocalFfiCallback) {
                 static_cast<int>(FfiCallbackMetadata::TrampolineType::kSync));
 
       // head -> tramp1
-      auto* e1 = fcm->MetadataOfTrampoline(tramp1);
+      auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1);
       EXPECT_EQ(isolate->ffi_callback_list_head(), e1);
       EXPECT_EQ(e1->list_prev(), nullptr);
       EXPECT_EQ(e1->list_next(), nullptr);
@@ -334,8 +334,8 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateIsolateLocalFfiCallback) {
 
     {
       // head -> tramp2 -> tramp1
-      auto* e1 = fcm->MetadataOfTrampoline(tramp1);
-      auto* e2 = fcm->MetadataOfTrampoline(tramp2);
+      auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1);
+      auto* e2 = fcm->MetadataEntryOfTrampoline(tramp2);
       EXPECT_EQ(isolate->ffi_callback_list_head(), e2);
       EXPECT_EQ(e2->list_prev(), nullptr);
       EXPECT_EQ(e2->list_next(), e1);
@@ -350,7 +350,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_CreateIsolateLocalFfiCallback) {
       EXPECT(!m2.IsLive());
 
       // head -> tramp1
-      auto* e1 = fcm->MetadataOfTrampoline(tramp1);
+      auto* e1 = fcm->MetadataEntryOfTrampoline(tramp1);
       EXPECT_EQ(isolate->ffi_callback_list_head(), e1);
       EXPECT_EQ(e1->list_prev(), nullptr);
       EXPECT_EQ(e1->list_next(), nullptr);
@@ -378,7 +378,7 @@ ISOLATE_UNIT_TEST_CASE(FfiCallbackMetadata_TrampolineRecycling) {
   EXPECT(!code.IsNull());
 
   auto port = PortMap::CreatePort(new FakeMessageHandler());
-  FfiCallbackMetadata::Metadata* list_head = nullptr;
+  FfiCallbackMetadata::MetadataEntry* list_head = nullptr;
 
   // Allocate and free one callback at a time, and verify that we don't reuse
   // them. Allocate enough that the whole page fills up with dead trampolines.
@@ -437,7 +437,7 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_DeleteTrampolines) {
 
   auto* fcm = FfiCallbackMetadata::Instance();
   std::unordered_set<FfiCallbackMetadata::Trampoline> tramps;
-  FfiCallbackMetadata::Metadata* list_head = nullptr;
+  FfiCallbackMetadata::MetadataEntry* list_head = nullptr;
 
   const auto& sync_func = Function::Handle(
       CreateTestFunction(FfiCallbackKind::kIsolateLocalStaticCallback));
@@ -470,23 +470,23 @@ VM_UNIT_TEST_CASE(FfiCallbackMetadata_DeleteTrampolines) {
 
   // Verify the list of callbacks.
   uword list_length = 0;
-  for (FfiCallbackMetadata::Metadata* m = list_head; m != nullptr;) {
+  for (FfiCallbackMetadata::MetadataEntry* me = list_head; me != nullptr;) {
     ++list_length;
-    auto tramp = fcm->TrampolineOfMetadata(m);
-    EXPECT(m->IsLive());
-    EXPECT_EQ(m->target_isolate(), isolate);
+    auto tramp = fcm->TrampolineOfMetadataEntry(me);
+    EXPECT(me->metadata()->IsLive());
+    EXPECT_EQ(me->metadata()->target_isolate(), isolate);
     EXPECT_EQ(tramps.count(tramp), 1u);
-    auto* next = m->list_next();
-    auto* prev = m->list_prev();
+    auto* next = me->list_next();
+    auto* prev = me->list_prev();
     if (prev != nullptr) {
-      EXPECT_EQ(prev->list_next(), m);
+      EXPECT_EQ(prev->list_next(), me);
     } else {
-      EXPECT_EQ(list_head, m);
+      EXPECT_EQ(list_head, me);
     }
     if (next != nullptr) {
-      EXPECT_EQ(next->list_prev(), m);
+      EXPECT_EQ(next->list_prev(), me);
     }
-    m = m->list_next();
+    me = me->list_next();
   }
   EXPECT_EQ(list_length, tramps.size());
 
@@ -518,7 +518,7 @@ static void RunBigRandomMultithreadedTest(uint64_t seed) {
   Random random(seed);
   std::vector<TrampolineWithPort> tramps;
   std::unordered_set<FfiCallbackMetadata::Trampoline> tramp_set;
-  FfiCallbackMetadata::Metadata* list_head = nullptr;
+  FfiCallbackMetadata::MetadataEntry* list_head = nullptr;
 
   const Function& async_func =
       Function::Handle(CreateTestFunction(FfiCallbackKind::kAsyncCallback));
@@ -589,13 +589,13 @@ static void RunBigRandomMultithreadedTest(uint64_t seed) {
 
     // Verify the isolate's list of callbacks.
     uword list_length = 0;
-    for (FfiCallbackMetadata::Metadata* m = list_head; m != nullptr;) {
+    for (FfiCallbackMetadata::MetadataEntry* me = list_head; me != nullptr;) {
       ++list_length;
-      auto tramp = fcm->TrampolineOfMetadata(m);
-      EXPECT(m->IsLive());
-      EXPECT_EQ(m->target_isolate(), isolate);
+      auto tramp = fcm->TrampolineOfMetadataEntry(me);
+      EXPECT(me->metadata()->IsLive());
+      EXPECT_EQ(me->metadata()->target_isolate(), isolate);
       EXPECT_EQ(tramp_set.count(tramp), 1u);
-      m = m->list_next();
+      me = me->list_next();
     }
     EXPECT_EQ(list_length, tramps.size());
     EXPECT_EQ(list_length, tramp_set.size());

runtime/vm/isolate.h

@@ -1299,7 +1299,7 @@ class Isolate : public IntrusiveDListEntry<Isolate> {
   void CloseReceivePort(const ReceivePort& receive_port);
 
   // Visible for testing.
-  FfiCallbackMetadata::Metadata* ffi_callback_list_head() {
+  FfiCallbackMetadata::MetadataEntry* ffi_callback_list_head() {
     return ffi_callback_list_head_;
   }
 
@@ -1673,7 +1673,7 @@ class Isolate : public IntrusiveDListEntry<Isolate> {
   Mutex mutex_;  // Protects compiler stats.
   IsolateMessageHandler* message_handler_ = nullptr;
   intptr_t defer_finalization_count_ = 0;
-  FfiCallbackMetadata::Metadata* ffi_callback_list_head_ = nullptr;
+  FfiCallbackMetadata::MetadataEntry* ffi_callback_list_head_ = nullptr;
   intptr_t ffi_callback_keep_alive_counter_ = 0;
   RelaxedAtomic<ThreadId> owner_thread_ = OSThread::kInvalidThreadId;
 

samples/ffi/httpIG/lib/http.dart

@@ -49,6 +49,7 @@ Future<String> httpGet(String uri) async {
   final response = await completer.future;
 
   rp.close();
+  callback.close();
 
   return response;
 }
@@ -59,20 +60,7 @@ late int counter;
 // Start a HTTP server on a background thread.
 ReceivePort httpServe(void Function(String) onRequest) {
   counter = 0;
-  final rp = ReceivePort()
-    ..listen(
-      (s) {
-        print('httpServe counter: $counter');
-        onRequest(s);
-      },
-      onError: (e, st) {
-        print('httpServe receiver get error $e $st');
-      },
-      onDone: () {
-        nativeHttpStopServing();
-      },
-    );
-
+  final rp = ReceivePort();
   final callback = NativeCallable<HttpCallback>.isolateGroupShared((
     Pointer<Utf8> requestPointer,
   ) {
@@ -83,6 +71,19 @@ ReceivePort httpServe(void Function(String) onRequest) {
     final s = utf8.decode(typedList);
     rp.sendPort.send(s);
   });
+  rp.listen(
+      (s) {
+        print('httpServe counter: $counter');
+        onRequest(s);
+      },
+      onError: (e, st) {
+        print('httpServe receiver get error $e $st');
+      },
+      onDone: () {
+        nativeHttpStopServing();
+        callback.close();
+      },
+    );
 
   // Invoke the native function to start the HTTP server. Our example
   // HTTP library will start a server on a background thread, and pass

sdk/lib/_internal/vm/lib/ffi_patch.dart

@@ -349,9 +349,11 @@ final class _NativeCallableListener<T extends Function>
 
 final class _NativeCallableIsolateGroupShared<T extends Function>
     extends _NativeCallableBase<T> {
-  bool _isKeepingIsolateAlive = false;
+  bool _isKeepingIsolateAlive = true;
 
-  _NativeCallableIsolateGroupShared(super._pointer);
+  _NativeCallableIsolateGroupShared(super._pointer) {
+    _updateNativeCallableKeepIsolateAliveCounter(1);
+  }
 
   @override
   void _close() {
@@ -367,7 +369,7 @@ final class _NativeCallableIsolateGroupShared<T extends Function>
   }
 
   @override
-  bool get _keepIsolateAlive => false;
+  bool get _keepIsolateAlive => _isKeepingIsolateAlive;
 }
 
 @patch

tests/ffi/isolate_group_shared_callback_test.dart

@@ -51,6 +51,10 @@ class NativeLibrary {
         .lookupFunction<FnRunnerNativeType, FnRunnerType>(
           "CallFunctionOnNewThreadNonBlocking",
         );
+    callFunctionOnNewThreadBlocking = ffiTestFunctions
+        .lookupFunction<FnRunnerNativeType, FnRunnerType>(
+          "CallFunctionOnNewThreadBlocking",
+        );
     callTwoIntFunction = ffiTestFunctions
         .lookupFunction<TwoIntFnNativeType, TwoIntFnType>("CallTwoIntFunction");
     sleep = ffiTestFunctions.lookupFunction<FnSleepNativeType, FnSleepType>(
@@ -112,6 +116,7 @@ Future<void> testNativeCallableHelloWorld() async {
     }
   });
   Expect.equals(42 + (1001 * 123) * 2, result);
+  callback.close();
 }
 
 void simpleFunctionThatThrows(int a, int b) {
@@ -128,12 +133,19 @@ Future<void> testNativeCallableThrows() async {
 
   result = 42;
   resultIsReady = false;
-  lib.callFunctionOnNewThreadNonBlocking(1001, callback.nativeFunction);
+  // The call is blocking so that tsan does not complain about read/write
+  // race between invoking the callback and closing it few lines down below.
+  // So the main thing this test checks is condition variable timeout,
+  // which is still valuable.
+  lib.callFunctionOnNewThreadBlocking(1001, callback.nativeFunction);
 
   mutexCondvar.runLocked(() {
-    conditionVariable.wait(mutexCondvar, 10 * sleepForMs);
+    // Just have short one second sleep - the condition variable is not
+    // going to be triggered.
+    conditionVariable.wait(mutexCondvar, 1 * sleepForMs);
     Expect.isFalse(resultIsReady);
   });
+  callback.close();
 }
 
 Future<void> testNativeCallableHelloWorldClosure() async {
@@ -171,6 +183,7 @@ Future<void> testNativeCallableHelloWorldClosure() async {
     }
   });
   Expect.equals(42 + (1001 * 123) * 2, result);
+  callback.close();
 }
 
 void testNativeCallableSync() {
@@ -225,6 +238,51 @@ void testNativeCallableAccessNonSharedVar() {
   callback.close();
 }
 
+Future<void> testKeepIsolateAliveTrue() async {
+  ReceivePort rpOnExit = ReceivePort("onExit");
+  Isolate.spawn(
+    (_) async {
+      final callback = NativeCallable<CallbackNativeType>.isolateGroupShared(
+        simpleFunction,
+      );
+      callback.keepIsolateAlive = true;
+    },
+    /*message=*/ null,
+    onExit: rpOnExit.sendPort,
+  );
+  try {
+    await rpOnExit.first.timeout(Duration(seconds: 5));
+    // should not fall through, should throw TimeoutException
+    Expect.isTrue(false);
+  } catch (e) {
+    print('caught $e');
+    Expect.isTrue(e is TimeoutException);
+  }
+  rpOnExit.close();
+}
+
+Future<void> testKeepIsolateAliveFalse() async {
+  ReceivePort rpOnExit = ReceivePort("onExit");
+  Isolate.spawn(
+    (_) async {
+      final callback = NativeCallable<CallbackNativeType>.isolateGroupShared(
+        simpleFunction,
+      );
+      callback.keepIsolateAlive = false;
+    },
+    /*message=*/ null,
+    onExit: rpOnExit.sendPort,
+  );
+  try {
+    await rpOnExit.first.timeout(Duration(seconds: 30));
+  } catch (e) {
+    // should not throw timeout exception
+    print('caught $e');
+    Expect.isTrue(false);
+  }
+  rpOnExit.close();
+}
+
 main(args, message) async {
   lib = NativeLibrary();
   // Simple tests.
@@ -234,5 +292,7 @@ main(args, message) async {
   testNativeCallableSync();
   testNativeCallableSyncThrows();
   testNativeCallableAccessNonSharedVar();
+  await testKeepIsolateAliveTrue();
+  await testKeepIsolateAliveFalse();
   print("All tests completed :)");
 }