[cronet_http] Fix: multiple Flutter engines would cause crash

[HosseinYousefi]

Upgraded jni and jnigen to 0.15.0. This fixes #1217 and closes #1720.

[github-actions]

PR Health

License Headers ✔️

// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/http/example/main.dart

This check can be disabled by tagging the PR with skip-license-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package	Leaked API symbol	Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

Breaking changes ⚠️

Package	Change	Current Version	New Version	Needed Version	Looking good?
cronet_http	Non-Breaking	1.6.0	1.6.1-wip	1.7.0 Got "1.6.1-wip" expected >= "1.7.0" (non-breaking changes)	⚠️

This check can be disabled by tagging the PR with skip-breaking-check.

Coverage ⚠️

File	Coverage
pkgs/cronet_http/lib/src/cronet_client.dart	💔 Not covered
pkgs/cronet_http/lib/src/jni/jni_bindings.dart	💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

[HosseinYousefi]

@knopp There seems to be another issue where engineId is null even though IntegrationTestWidgetsFlutterBinding.ensureInitialized(); is called in the CI.

It fails when trying to test a post inside Isolate.run:

http/pkgs/http_client_conformance_tests/lib/src/isolate_test.dart

Lines 16 to 17 in c8355d1

	await Isolate.run(
	() => clientFactory().post(Uri.http(host, ''), body: 'Hello World!'));

Do you have any idea why that might be?

--

Update: I guess the engineId is attached to the main isolate? I wonder if we could attach it to the isolate group instead, this way it would be non-null from all of the isolates

[HosseinYousefi]

@brianquinlan If I change the conformance test so that it creates the client itself in the main isolate and just does the POST request in another isolate, then it would work:

Future<void> _testPost(Client Function() clientFactory, String host) async {
+ final client = clientFactory();
  await Isolate.run(
      () => 
-         clientFactory()
+         client
           .post(Uri.http(host, ''), body: 'Hello World!'));
}

But I'm not sure if you want this behavior for cronet_http.

[knopp]

Can we intercept background isolate creation somehow in the engine to set the engine id? Right now it is only set for the main isolate.

[brianquinlan]

@brianquinlan If I change the conformance test so that it creates the client itself in the main isolate and just does the POST request in another isolate, then it would work:

Future<void> _testPost(Client Function() clientFactory, String host) async {
+ final client = clientFactory();
  await Isolate.run(
      () => 
-         clientFactory()
+         client
           .post(Uri.http(host, ''), body: 'Hello World!'));
}

But I'm not sure if you want this behavior for cronet_http.

At the time that I wrote this test, native pointers could not be shared between isolates. Now that they can be, I think that we should test both scenarios, i.e.:

1. create the Client in one isolate, use it in another
2. create the Client in the non-main isolate and use it there

Performing HTTP requests in a background Isolate is definitely something that people do.

[HosseinYousefi]

Can we intercept background isolate creation somehow in the engine to set the engine id? Right now it is only set for the main isolate.

If we keep a map from isolate group ids to the engine ids, then we could fetch the right engine id using the isolate group id.

Moreover, what if we just have engine id be exactly equal to the isolate group id? I assume that each engine is attached to a single isolate group anyway so this reduces the number of maps we are keeping. This way there is no need to set any engine id, instead we can just call Dart_CurrentIsolateGroup from any isolate and get it.

cc @mkustermann

[mkustermann]

Reading through the code, it seems the implementation does roughtly this:

• package:jni is a real flutter plugin pub package. It's Java code will be invoked (by flutter engine) when the plugin is loaded. That Java code will call into package:jni C code to set a bunch of things, including cache some java context objects (e.g. application context) via jni global refs.
• ??? If using multiple flutter engines, it may do the above multiple times with different context objects
• The bindings of cronet will use package:jni to get application contexts which it needs due to cronet java API needing it (and it gets the context from C code of package:jni)

Now looking through the cronet API it says:
"""
context Android Context to retrieve the application context. A reference to only the application context will be kept, so as to avoid extending the lifetime of context unnecessarily.
"""
And googling around it seemingly suggests that "android application context" has same lifetime as the OS process itself. If this is the case, then I don't see an issue caching the "android application context" in package:jni C code and using across isolates or isolate groups. If this isn't the case (it somehow sounds like it, @HosseinYousefi ?) and an OS process can live longer and multiple "android application context"s can be created inside that process than the current caching of the global ref in C code seems problematic.

Now from a Dart perspective: The OS process can have multiple isolate groups, each of which can have multiple isolates. Flutter may start an isolate group and one particular UI isolate per flutter engine. Any isolate can spawn more isolates at runtime. When the android lifecycle system tells flutter it shuts down the current activity or UI, flutter may stop the UI isolate, but not necessarily background isolates.

