Migrate json decoder in FlutterCodecs and the tests to swift#181921
Migrate json decoder in FlutterCodecs and the tests to swift#181921LongCatIsLooong wants to merge 21 commits into
Conversation
from crashing the program. This is also for strict concurrency migration (flutter#181684) because we won't be able to test isolation contracts in objective-c (although it is unlikely that the codecs stuff need extra isolation keywords since they should be thread safe but it might need to be marked as sendable).
| #expect(codec.decode(Data()) == nil) | ||
| } | ||
|
|
||
| @available(macOS 13.0, *) |
There was a problem hiding this comment.
This (and same for the test below this one) is unfortunate because https://developer.apple.com/documentation/swift/collection/contains(_:) is only available on macOS 13.0+.
There was a problem hiding this comment.
I'm using the @available (instead of re-implementing) under the assumption that we have macOS 13+ bots. Reimplementing contains would be annoying to maintain and the implementation has to be copied to the test below because they are both exit tests and exit tests can't capture anything from the context.
| // "(__DATA,__common)" or "(__DATA,__objc_data)". | ||
| // Ideally, iOS binaries (Flutter.framework/Flutter) should only export | ||
| // Objective-C Symbols from the Flutter namespace, of type "(__DATA,__common)" or | ||
| // "(__DATA,__objc_data)". However, to allow Swift symbols to be exported in |
There was a problem hiding this comment.
copied from the PR description of #167530
There was a problem hiding this comment.
Code Review
This pull request migrates the JSON decoding logic in FlutterCodecs from Objective-C to Swift. This change addresses a crash with malformed UTF-16 and simplifies the code by leveraging JSONSerialization with the .fragmentsAllowed option in Swift. The corresponding unit tests have also been migrated to Swift, and the Objective-C tests have been removed. Additionally, the verify_exported.dart script has been updated to correctly handle and allow new internal Swift symbols introduced by this change. I have found one critical issue in the symbol verification script that could lead to a crash, and I have provided a suggestion to fix it.
|
After looking at the internal golden changes, I don't think any of the google testing failures are relevant. |
|
Just wanna double check if this is ready for review? The checkboxes in PR description aren't filled up. Also it's unclear to me in the description whether this fixes #179727 - If it does, can you please explain what the issue is, and how your PR fixes the issue? It's puzzling to me (and future audience) how a language migration alone can fix a crash. You can also put "fixes #179727" so that it auto-closes the issue. |
Yeah it is ready for review. I don't think the google testing failures are related. But this doesn't completely fix #17927 imo, we also need to add asserts to the framework to make sure the text string isn't malformed. I've copied #179978 (comment) to the PR description. |
| ASSERT_TRUE([value isEqualTo:decoded]); | ||
| } | ||
| // Consider adding new tests to FlutterCodecsTests.swift instead. | ||
| // These legacy tests are kept until Swift 6.2 becomes available on CI. |
There was a problem hiding this comment.
why are some tests in this files moved to swift but not others?
There was a problem hiding this comment.
I added re-wrote these two tests in swift but found out exit tests are only supported in Swift 6.2 (so the swift versions currently do not run on CI yet).
There was a problem hiding this comment.
exit tests are only supported in Swift 6.2
Can you update the comment to include this info?
| if (isSimpleValue) { | ||
| // NSJSONSerialization does not support top-level simple values. | ||
| // We expand encoding to singleton array, then decode that and extract | ||
| // the single entry. |
There was a problem hiding this comment.
I assume this is done by fragmentsAllowed? why we didn't use this option before?
There was a problem hiding this comment.
Yeah it appears so and the tests are passing. I assume it was an oversight since the reason for not using .fragmentsAllowed wasn't documented.
| // These two tests currently do not run on CI. | ||
| #if compiler(>=6.2) | ||
| @available(macOS 13.0, *) | ||
| @Test |
There was a problem hiding this comment.
nit: remove line break to be consistent with your other tests
| data: Data([0xDF, 0xFF]), | ||
| encoding: NSUTF16StringEncoding | ||
| )! | ||
| FlutterJSONMessageCodec.sharedInstance().encode(malformedString) |
There was a problem hiding this comment.
does the crash happen on this line?
There was a problem hiding this comment.
Yeah,
let malformedString = NSString(
data: Data([0xDF, 0xFF]),
encoding: NSUTF16StringEncoding
)!
evaluates fine in a swift playground. The test does check stderr for error message and verifies if it matches the message we set in the implementation ("failed to convert to UTF8").
| } | ||
|
|
||
| // Exit tests are only available on Swift 6.2 and later. | ||
| // These two tests currently do not run on CI. |
There was a problem hiding this comment.
is there a timeline on supporting Swift 6.2 on CI? @vashworth
There was a problem hiding this comment.
I think @okorohelijah is working on this and if I recall it's one PR away?
| // converts `data` to a `String` using [NSString initWithBytes:length:encoding:], | ||
| // before passing it to the JSON parser. In Swift the initializer replaces | ||
| // unpaired surrogates in the String with `U+FFFD`, but this doesn't happen in | ||
| // Objective-C. |
There was a problem hiding this comment.
Can you link to the issue here? Also this behavioral difference sounds like a bug to me. May worth filing a radar?
There was a problem hiding this comment.
Updated the comment.
| // unpaired surrogates in the String with `U+FFFD`, but this doesn't happen in | ||
| // Objective-C. | ||
| @objc extension JSONSerialization { | ||
| @objc public class func decodeJSON(_ data: Data) throws -> Any { |
There was a problem hiding this comment.
nit: encode/decode typically refers to Codable API in modern swift (post-JSONSerialization-era). Try "serialization/deserialization" instead.
There was a problem hiding this comment.
changed the extension from JSONSerialization to FlutterJSONCodec
| @objc public class func decodeJSON(_ data: Data) throws -> Any { | ||
| return try JSONSerialization.jsonObject( | ||
| with: data, | ||
| options: [.fragmentsAllowed] |
There was a problem hiding this comment.
This is hidden logic that could very well shoot us in the foot in the future. Consider incorporating this info in your function name (e.g. func jsonObjectAllowingFragments).
There was a problem hiding this comment.
With this extension, we end up with 2 very similarly named APIs, one with [.fragmentsAllowed] option, and the other with [] option:
JSONSerialization.jsonObject(with: data)
JSONSerialization.decodeJSON(data)
So this can be quite confusing and open to misuse
There was a problem hiding this comment.
changed the extension from JSONSerialization to FlutterJSONCodec, so from the context it should be clear that this is specialized for FlutterJSONCodec.
| (entry.name.startsWith(r'_OBJC_METACLASS_$_Flutter') || | ||
| entry.name.startsWith(r'_OBJC_CLASS_$_Flutter')); | ||
| // Swift's name mangling uses s followed by symbol length followed by symbol. | ||
| final swiftInternalRegExp = RegExp(r'^_\$s\d+InternalFlutterSwift'); |
There was a problem hiding this comment.
I assume this regex doesn't support extension, and that's why you manually wrote all the matching logic? I wonder if you have tried writing another regex for extension case?
There was a problem hiding this comment.
Yeah regular expressions would work, but there can be false negatives for extensions, e.g., _$sSo19NSJSONSerializationC26InternalFlutterSwiftCommonE10decodeJSONyyp10Foundation4DataVKFZ we don't know the external class name and the class name may have digits in it.
00e6add to
71f55a5
Compare
|
The new commit should fail the test because the mangled name is now: |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
Code Review
This pull request refactors JSON decoding in FlutterJSONMessageCodec by introducing a Swift extension to safely handle unpaired surrogates. It migrates several codec tests from Objective-C to Swift and updates the verify_exported.dart script to allow internal Swift symbols by demangling them to check their module origin. Review feedback identifies incorrect Unicode escape sequences in the new Swift tests and suggests a performance optimization for the symbol verification script to avoid excessive process execution.
| let rawJSON = "\"\(malformedString)\"" | ||
| try #require(malformedString.character(at: 0) == 0xDFFF) | ||
| try #require( | ||
| !rawJSON.contains("u{FFFD}"), |
There was a problem hiding this comment.
The Unicode escape sequence in Swift requires a backslash (e.g., \u{FFFD}). As written, this checks for the literal string "u{FFFD}" instead of the Unicode replacement character. Since the sanitization process produces the character itself, this check is likely not performing the intended validation.
| !rawJSON.contains("u{FFFD}"), | |
| !rawJSON.contains("\u{FFFD}"), |
| "hello u{263A} world", | ||
| "hello u{1F602} world", |
There was a problem hiding this comment.
These string literals appear to be missing the backslash for Unicode escape sequences. Without the backslash, they are treated as literal text (e.g., "u{263A}") rather than the intended Unicode characters. This reduces the effectiveness of the test for non-ASCII character handling.
| "hello u{263A} world", | |
| "hello u{1F602} world", | |
| "hello \u{263A} world", | |
| "hello \u{1F602} world", |
| final ProcessResult demangledResult = Process.runSync('swift', <String>[ | ||
| 'demangle', | ||
| '--tree-only', | ||
| entry.name, | ||
| ]); |
There was a problem hiding this comment.
Calling Process.runSync for every Swift symbol in a loop can significantly degrade the performance of this script. Since the goal is to identify symbols from specific internal modules, a regular expression check on the mangled name (which includes the module name length and name, e.g., _\$s26InternalFlutterSwiftCommon) would be much more efficient and should be sufficiently accurate for this verification task.
|
@hellohuanlin could you take a look when you get a chance |
| ASSERT_TRUE([value isEqualTo:decoded]); | ||
| } | ||
| // Consider adding new tests to FlutterCodecsTests.swift instead. | ||
| // These legacy tests are kept until Swift 6.2 becomes available on CI. |
There was a problem hiding this comment.
exit tests are only supported in Swift 6.2
Can you update the comment to include this info?
| // unpaired surrogates in the String with `U+FFFD`, but this doesn't happen in | ||
| // Objective-C. | ||
| // | ||
| // Seealso: https://github.com/flutter/flutter/issues/179727. |
| // | ||
| // iOS binaries (Flutter.framework/Flutter) should only export Objective-C | ||
| // Symbols from the Flutter namespace. These are either of type | ||
| // "(__DATA,__common)" or "(__DATA,__objc_data)". |
There was a problem hiding this comment.
I believe @cbracken's recent change supported swift extensions so this can be reverted?
|
Moved the |
Use `swift demangle --tree-only` to get the module name of a swift symbol. Extracted from flutter#181921 The comments added at the top of the file were copied from the PR description of flutter#167530 ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Use `swift demangle --tree-only` to get the module name of a swift symbol. Extracted from flutter#181921 The comments added at the top of the file were copied from the PR description of flutter#167530 ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
textCrashes the App on iOS #179727JSONSerializationcan already handle that.This is mostly for strict concurrency migration (Migrate the Flutter engine to Swift 6 #181684) because we won't be able to test isolation contracts in objective-c (although it is unlikely that the codecs stuff would need extra isolation keywords since they should be thread safe but it might need to be marked as sendable).JSONSerialization.jsonObjectsanitizes the input string data in swift but not in objective-c because this initializer does string sanitization in swift but not in objective-c: developer.apple.com/documentation/foundation/nsstring/init(bytes:length:encoding:)This might have to do with the
NSStringandStringbridging. This may imply that the swift implementation can be slower because it has to sanitize the string before sending it to the JSON parser.Closes #179978
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.