[pigeon] Treat NSNull as null for non-null Flutter API returns - #12531
[pigeon] Treat NSNull as null for non-null Flutter API returns#12531Xelorium wants to merge 2 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request updates Pigeon to version 27.3.3. It modifies the Swift generator to check if a Flutter API response is nil or NSNull when validating non-nullable return values, preventing crashes when FlutterStandardReader substitutes NSNull for nil. It also updates the generated Swift files, adds a regression test in NullableReturnsTests.swift, and adds a generator unit test in swift_generator_test.dart. There are no review comments, so no further feedback is provided.
fbfef09 to
274fd02
Compare
`FlutterStandardReader` substitutes `NSNull` for a `nil` element of a list, so a null reply for a non-null return value arrives as `NSNull` rather than as `nil`. The generated `listResponse[0] == nil` check did not catch that, and the following force cast aborted the process. In `webview_flutter_wkwebview` this crashes apps whenever the native instance manager cannot resolve the returned instance, which the plugin itself causes by clearing the manager on scene disconnect and app termination while an authentication challenge is in flight.
274fd02 to
a3cd890
Compare
|
Thanks for the pointer on flutter/flutter#191254 — opened this as suggested there. Small status note in case it helps whoever picks it up: the branch is rebased onto the 28.0.0 async change, so it is mergeable again, and the CLA check is green. The test matrix hasn't run yet since the No rush — happy to make any changes you'd like when you get a chance to look. |
bparrishMines
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
I think this may be the true cause for flutter/flutter#162437. Thanks for debugging this.
@tarrinneal Where are you with the FFI/JNI integration? Can this land without disrupting #11352?
| @Test | ||
| func nullReplyForNonNullReturnFailsWithoutCrashing() async throws { | ||
| let binaryMessenger = MockBinaryMessenger<NSNull>(codec: codec) | ||
| binaryMessenger.result = NSNull() | ||
| let api = FlutterIntegrationCoreApi(binaryMessenger: binaryMessenger) | ||
|
|
||
| do { | ||
| // `sendMultipleNullableTypes` has a non-null return value. | ||
| _ = try await api.sendMultipleNullableTypes(aBool: nil, anInt: nil, aString: nil) | ||
| Issue.record("Expected a null-error but the call succeeded.") | ||
| } catch let error as PigeonError { | ||
| #expect(error.code == "null-error") | ||
| } | ||
| } |
There was a problem hiding this comment.
This should be moved in to the NullableReturnsTests or move this class to a separate file.
There was a problem hiding this comment.
Moving into the NullableReturnsTests class would be preferred.
I actually already added logic to fix this in the ffi pr. If I haven't already changed it to solve this problem, I can. |
|
This pr can land first, it doesn't really matter |
| // `FlutterStandardReader` substitutes `NSNull` for a `nil` element of | ||
| // a list, so a null reply can arrive as either. See | ||
| // https://github.com/flutter/flutter/issues/191254. |
| // `FlutterStandardReader` substitutes `NSNull` for a `nil` element of a | ||
| // list, so both need to be treated as a null reply before the value is | ||
| // cast. See https://github.com/flutter/flutter/issues/191254. |
There was a problem hiding this comment.
this isn't needed either
|
Make sure to run format and analyze |
| /// Regression test for https://github.com/flutter/flutter/issues/191254. | ||
| /// | ||
| /// `FlutterStandardReader` substitutes `NSNull` for a `nil` element of a list, | ||
| /// so a null reply for a non-null return value arrives as `NSNull` rather than | ||
| /// as `nil`. Before the fix that value was force-cast to the return type, which | ||
| /// aborted the process instead of reporting an error. | ||
| @MainActor | ||
| struct NullReplyForNonNullReturnTests { | ||
| let codec = FlutterStandardMessageCodec.sharedInstance() | ||
|
|
||
| @Test | ||
| func nullReplyForNonNullReturnFailsWithoutCrashing() async throws { |
There was a problem hiding this comment.
the comments aren't needed if the test name is something like nonNullReturnFailsOnNSNullResponse.
…nto NullableReturnsTests
|
Thanks for the reviews! Addressed all the comments: dropped the explanatory comments in the generator and the generator test, and folded the regression test into |
Description
FlutterStandardReadersubstitutesNSNullfor anilelement when it decodes a list, so a null reply for a non-null return value reaches the generated Swift code asNSNullrather than asnil. The generated guard only checkslistResponse[0] == nil, soNSNullfalls through to the force cast below it and aborts the process:This crashes production apps through
webview_flutter_wkwebview: the plugin clears its native instance manager inWebViewFlutterPlugin.tearDownProxyAPIRegistrar(), which iOS triggers onsceneDidDisconnect/applicationWillTerminate. AWKNavigationDelegate.didReceiveAuthenticationChallengereply that is still in flight then carries an identifier the native instance manager can no longer resolve, the codec reader returnsnil, and the app dies with:The same shape was reported before for other types (
URLRequestWrapperin flutter/flutter#162437), which is expected: the generator emits this guard for every non-null return value, so any unresolvable instance crashes instead of reporting an error.This PR treats
NSNullas a null reply, so the existingnull-errorpath handles it.Verification
NSNull.platform_teststhat repliesNSNullto a non-null Flutter API return. It passes with this change; with the generated guard reverted it aborts withCould not cast value of type 'NSNull' (0x...) to 'test_plugin.AllNullableTypes' (0x...).dart testandflutter_plugin_tools formatpass locally.Fixes flutter/flutter#191254
Pre-Review Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).