Repository navigation
[native_assets] Record bundled code assets under their install name - #190819
Conversation
github-actions
Bot
added
tool
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the macOS and iOS native assets compilation to record bundled framework assets under their @rpath-relative install name in the native assets manifest, while maintaining their correct location within the bundle. It introduces a helper function frameworkInstallName to generate this install name and updates the corresponding asset target location logic and tests to verify this behavior. I have no feedback to provide as there are no review comments.
|
Could someone add the CICD label? Presubmits have not started on this PR, so there is no test signal for a reviewer to work from. The branch is still clean against master. |
dcharkes
left a comment
dcharkes
left a comment
There was a problem hiding this comment.
Thanks for the detailed explanation in #190799 🙏
Would it be possible to make the integration test that runs the app on MacOS/iOS add some C code so that it would crash without the fix. So that we also verify it end-to-end on CI.
|
Done in the I left the devicelab tasks alone. Replacing the framework under a running app needs a writable bundle, which rules out the on-device task, and there is no macOS native assets task there today. |
Perfect. Yes, running it on one OS is enough. 👍 |
This error makes no sense, this PR is not changing the yaml file. I'll ping infra. |
@wilyan09007 rebasing on master should fix this. |
On macOS and iOS a bundled code asset is stamped with the install name `@rpath/<name>.framework/<name>`, but NativeAssetsManifest.json recorded it as `<name>.framework/<name>`. The manifest entry is the string the Dart VM passes to `dlopen`, so the two never matched and dyld had to fall back on identifying the library by the file it came from. Rebuilding an app replaces that file underneath a debug instance that is still running, so the next lazily resolved `@Native` binding maps a second copy of the asset into the process. Two copies of a C library do not share its global state, and a handle produced by the first one is not valid in the second. Both strings now come from `frameworkInstallName`, so they cannot drift apart again. The framework is still bundled at the unprefixed path, and `parseNativeAssetsManifest` drops the prefix for the callers that use the manifest to locate the framework on disk. Fixes flutter#190799
Adds a pair of native functions over state the library keeps on the heap. One stores a value, the other reads it back through a binding of its own, so it resolves the asset again on first use rather than reusing whatever the first call resolved. The test stores state, replaces the bundled framework with a byte for byte copy of itself, which is what a rebuild does to a bundle a debug instance is still running, and then hot reloads a build that reads the state back. Mapped twice, the second copy has nothing stored in it and the app dies on the read.
b7abc73 to
882b846
Compare
|
@vashworth @jmagman Would the iOS team like to review this before merging? |
On macOS and iOS,
flutter_toolsstamps a bundled code asset's framework with the install name@rpath/<name>.framework/<name>, but records that asset inNativeAssetsManifest.jsonas<name>.framework/<name>. The manifest entry is the string the Dart VM hands todlopen, so the twonever match. dyld cannot recognize the library under a name it has no install name for, so it falls
back on identifying the file on disk, and that identity only holds until the file is replaced.
Rebuilding an app while a debug instance is still running replaces it, so the next lazily resolved
@Nativebinding maps a second copy of the asset into the process. Two copies of a C library do notshare its global state, and a handle produced by the first copy is not valid in the second. For
package:sqlite3that is a segfault on the first write after a rebuild, and the operation is lost.Both strings now come from one
frameworkInstallNamehelper, so the name stamped into the frameworkand the name recorded in the manifest cannot drift apart again. The framework is still bundled at the
unprefixed path, and
DarwinAddToAppNativeAssets.parseNativeAssetsManifestdrops the prefix for theadd-to-app and Swift Package callers that use the manifest to find the framework on disk.
The
flutter testpath is unchanged. The tester loads the dylib from where it was built and itsinstall name is already the absolute path that the manifest records, which is why
dart runandflutter testnever showed this.Fixes #190799
The reporter of that issue measured the dyld behaviour on macOS, including that a manifest entry of
@rpath/<name>.framework/<name>resolves throughdlopenand leaves a single mapping across arebuild. iOS goes through the same helper and had the same mismatch, so it is fixed here too. I do
not have a Mac, so beyond the unit tests added here I am relying on the existing macOS native assets
integration tests (
flutter runwith a bundled code asset, in all three build modes) on CI.Pre-launch Checklist
///).