[video_player] Fix iOS crash with multiple players #4202

turekj · 2023-06-12T19:04:21Z

This PR fixes crash in video_player package on iOS.

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:

[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.

Pre-launch Checklist

flutter-dashboard · 2023-06-12T19:04:25Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

hellohuanlin

can you explain in the PR description why the crash happens and how this fixes the crash?

hellohuanlin · 2023-06-14T17:31:27Z

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m


  [self.player replaceCurrentItemWithPlayerItem:nil];
+  _player = nil;


why is this required?

After applying the original fix (removing the observer), I started observing crashes when doing Hot Reload with Flutter debugger attached on the screen displaying the player. The code crashes at:

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

The immediate reason for the crash is that the player is no longer KVO subscribed to the property rate, therefore removeObserver call crashes. I've added additional logging to pinpoint why that happens. It turns out that in the flow described, the disposeSansEventChannel is called twice in a row for the same player.

The issue is not present in the original code, because of this line:

[self.player replaceCurrentItemWithPlayerItem:nil]

Upon a first call to the disposeSansEventChannel method, the self.player.currentItem is unassigned. On the second call, the currentItem is nil and therefore any of the removeObserver calls become no-op.

I applied the same solution to the player.

when it's called twice, should we short circuit up front?

- (void)dispose... { if (!_player) { return; } ... }

I'll short circuit it with:

- (void)disposeSansEventChannel { if (_disposed) { return; } _disposed = YES; // ... }

as _disposed is already set there.

makes sense. Can you also add some comments here?

@hellohuanlin I've updated the code according to our discussion. I've added a comment on the short circuit method.

I don't think removing the observer requires additional comments as the API specifies to clear any observations you've subscribed to in the first place. LMK if you'd add anything else, please 🙏

turekj · 2023-06-14T18:59:56Z

@hellohuanlin I've added the detailed description in the PR (as requested).

I'll update the code as requested and provide some additional comments.

hellohuanlin · 2023-06-14T19:47:46Z

I'll update the code as requested and provide some additional comments.

Oh i meant comment in the code. It would be helpful for future readers.

hellohuanlin · 2023-06-16T17:16:02Z

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

@@ -519,6 +519,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 directly
+  // and also receive onTextureUnregistered: callback leading to possible


nit: just wanna be clearer

"the leftover players are disposed twice - once directly (from which function?), and once when receiving onTextureUnregistered callback"

@hellohuanlin I've updated the comment yet again to name the calling methods!

stuartmorgan-g · 2023-06-26T19:11:22Z

I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].

I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
...

I added new tests to check the change I am making, or this PR is [test-exempt].

@turekj Could you update the PR to add these missing components? All of these elements are required before the PR can be fully reviewed and landed.

stuartmorgan-g · 2023-07-13T19:03:24Z

From PR triage: @turekj Are you still planning on updating this per the feedback above?

turekj · 2023-07-14T10:53:32Z

@hellohuanlin I've updated the comments so that they're clearer ✅

@stuartmorgan I've added a changelog entry, bumped the package number, and also implemented the test cases for both of the changes I made.

hellohuanlin

Some feedbacks mainly on unit tests

hellohuanlin · 2023-07-14T17:34:21Z

packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m

+  }
+
+  // [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


what does this 1 sec delay come from?

The [FLTVideoPlayerPlugin dispose:error:] has this piece of code:

dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ if (!player.disposed) { [player dispose]; } });

hellohuanlin · 2023-07-14T17:37:49Z

packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m

