[video_player] Move [player dispose] to onUnregistered

[cyanglaz]

Description

Fix the issue where the player object is destructed before the texture finishes unregistering which leads to crashing the app.

The onTetureUnregistered callback is only available after flutter/engine#12695. Until this change in engine is moved to "stable", we use a temporary solution.
The temporary fix delays the dispose of the FLTPlayer which avoid the race condition.

This PR also added driver test for the example app which runs the crashing scenario in flutter/flutter#41125 and makes sure it is not crashing. The test will only crash on iOS 12 and above.

Related Issues

flutter/flutter#41125

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
• All existing and new tests are passing.
• I updated/added relevant documentation (doc comments with ///).
• The analyzer (flutter analyze) does not report any problems on my PR.
• I read and followed the Flutter Style Guide.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
• I updated CHANGELOG.md to add a description of the change.
• I signed the CLA.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

• Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
• No, this is not a breaking change.

[bparrishMines]

LGTM with passing tests

[cyanglaz]

We will skip the test until we move iOS integration test to firebase device lab. @collinjackson
To skip test and pass the CI. A fix in the plugin_tools needs to be landed. The PR for the fix is here:
https://github.com/flutter/plugin_tools/pull/64

[Hixie]

We will skip the test until we move iOS integration test to firebase device lab. @collinjackson
To skip test and pass the CI. A fix in the plugin_tools needs to be landed. The PR for the fix is here:
flutter/plugin_tools#64

SGTM.

[cyanglaz]

@fkorotkov has been looking into the video player hanging issue for flutter driver tests. Meanwhile, since it is a fix to a customer critical issue, we can land it with the skipped tests. We will un-skip it when the Cirrus issue is resolved.

[fkorotkov]

@cyanglaz BTW did you test with Xcode 10.2? I did try on 11.2.1 where I had the version error. I'll try 10.2 now. 🤔

[cyanglaz]

@cyanglaz BTW did you test with Xcode 10.2? I did try on 11.2.1 where I had the version error. I'll try 10.2 now. 🤔

The version error you emailed me seems related to the pub version script. It basically says the version in pubspec in your patch is too high compares to the version in pubspec on master. I replied your email with some thoughts about it. Please let me know if you haven't received the email, I can send it again. :D

[fkorotkov]

@cyanglaz I've removed the skip thing on master and was able to run it locally the tests within the VM. But I have a newer version of the virtualization locally since it's not possible to run the version that Cirrus is using on Catalina. I'm in the process of upgrading the mac infrastruture which includes upgrading to Catalina, migrating to the 2.0 version of virtualization and a few other improvements. If everything will go smooth it will be available in two weeks. I'll ping you once the upgrade will be available so we can test the test. 😅 Sorry for the inconveniences!

[cyanglaz]

@fkorotkov Thanks that sounds great!! I have created an issue for it .flutter/flutter#43012, let's track it there :)

[dannyvalentesonos]

@cyanglaz I am testing this fix, and onTextureUnregistered is never called.

After digging a little, it's because the FlutterTexture interface has this method declaration:

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

Yet, the video player plugin overrides it like this:

- (void)onTextureUnregistered {
  dispatch_async(dispatch_get_main_queue(), ^{
    [self dispose];
  });
}

When I change it to

- (void)onTextureUnregistered:(NSObject<FlutterTexture>*)texture {
  dispatch_async(dispatch_get_main_queue(), ^{
    [self dispose];
  });
}

it works perfectly.

I'm not sure why the FlutterTexture API would pass the texture pointer to itself... Anyway, this has not been fully fixed.

Thanks.

[cyanglaz]

@dannyvalentesonos you are absolutely correct! Do you mind opening a PR with your fix? I will be happy to review it. Thanks.

[shihchanghsiungsonos]

@cyanglaz here is the PR: #2480. Please review. Thanks!