Use the Dart isolate ownership API on the root isolate #163703

liamappelbe · 2025-02-20T04:33:08Z

The isolate ownership API was introduced recently to solve some deadlock bugs in native callbacks.

A native callback is a call from native code into a Dart function. Currently all such callbacks must run that Dart function in the isolate that created the callback (called the target isolate). The only native callback primitives at the moment are NativeCallable.isolateLocal (blocking, but must be invoked from the same thread as the target isolate, and the target isolate must be currently entered on that thread) and NativeCallable.listener (non-blocking, can be invoked from any thread).

To build blocking callbacks that can be called from any thread, we can use a NativeCallable.listener, and use a synchronization object like a mutex or a condition variable to block until the callback is complete. However, if we try to do this on the thread that is currently entered in the target isolate, we will deadlock: we invoke the listener, a message is sent to the target isolate, and we block waiting for the message to be handled, so we never pass control flow back to the isolate to handle the message, and never stop waiting.

To fix this deadlock, Ffigen and Jnigen both have a mechanism that checks if we're on the target isolate's thread first:

If the native caller is already on the same thread as the target isolate, and the target isolate is entered:
- Call the Dart function directly using NativeCallable.isolateLocal or similar
Otherwise, if the native caller is coming from a different thread:
- Call the Dart function asynchronously using NativeCallable.listener or similar
- Block until the callback finishes

However, this neglects the case where we're on the target isolate's thread, but not entered into the isolate. This case happens in Flutter when the callback is invoked from the UI thread (or the platform thread when thread merging is enabled), and the target isolate is the root isolate. When the native callback is invoked, the root isolate is not entered, so we hit the second case: we send a message to the root isolate, and block to wait for a response. Since the root isolate is exclusively run on the UI thread, and we're blocking the UI thread, the message will never be handled, and we deadlock.

The isolate ownership API fixes this by allowing the embedder to inform the VM that it will run a particular isolate exclusively on a particular thread, using Dart_SetCurrentThreadOwnsIsolate. Other native code can then query that ownership using Dart_GetCurrentThreadOwnsIsolate. This lets us add a third case to our conditional:

If the native caller is on the thread that is currently entered in the target isolate:
- Call the Dart function directly using NativeCallable.isolateLocal or similar
Otherwise, if the native caller is on the thread that owns the target isolate
- Enter the target isolate
- Call the Dart function directly using NativeCallable.isolateLocal or similar
- Exit the target isolate
Otherwise, the native caller is coming from an unrelated thread:
- Call the Dart function asynchronously using NativeCallable.listener or similar
- Block until the callback finishes

Note: We don't need to set the ownership of VM managed threads, because they run in a thread pool exclusively used by the VM, so there's no way for native code to be executed on the thread (except by FFI, in which case we're entered into the isolate anyway). We only need this for Flutter's root isolate because work can be sent to the UI thread/platform thread using OS specific APIs like Android's Looper.getMainLooper().

knopp

This looks good to me.

@chinmaygarde, any objections merging this?
@liamappelbe, is this already wired in the NativeCalable.isolateLocal trampoline?

knopp · 2025-02-21T16:19:36Z

Actually, maybe I don't quite understand how this works. Since Dart_GetCurrentThreadOwnsIsolate requires a send port, and isolate local FFI metadata doesn't have a send port (it is synchronous), how does the trampoline know that current thread owns the isolate and should enter it?

knopp · 2025-02-21T16:34:22Z

I was hoping that instead of failing in DLRT_GetFfiCallbackMetadata when invoked outside of isolate, it could get check that the metadata isolate is owned by current thread and enter it, but I guess that's not the plan then? Would be nice to not have to do this manually when calling the trampoline from native callbacks.

liamappelbe · 2025-02-24T00:20:15Z

@liamappelbe, is this already wired in the NativeCalable.isolateLocal trampoline?

No, we'll be handling this at the level of jnigen/ffigen generated bindings for now. We might introduce another variant of NativeCalable that is blocking and can be invoked from any thread, but I don't think we'll ever change the semantics of NativeCalable.isolateLocal.

One reason I'm hesitant to add NativeCallable.blocking right away is that there are subtle deadlocks that can happen (such as the one we're trying to solve here), and it's going to be hard to be certain we've found them all. It's much easier to make changes to ffigen and jnigen to fix these issues (O(days)), than to make Dart VM changes and wait for them to land in flutter stable (O(months)). Maybe once blocking callbacks in ffigen and jnigen are well battle tested, and we're sure we've squashed all the bugs, we can take all that knowledge and use it to build a good NativeCallable.blocking implementation.

