[google_maps_flutter] Fix iOS crash by observing map frame change only once #3426

guykogus · 2021-01-14T23:22:00Z

I cannot to tell you the stress this bug has caused me 😅

The view method gets called a lot. Every draw cycle, in fact. And if the frame should change, for example when the keyboard appears, every single one of those observeValueForKeyPath: observations will be called, which crashes an app with the following exception: EXC_BAD_ACCESS KERN_PROTECTION_FAILURE

Pre-launch Checklist

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

MarceLifeBites · 2021-01-20T17:46:46Z

Thx @guykogus , we are having de same bug here, and fix it with help a lot with our platform stability, so i wish they can release the solution as soon as possible

pawlowskim · 2021-03-09T12:52:15Z

Exactly the same happens for us.

Crashed: com.apple.main-thread
EXC_BAD_ACCESS KERN_PROTECTION_FAILURE 0x000000016f713ff8
-[GMSMapView internalSetCamera:]

Crashed: com.apple.main-thread
0  libsystem_pthread.dylib        0x1cfd358c4 ___chkstk_darwin + 64
1  libsystem_pthread.dylib        0x1cfd358c4 thread_chkstk_darwin + 64
2  Foundation                     0x18a9b36cc NSKeyValueWillChange + 128
3  Foundation                     0x18a9b0d24 NSKeyValueWillChangeWithPerThreadPendingNotifications + 384
4  Runner                         0x10096123c -[GMSMapView internalSetCamera:] + 32268
5  Runner                         0x10095e808 -[GMSMapView updateWithCamera:] + 21464
6  Foundation                     0x18aa19614 __NSThreadPerformPerform + 184
7  CoreFoundation                 0x1896a5be0 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24
8  CoreFoundation                 0x1896a5ae0 __CFRunLoopDoSource0 + 204
9  CoreFoundation                 0x1896a4e28 __CFRunLoopDoSources0 + 256
10 CoreFoundation                 0x18969f3d0 __CFRunLoopRun + 776
11 CoreFoundation                 0x18969eb90 CFRunLoopRunSpecific + 572
12 GraphicsServices               0x19f9c1598 GSEventRunModal + 160
13 UIKitCore                      0x18bf88638 -[UIApplication _run] + 1052
14 UIKitCore                      0x18bf8dbb8 UIApplicationMain + 164
15 Runner                         0x1005f7948 main + 10 (AppDelegate.swift:10)
16 libdyld.dylib                  0x18937d588 start + 4

Flutter 1.22.5, google_maps_flutter: 1.0.6. For us it happens when user stays on a map for ~30 minutes (app in foreground, screen is on) and we are tracking his position and update his position marker (custom one, so we are re-drawing circle on a map). To simulate it, I just use more or less this piece of code:

int a = 0;
    Timer.periodic(const Duration(milliseconds: 10), (timer) {
      if (_mapController != null && position != null && mounted) {
        a++;
        double b = a > 50000 ? -0.0003 : 0.0003;
        setState(() {
          position = Position(
              longitude: position.longitude,
              latitude: position.latitude + b);
        });
        _mapController.animateCamera(CameraUpdate.newLatLng(
            LatLng(position.latitude, position.longitude)));
      }
    });

I can confirm changes from this pull seems to resolves the problem. Beside, it should be released with version lower than 2.0.0 because plenty of projects depends on more libraries than only google maps and won't be migrated to flutter 2.0.0 for a while (dependency hell 😢 )

guykogus · 2021-03-09T14:00:40Z

I can confirm changes from this pull seems to resolves the problem. Beside, it should be released with version lower than 2.0.0 because plenty of projects depends on more libraries than only google maps and won't be migrated to flutter 2.0.0 for a while (dependency hell 😢 )

Unfortunately there's no branch in the original repo to merge with to keep it at version < 2.0 ☹️

pawlowskim · 2021-03-09T14:17:22Z

I can confirm changes from this pull seems to resolves the problem. Beside, it should be released with version lower than 2.0.0 because plenty of projects depends on more libraries than only google maps and won't be migrated to flutter 2.0.0 for a while (dependency hell 😢 )

Unfortunately there's no branch in the original repo to merge with to keep it at version < 2.0 ☹️

I'm aware, but the fix won't be usable for old projects that have dozens of non flutter / dart team libraries dependencies. Until then, guess we will use your fork:

  google_maps_flutter:
      git:
          url: git://github.com/guykogus/flutter_plugins
          path: packages/google_maps_flutter/google_maps_flutter
          ref: 6156ea3fa2ecf4b0e7ff35ba9a5fb1414e181d6d

