-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cronet_http] Crash occurring on 32bit android devices since version 1.2.1 #1213
Comments
What version of Android are you using on your Galaxy A11? |
Can you reproduce this with |
I looked at the change history - the only thing that changed in I couldn't find any similar issues reported on Any help with a reproduction that doesn't require owning a specific phone module would be helpful. |
@HosseinYousefi Can you think of any way that this issue could be related to If not, I'll see more help to reproduce @veloce |
I mean if the only change is the jni version. Then maybe that causes the issue. The error message doesn't seem to be related to jni though. Does it also happen on flutter stable? |
I could reproduce on flutter stable (still with the Galaxy A11). This time I managed to get more logs:
Full log here: |
There are some Cronet-related log lines before the failure e.g. Could you try testing these scenarios (to see if you get the same error):
|
Looks like it is not due to cronet itself. Based on this results I had doubts on what I submitted earlier, so I tried again with For each scenario I used flutter stable and a debug build (and with cronetHttpNoPlay
app-debug.apk_2024_5_31_9_46_1.log 2 Remove dependency on
|
Now I could finally reproduce the problem happening in cronet. I got confused because I was seeing both errors in crashlytics (this one and the With this Gradle config:
The cronet example app is now running on the Galaxy A11. And now I can reproduce the difference between
app-release.apk_2024_5_31_12_0_30.log And app-release.apk_2024_5_31_12_16_11.log EDIT I also tried with |
It looks like that exception comes from here: It looks like the documented cache mode constants: Match what
What cache mode are you using in |
Using whatever If I remove these lines in http/pkgs/cronet_http/lib/src/cronet_client.dart Lines 105 to 109 in 6337ee3
So the origin of the bug is in the new 76deb75#diff-a5b5707c935a2d47feed8e55b3d6f5a8fa7118d44d6847ea5dc0dd892de91a2fR2549 Where the
Specifically this line I think: |
@veloce Amazing analysis! static final _id_enableHttpCache = _class.instanceMethodId(
r"enableHttpCache",
r"(IJ)Lorg/chromium/net/CronetEngine$Builder;",
);
+ static final _enableHttpCache = ProtectedJniExtensions.lookup<
+ ffi.NativeFunction<
+ jni.JniResult Function(ffi.Pointer<ffi.Void>,
+ jni.JMethodIDPtr, ffi.VarArgs<(ffi.Int64, ffi.Int64)>)>>(
+ "globalEnv_CallObjectMethod")
+ .asFunction<
+ jni.JniResult Function(
+ ffi.Pointer<ffi.Void>, jni.JMethodIDPtr, int, int)>();
/// from: public org.chromium.net.CronetEngine$Builder enableHttpCache(int i, long j)
/// The returned object must be released after use, by calling the [release] method.
CronetEngine_Builder enableHttpCache(
int i,
int j,
) {
- return _id_enableHttpCache(
- this, const $CronetEngine_BuilderType(), [jni.JValueInt(i), j]);
+ return _enableHttpCache(
+ reference.pointer, _id_enableHttpCache as jni.JMethodIDPtr, i, j)
+ .object(const $CronetEngine_BuilderType());
}
|
The signature is @HosseinYousefi can you take a look at this? This looks like a |
@veloce Thanks for the analysis. I will fix it. |
The reason that this is From dart-lang/native#1090 (comment):
Famous last words! |
We've addressed this bug, but it's not yet in a stable release. So we can either:
Do you have mixing and matching of |
Yes, unfortunately I do. Otherwise I could use I could revert the change back to using arrays but this problem only happens for macOS arm64 (which is not used much with jnigen), so I'd prefer not to do that as we gain significant performance boost from using varargs. |
How is using AbiSpecificInteger with double and float a workaround here? Wouldn't that only work properly for types that are doubles/floats? I'm trying to understand whether we should add that as a feature for |
It's just a temporary solution to fix the current issue, right? We can create an abi specific "floating point number" for float32 that is only float64 for mac arm64 (and float32 for all other archs). This fixes the varargs issue for mac arm64, while providing the same experience for every other arch. |
Could I go back to |
I'd rather make a version that breaks macOS arm64, as no one really uses jni for macOS for the time being. It will just break the CI which is fine. I'll open a PR on both repos. |
Thanks! |
Using the official example, I consistently reproduce a crash when launching the application on a Samsung Galaxy A11 running on android 10 (using Browserstack App Live).
I'm building the application using flutter main channel.
Here are the logs:
The application runs fine on the same device with
cronet_http
version 1.2.0.I cannot ascertain it, but based on crash reports in my application it looks like these also suffer from the crash (and probably others):
The text was updated successfully, but these errors were encountered: