fix(jni/ffi): dart to native type conversion #3372
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Fixes
- dart to native type conversion ([#3372](https://github.com/getsentry/sentry-dart/pull/3372))If none of the above apply, you can opt out of this check by adding |
| @visibleForTesting | ||
| JObject dartToJObject(Object? value) => switch (value) { | ||
| String s => s.toJString(), | ||
| bool b => b.toJBoolean(), | ||
| int i => i.toJLong(), | ||
| double d => d.toJDouble(), | ||
| List<dynamic> l => _dartToJList(l), | ||
| Map<String, dynamic> m => _dartToJMap(m), | ||
| _ => null | ||
| List<dynamic> l => dartToJList(l), | ||
| Map<String, dynamic> m => dartToJMap(m), | ||
| _ => value.toString().toJString() | ||
| }; | ||
|
|
||
| JList<JObject?> _dartToJList(List<dynamic> values) { | ||
| final jList = JList.array(JObject.nullableType); | ||
| for (final v in values) { | ||
| final j = _dartToJObject(v); | ||
| @visibleForTesting | ||
| JList<JObject> dartToJList(List<dynamic> values) { | ||
| final jList = JList.array(JObject.type); | ||
| for (final v in values.nonNulls) { | ||
| final j = dartToJObject(v); | ||
| jList.add(j); | ||
| j?.release(); | ||
| j.release(); | ||
| } | ||
| return jList; | ||
| } | ||
|
|
||
| JMap<JString, JObject?> _dartToJMap(Map<String, dynamic> json) { | ||
| final jMap = JMap.hash(JString.type, JObject.nullableType); | ||
| for (final entry in json.entries) { | ||
| @visibleForTesting | ||
| JMap<JString, JObject> dartToJMap(Map<String, dynamic> json) { | ||
| final jMap = JMap.hash(JString.type, JObject.type); | ||
| for (final entry in json.entries.where((e) => e.value != null)) { | ||
| final jk = entry.key.toJString(); | ||
| final jv = _dartToJObject(entry.value); | ||
| final jv = dartToJObject(entry.value); | ||
| jMap[jk] = jv; | ||
| jk.release(); | ||
| jv?.release(); | ||
| jv.release(); | ||
| } | ||
| return jMap; |
There was a problem hiding this comment.
java drops null values so we can just directly filter null values out
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| _ => null | ||
| List<dynamic> l => dartToJList(l), | ||
| Map<String, dynamic> m => dartToJMap(m), | ||
| _ => value.toString().toJString() |
There was a problem hiding this comment.
Bug: Null values in setContexts converted to "null" string
The dartToJObject function accepts Object? but the null check was removed from setContexts. When null is passed as a value, it falls through to value.toString().toJString() which converts it to the literal string "null". Previously, setContexts had an early return for null values (if (jVal == null) return;). Now setContexts("key", null) sets the context to "null" string instead of being ignored. This is inconsistent with the Cocoa implementation which properly handles null, and with setExtra which has a null guard. Flagging per review rules about behavioral compatibility.
Additional Locations (1)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3372 +/- ##
==========================================
+ Coverage 88.19% 89.96% +1.76%
==========================================
Files 291 95 -196
Lines 9957 3198 -6759
==========================================
- Hits 8782 2877 -5905
+ Misses 1175 321 -854 ☔ View full report in Codecov by Sentry. |
iOS Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c0dde50 | 1268.90 ms | 1275.61 ms | 6.71 ms |
| 1f639ee | 1252.43 ms | 1257.82 ms | 5.38 ms |
| 7cfee3b | 1260.90 ms | 1273.14 ms | 12.24 ms |
| 192b44c | 1269.08 ms | 1275.52 ms | 6.44 ms |
| 7b21e8b | 1256.79 ms | 1267.12 ms | 10.33 ms |
| 6f47800 | 1247.52 ms | 1259.37 ms | 11.85 ms |
| ad121c0 | 1275.04 ms | 1280.59 ms | 5.55 ms |
| e04b24b | 1230.22 ms | 1233.90 ms | 3.67 ms |
| 819c1e7 | 1250.59 ms | 1249.08 ms | -1.51 ms |
| d0aa4b6 | 1268.23 ms | 1268.39 ms | 0.15 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c0dde50 | 5.53 MiB | 6.01 MiB | 488.14 KiB |
| 1f639ee | 5.53 MiB | 6.00 MiB | 479.95 KiB |
| 7cfee3b | 20.70 MiB | 22.46 MiB | 1.75 MiB |
| 192b44c | 5.53 MiB | 5.96 MiB | 444.33 KiB |
| 7b21e8b | 5.53 MiB | 6.00 MiB | 479.96 KiB |
| 6f47800 | 7.86 MiB | 9.44 MiB | 1.58 MiB |
| ad121c0 | 5.53 MiB | 6.01 MiB | 488.11 KiB |
| e04b24b | 5.53 MiB | 6.00 MiB | 480.00 KiB |
| 819c1e7 | 5.53 MiB | 6.00 MiB | 479.96 KiB |
| d0aa4b6 | 5.53 MiB | 6.02 MiB | 502.04 KiB |
Android Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 73a3c38 | 478.18 ms | 526.62 ms | 48.44 ms |
| 40c8f93 | 417.10 ms | 482.60 ms | 65.50 ms |
| 8541716 | 437.14 ms | 443.65 ms | 6.51 ms |
| d3fb366 | 391.49 ms | 385.85 ms | -5.64 ms |
| 2cf9161 | 454.12 ms | 512.67 ms | 58.55 ms |
| 1f639ee | 429.98 ms | 476.60 ms | 46.62 ms |
| b6c8720 | 457.41 ms | 519.04 ms | 61.63 ms |
| a69a51f | 437.18 ms | 450.60 ms | 13.42 ms |
| 6bcdc99 | 440.23 ms | 435.77 ms | -4.46 ms |
| 393f8ec | 360.07 ms | 362.70 ms | 2.64 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 73a3c38 | 6.54 MiB | 7.69 MiB | 1.15 MiB |
| 40c8f93 | 13.93 MiB | 15.00 MiB | 1.06 MiB |
| 8541716 | 13.93 MiB | 15.00 MiB | 1.06 MiB |
| d3fb366 | 13.93 MiB | 15.06 MiB | 1.13 MiB |
| 2cf9161 | 6.54 MiB | 7.70 MiB | 1.16 MiB |
| 1f639ee | 13.93 MiB | 15.00 MiB | 1.06 MiB |
| b6c8720 | 6.54 MiB | 7.69 MiB | 1.15 MiB |
| a69a51f | 13.93 MiB | 15.06 MiB | 1.13 MiB |
| 6bcdc99 | 13.93 MiB | 15.00 MiB | 1.06 MiB |
| 393f8ec | 13.93 MiB | 15.06 MiB | 1.13 MiB |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| run: (iScope) { | ||
| using((arena) { | ||
| final jKey = key.toJString()..releasedBy(arena); | ||
| final jVal = _dartToJObject(value)?..releasedBy(arena); | ||
|
|
||
| if (jVal == null) return; | ||
| final jVal = dartToJObject(value)..releasedBy(arena); | ||
|
|
||
| final scope = iScope.as(const native.$Scope$Type()) | ||
| ..releasedBy(arena); |
There was a problem hiding this comment.
Bug: setContexts converts null values to "null" string, sending incorrect context data to native.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
The refactoring of dartToJObject now converts null values to the string "null" via value.toString().toJString() instead of returning null. Concurrently, the explicit null check (if (jVal == null) return;) in setContexts was removed. This combination causes the native Sentry SDK to receive the string "null" for context values that were intended to be null or absent, leading to spurious context data in error events.
💡 Suggested Fix
Reintroduce an explicit null check in setContexts to prevent null values (or their string representation) from being sent to the native layer, or modify dartToJObject to return null for null inputs.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/flutter/lib/src/native/java/sentry_native_java.dart#L239-L245
Potential issue: The refactoring of `dartToJObject` now converts `null` values to the
string `"null"` via `value.toString().toJString()` instead of returning `null`.
Concurrently, the explicit null check (`if (jVal == null) return;`) in `setContexts` was
removed. This combination causes the native Sentry SDK to receive the string `"null"`
for context values that were intended to be `null` or absent, leading to spurious
context data in error events.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4082923
Fixes conversion of non-supported types in Dart conversion to FFI/JNI types.
Now if we try to convert an unsupported type we just fallback to its
.toString()representation💚 How did you test it?
Integration test. manual test
📝 Checklist
sendDefaultPiiis enabled🔮 Next steps