Repository navigation
fix(ios): stop the attestation plugin pinning the Flutter engine - #9395
Conversation
This comment has been minimized.
This comment has been minimized.
dcadenas
left a comment
dcadenas
left a comment
There was a problem hiding this comment.
The weak reference is the right fix, and the engine-source claims behind it hold.
I read those claims at the Flutter version this repo pins rather than taking them from the description:
FlutterImplicitEngineBridgeImpl.pluginRegistryreturns the engine, so the registry the app hands this plugin really is theFlutterEngine.-[FlutterEngine dealloc]is the only non-test place Flutter dispatchesdetachFromEngineForRegistrar:, so a strong reference from a process-lifetime static does disable that hook for every plugin in the app.- The implicit engine's
allowHeadlessExecutionis read-only, read once, and assigned nowhere, so it is NO.
I also went looking for a second strong path to the engine, because the fix only pays off if the engine can now reach dealloc.
AppDelegate never stores the bridge.
The messenger and texture-registry relays hold their parent weakly, so every registrar.messenger() holder in the app is engine-free.
FlutterEngineBaseRegistrar.flutterEngine is already a weak property.
No other Swift file under Runner names the engine, a registry, or a registrar.
The plugin's static was the only strong path, so dropping it is sufficient for the stated goal.
Both lookup sites already treated a missing WKWebView as recoverable, with policy rollback and WEBVIEW_NOT_FOUND on attach and a deliberate silent skip on detach.
A nil registry therefore lands on a path that was already correct rather than a new one.
The new test discriminates: it holds a weak reference across an autoreleasepool and asserts a positive control inside the scope, so it fails if the registry is retained instead of passing vacuously.
What I did not verify
- I did not build or run the iOS target.
xcodebuild test -only-testing:RunnerTestsis the only thing that exercisesNostrBridgeAttestationEnginePinTests, and no GitHub workflow runs it, so the green checks here say nothing about that test. It rests on the simulator run reported in the description. dart formatandflutter analyzepass on the repo-pinned SDK, but the entire diff sits undermobile/ios, which neither command reaches. The declared check set has no runnable local command for that directory.- I did not confirm that the
sendOnChannel:shell assert survives in the shipped release engine binary. That claim is in the description's causal narrative, not in the changed code.
Accepted
The destroyContext to dealloc window remains open, and this change narrows a process-lifetime pin down to that window rather than introducing it.
The teardown-trigger design question stays open on #9384.
Reviewed at 0452cda.
The other review request on this pull request is still outstanding, and the merge decision stays with you.
0452cda to
1231dda
Compare
This comment has been minimized.
This comment has been minimized.
|
@dcadenas — following up on the three things your review listed as not verified. 1. 2. 3. The release-binary assert. You were right to leave this unconfirmed, because the description overclaimed it. I checked stock Flutter 3.47.2's |
dcadenas
left a comment
dcadenas
left a comment
There was a problem hiding this comment.
The rebase changed nothing in the diff, and two of the three gaps my earlier review left open are now closed by CI.
That earlier approval was formed against the pre-rebase commit. This one is pinned to 1231dda.
The rewrite is a pure rebase, checked on the two commits directly: git patch-id --stable is identical, the commit message is byte-identical, and both touched files carry the same blob hashes.
Only the parent moved.
The base advance touches nothing under mobile/ios and changes no Podfile, pubspec, or SDK pin, so the surroundings of this diff are unchanged and the engine-source reasoning I checked before still holds at this head.
Now covered
RunnerTestsruns in CI. Mobile CI run 35662447640 is on this head, and itsiOS Native Testsjob log reports** TEST SUCCEEDED **with 33 passed and 0 failed, includingNostrBridgeAttestationEnginePinTests.testSetupDoesNotRetainThePluginRegistry().- That job builds the Runner target, so the Swift in this diff is now compile-checked upstream rather than only on your simulator.
Still not covered
dart formatandflutter analyzepass on the repo-pinned Flutter 3.47.2, but neither command reachesmobile/ios, where the entire diff sits. The declared check set has no local command for that directory, so CI is the only automated coverage of the changed code.- I did not build or run the iOS target myself.
Accepted
The destroyContext to dealloc window stays open. This change narrows a process-lifetime pin down to that window rather than introducing it.
The teardown-trigger design question stays open on #9384.
The description now names the artifact the assert string was actually checked in and says plainly that the App Store build uses a different engine, with the shipped-binary claim resting on the production crash reports in #4400. That is the correction the earlier review asked for.
Reviewed at 1231dda.
The other review request on this pull request is still outstanding, and the merge decision stays with you.
|
@mbradley — friendly nudge on this one when you have a moment. CI is fully green (including the new |
`NostrBridgeAttestationPlugin` held its `FlutterPluginRegistry` in a `private let`, and on iOS that registry *is* the `FlutterEngine` — `FlutterImplicitEngineBridgeImpl.pluginRegistry` returns `_engine`. The plugin itself lives in a process-lifetime `shared` static, so that one reference pinned the engine for the life of the process. `-[FlutterEngine dealloc]` is the only non-test place Flutter dispatches `detachFromEngineForRegistrar:`, so the pin silently disabled `detachFromEngine(for:)` for every plugin in the app — including `BackgroundUploaderPlugin`, whose hook #9381 had just made reachable. The consequence is worse than a stale-channel warning. The messenger relay's `parent` is weak and nils only at dealloc, so a pinned engine keeps forwarding, while the view controller's dealloc has already run `destroyContext` and dropped the shell (the implicit engine's `allowHeadlessExecution` is NO). The send then reaches `sendOnChannel:`, whose shell assert is present in the release engine binary, raising `NSInternalInconsistencyException: Sending a message before the FlutterEngine has been run` — the signature #4400 tracked. Weak lets the engine dealloc, so the hook fires and the residual destroyContext-to-dealloc window degrades to the benign dead-channel log instead of a crash. Both call sites already treat a missing WKWebView as recoverable, so they take that path when the registry is gone. Closes #9393
1231dda to
5ef7009
Compare
This comment has been minimized.
This comment has been minimized.
|
Rebased onto Cause. Resolution, verified mechanically rather than by eye:
So Verification before push: ran @dcadenas — your approval is pinned to |
Mobile PR PreviewPreview refreshed for Last refresh:
|
dcadenas
left a comment
dcadenas
left a comment
There was a problem hiding this comment.
Your rebase is clean, and with #9384 now on main the fix does what it set out to do.
I pushed one comment-only commit on top. Please take a look at 3131ec2.
Changes I pushed
3131ec2removes a stale example from two comments that #9384 added. The video player's engine-detach doc comment and the doc comment on its view-controller dealloc test both named the attestation plugin as what keeps the engine alive. Once the registry reference is weak, that is no longer true.- The general point stays in both comments: anything in the app that strongly holds the engine delays its dealloc.
The commit touches the divine_video_player package, which is outside the files the description covers. It changes comments only, no code.
Rebase
- Only the base moved. The commit message and the plugin change are byte-identical to
1231dda. - The test file's added lines are identical and nothing was removed. Its first 862 lines are exactly
main's file, with the new test class appended afterDivineVideoPlayerEngineTeardownTests.
Together with #9384
#9384 has merged, so the description's note that the teardown question is "currently under review" there is out of date.
Because this change stops the engine being pinned, the engine-detach hook #9384 added can now actually run.
That hook tears down the players without touching the engine, and #9384's own tests cover it.
Both test classes ran in the same CI job below.
Checks on 3131ec2
- Mobile CI
iOS Native Tests(job):** TEST SUCCEEDED **, 45 passed, 0 failed. That includesNostrBridgeAttestationEnginePinTests.testSetupDoesNotRetainThePluginRegistry()and all 12DivineVideoPlayerEngineTeardownTestscases. - Divine Video Player CI passed, including Apple Diagnostics and the package build.
- Every other check passed. The daily service-test publish was skipped.
divine_video_player'sapple_engine_teardown_contract_test.dartpassed 10 of 10 locally on Flutter 3.47.2. It reads the Swift source, so it confirms the comment edit left the checked functions intact.
Not verified
- I did not build or run the iOS target myself. CI is the only build of the Swift here.
- No on-device reproduction.
- The window between the shell being destroyed and the engine deallocating is unchanged and still out of scope, as the description says. Plugins that tear down only at engine dealloc still wait for it.
@realmeylisdev please review the commit I pushed. The merge decision stays with you.
@mbradley your review request is still pending, and the head has moved since it was made.
|
Reviewed Confirmed: both hunks sit entirely inside CI is green on Status: still waiting on @mbradley's review, which is still open from the original request. |
2 participants
Description
NostrBridgeAttestationPluginheld itsFlutterPluginRegistryin aprivate let, and on iOS that registry is theFlutterEngine— this app reaches it throughFlutterImplicitEngineBridgeImpl.pluginRegistry, which returns_engine. Because the plugin instance lives in a process-lifetimesharedstatic, that one reference pinned the engine for the life of the process.-[FlutterEngine dealloc]is the only non-test place Flutter dispatchesdetachFromEngineForRegistrar:, so the pin silently disableddetachFromEngine(for:)for every plugin in the app — includingBackgroundUploaderPlugin, whose hook #9381 had just made reachable by publishing the instance.The symptom is a crash, not a stale-channel warning
#9393 describes the surviving symptom as a "Communicating on a dead channel" warning per progress tick. That warning cannot occur here. It comes from
FlutterBinaryMessengerRelay, and only once itsparent— which isweak— has gone nil, which is exactly what the pin prevents. So the relay keeps forwarding into an engine whose shell is already gone:allowHeadlessExecution: self.engineAllowHeadlessExecution, areadonlyproperty that is declared in no internal header and assigned nowhere, so its ivar zero-initializes toNO.notifyViewControllerDeallocated→destroyContext→_shell.reset().-[FlutterEngine sendOnChannel:message:binaryReply:]asserts_shell && _shell->IsSetup(), raisingNSInternalInconsistencyException: Sending a message before the FlutterEngine has been run. The assert is live in shipped release builds, not just debug: Crashlytics recorded that exact exception from release 1.0.13 (fix(crashlytics): stop iOS native player callbacks after engine teardown #4400), and the assert string is present in stock Flutter 3.47.2'sios-releaseengine artifact.The fix
Make the reference weak. The engine can then dealloc, the teardown hook fires, and the residual window degrades to the benign dead-channel log instead of a crash. Both call sites already treat a missing
WKWebViewas a recoverable miss, so a torn-down engine takes the same path as a stale identifier; the lookup moves into oneresolveWebViewhelper rather than duplicating the guard.Checked that nothing else re-pins the engine:
engineBridgeis only ever a parameter inAppDelegate, never stored in a property, and no channel-handler closure captures it.Related Issue: Closes #9393
Out of Scope
destroyContext→dealloc window. The plugin's own doc comment already names it. Closing it means choosing a teardown trigger —FlutterViewControllerWillDeallocis exported in the engine binary but declared in no public header — and that is the same open design question currently under review on fix(video-player): stop iOS delivering video frames into a torn-down Flutter engine #9384 fordivine_video_player. Shipping a design here would pre-empt that decision; fixing the pin helps both sides regardless of how fix(video-player): stop iOS delivering video frames into a torn-down Flutter engine #9384 resolves.FlutterPluginprotocol declares no detach hook at all, so there is no teardown path there to fix.check_ios_extension_signing_contract.shfails identically on cleanorigin/main. It is pre-existing, absent from the CI guards manifest, and outside this diff.Verification
iOS Native Testsjob (ci(mobile): run the native iOS XCTests on pull requests that touch iOS code #9388) ranRunnerTestson the rebased head: 33 passed, 0 failed, includingNostrBridgeAttestationEnginePinTests(job). The same 33/33 held locally on an iPhone 18 Pro simulator before the rebase, which moved only the base (git patch-id --stableis identical before and after).NostrBridgeAttestationEnginePinTestsdiscriminates — reverting just theweakkeyword to a strong reference turns it red, restoring it turns it green.check_native_transport_security.sh,check_ios_shipping_versions.sh,check_privacy_manifest_coverage.sh.ios-releaseartifact; the App Store build uses Shorebird's engine, whose iOS artifacts were not available locally, so the shipped-binary claim rests on fix(crashlytics): stop iOS native player callbacks after engine teardown #4400's production crash reports rather than on that check.Type of Change