guykogus · 2021-03-09T14:40:25Z

I'm aware, but the fix won't be usable for old projects that have dozens of non flutter / dart team libraries dependencies. Until then, guess we will use your fork:
  google_maps_flutter:
      git:
          url: git://github.com/guykogus/flutter_plugins
          path: packages/google_maps_flutter/google_maps_flutter
          ref: 6156ea3fa2ecf4b0e7ff35ba9a5fb1414e181d6d

You can use ref: via if you want. I've got another fix there for Android, which can crash if the native Android map loads after the Flutter map is unmounted (i.e. the StatefulWidget is disposed). Unfortunately the PR was rejected cos they couldn't reproduce it.

sangTroo · 2021-03-09T14:41:47Z

Flutter 1.22.5, google_maps_flutter: 1.0.6. For us it happens when user stays on a map for ~30 minutes (app in foreground, screen is on) and we are tracking his position and update his position marker (custom one, so we are re-drawing circle on a map). To simulate it, I just use more or less this piece of code:

We have the same problem. Will try this branch to see if it fixes the issue. The app that we're building is heavily using maps. And hasn't been stable since this bug keeps the app crashing

carlosmobile · 2021-03-09T14:48:44Z

Hi! We have the same problem. We are using a fork with this changes and fixed the crash. Thanks!

cyanglaz

Thanks for providing the fix, the change looks good.

It's not clear to me in what scenario the view is called multiple times for a single webview.
It looks like it's only called once per controller, see: https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L161

Again, this is a good fix nonetheless, the old code is pretty confusing and didn't do what it says it wants to do.

However, we should add some tests before landing this. I wonder If you know a way to reproduce this consistently.

via-guy · 2021-04-29T22:10:58Z

Thanks for providing the fix, the change looks good.

It's not clear to me in what scenario the view is called multiple times for a single webview.
It looks like it's only called once per controller, see: https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L161

Again, this is a good fix nonetheless, the old code is pretty confusing and didn't do what it says it wants to do.

However, we should add some tests before landing this. I wonder If you know a way to reproduce this consistently.

I couldn't come up with one when I made this fix. It happens in our project, and clearly it does for others too. Do any of you have a simple sample test that can be provided here, @pawlowskim @sangTroo @carlosmobile ?

sangTroo · 2021-06-16T13:19:52Z

Any update when this fix will be merged?

sangTroo · 2021-06-16T13:20:41Z

Thanks for providing the fix, the change looks good.
It's not clear to me in what scenario the view is called multiple times for a single webview.
It looks like it's only called once per controller, see: https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L161
Again, this is a good fix nonetheless, the old code is pretty confusing and didn't do what it says it wants to do.
However, we should add some tests before landing this. I wonder If you know a way to reproduce this consistently.

I couldn't come up with one when I made this fix. It happens in our project, and clearly it does for others too. Do any of you have a simple sample test that can be provided here, @pawlowskim @sangTroo @carlosmobile ?

Hi,
What kind of simple sample test do you want, sorry I don't get what you mean

stuartmorgan-g

It looks like this should be easily testable with a native unit test:

Allow injecting a mock GMSMapView
Call view several times
Ensure that addObserver:... is not called on the mock repeatedly.

via-guy · 2021-10-06T10:32:27Z

@stuartmorgan I've been playing a bit with doing something like that but I don't really know how to, at least not without breaking the current code structure/API. Any extra guide would be greatly appreciated.

stuartmorgan-g · 2021-10-25T19:26:58Z

@stuartmorgan I've been playing a bit with doing something like that but I don't really know how to, at least not without breaking the current code structure/API. Any extra guide would be greatly appreciated.

Can you elaborate on the issue you are having?

If it's the mock injection, for something that's created in init like the map view you generally add a new init method with the thing you want to inject added, which becomes the main initializer, and the real one delegates to that. E.g.:

- (instancetype)initWithFrame:(CGRect)frame
               viewIdentifier:(int64_t)viewId
                    arguments:(id _Nullable)args
                    registrar:(NSObject<FlutterPluginRegistrar>*)registrar {
    GMSCameraPosition* camera = ToOptionalCameraPosition(args[@"initialCameraPosition"]);
    GMSMapView* mapView = [GMSMapView mapWithFrame:frame camera:camera];
    return [self initWithMapView:mapView ...];
};