Since Dart_GetCurrentThreadOwnsIsolate requires a send port, and isolate local FFI metadata doesn't have a send port (it is synchronous), how does the trampoline know that current thread owns the isolate and should enter it?

We're using ports because they're much safer than Dart_Isolate in terms of use-after-free errors. In ffigen I'm just going to grab the isolate's main port: Dart_GetMainPortId. I can safely hold a reference to that Dart_Port and use it to check for isolate ownership long after the isolate and port have been destroyed (because they're essentially just a 64-bit random number).

Would be nice to not have to do this manually when calling the trampoline from native callbacks.

Most users are doing interop using ffigen/jnigen, so they won't have to check ownership manually.

chinmaygarde

Got distracted by the two callbacks. This patch LGTM.

chinmaygarde · 2025-02-26T19:09:34Z

engine/src/flutter/runtime/dart_isolate.cc

+    }
  }

  if (root_isolate_create_callback) {


This patch lgtm but I got totally confused by the two "root isolate create callbacks". One in the settings, and another passed as an argument. Why are there two? Why does only one run in isolate scope? Why can't one wrap the other?

Not sure. I've only touched this code a couple of times 😅

…er/flutter#163703)

Roll Flutter from 1659206 to 2e570ca (75 revisions) flutter/flutter@1659206...2e570ca 2025-02-27 [email protected] Roll Skia from ac08df9c8d82 to e5a33102401c (1 revision) (flutter/flutter#164288) 2025-02-27 [email protected] Roll Skia from 6018dff0233a to ac08df9c8d82 (1 revision) (flutter/flutter#164275) 2025-02-27 [email protected] Roll Dart SDK from 4a218fbffc80 to fcda71ce147b (1 revision) (flutter/flutter#164276) 2025-02-27 [email protected] Roll Skia from 0f2e106c9abc to 6018dff0233a (1 revision) (flutter/flutter#164266) 2025-02-27 [email protected] Add windowing channel support to Linux embedder (flutter/flutter#163180) 2025-02-27 [email protected] Roll Dart SDK from 7fa5901bd8a3 to 4a218fbffc80 (2 revisions) (flutter/flutter#164260) 2025-02-27 [email protected] Roll Skia from fdd97386193e to 0f2e106c9abc (4 revisions) (flutter/flutter#164259) 2025-02-27 [email protected] Run more builds faster (flutter/flutter#164125) 2025-02-27 [email protected] Increase customer test timeout to 60 minutes (flutter/flutter#164239) 2025-02-27 [email protected] Reland "Add a buildtools directory and move third_party/ninja to the project root in order to match the expectations of depot_tools" (flutter/flutter#164240) 2025-02-27 [email protected] Use the Dart isolate ownership API on the root isolate (flutter/flutter#163703) 2025-02-26 [email protected] Roll Fuchsia Linux SDK from g-2SJtblPjjaH7Egy... to 1elkOxihZuTEiTXzY... (flutter/flutter#164243) 2025-02-26 [email protected] Show Linux driver information in flutter doctor (flutter/flutter#163980) 2025-02-26 [email protected] Implement opacity `FlutterMutator` for hcpp (flutter/flutter#164147) 2025-02-26 [email protected] Roll Fuchsia Linux SDK from g-2SJtblPjjaH7Egy... to 1elkOxihZuTEiTXzY... (flutter/flutter#164232) 2025-02-26 [email protected] Roll Skia from 47b84f354604 to fdd97386193e (8 revisions) (flutter/flutter#164221) 2025-02-26 [email protected] [Impeller] Reland: move AHB check into Flutter main, don't disable ImageReader on 29. (flutter/flutter#164201) 2025-02-26 [email protected] Roll Dart SDK from 80865748abe0 to 7fa5901bd8a3 (3 revisions) (flutter/flutter#164226) 2025-02-26 [email protected] Update fuchsia_test_scripts_version to the latest version (flutter/flutter#164123) 2025-02-26 [email protected] Guard against zero item extent for carousel (flutter/flutter#163310) 2025-02-26 [email protected] [Impeller] work around for crashy Nexus 5 Driver. (flutter/flutter#164040) 2025-02-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add a buildtools directory and move third_party/ninja to the project root in order to match the expectations of depot_tools (#163890)" (flutter/flutter#164209) 2025-02-26 [email protected] Add localization for `Back` and `Cancel` buttons in CupertinoNavigationBar (flutter/flutter#162581) 2025-02-26 [email protected] Added compilation failure if a max ubo size is exceeded (flutter/flutter#164038) 2025-02-26 [email protected] Roll Skia from 5336e20498d9 to 47b84f354604 (2 revisions) (flutter/flutter#164188) 2025-02-26 [email protected] Add a buildtools directory and move third_party/ninja to the project root in order to match the expectations of depot_tools (flutter/flutter#163890) 2025-02-26 [email protected] Roll Skia from 3ce6f25dc13e to 5336e20498d9 (9 revisions) (flutter/flutter#164174) 2025-02-26 [email protected] Roll Dart SDK from b8292dfeaa67 to 80865748abe0 (9 revisions) (flutter/flutter#164168) 2025-02-26 [email protected] [Engine] Remove dead code for RoundedSuperellipse (flutter/flutter#164163) 2025-02-26 [email protected] [Impeller] detect mediatek soc and fall back to GLES. (flutter/flutter#164126) 2025-02-26 [email protected] [Impeller] make DLOG into LOG for startup errors. (flutter/flutter#164110) 2025-02-26 [email protected] Intercept error when iOS 18.4 crashes with JIT mode and give guided error (flutter/flutter#164072) 2025-02-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] move AHB check to Vulkan, use Vulkan surface on 29. (#164109)" (flutter/flutter#164166) 2025-02-26 [email protected] Run new gallery transition perf benchmark on Galaxy S24 (flutter/flutter#163665) 2025-02-26 [email protected] [Impeller] move AHB check to Vulkan, use Vulkan surface on 29. (flutter/flutter#164109) 2025-02-26 [email protected] [iOS] Add platform view to integration_test example (flutter/flutter#164144) 2025-02-26 [email protected] Fix minor issues in documentation of WidgetsApp (flutter/flutter#163942) 2025-02-26 [email protected] Fix extra numbers showing up when enabling VoiceControl (flutter/flutter#163593) 2025-02-26 [email protected] Set SliverResizingHeader's maxScrollObstructionExtent to minExtent (flutter/flutter#162955) 2025-02-26 [email protected] Roll Dart SDK from aea6fff33f06 to b8292dfeaa67 (1 revision) (flutter/flutter#163973) 2025-02-26 [email protected] [DisplayList] Delete all legacy Skia-oriented method overloads in DlCanvas (flutter/flutter#164054) 2025-02-25 [email protected] Clean up leak tracker instrumentation tech debt. (flutter/flutter#164070) 2025-02-25 [email protected] Revert "Marks Linux_pixel_7pro service_extensions_test to be flaky" (flutter/flutter#163882) 2025-02-25 [email protected] Check if simctl is installed before trying to list devices or runtimes (flutter/flutter#163895) 2025-02-25 [email protected] Update dragDevices doc to include default PointerDeviceKind.trackpad (flutter/flutter#163898) 2025-02-25 [email protected] Update multiple flutters benchmark test to latest gradle and agp and gradle defined dependencies (flutter/flutter#164029) ...

…er/flutter#163703)

Roll Flutter from 1659206 to 2e570ca (75 revisions) flutter/flutter@1659206...2e570ca 2025-02-27 [email protected] Roll Skia from ac08df9c8d82 to e5a33102401c (1 revision) (flutter/flutter#164288) 2025-02-27 [email protected] Roll Skia from 6018dff0233a to ac08df9c8d82 (1 revision) (flutter/flutter#164275) 2025-02-27 [email protected] Roll Dart SDK from 4a218fbffc80 to fcda71ce147b (1 revision) (flutter/flutter#164276) 2025-02-27 [email protected] Roll Skia from 0f2e106c9abc to 6018dff0233a (1 revision) (flutter/flutter#164266) 2025-02-27 [email protected] Add windowing channel support to Linux embedder (flutter/flutter#163180) 2025-02-27 [email protected] Roll Dart SDK from 7fa5901bd8a3 to 4a218fbffc80 (2 revisions) (flutter/flutter#164260) 2025-02-27 [email protected] Roll Skia from fdd97386193e to 0f2e106c9abc (4 revisions) (flutter/flutter#164259) 2025-02-27 [email protected] Run more builds faster (flutter/flutter#164125) 2025-02-27 [email protected] Increase customer test timeout to 60 minutes (flutter/flutter#164239) 2025-02-27 [email protected] Reland "Add a buildtools directory and move third_party/ninja to the project root in order to match the expectations of depot_tools" (flutter/flutter#164240) 2025-02-27 [email protected] Use the Dart isolate ownership API on the root isolate (flutter/flutter#163703) 2025-02-26 [email protected] Roll Fuchsia Linux SDK from g-2SJtblPjjaH7Egy... to 1elkOxihZuTEiTXzY... (flutter/flutter#164243) 2025-02-26 [email protected] Show Linux driver information in flutter doctor (flutter/flutter#163980) 2025-02-26 [email protected] Implement opacity `FlutterMutator` for hcpp (flutter/flutter#164147) 2025-02-26 [email protected] Roll Fuchsia Linux SDK from g-2SJtblPjjaH7Egy... to 1elkOxihZuTEiTXzY... (flutter/flutter#164232) 2025-02-26 [email protected] Roll Skia from 47b84f354604 to fdd97386193e (8 revisions) (flutter/flutter#164221) 2025-02-26 [email protected] [Impeller] Reland: move AHB check into Flutter main, don't disable ImageReader on 29. (flutter/flutter#164201) 2025-02-26 [email protected] Roll Dart SDK from 80865748abe0 to 7fa5901bd8a3 (3 revisions) (flutter/flutter#164226) 2025-02-26 [email protected] Update fuchsia_test_scripts_version to the latest version (flutter/flutter#164123) 2025-02-26 [email protected] Guard against zero item extent for carousel (flutter/flutter#163310) 2025-02-26 [email protected] [Impeller] work around for crashy Nexus 5 Driver. (flutter/flutter#164040) 2025-02-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add a buildtools directory and move third_party/ninja to the project root in order to match the expectations of depot_tools (#163890)" (flutter/flutter#164209) 2025-02-26 [email protected] Add localization for `Back` and `Cancel` buttons in CupertinoNavigationBar (flutter/flutter#162581) 2025-02-26 [email protected] Added compilation failure if a max ubo size is exceeded (flutter/flutter#164038) 2025-02-26 [email protected] Roll Skia from 5336e20498d9 to 47b84f354604 (2 revisions) (flutter/flutter#164188) 2025-02-26 [email protected] Add a buildtools directory and move third_party/ninja to the project root in order to match the expectations of depot_tools (flutter/flutter#163890) 2025-02-26 [email protected] Roll Skia from 3ce6f25dc13e to 5336e20498d9 (9 revisions) (flutter/flutter#164174) 2025-02-26 [email protected] Roll Dart SDK from b8292dfeaa67 to 80865748abe0 (9 revisions) (flutter/flutter#164168) 2025-02-26 [email protected] [Engine] Remove dead code for RoundedSuperellipse (flutter/flutter#164163) 2025-02-26 [email protected] [Impeller] detect mediatek soc and fall back to GLES. (flutter/flutter#164126) 2025-02-26 [email protected] [Impeller] make DLOG into LOG for startup errors. (flutter/flutter#164110) 2025-02-26 [email protected] Intercept error when iOS 18.4 crashes with JIT mode and give guided error (flutter/flutter#164072) 2025-02-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] move AHB check to Vulkan, use Vulkan surface on 29. (#164109)" (flutter/flutter#164166) 2025-02-26 [email protected] Run new gallery transition perf benchmark on Galaxy S24 (flutter/flutter#163665) 2025-02-26 [email protected] [Impeller] move AHB check to Vulkan, use Vulkan surface on 29. (flutter/flutter#164109) 2025-02-26 [email protected] [iOS] Add platform view to integration_test example (flutter/flutter#164144) 2025-02-26 [email protected] Fix minor issues in documentation of WidgetsApp (flutter/flutter#163942) 2025-02-26 [email protected] Fix extra numbers showing up when enabling VoiceControl (flutter/flutter#163593) 2025-02-26 [email protected] Set SliverResizingHeader's maxScrollObstructionExtent to minExtent (flutter/flutter#162955) 2025-02-26 [email protected] Roll Dart SDK from aea6fff33f06 to b8292dfeaa67 (1 revision) (flutter/flutter#163973) 2025-02-26 [email protected] [DisplayList] Delete all legacy Skia-oriented method overloads in DlCanvas (flutter/flutter#164054) 2025-02-25 [email protected] Clean up leak tracker instrumentation tech debt. (flutter/flutter#164070) 2025-02-25 [email protected] Revert "Marks Linux_pixel_7pro service_extensions_test to be flaky" (flutter/flutter#163882) 2025-02-25 [email protected] Check if simctl is installed before trying to list devices or runtimes (flutter/flutter#163895) 2025-02-25 [email protected] Update dragDevices doc to include default PointerDeviceKind.trackpad (flutter/flutter#163898) 2025-02-25 [email protected] Update multiple flutters benchmark test to latest gradle and agp and gradle defined dependencies (flutter/flutter#164029) ...

liamappelbe added 2 commits February 20, 2025 11:51

Set isolate ownership

e349d71

test

d05ec13

github-actions bot added the engine flutter/engine related. See also e: labels. label Feb 20, 2025

knopp self-requested a review February 21, 2025 16:11

knopp approved these changes Feb 21, 2025

View reviewed changes

liamappelbe requested a review from chinmaygarde February 26, 2025 04:19

chinmaygarde approved these changes Feb 26, 2025

View reviewed changes

Merge branch 'master' into isolate_ownership

48ac0f9

liamappelbe enabled auto-merge February 26, 2025 22:53

liamappelbe added this pull request to the merge queue Feb 27, 2025

auto-merge was automatically disabled February 27, 2025 00:39
Pull Request is not mergeable

Merged via the queue into flutter:master with commit 0f3b092 Feb 27, 2025
174 of 175 checks passed

liamappelbe deleted the isolate_ownership branch February 27, 2025 00:39

engine-flutter-autoroll mentioned this pull request Feb 27, 2025

Roll Flutter from 16592066d70b to 2e570ca393c8 (75 revisions) flutter/packages#8731

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 27, 2025

0f3b092 Use the Dart isolate ownership API on the root isolate (flutt…

6d6a1aa

…er/flutter#163703)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

0f3b092 Use the Dart isolate ownership API on the root isolate (flutt…

89ad0e6

…er/flutter#163703)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

0f3b092 Use the Dart isolate ownership API on the root isolate (flutt…

ac945d2

…er/flutter#163703)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025

0f3b092 Use the Dart isolate ownership API on the root isolate (flutt…

c50f15d

…er/flutter#163703)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use the Dart isolate ownership API on the root isolate #163703

Use the Dart isolate ownership API on the root isolate #163703

Uh oh!

liamappelbe commented Feb 20, 2025

Uh oh!

knopp left a comment

Uh oh!

knopp commented Feb 21, 2025

Uh oh!

knopp commented Feb 21, 2025 •

edited

Loading

Uh oh!

liamappelbe commented Feb 24, 2025

Uh oh!

chinmaygarde left a comment

Uh oh!

chinmaygarde Feb 26, 2025

Uh oh!

liamappelbe Feb 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Use the Dart isolate ownership API on the root isolate #163703

Use the Dart isolate ownership API on the root isolate #163703

Uh oh!

Conversation

liamappelbe commented Feb 20, 2025

Uh oh!

knopp left a comment

Choose a reason for hiding this comment

Uh oh!

knopp commented Feb 21, 2025

Uh oh!

knopp commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liamappelbe commented Feb 24, 2025

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

liamappelbe Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

knopp commented Feb 21, 2025 •

edited

Loading