[google_maps_flutter] Started dispatching platform messages from platform thread

[gaaclarke]

Issue: https://github.com/flutter/flutter/issues/135252

Testing: This adds no new functionality and thus should be covered in tests. The problem is that we are permissive to unsafe platform channel usage in the release flutter engine. We should maybe consider running tests against a debug version of the engine.

Pre-launch Checklist

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• [x] I signed the CLA.
• [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• [x] I linked to at least one issue that this PR fixes in the description above.
• [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• [x] I updated/added relevant documentation (doc comments with ///).
• [ ] I added new tests to check the change I am making, or this PR is test-exempt.
• [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[stuartmorgan]

Testing: This adds no new functionality

The criteria for testing is not that we only test new functionality, it's https://github.com/flutter/flutter/wiki/Tree-hygiene#tests

and thus should be covered in tests.

The thing you are fixing here is not meaningfully covered by current tests, as evidenced by the fact that they pass.

This can be tested via native unit tests. Some relevant examples:

• https://github.com/flutter/packages/blob/e4ea6bf72e1b9f47176b1a0a74669126bd0a26f4/packages/camera/camera_avfoundation/example/ios/RunnerTests/ThreadSafeEventChannelTests.m#L39
• https://github.com/flutter/packages/blob/e4ea6bf72e1b9f47176b1a0a74669126bd0a26f4/packages/image_picker/image_picker_ios/example/ios/RunnerTests/ImagePickerPluginTests.m#L410

[stuartmorgan]

We should maybe consider running tests against a debug version of the engine.

Do the engine builders publish those artifacts?

[gaaclarke]

The criteria for testing is not that we only test new functionality, it's https://github.com/flutter/flutter/wiki/Tree-hygiene#tests

Right, the point is that the code in this PR has test coverage. It is executed inside of a test and it is evaluated to give the correct results, not that the thing that caused issue is asserted to be fixed in a test. Those examples aren't doing exactly this.

I'll be the first one asserting that platform channels are being used from the correct thread. Mocking out platform channels will be a poor substitute for asserting proper platform channel usage across all channels though.

We'd get that with a debug engine, but that's outside of the scope of this PR. It also doesn't solve the problem for 3p customers either since they should have access to this kind of testing without having to build a debug engine.

I'll see if I can come up with a mocking unit test in the meantime.

Do the engine builders publish those artifacts?

I don't think so.

[gaaclarke]

@stuartmorgan I looked into adding a test at /Users/aaclarke/dev/packages/packages/google_maps_flutter/google_maps_flutter_ios/example/ios14/ios/RunnerTests. Those tests fail to link:

ld: warning: ignoring file '/Users/aaclarke/Library/Developer/Xcode/DerivedData/Runner-cfyxahhvdbsudtbrisrwbppvdqok/Build/Products/Debug-iphonesimulator/google_maps_flutter_ios/libgoogle_maps_flutter_ios.a[10](GoogleMapPolylineController.o)': found architecture 'x86_64', required architecture 'arm64'
ld: warning: ignoring file '/Users/aaclarke/Library/Developer/Xcode/DerivedData/Runner-cfyxahhvdbsudtbrisrwbppvdqok/Build/Products/Debug-iphonesimulator/google_maps_flutter_ios/libgoogle_maps_flutter_ios.a[9](GoogleMapPolygonController.o)': found architecture 'x86_64', required architecture 'arm64'
ld: warning: ignoring file '/Users/aaclarke/Library/Developer/Xcode/DerivedData/Runner-cfyxahhvdbsudtbrisrwbppvdqok/Build/Products/Debug-iphonesimulator/google_maps_flutter_ios/libgoogle_maps_flutter_ios.a[8](GoogleMapMarkerController.o)': found architecture 'x86_64', required architecture 'arm64'
ld: warning: ignoring file '/Users/aaclarke/Library/Developer/Xcode/DerivedData/Runner-cfyxahhvdbsudtbrisrwbppvdqok/Build/Products/Debug-iphonesimulator/google_maps_flutter_ios/libgoogle_maps_flutter_ios.a[7](GoogleMapController.o)': found architecture 'x86_64', required architecture 'arm64'
ld: warning: ignoring file '/Users/aaclarke/Library/Developer/Xcode/DerivedData/Runner-cfyxahhvdbsudtbrisrwbppvdqok/Build/Products/Debug-iphonesimulator/google_maps_flutter_ios/libgoogle_maps_flutter_ios.a[6](GoogleMapCircleController.o)': found architecture 'x86_64', required architecture 'arm64'
ld: warning: ignoring file '/Users/aaclarke/Library/Developer/Xcode/DerivedData/Runner-cfyxahhvdbsudtbrisrwbppvdqok/Build/Products/Debug-iphonesimulator/google_maps_flutter_ios/libgoogle_maps_flutter_ios.a[5](google_maps_flutter_ios-dummy.o)': found architecture 'x86_64', required architecture 'arm64'
ld: warning: ignoring file '/Users/aaclarke/Library/Developer/Xcode/DerivedData/Runner-cfyxahhvdbsudtbrisrwbppvdqok/Build/Products/Debug-iphonesimulator/google_maps_flutter_ios/libgoogle_maps_flutter_ios.a[4](FLTGoogleMapTileOverlayController.o)': found architecture 'x86_64', required architecture 'arm64'
ld: warning: ignoring file '/Users/aaclarke/Library/Developer/Xcode/DerivedData/Runner-cfyxahhvdbsudtbrisrwbppvdqok/Build/Products/Debug-iphonesimulator/google_maps_flutter_ios/libgoogle_maps_flutter_ios.a[3](FLTGoogleMapsPlugin.o)': found architecture 'x86_64', required architecture 'arm64'
ld: warning: ignoring file '/Users/aaclarke/Library/Developer/Xcode/DerivedData/Runner-cfyxahhvdbsudtbrisrwbppvdqok/Build/Products/Debug-iphonesimulator/google_maps_flutter_ios/libgoogle_maps_flutter_ios.a[2](FLTGoogleMapJSONConversions.o)': found architecture 'x86_64', required architecture 'arm64'
ld: warning: Could not find or use auto-linked framework 'CoreAudioTypes': framework 'CoreAudioTypes' not found
ld: Undefined symbols:
  _OBJC_CLASS_$_FLTGoogleMapsPlugin, referenced from:
       in GeneratedPluginRegistrant.o
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Let me know if there exists a better place to test or if I'm running these wrong.

[gaaclarke]

So we do serve debug builds, but they are jit. So I'm not sure if that will work.

[stuartmorgan]

Right, the point is that the code in this PR has test coverage.

I'm not sure why you are asserting things that aren't in dispute and aren't related to the policy for test exemptions. The policy is that changes need to be tested, and this change is not being tested, as evidenced by the fact that no tests are failing in our CI without this PR.

Let me know if there exists a better place to test or if I'm running these wrong.

It looks like you may have an old Cocoapods cache, and are getting an old version of Google Maps even with the iOS 14 example. If so you could update Cocoapods locally, but since this isn't iOS-version specific, it should be in the oldest iOS example per the README in the enclosing directory (which will avoid the issue altogether).

[gaaclarke]

Test against the private API has been added. I still urge the plugin maintainers to consider a more holistic solution to testing the thread safety of platform channels. @hellohuanlin PTAL

[hellohuanlin]

haven't reviewed yet. it doesn't seem to be related to the linked issue. if so, maybe a good idea to create a new issue about calling method channel on the wrong thread, in case of confusion to future readers.

[gaaclarke]

i could be missing something, but this doesn't seem to be related to the linked issue on tiles not being displayed, but it's rather a separate issue that you discovered while researching it?

I cannot rule out that it isn't applicable. This PR fixes a race condition and how it manifests is difficult for me to guess. It may result in lost message or crashes.

[jmagman]

Looks like once the CHANGELOG conflict is resolved, this is ready to be merged?

[gaaclarke]

Looks like once the CHANGELOG conflict is resolved, this is ready to be merged?

rebased and put in the queue

[auto-submit[bot]]

auto label is removed for flutter/packages/6069, due to - The status or check suite Linux repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

[auto-submit[bot]]

auto label is removed for flutter/packages/6069, due to - The status or check suite Linux_web web_build_all_packages master has failed. Please fix the issues identified (or deflake) before re-applying this label.