- (instancetype)initWithMapView:(GMSMapView)mapView
               viewIdentifier:(int64_t)viewId
                    arguments:(id _Nullable)args
                    registrar:(NSObject<FlutterPluginRegistrar>*)registrar {
  if (self = [super init]) {
    _mapView = mapView;
    _viewId = viewId;

    _mapView.accessibilityElementsHidden = NO;
    ...

You expose the new init method in a ...-Test.h header file, and use it in the unit test.

flutter-dashboard · 2022-01-06T22:57:21Z

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.

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.

stuartmorgan-g · 2022-01-26T21:10:44Z

@guykogus Are you planning on adding tests per #3426 (review) so that this can move forward?

guykogus · 2022-01-26T22:21:59Z

@stuartmorgan yes, I really want to because this change is essential, I just haven't had a chance to do it...

via-guy · 2022-02-14T18:01:10Z

@stuartmorgan I've added some tests, but I'm not sure if I've really done it correctly. Basically I just tested to see if it doesn't lag a long time to loop over view. On the master branch the test fails, but it passes on mine.

stuartmorgan-g · 2022-02-14T18:32:08Z

packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md

@@ -2,6 +2,7 @@

 * Removes dependencies from `pubspec.yaml` that are only needed in `example/pubspec.yaml`
 * Updates Android compileSdkVersion to 31.
+* Fix iOS crash on `EXC_BAD_ACCESS KERN_PROTECTION_FAILURE` if the map frame changes long after creation.


Fixes

(Per the repo CHANGELOG style guide.)

packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md

stuartmorgan-g · 2022-02-14T18:33:32Z

packages/google_maps_flutter/google_maps_flutter/example/ios/RunnerTests/FlutterMock.h

+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface MockRegistrar : NSObject <FlutterPluginRegistrar>


Why are you mocking these manually instead of with OCMock?

I've never used OCMock. Thanks for the tip!

stuartmorgan-g · 2022-02-14T18:35:32Z

packages/google_maps_flutter/google_maps_flutter/example/ios/RunnerTests/GoogleMapsTests.m

+  }
+  [[controller view] setValue:[NSValue valueWithCGRect:CGRectMake(0, 0, 0, 0)] forKey:@"frame"];
+
+  [viewExpectation fulfill];


I don't understand what this test is supposed to be doing. It's not asserting what this PR is actually intended to fix.

I described what a test should look like above; I'm not sure why you've done something completely different.

That's a good point. Got distracted with seeing if I could actually test for the crash itself to happen. I'll update it.

via-guy · 2022-02-14T19:37:45Z

@stuartmorgan I've updated according to your suggestions, but I'm not sure about the placement of GoogleMapController-Test.h. I added it into the example project itself, because otherwise it would be exported with the pod. Is that correct, or is there some way in the podspec to specify target test files?

stuartmorgan-g · 2022-02-28T20:12:26Z

@stuartmorgan I've updated according to your suggestions, but I'm not sure about the placement of GoogleMapController-Test.h. I added it into the example project itself, because otherwise it would be exported with the pod. Is that correct, or is there some way in the podspec to specify target test files?

It should live next to the source file; you can use a module map to control how it's exposed. See #4430 for an example of setting that up.

stuartmorgan-g · 2022-02-28T20:16:54Z

packages/google_maps_flutter/google_maps_flutter/example/ios/Podfile

@@ -31,6 +31,8 @@ target 'Runner' do
  flutter_install_all_ios_pods File.dirname(File.realpath(__FILE__))
  target 'RunnerTests' do
    inherit! :search_paths
+
+    pod 'OCMock'


This should have a version. See other uses for examples.

stuartmorgan-g · 2022-02-28T20:25:00Z

...s/google_maps_flutter/google_maps_flutter/example/ios/RunnerTests/GoogleMapController-Test.h

+
+@interface FLTGoogleMapController (Test)
+
+- (instancetype)initWithMapView:(GMSMapView *)mapView


All methods need declaration comments, per the ObjC style guide.

packages/google_maps_flutter/google_maps_flutter/ios/Classes/GoogleMapController.m

stuartmorgan-g · 2022-02-28T20:36:28Z

packages/google_maps_flutter/google_maps_flutter/example/ios/RunnerTests/MockGMSMapView.h

+
+@import GoogleMaps;
+
+@interface MockGMSMapView : GMSMapView


Needs a declaration comment.

stuartmorgan-g · 2022-02-28T20:36:36Z

packages/google_maps_flutter/google_maps_flutter/example/ios/RunnerTests/MockGMSMapView.h

+
+@interface MockGMSMapView : GMSMapView
+
+@property(nonatomic, assign, readonly) NSInteger frameObserverCount;


Needs a declaration comment.

stuartmorgan-g · 2022-02-28T20:41:12Z

packages/google_maps_flutter/google_maps_flutter/example/ios/RunnerTests/MockGMSMapView.h

+
+@import GoogleMaps;
+
+@interface MockGMSMapView : GMSMapView


You should probably call this PartiallyMockedMapView or something along those lines; the assumption for a mock object is not that it is the real object but with some overridden behavior, but that it has no behavior other than what it is given in test setup.

stuartmorgan-g · 2022-03-07T20:42:19Z

packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md

@@ -3,6 +3,7 @@
 * Removes dependencies from `pubspec.yaml` that are only needed in `example/pubspec.yaml`
 * Updates Android compileSdkVersion to 31.
 * Internal code cleanup for stricter analysis options.
+* Fixes iOS crash on `EXC_BAD_ACCESS KERN_PROTECTION_FAILURE` if the map frame changes long after creation.


You can't add changes to a version that has already been published. It needs a new version.

stuartmorgan-g · 2022-03-07T20:44:30Z

...ges/google_maps_flutter/google_maps_flutter/example/ios/RunnerTests/PartiallyMockedMapView.h

+
+@import GoogleMaps;
+
+// Defines a map view used for testing key-value observing.


Please use Doxygen style: https://google.github.io/styleguide/objcguide.html#declaration-comments.

stuartmorgan-g · 2022-03-07T20:44:43Z

...ges/google_maps_flutter/google_maps_flutter/example/ios/RunnerTests/PartiallyMockedMapView.h

+// Defines a map view used for testing key-value observing.
+@interface PartiallyMockedMapView : GMSMapView
+
+/// Counts the number of times that `frame` KVO has been added


Same. You're also missing the period; comments should be properly punctuated, per the style guide.

Also, this is a property, so should describe a noun rather than a verb; methods do things, properties are things. I.e., "The number of times that [...]".

stuartmorgan-g · 2022-03-07T20:47:34Z

packages/google_maps_flutter/google_maps_flutter/ios/Classes/GoogleMapController_Test.h

+@interface FLTGoogleMapController (Test)
+
+/**
+ * Initialize a map controller with a concrete map view.


Nit: Initializes

(Per style guide.)

packages/google_maps_flutter/google_maps_flutter/ios/Classes/google_maps_flutter-umbrella.h

via-guy · 2022-03-08T12:07:59Z

@stuartmorgan done

jmagman · 2022-03-18T17:40:48Z

There's a format error, please run:

patch -p1 <<DONE
diff --git a/packages/google_maps_flutter/google_maps_flutter/ios/Classes/google_maps_flutter-umbrella.h b/packages/google_maps_flutter/google_maps_flutter/ios/Classes/google_maps_flutter-umbrella.h
index 5af927f61..50880a2b9 100644
--- a/packages/google_maps_flutter/google_maps_flutter/ios/Classes/google_maps_flutter-umbrella.h
+++ b/packages/google_maps_flutter/google_maps_flutter/ios/Classes/google_maps_flutter-umbrella.h
@@ -3,8 +3,8 @@
 // found in the LICENSE file.
 
 #import <Foundation/Foundation.h>
-#import <google_maps_flutter/FLTGoogleMapsPlugin.h>
 #import <google_maps_flutter/FLTGoogleMapTileOverlayController.h>
+#import <google_maps_flutter/FLTGoogleMapsPlugin.h>
 #import <google_maps_flutter/JsonConversions.h>
 
 FOUNDATION_EXPORT double google_maps_flutterVersionNumber;

DONE

via-guy · 2022-03-20T08:11:34Z

@jmagman done

stuartmorgan-g

LGTM!

…change only once (flutter/plugins#3426)

…y once (flutter#3426)

guykogus requested a review from cyanglaz as a code owner January 14, 2021 23:22

google-cla bot added the cla: yes label Jan 14, 2021

stuartmorgan-g added the p: google_maps_flutter label Jan 29, 2021

github-actions bot added the platform-ios label Mar 9, 2021

pawlowskim mentioned this pull request Mar 9, 2021

App crash on Google Map layer change (iOS) flutter/flutter#40650

Closed

samderlust mentioned this pull request Mar 9, 2021

[google_maps_flutter] Flutter app crash on iPadOS EXC_BAD_ACCESS (code=2, address=0x16da77ff8) flutter/flutter#72821

Closed

cyanglaz reviewed Apr 29, 2021

View reviewed changes

stuartmorgan-g added the needs tests label Jul 26, 2021

stuartmorgan-g suggested changes Oct 5, 2021

View reviewed changes

godofredoc changed the base branch from master to main January 6, 2022 22:57

stuartmorgan-g suggested changes Feb 14, 2022

View reviewed changes

stuartmorgan-g suggested changes Feb 28, 2022

View reviewed changes

stuartmorgan-g suggested changes Mar 7, 2022

View reviewed changes

guykogus added 19 commits March 20, 2022 10:10

Fix observing frame change only once

1f3c635

Update pubspec and CHANGELOG

2ff340f

Remove hacky flags

6a25978

Add tests

94456f9

Update CHANGELOG

416cf25

Fix formatting

9bca4e0

Add missing licence

9f483b1

Create mock map view to test counting the frame observers

6ec103d

Clean code

cb7f8f4

Create modulemap and move test class

f74a95a

PR commits

0529ae2

Fix formatting

672bdbd

Add comment

5a9477a

Clean comment

a9cc1b4

Fix CHANGELOG

95f8ca3

PR fixes

dbbc976

Add JsonConversions.h to plugin header

f2d83f3

Move headers to umbrella

9c39b5e

Format

fe83a32

stuartmorgan-g approved these changes Mar 21, 2022

View reviewed changes

stuartmorgan-g added waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. and removed needs tests labels Mar 21, 2022

fluttergithubbot merged commit 5b49d0f into flutter:main Mar 21, 2022

engine-flutter-autoroll mentioned this pull request Mar 22, 2022

Roll Plugins from 1c61a217014c to 3069bc3b8410 (4 revisions) flutter/flutter#100559

Merged

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

5b49d0f76 [google_maps_flutter] Fix iOS crash by observing map frame …

95cb6d7

…change only once (flutter/plugins#3426)

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[google_maps_flutter] Fix iOS crash by observing map frame change onl…

b72e15c

…y once (flutter#3426)


		NS_ASSUME_NONNULL_BEGIN

		@interface MockRegistrar : NSObject <FlutterPluginRegistrar>


		@interface FLTGoogleMapController (Test)

		- (instancetype)initWithMapView:(GMSMapView *)mapView


		@interface MockGMSMapView : GMSMapView

		@property(nonatomic, assign, readonly) NSInteger frameObserverCount;


		@import GoogleMaps;

		// Defines a map view used for testing key-value observing.

[google_maps_flutter] Fix iOS crash by observing map frame change only once #3426

[google_maps_flutter] Fix iOS crash by observing map frame change only once #3426

Conversation

guykogus commented Jan 14, 2021

Pre-launch Checklist

MarceLifeBites commented Jan 20, 2021

pawlowskim commented Mar 9, 2021

guykogus commented Mar 9, 2021

pawlowskim commented Mar 9, 2021

guykogus commented Mar 9, 2021 • edited Loading

sangTroo commented Mar 9, 2021 • edited Loading

carlosmobile commented Mar 9, 2021

cyanglaz left a comment

Choose a reason for hiding this comment

via-guy commented Apr 29, 2021

sangTroo commented Jun 16, 2021

sangTroo commented Jun 16, 2021

stuartmorgan-g left a comment

Choose a reason for hiding this comment

via-guy commented Oct 6, 2021

stuartmorgan-g commented Oct 25, 2021 • edited Loading

flutter-dashboard bot commented Jan 6, 2022

stuartmorgan-g commented Jan 26, 2022

guykogus commented Jan 26, 2022

via-guy commented Feb 14, 2022

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

via-guy commented Feb 14, 2022

stuartmorgan-g commented Feb 28, 2022

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

Choose a reason for hiding this comment

via-guy commented Mar 8, 2022

jmagman commented Mar 18, 2022

via-guy commented Mar 20, 2022

stuartmorgan-g left a comment

Choose a reason for hiding this comment

guykogus commented Mar 9, 2021 •

edited

Loading

sangTroo commented Mar 9, 2021 •

edited

Loading

stuartmorgan-g commented Oct 25, 2021 •

edited

Loading