[google_maps_flutter] Started dispatching platform messages from platform thread #6069

gaaclarke · 2024-02-06T21:04:19Z

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

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

stuartmorgan-g · 2024-02-06T21:36:27Z

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:

packages/packages/camera/camera_avfoundation/example/ios/RunnerTests/ThreadSafeEventChannelTests.m

Line 39 in e4ea6bf

- (void)testSetStreamHandler_shouldDispatchToMainThreadIfCalledFromBackgroundThread {
packages/packages/image_picker/image_picker_ios/example/ios/RunnerTests/ImagePickerPluginTests.m

Line 410 in e4ea6bf

plugin.callContext = [[FLTImagePickerMethodCallContext alloc]

stuartmorgan-g · 2024-02-06T21:36:49Z

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

Do the engine builders publish those artifacts?

gaaclarke · 2024-02-06T22:06:05Z

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 · 2024-02-06T22:32:47Z

@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 · 2024-02-06T22:47:20Z

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

stuartmorgan-g · 2024-02-07T19:01:35Z

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 · 2024-02-08T01:00:42Z

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 · 2024-02-08T04:13:16Z

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.

...utter/google_maps_flutter_ios/example/ios12/ios/RunnerTests/FLTTileProviderControllerTests.m

hellohuanlin

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? Could you update the PR description and maybe link to a new issue, in case of confusion to future readers?

hellohuanlin · 2024-02-09T00:14:52Z

...utter/google_maps_flutter_ios/example/ios12/ios/RunnerTests/FLTTileProviderControllerTests.m

+
+@implementation FLTTileProviderControllerTests
+
+- (void)testCallChannelOnPlatformThread {


nit: include function name being tested testRequestTileMustCallChannelOnPlatformThread

hellohuanlin · 2024-02-09T00:15:59Z

...utter/google_maps_flutter_ios/example/ios12/ios/RunnerTests/FLTTileProviderControllerTests.m

+  XCTestExpectation *expectation = [self expectationWithDescription:@"invokeMethod"];
+  OCMStub([channel invokeMethod:[OCMArg any] arguments:[OCMArg any] result:[OCMArg any]])
+      .andDo(^(NSInvocation *invocation) {
+        XCTAssertTrue([[NSThread currentThread] isMainThread]);


nit: XCTAssertTrue([[NSThread currentThread] isMainThread], @"Method channel must be called on main thread")

hellohuanlin · 2024-02-09T00:16:27Z

...utter/google_maps_flutter_ios/example/ios12/ios/RunnerTests/FLTTileProviderControllerTests.m

+        [expectation fulfill];
+      });
+  id receiver = OCMProtocolMock(@protocol(GMSTileReceiver));
+  [controller requestTileForX:0 y:0 zoom:0 receiver:receiver];


nit: can just inline this receiver?

gaaclarke · 2024-02-09T01:19:46Z

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.

stuartmorgan-g

LGTM

jmagman · 2024-02-14T20:34:05Z

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

…form thread.

gaaclarke · 2024-02-14T20:38:39Z

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

rebased and put in the queue

auto-submit · 2024-02-14T20:51:14Z

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 · 2024-03-05T21:00:10Z

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.

…rom platform thread (flutter/packages#6069)

flutter/packages@b21c542...756dcc1 2024-03-15 [email protected] [go_router] Use `leak_tracker_flutter_testing` (flutter/packages#6210) 2024-03-15 [email protected] [camera_web][google_maps_flutter] Fix tests throwing errors after test completion with manual roll (flutter/packages#6318) 2024-03-14 [email protected] [pigeon] Fixes double prefixes added to enum names for Objc HostApis and FlutterApis (flutter/packages#6263) 2024-03-14 [email protected] [webview_flutter_android][webview_flutter_wkwebview] Adds platform implementations for onHttpError (flutter/packages#6149) 2024-03-14 [email protected] [image_picker_android] Fix deprecation warnings by branching based on build version, and suppressing only when needed (flutter/packages#6233) 2024-03-14 [email protected] [google_maps_flutter] Started dispatching platform messages from platform thread (flutter/packages#6069) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…form thread (flutter#6069) Issue: flutter/flutter#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.

github-actions bot added p: google_maps_flutter platform-ios labels Feb 6, 2024

gaaclarke force-pushed the invoke-on-main branch from 8db0774 to 3e13780 Compare February 6, 2024 21:05

gaaclarke marked this pull request as ready for review February 6, 2024 21:10

gaaclarke requested a review from hellohuanlin as a code owner February 6, 2024 21:10

gaaclarke mentioned this pull request Feb 7, 2024

Run plugin tests against the debug engine flutter/flutter#143093

Open

gaaclarke mentioned this pull request Feb 7, 2024

google_maps_flutter tests do not run on arm64 simulator flutter/flutter#143133

Closed

stuartmorgan-g requested changes Feb 8, 2024

View reviewed changes

...utter/google_maps_flutter_ios/example/ios12/ios/RunnerTests/FLTTileProviderControllerTests.m Outdated Show resolved Hide resolved

...utter/google_maps_flutter_ios/example/ios12/ios/RunnerTests/FLTTileProviderControllerTests.m Outdated Show resolved Hide resolved

gaaclarke requested a review from stuartmorgan-g February 8, 2024 21:54

hellohuanlin reviewed Feb 9, 2024

View reviewed changes

stuartmorgan-g approved these changes Feb 9, 2024

View reviewed changes

gaaclarke added 7 commits February 14, 2024 12:37

[google_maps_flutter] Started dispatching platform messages from plat…

351de65

…form thread.

format

223c75d

added unit test against private api

2dcd509

clean up projs

eab29ff

format

d5e5171

review feedback

907c314

removed warning

6ab94c6

gaaclarke force-pushed the invoke-on-main branch from 6bdfa26 to 6ab94c6 Compare February 14, 2024 20:37

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 14, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 14, 2024

gaaclarke and others added 2 commits February 14, 2024 14:28

oops updated version

b3dc22c

Merge branch 'main' into invoke-on-main

e6d900f

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 5, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 5, 2024

Merge branch 'main' into invoke-on-main

187c8b0

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 14, 2024

auto-submit bot merged commit 2218300 into flutter:main Mar 14, 2024

engine-flutter-autoroll mentioned this pull request Mar 15, 2024

Roll Packages from b21c5424783a to 756dcc11180b (6 revisions) flutter/flutter#145214

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 15, 2024

2218300 [google_maps_flutter] Started dispatching platform messages f…

b48a573

…rom platform thread (flutter/packages#6069)

reidbaker mentioned this pull request Apr 1, 2024

[camera_android_camerax] Move integration_test dependency to tests reidbaker/packages#4

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps_flutter] Started dispatching platform messages from platform thread #6069

[google_maps_flutter] Started dispatching platform messages from platform thread #6069

gaaclarke commented Feb 6, 2024 •

edited

Loading

stuartmorgan-g commented Feb 6, 2024

stuartmorgan-g commented Feb 6, 2024

gaaclarke commented Feb 6, 2024

gaaclarke commented Feb 6, 2024

gaaclarke commented Feb 6, 2024

stuartmorgan-g commented Feb 7, 2024

gaaclarke commented Feb 8, 2024

hellohuanlin commented Feb 8, 2024

hellohuanlin left a comment •

edited

Loading

hellohuanlin Feb 9, 2024

hellohuanlin Feb 9, 2024

hellohuanlin Feb 9, 2024 •

edited

Loading

gaaclarke commented Feb 9, 2024

stuartmorgan-g left a comment

jmagman commented Feb 14, 2024

gaaclarke commented Feb 14, 2024

auto-submit bot commented Feb 14, 2024

auto-submit bot commented Mar 5, 2024


		@implementation FLTTileProviderControllerTests

		- (void)testCallChannelOnPlatformThread {

[google_maps_flutter] Started dispatching platform messages from platform thread #6069

[google_maps_flutter] Started dispatching platform messages from platform thread #6069

Conversation

gaaclarke commented Feb 6, 2024 • edited Loading

Pre-launch Checklist

stuartmorgan-g commented Feb 6, 2024

stuartmorgan-g commented Feb 6, 2024

gaaclarke commented Feb 6, 2024

gaaclarke commented Feb 6, 2024

gaaclarke commented Feb 6, 2024

stuartmorgan-g commented Feb 7, 2024

gaaclarke commented Feb 8, 2024

hellohuanlin commented Feb 8, 2024

hellohuanlin left a comment • edited Loading

Choose a reason for hiding this comment

hellohuanlin Feb 9, 2024

Choose a reason for hiding this comment

hellohuanlin Feb 9, 2024

Choose a reason for hiding this comment

hellohuanlin Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

gaaclarke commented Feb 9, 2024

stuartmorgan-g left a comment

Choose a reason for hiding this comment

jmagman commented Feb 14, 2024

gaaclarke commented Feb 14, 2024

auto-submit bot commented Feb 14, 2024

auto-submit bot commented Mar 5, 2024

gaaclarke commented Feb 6, 2024 •

edited

Loading

hellohuanlin left a comment •

edited

Loading

hellohuanlin Feb 9, 2024 •

edited

Loading