So isolates in a group can outlive the UI isolate, so they shouldn't depend on resources only available to the UI isolate (such as an activity context).

So we'd want to avoid sharing a cronet client across isolates in an isolate group if shutting the UI isolate down will make this cronet client defunct/invalid.

If we keep a map from isolate group ids to the engine ids, then we could fetch the right engine id using the isolate group id.

@HosseinYousefi Could you explain why "engine id" matters here? Isn't this purely a question about lifecycle of android context (and other?) objects and from which isolates/threads those objects with lifecycle can be used?

[knopp]

Can we intercept background isolate creation somehow in the engine to set the engine id? Right now it is only set for the main isolate.

If we keep a map from isolate group ids to the engine ids, then we could fetch the right engine id using the isolate group id.

Moreover, what if we just have engine id be exactly equal to the isolate group id? I assume that each engine is attached to a single isolate group anyway so this reduces the number of maps we are keeping. This way there is no need to set any engine id, instead we can just call Dart_CurrentIsolateGroup from any isolate and get it.

cc @mkustermann

On most platforms the engineId is simply a casted pointer to engine instance.

[HosseinYousefi]

that "android application context" has same lifetime as the OS process itself. If this is the case, then I don't see an issue caching the "android application context" in package:jni C code and using across isolates or isolate groups.

We're getting the application context from onAttachedToEngine callback of package:jnis FlutterPlugin and it seems that when another onAttachedToEngine gets called and therefore overrides the cached "global android application context", we get crashes suggesting that we should be using one context per flutter engine.

@HosseinYousefi Could you explain why "engine id" matters here? Isn't this purely a question about lifecycle of android context (and other?) objects and from which isolates/threads those objects with lifecycle can be used?

Because we store a map from "engine id" to "android application context" to be able to retrieve one context per flutter engine.

https://github.com/dart-lang/native/blob/28b7c5fcfc1492d1821af0cdcb3c277c372110b1/pkgs/jni/android/src/main/java/com/github/dart_lang/jni/JniPlugin.java#L18

So isolates in a group can outlive the UI isolate, so they shouldn't depend on resources only available to the UI isolate (such as an activity context).

Yes and we should remove the stored context from the map when onDetachedFromEngine gets called.

https://github.com/dart-lang/native/blob/28b7c5fcfc1492d1821af0cdcb3c277c372110b1/pkgs/jni/android/src/main/java/com/github/dart_lang/jni/JniPlugin.java#L41-L46

That's not a problem for fetching the short-lived activity because we explicitly document that it should not be used from the other threads:

https://github.com/dart-lang/native/blob/28b7c5fcfc1492d1821af0cdcb3c277c372110b1/pkgs/jni/lib/src/jni.dart#L228-L287

This is a general package:jni problem, regardless of whether cronet specifically is being affected because of it: dart-lang/native#1865

[mkustermann]

(Apologies I was accidentally looking at the old code from the archived https://github.com/dart-lang/jnigen)

We're getting the application context from onAttachedToEngine callback of package:jnis FlutterPlugin and it seems that when another onAttachedToEngine gets called and therefore overrides the cached "global android application context", we get crashes suggesting that we should be using one context per flutter engine.

Well, specifically for the "global android application context" it seems there's only one per process so why not just a static global for it, initialize it once, on any second attachment call check that it's the same reference (which it should if there's only one per process) and never clear it out?

See docs which says "Return the context of the single, global Application object of the current process. "

we get crashes suggesting that we should be using one context per flutter engine.

What kind of crash?

[HosseinYousefi]

Well, specifically for the "global android application context" it seems there's only one per process so why not just a static global for it, initialize it once, on any second attachment call check that it's the same reference (which it should if there's only one per process) and never clear it out?

Somehow it seems that it's not the case. Maybe the context we get from onAttachedToEngine is not the global android application context but something tied to the Flutter engine. I think we need this kind of context as well specially for UI, but maybe then we can afford to limit the context getter to the platform thread as well.

I could try getting the application context via ActivityThread.currentApplication() and see if things would be fixed that way for cronet or not. This API is annotated with @UnsupportedAppUsage so I'm not sure how stable it will be.

Edit: yeah,context.getApplicationContext() should theoretically provide something that doesn't change.

What kind of crash?

There are two specified in #1217 and #1842. For the second one I asked if they still face the same problem if they remove any background tasks via creating new flutter engines and they don't. I should try to reproduce this in a small example myself to make sure I'm solving the right problem here.

[mkustermann]

Somehow it seems that it's not the case. Maybe the context we get from onAttachedToEngine is not the global android application context but something tied to the Flutter engine.

If so, can you get the global one by calling Context.getApplicationContext() on the one you get from flutter? The real application context probably satisfies context == context.GetApplicationContext() condition.