[camera]use weak self to fix a crash due to orientation change#6277
[camera]use weak self to fix a crash due to orientation change#6277auto-submit[bot] merged 4 commits intoflutter:mainfrom
Conversation
hellohuanlin
force-pushed
the
camera_weak_self
branch
3 times, most recently
from
f830933 to
3925680
Compare
hellohuanlin
force-pushed
the
camera_weak_self
branch
from
3925680 to
c3d142d
Compare
| [self.camera setDeviceOrientation:orientation]; | ||
| [weakSelf.camera setDeviceOrientation:orientation]; | ||
| // `CameraPlugin::sendDeviceOrientation` can be called on any queue. | ||
| [self sendDeviceOrientation:orientation]; |
There was a problem hiding this comment.
This line is the root cause, but i also went through all other usage of "strong self"s in case of similar crashes in the future
More detailed discussion can be found here where I explained why orientation change is identified as the root cause.
jmagman
left a comment
jmagman
left a comment
There was a problem hiding this comment.
Perhaps this could also use:
- (void)detachFromEngineForRegistrar:(NSObject<FlutterPluginRegistrar> *)registrar {
[UIDevice.currentDevice endGeneratingDeviceOrientationNotifications];And additionally in -detachFromEngineForRegistrar something to clear out the camera inProgressSavePhotoDelegates to release their completion blocks? Unfortunately we can't also do queue cancellation as with NSOperationQueues (which I'll never fail to bring up whenever we discuss the gcd API 🙂 )
| __weak typeof(self) weakSelf = self; | ||
| FLTRequestCameraPermissionWithCompletionHandler(^(FlutterError *error) { | ||
| if (error) { | ||
| [result sendFlutterError:error]; |
There was a problem hiding this comment.
If self is nil this shouldn't be sending error messages or it will hit the same crash. It also shouldn't continue on to schedule FLTRequestAudioPermissionWithCompletionHandlers. I recommend the strongSelf/return early pattern for all of the non-trivial blocks.
hellohuanlin
force-pushed
the
camera_weak_self
branch
from
f90c370 to
4fdb87d
Compare
| [[FLTThreadSafeMethodChannel alloc] initWithMethodChannel:methodChannel]; | ||
| } | ||
|
|
||
| - (void)detachFromEngineForRegistrar:(NSObject<FlutterPluginRegistrar> *)registrar { |
There was a problem hiding this comment.
I did not clear out inProgressSavePhotoDelegates since the delegate does not hold strong reference to its owner anymore (deleted the outdated comment here)
jmagman
left a comment
jmagman
left a comment
There was a problem hiding this comment.
LGTM with nits and one question
| @@ -1,3 +1,7 @@ | |||
| ## 0.9.8+4 | |||
|
|
|||
| * Fixes a crash due to sending orientation change events when the engine is teared down. | |||
There was a problem hiding this comment.
| * Fixes a crash due to sending orientation change events when the engine is teared down. | |
| * Fixes a crash due to sending orientation change events when the engine is torn down. |
| FLTEnsureToRunOnMainQueue(^{ | ||
| [self.channel setStreamHandler:handler]; | ||
| [weakSelf.channel setStreamHandler:handler]; | ||
| completion(); |
There was a problem hiding this comment.
Should the completion be run if this is dead?
There was a problem hiding this comment.
Me being lazy. Should've used if (!strongSelf) return; here.
Looking at the current usage, running completion should be fine, but I think it's better not to run it.
hellohuanlin
added
the
autosubmit
2 participants
This is to fix a crash when reopening the app. More detailed discussion can be found here where I explained why
orientationChangeis identified as the root cause.Also went through all other usages of "strong self", but I don't think it's worth writing unit tests for them, since writing weak self is a pretty standard practice. But still, I want to write the test for
CameraPlugin::orientationChangedsince it is believed to be the cause.List which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#109143
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch 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.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.