Skip to content

Commit

Permalink
[video_player] Fix iOS crash with multiple players (flutter#4202)
Browse files Browse the repository at this point in the history
This PR fixes crash in `video_player` package on iOS. 

flutter#124937

### Detailed description

I can observe the crash when displaying a couple of the different players (different URLs) inside a `ListView`. The crash happens inside of `AVFoundation` framework:

```objc
[AVPlayer _createAndConfigureFigPlayerWithType:completionHandler:]
```

In order to debug the issue, I ran the application using the plugin with `Zombie Objects` inspection turned on. The `Zombie Objects` reports the following issue:

```
*** -[FLTVideoPlayer retainWeakReference]: message sent to deallocated instance 0x6030009b2e10
```

This, in conjunction with the `NSKeyValueWillChange` line present in the stack trace led me to believe, that culprit is sending a KVO notification to the `FLTVideoPlayer` instance that's deallocated.

Next, I examined the plugin code and identified one property that doesn't have the KVO removed. In `addObserversForItem:player:` method in `FLTVideoPlayerPlugin.m`:

```
  [player addObserver:self
           forKeyPath:@"rate"
              options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew
              context:rateContext];
```

The observer for `@"rate"` is never cleaned up. To be entirely sure that the issue comes from KVO that's not cleaned up, I've added the following class:

```
@implementation EmptyObserver

- (void)observeValueForKeyPath:(NSString *)path
                      ofObject:(id)object
                        change:(NSDictionary *)change
                       context:(void *)context {}

@EnD
```

and registered the observation as follows (notice that `EmptyObserver` is never retained and deallocated instantly):

```
  [player addObserver:[[EmptyObserver alloc] init]
           forKeyPath:@"rate"
              options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew
              context:rateContext];
```

The exception I got seems to be matching the underlying issue:

```
*** -[EmptyObserver retainWeakReference]: message sent to deallocated instance 0x6020001ceb70
```

This means the fix for the issue is to add the following to `disposeSansEventChannel` method:

```
[self.player removeObserver:self forKeyPath:@"rate"];
```

After applying the patch, I can no longer crash the player.
  • Loading branch information
turekj committed Jul 18, 2023
1 parent d4d761b commit 3e8b813
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 1 deletion.
5 changes: 5 additions & 0 deletions packages/video_player/video_player_avfoundation/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 2.4.9

* Fixes the iOS crash when using multiple players on the same screen.
See: https://github.com/flutter/flutter/issues/124937

## 2.4.8

* Fixes missing `isPlaybackLikelyToKeepUp` check for iOS video player `bufferingEnd` event and `bufferingStart` event.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ @interface FLTVideoPlayer : NSObject <FlutterStreamHandler>
@property(readonly, nonatomic) AVPlayer *player;
@property(readonly, nonatomic) AVPlayerLayer *playerLayer;
@property(readonly, nonatomic) int64_t position;

- (void)onTextureUnregistered:(NSObject<FlutterTexture> *)texture;
@end

@interface FLTVideoPlayerPlugin (Test) <FLTAVFoundationVideoPlayerApi>
Expand Down Expand Up @@ -442,6 +444,110 @@ - (void)testSeekToleranceWhenSeekingToEnd {
return initializationEvent;
}

// Checks whether [AVPlayer rate] KVO observations are correctly detached.
// - https://github.com/flutter/flutter/issues/124937
//
// Failing to de-register results in a crash in [AVPlayer willChangeValueForKey:].
- (void)testDoesNotCrashOnRateObservationAfterDisposal {
NSObject<FlutterPluginRegistry> *registry =
(NSObject<FlutterPluginRegistry> *)[[UIApplication sharedApplication] delegate];
NSObject<FlutterPluginRegistrar> *registrar =
[registry registrarForPlugin:@"testDoesNotCrashOnRateObservationAfterDisposal"];

AVPlayer *avPlayer = nil;
__weak FLTVideoPlayer *player = nil;

// Autoreleasepool is needed to simulate conditions of FLTVideoPlayer deallocation.
@autoreleasepool {
FLTVideoPlayerPlugin *videoPlayerPlugin =
(FLTVideoPlayerPlugin *)[[FLTVideoPlayerPlugin alloc] initWithRegistrar:registrar];

FlutterError *error;
[videoPlayerPlugin initialize:&error];
XCTAssertNil(error);

FLTCreateMessage *create = [FLTCreateMessage
makeWithAsset:nil
uri:@"https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4"
packageName:nil
formatHint:nil
httpHeaders:@{}];
FLTTextureMessage *textureMessage = [videoPlayerPlugin create:create error:&error];
XCTAssertNil(error);
XCTAssertNotNil(textureMessage);

player = videoPlayerPlugin.playersByTextureId[textureMessage.textureId];
XCTAssertNotNil(player);
avPlayer = player.player;

[videoPlayerPlugin dispose:textureMessage error:&error];
XCTAssertNil(error);
}

// [FLTVideoPlayerPlugin dispose:error:] selector is dispatching the [FLTVideoPlayer dispose] call
// with a 1-second delay keeping a strong reference to the player. The polling ensures the player
// was truly deallocated.
[self expectationForPredicate:[NSPredicate predicateWithFormat:@"self != nil"]
evaluatedWithObject:player
handler:nil];
[self waitForExpectationsWithTimeout:10.0 handler:nil];

[avPlayer willChangeValueForKey:@"rate"]; // No assertions needed. Lack of crash is a success.
}

// During the hot reload:
// 1. `[FLTVideoPlayer onTextureUnregistered:]` gets called.
// 2. `[FLTVideoPlayerPlugin initialize:]` gets called.
//
// Both of these methods dispatch [FLTVideoPlayer dispose] on the main thread
// leading to a possible crash when de-registering observers twice.
- (void)testHotReloadDoesNotCrash {
NSObject<FlutterPluginRegistry> *registry =
(NSObject<FlutterPluginRegistry> *)[[UIApplication sharedApplication] delegate];
NSObject<FlutterPluginRegistrar> *registrar =
[registry registrarForPlugin:@"testHotReloadDoesNotCrash"];

__weak FLTVideoPlayer *player = nil;

// Autoreleasepool is needed to simulate conditions of FLTVideoPlayer deallocation.
@autoreleasepool {
FLTVideoPlayerPlugin *videoPlayerPlugin =
(FLTVideoPlayerPlugin *)[[FLTVideoPlayerPlugin alloc] initWithRegistrar:registrar];

FlutterError *error;
[videoPlayerPlugin initialize:&error];
XCTAssertNil(error);

FLTCreateMessage *create = [FLTCreateMessage
makeWithAsset:nil
uri:@"https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4"
packageName:nil
formatHint:nil
httpHeaders:@{}];
FLTTextureMessage *textureMessage = [videoPlayerPlugin create:create error:&error];
XCTAssertNil(error);
XCTAssertNotNil(textureMessage);

player = videoPlayerPlugin.playersByTextureId[textureMessage.textureId];
XCTAssertNotNil(player);

[player onTextureUnregistered:nil];
XCTAssertNil(error);

[videoPlayerPlugin initialize:&error];
XCTAssertNil(error);
}

// [FLTVideoPlayerPlugin dispose:error:] selector is dispatching the [FLTVideoPlayer dispose] call
// with a 1-second delay keeping a strong reference to the player. The polling ensures the player
// was truly deallocated.
[self expectationForPredicate:[NSPredicate predicateWithFormat:@"self != nil"]
evaluatedWithObject:player
handler:nil];
[self waitForExpectationsWithTimeout:10.0
handler:nil]; // No assertions needed. Lack of crash is a success.
}

- (void)validateTransformFixForOrientation:(UIImageOrientation)orientation {
AVAssetTrack *track = [[FakeAVAssetTrack alloc] initWithOrientation:orientation];
CGAffineTransform t = FLTGetStandardizedTransformForTrack(track);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,14 @@ - (FlutterError *_Nullable)onListenWithArguments:(id _Nullable)arguments
/// is useful for the case where the Engine is in the process of deconstruction
/// so the channel is going to die or is already dead.
- (void)disposeSansEventChannel {
// This check prevents the crash caused by removing the KVO observers twice.
// When performing a Hot Restart, the leftover players are disposed once directly
// by [FLTVideoPlayerPlugin initialize:] method and then disposed again by
// [FLTVideoPlayer onTextureUnregistered:] call leading to possible over-release.
if (_disposed) {
return;
}

_disposed = YES;
[_playerLayer removeFromSuperlayer];
[_displayLink invalidate];
Expand All @@ -514,6 +522,7 @@ - (void)disposeSansEventChannel {
[currentItem removeObserver:self forKeyPath:@"presentationSize"];
[currentItem removeObserver:self forKeyPath:@"duration"];
[currentItem removeObserver:self forKeyPath:@"playbackLikelyToKeepUp"];
[self.player removeObserver:self forKeyPath:@"rate"];

[self.player replaceCurrentItemWithPlayerItem:nil];
[[NSNotificationCenter defaultCenter] removeObserver:self];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: video_player_avfoundation
description: iOS implementation of the video_player plugin.
repository: https://github.com/flutter/packages/tree/main/packages/video_player/video_player_avfoundation
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22
version: 2.4.8
version: 2.4.9

environment:
sdk: ">=2.18.0 <4.0.0"
Expand Down

0 comments on commit 3e8b813

Please sign in to comment.