flutter#182361 Fix delegate copy on plugins init#182362
Conversation
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
github-actions
Bot
added
platform-ios
eMxPi
changed the title
![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 correctly fixes a potential crash in application:didFinishLaunchingWithOptions:isFallbackForScene: by iterating over a copy of the _delegates array. This prevents a "mutation during enumeration" error if a delegate modifies the delegate list. The added tests in FlutterPluginAppLifeCycleDelegateTest.mm effectively verify this fix.
However, the same unsafe iteration pattern for (... in _delegates) is used in many other methods within FlutterPluginAppLifeCycleDelegate.mm. These could also lead to crashes under similar circumstances. To make the fix complete and robust, all direct iterations over _delegates should be changed to iterate over [_delegates allObjects].
Some of the methods with this issue include:
applicationDidEnterBackground:isFallbackForScene:applicationWillEnterForeground:isFallbackForScene:applicationWillResignActive:isFallbackForScene:applicationDidBecomeActive:isFallbackForScene:handleWillTerminate:application:didRegisterUserNotificationSettings:- and several others.
I recommend applying this fix consistently across the entire file to prevent similar crashes in other lifecycle events.
|
FYI @vashworth We'll need to fix all of these delegate loops, not just the one that crashes in that particular case. |
| [self.container addDelegate:[[FakePlugin alloc] init]]; | ||
| } else { | ||
| // Case 2: Remove itself during the loop over _delegates | ||
| [self.container removeDelegate:self]; |
There was a problem hiding this comment.
IIUC the new convention is to put this kind of logic in didInitializeImplicitFlutterEngine? @vashworth
There was a problem hiding this comment.
Correct, but it's not guaranteed that people have migrated yet.
We pushed a new commit with all the delegate methods |
|
Tests are failing: |
|
@eMxPi This PR is trying to merge into the |
|
I believe you have a bad merge. There are lots of files in this pull request that I do not think you modified. |
reidbaker
left a comment
reidbaker
left a comment
There was a problem hiding this comment.
The changes in the content aware hash (and others) are unrelated to the pr described changes.
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
@vashworth this issue is creating a huge amount of crash for us (and I think for many people who are registering plugins). Do you think we can patch at least the 3.41.x ? Waiting for the next release would be problematic for us |
|
@eMxPi Please re-read the comment you are quoting. We do not make fixes directly to stable branches first, and we don't fix old branches while leaving newer branches unfixed. The only way this PR will proceed is if it targets After it lands in master, the cherry pick process can start. |
Yes I saw it, but since the crash is targeting the current release, it'd be faster and better for the community to have this patch instead of waiting for next release (we're currently facing 200k crash each day because of this issue). That's why I'm asking if we can target the 3.41 and i'll rebase accordingly |
Two different people have explained the process by which that can happen. If you are willing to follow that process, we can proceed with this PR, otherwise we can close it and someone else will need to fix the issue. |
github-actions
Bot
removed
e: impeller
Being new is fine, but when you are new to a large, well-established project with clearly developed and documented processes, repeatedly asking if you can ignore the process instead of following it is generally not going to be a productive approach.
Explaining why this process is industry-standard for large projects would be off-topic for this PR. |
|
Formatting is failing: |
|
Mac mac_unopt is still failing: |
Yes, I would like to hotfix this to 3.41 ASAP. Please address open comments and failing tests |
hi @vashworth I think I fixed the tests and all the comments, tell me if I'm missing anything! |
You didn't address either of my 2 comments. |
sorry @hellohuanlin I've just pushed a new commit with your suggestions. Thanks for you review |
|
autosubmit label was removed for flutter/flutter/182362, because This PR has not met approval requirements for merging. Changes were requested by {hellohuanlin, reidbaker}, please make the needed changes and resubmit this PR.
|
|
autosubmit label was removed for flutter/flutter/182362, because This PR has not met approval requirements for merging. Changes were requested by {hellohuanlin}, please make the needed changes and resubmit this PR.
|
@vashworth or @hellohuanlin could you add the label cp:stable to cherry pick on 3.41 please ? |
| * delegates during a lifecycle notification loop. | ||
| */ | ||
| @interface MutatingPlugin : NSObject <FlutterApplicationLifeCycleDelegate> | ||
| @property(nonatomic, weak) FlutterPluginAppLifeCycleDelegate* lifecycleDelegate; |
There was a problem hiding this comment.
Oh Interesting use case, where a plugin (MutatingPlugin) holds a reference to the lifecycleDelegate and registers other plugin (FakePlugin in your example) (rather than FakePlugin registering itself directly).
I am curious what is your actual use case in your app, if you don't mind? @eMxPi
There was a problem hiding this comment.
We're using several pubs, but the plugins we're registering "'manually" in the AppDelegate are Batch (batch_flutter) and background_locator_2
There was a problem hiding this comment.
Very interesting use case. thanks for the reference.
There was a problem hiding this comment.
Very interesting use case. thanks for the reference.
@hellohuanlin no problem. Tell me if you need any more details.
In the meantime, do you think we can apply this to the 3.38.X, we've made a lot of tests with 3.41.3 and are facing several issues linked to accessibility. I'll come back to you with more details
There was a problem hiding this comment.
We only hotfix previous stable releases in truly extraordinary circumstances; this would not meet that bar. If you need a fix for 3.38, you would either need to maintain your own fork of the engine, or find a plugin-level workaround (such as doing any additional registration asynchronously).
There was a problem hiding this comment.
@stuartmorgan-g yes I know, but since the 3.41 has several accessibility issues (i'm waiting for the full report of our tester to give you details about it) I think fixing the regression introduced by the 3.38 would be appreciated by the community.
As you can see in the fix i've produced, it's not in the way plugins are registered but I the modification of the code in 3.38 (no longer using a copy but a direct reference to a changing object). So restoring the right way the engine is (and should) work would be appreciated
There was a problem hiding this comment.
I think fixing the regression introduced by the 3.38 would be appreciated by the community.
There will always be some people who want specific fixes in arbitrary old versions of Flutter; that doesn't change our policy. If this had been reported earlier, it could have been fixed in 3.38.x via our normal process. Since it wasn't, that's no longer an option.
As you can see in the fix i've produced, it's not in the way plugins are registered but I the modification of the code in 3.38 (no longer using a copy but a direct reference to a changing object).
I understand both the bug and the fix. What I described is not a fix, it's a client-level workaround.
8 participants
Fix #182361
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.