+  } while ([[NSDate date] timeIntervalSinceNow] < [end timeIntervalSinceNow]);
+
+  if (condition()) {
+    return;


is this check necessary?

Not valid anymore

hellohuanlin · 2023-07-14T17:43:18Z

packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m

@@ -439,4 +546,22 @@ - (void)validateTransformFixForOrientation:(UIImageOrientation)orientation {
  XCTAssertEqual(t.ty, expectY);
 }

+- (void)waitForCondition:(BOOL (^)(void))condition withTimeout:(NSTimeInterval)timeout {


i think you can just use NSPredicate and expectation for this kind of stuff

Great idea, I've refactored the code ✅

hellohuanlin · 2023-07-14T17:46:00Z

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

@@ -519,6 +519,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 directly
+  // by [FLTVideoPlayerPlugin initialize:] method and also receive


nit: "the leftover players are disposed once directly by ... method and then disposed again when receiving ..."

Updated the wording ✅

turekj · 2023-07-14T20:06:48Z

@hellohuanlin implemented all rounds of feedback ✅

stuartmorgan-g · 2023-07-17T23:11:37Z

@turekj Could you resolve the conflicts that have shown up? Once that's done this can be landed.

turekj · 2023-07-18T09:18:00Z

@stuartmorgan conflicts resolved ✅

stuartmorgan-g · 2023-07-18T12:49:52Z

Thanks!

…ackages#4202)

flutter/packages@6889cca...3e8b813 2023-07-18 [email protected] [video_player] Fix iOS crash with multiple players (flutter/packages#4202) 2023-07-17 [email protected] [pigeon] Enable Android emulator tests in CI (flutter/packages#4484) 2023-07-17 [email protected] [video_player] Add optional web options [Platform interface] (flutter/packages#4433) 2023-07-17 [email protected] [google_maps_flutter_platform_interface] Platform interface changes for #3258 (flutter/packages#4478) 2023-07-17 [email protected] [video_player] fix: add missing isPlaybackLikelyToKeepUp check. (flutter/packages#3826) 2023-07-17 [email protected] [camerax] Add flash configuration for image capture (flutter/packages#3800) 2023-07-17 [email protected] Remove `equatable` and `xml` allowances (flutter/packages#4489) 2023-07-17 [email protected] [ci] Switch Linux platform tests to LUCI (flutter/packages#4479) 2023-07-17 [email protected] [ci] Adjust bot configurations (flutter/packages#4485) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

flutter/packages@6889cca...3e8b813 2023-07-18 [email protected] [video_player] Fix iOS crash with multiple players (flutter/packages#4202) 2023-07-17 [email protected] [pigeon] Enable Android emulator tests in CI (flutter/packages#4484) 2023-07-17 [email protected] [video_player] Add optional web options [Platform interface] (flutter/packages#4433) 2023-07-17 [email protected] [google_maps_flutter_platform_interface] Platform interface changes for flutter#3258 (flutter/packages#4478) 2023-07-17 [email protected] [video_player] fix: add missing isPlaybackLikelyToKeepUp check. (flutter/packages#3826) 2023-07-17 [email protected] [camerax] Add flash configuration for image capture (flutter/packages#3800) 2023-07-17 [email protected] Remove `equatable` and `xml` allowances (flutter/packages#4489) 2023-07-17 [email protected] [ci] Switch Linux platform tests to LUCI (flutter/packages#4479) 2023-07-17 [email protected] [ci] Adjust bot configurations (flutter/packages#4485) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

turekj requested a review from hellohuanlin as a code owner June 12, 2023 19:04

flutter-dashboard bot added the needs tests label Jun 12, 2023

github-actions bot added p: video_player platform-ios labels Jun 12, 2023

hellohuanlin requested changes Jun 14, 2023

View reviewed changes

turekj requested a review from hellohuanlin June 16, 2023 14:44

hellohuanlin reviewed Jun 16, 2023

View reviewed changes

turekj added 4 commits July 14, 2023 12:42

Fix iOS video_player package crash

4819b23

Prevent a crash on hot restart due to player being disposed twice

3a84182

Add short-circuit for dispose method, do not clean up the _player

cf05e81

Update the commend according to PR

2ba7752

turekj force-pushed the fix-ios-video-player-crash branch from 345f3d1 to 2ba7752 Compare July 14, 2023 10:43

Bump version, update changelog

4b934f0

turekj added 2 commits July 14, 2023 18:19

Add tests for FLTVideoPlayer deallocation scenario

cb08b43

Format the code

84b0e24

hellohuanlin requested changes Jul 14, 2023

View reviewed changes

Update code according to CR

2e25083

hellohuanlin approved these changes Jul 14, 2023

View reviewed changes

stuartmorgan-g requested a review from tarrinneal July 16, 2023 22:59

tarrinneal approved these changes Jul 17, 2023

View reviewed changes

turekj added 2 commits July 18, 2023 11:16

Merge branch 'main' into fix-ios-video-player-crash

d7fdc34

Bump package version

efd2567

stuartmorgan-g added autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests labels Jul 18, 2023

auto-submit bot merged commit 3e8b813 into flutter:main Jul 18, 2023

engine-flutter-autoroll mentioned this pull request Jul 18, 2023

Roll Packages from 6889cca8ba78 to 3e8b8130cda3 (9 revisions) flutter/flutter#130802

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 18, 2023

3e8b813 [video_player] Fix iOS crash with multiple players (flutter/p…

6ff60f1

…ackages#4202)

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[video_player] Fix iOS crash with multiple players #4202

[video_player] Fix iOS crash with multiple players #4202

turekj commented Jun 12, 2023 •

edited

Loading

flutter-dashboard bot commented Jun 12, 2023

hellohuanlin left a comment

hellohuanlin Jun 14, 2023

turekj Jun 14, 2023 •

edited

Loading

hellohuanlin Jun 14, 2023

turekj Jun 14, 2023

hellohuanlin Jun 14, 2023

turekj Jun 16, 2023

turekj commented Jun 14, 2023

hellohuanlin commented Jun 14, 2023

hellohuanlin Jun 16, 2023

turekj Jul 14, 2023 •

edited

Loading

stuartmorgan-g commented Jun 26, 2023

stuartmorgan-g commented Jul 13, 2023

turekj commented Jul 14, 2023 •

edited

Loading

hellohuanlin left a comment

hellohuanlin Jul 14, 2023

turekj Jul 14, 2023

hellohuanlin Jul 14, 2023

turekj Jul 14, 2023

hellohuanlin Jul 14, 2023

turekj Jul 14, 2023

hellohuanlin Jul 14, 2023

turekj Jul 14, 2023

turekj commented Jul 14, 2023

stuartmorgan-g commented Jul 17, 2023

turekj commented Jul 18, 2023

stuartmorgan-g commented Jul 18, 2023


		[self.player replaceCurrentItemWithPlayerItem:nil];
		_player = nil;

[video_player] Fix iOS crash with multiple players #4202

[video_player] Fix iOS crash with multiple players #4202

Conversation

turekj commented Jun 12, 2023 • edited Loading

Detailed description

Pre-launch Checklist

flutter-dashboard bot commented Jun 12, 2023

hellohuanlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turekj Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turekj commented Jun 14, 2023

hellohuanlin commented Jun 14, 2023

Choose a reason for hiding this comment

turekj Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

stuartmorgan-g commented Jun 26, 2023

stuartmorgan-g commented Jul 13, 2023

turekj commented Jul 14, 2023 • edited Loading

hellohuanlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turekj commented Jul 14, 2023

stuartmorgan-g commented Jul 17, 2023

turekj commented Jul 18, 2023

stuartmorgan-g commented Jul 18, 2023

turekj commented Jun 12, 2023 •

edited

Loading

turekj Jun 14, 2023 •

edited

Loading

turekj Jul 14, 2023 •

edited

Loading

turekj commented Jul 14, 2023 •

edited

Loading