Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash on iOS Apple Maps when loading really large polylines #4468

Merged

Conversation

ascherkus
Copy link
Contributor

Switch to using heap allocation to prevent corrupting the stack.

Does any other open PR do the same thing?

No.

What issue is this PR fixing?

No issue filed -- discovered through our own crash monitoring software.

How did you test this PR?

With iOS and Apple Maps load a <Polyline> with ~60,000 coordinates and test on a real device (does not seem to crash on iOS Simulator) and deploy / launch debugger using Xcode.

Set a breakpoint in AIRMapPolyline.m on setCoordinates().

As soon as the CLLocationCoordinate2D coords array is allocated continuing execution raises a EXC_BAD_ACCESS since the stack has been corrupted.

Switching to heap allocating the temporary coords array allows the polyline to render.

Does not affect Android / Google Maps as that code uses GMSMutablePath and calls addCoordinate in a loop (which presumably is heap allocated under the hood).

bhishaksanyal
bhishaksanyal previously approved these changes Sep 30, 2022
@hamaron
Copy link

hamaron commented Oct 5, 2022

This is a very good catch. Good job, @ascherkus! I hope this PR gets merged soon.

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Dec 5, 2022
@ascherkus
Copy link
Contributor Author

Can someone with write access take a look at this PR?

@github-actions github-actions bot removed the stale label Dec 6, 2022
@ascherkus ascherkus force-pushed the fix-ios-apple-maps-polyline-crash branch from 6eb67d5 to 67b476f Compare January 18, 2023 16:53
@ascherkus
Copy link
Contributor Author

Rebased my PR on top of latest branch -- would love to get this into the next release if possible. Let me know what's missing or what else I should look at to get this merged. Thanks!

Copy link
Contributor

@the-habu the-habu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice optimization.

@Simon-TechForm I can't really vote for approval, as I am no (objective) c expert, but to upvote.
Looks fine to someone who worked a little bit with C and objective C.

bhishaksanyal
bhishaksanyal previously approved these changes Feb 1, 2023
@the-habu
Copy link
Contributor

the-habu commented Mar 4, 2023

@ascherkus any chance we can get that PR rolling again?

Switch to using heap allocation to prevent corrupting the stack.
@ascherkus ascherkus force-pushed the fix-ios-apple-maps-polyline-crash branch from 67b476f to 807106a Compare March 4, 2023 19:54
@ascherkus
Copy link
Contributor Author

@ascherkus any chance we can get that PR rolling again?

Not sure what else to do here -- it's rebased, reviewed, but I can't merge as it's missing one approving review.

@monholm monholm changed the base branch from master to beta April 15, 2023 21:42
@monholm
Copy link
Collaborator

monholm commented Apr 15, 2023

Hello @ascherkus.

I sincerely apologize for the late reponse to this pr. The very limited time I've had to spend on this project was put towards migrating to the new react-native arch.

I'll test out the proposed changes immediately.

@monholm monholm merged commit e48e1af into react-native-maps:beta Apr 16, 2023
1 check passed
react-native-maps-bot pushed a commit that referenced this pull request Apr 16, 2023
## [1.5.1-beta.2](v1.5.1-beta.1...v1.5.1-beta.2) (2023-04-16)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](#4468)) ([e48e1af](e48e1af))
@react-native-maps-bot
Copy link
Collaborator

🎉 This PR is included in version 1.5.1-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

react-native-maps-bot pushed a commit that referenced this pull request Apr 20, 2023
# [1.6.0](v1.5.0...v1.6.0) (2023-04-20)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](#4468)) ([e48e1af](e48e1af))
* **ios:** support for use_frameworks! :linkage => :static ([b0c2d42](b0c2d42))
* **marker:** remove spamming warning from MapMarker ([#4644](#4644)) ([8825312](8825312)), closes [#4536](#4536)
* **types:** missing PolygonPressEvent type export ([#4410](#4410)) ([d3557a3](d3557a3))

### Features

* enable npm provenance ([#4686](#4686)) ([3498c3f](3498c3f))
* **ios:** bump googlemaps to 7.4.0 ([#4679](#4679)) ([661cddb](661cddb))
@react-native-maps-bot
Copy link
Collaborator

🎉 This PR is included in version 1.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

jijoy added a commit to Deepfleet/react-native-maps that referenced this pull request Aug 31, 2023
* chore(deps): bump dev deps

* chore(android): rename android package

* chore(android): remove redundant class prefix

* chore(android): cleanup MapsPackage

* chore(android): bump compileSdkVersion and targetSdkVersion to 31

* feat(android): pin androidx.work version

Starting this november, all android apps (including updates) will require targetSdkVersion 31
and as a result compileSdkVersion 31, therefore not marking as a breaking change

* ci(pullRequest): stop linting pr titles

* chore(release): 1.4.0-beta.1 [skip ci]

# [1.4.0-beta.1](react-native-maps/react-native-maps@v1.3.2...v1.4.0-beta.1) (2022-10-10)

### Features

* **android:** pin androidx.work version ([73f21c7](react-native-maps@73f21c7))

* ci(pullRequest): stop linting pr title

* chore(deps): bump dev deps

* ci(pullRequest): check formatting

* style(prettier): fix formatting

* chore(husky): check formatting on pre-commit

* build(deps): bump @sideway/formula from 3.0.0 to 3.0.1 in /example

Bumps [@sideway/formula](https://github.com/sideway/formula) from 3.0.0 to 3.0.1.
- [Release notes](https://github.com/sideway/formula/releases)
- [Commits](hapijs/formula@v3.0.0...v3.0.1)

---
updated-dependencies:
- dependency-name: "@sideway/formula"
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* chore(deps): bump dev-deps

* chore(release): 1.5.0-beta.1 [skip ci]

# [1.5.0-beta.1](react-native-maps/react-native-maps@v1.4.0...v1.5.0-beta.1) (2023-04-15)

### Features

* **android:** pin androidx.work version ([73f21c7](react-native-maps@73f21c7))

* chore(release): 1.5.0 [skip ci]

# [1.5.0](react-native-maps/react-native-maps@v1.4.0...v1.5.0) (2023-04-15)

### Features

* **android:** pin androidx.work version ([73f21c7](react-native-maps@73f21c7))

* chore(example): bump to react-native 0.70.8 (react-native-maps#4673)

* fix(types): missing PolygonPressEvent type export (react-native-maps#4410)

* chore(release): 1.5.1-beta.1 [skip ci]

## [1.5.1-beta.1](react-native-maps/react-native-maps@v1.5.0...v1.5.1-beta.1) (2023-04-16)

### Bug Fixes

* **types:** missing PolygonPressEvent type export ([react-native-maps#4410](react-native-maps#4410)) ([d3557a3](react-native-maps@d3557a3))

* fix(ios): crash on Apple Maps when loading large polylines (react-native-maps#4468)

Switch to using heap allocation to prevent corrupting the stack.

* chore(release): 1.5.1-beta.2 [skip ci]

## [1.5.1-beta.2](react-native-maps/react-native-maps@v1.5.1-beta.1...v1.5.1-beta.2) (2023-04-16)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([react-native-maps#4468](react-native-maps#4468)) ([e48e1af](react-native-maps@e48e1af))

* chore: stop updating version in package.json

Updating the version requires the Podfile.lock to update after each release.

* chore(example): use ruby bundler

* chore(example): update gemfile.lock

* test(detox): add basic setup

* chore(example): default to using patched Google-Maps-iOS-Utils

* ci: add build workflow for android and ios

* ci: rename build jobs

* ci: build ios using frameworks

* ci(stale): specify permissions

* fix(ios): support for use_frameworks! :linkage => :static

react-native-google-maps relies on headers from react-native-maps.
When using frameworks, headers from react-native-maps are no longer found, as the framework search
path for react-native-maps isn't added to the react-native-google-maps framework.

Likewise, removing the import prefix from Google-Maps-iOS-Utils resolves the imports
both with and without use_frameworks.

Note: this only brings support for `use_frameworks!` with `:linkage => :static.

* chore: rename example app

* chore(example): migrate to recommended api key setup

* ci: add placeholder google maps api key

* ci: fix xcworkspace name

* chore(release): 1.5.1-beta.3 [skip ci]

## [1.5.1-beta.3](react-native-maps/react-native-maps@v1.5.1-beta.2...v1.5.1-beta.3) (2023-04-18)

### Bug Fixes

* **ios:** support for use_frameworks! :linkage => :static ([b0c2d42](react-native-maps@b0c2d42))

* feat(ios): bump googlemaps to 7.4.0 (react-native-maps#4679)

According to the release notes, this version is functionally identical to v.7.3.0

* chore(release): 1.6.0-beta.1 [skip ci]

# [1.6.0-beta.1](react-native-maps/react-native-maps@v1.5.1-beta.3...v1.6.0-beta.1) (2023-04-18)

### Features

* **ios:** bump googlemaps to 7.4.0 ([react-native-maps#4679](react-native-maps#4679)) ([661cddb](react-native-maps@661cddb))

* fix(marker): remove spamming warning from MapMarker (react-native-maps#4644)

Resolves react-native-maps#4536

* chore(release): 1.6.0-beta.2 [skip ci]

# [1.6.0-beta.2](react-native-maps/react-native-maps@v1.6.0-beta.1...v1.6.0-beta.2) (2023-04-18)

### Bug Fixes

* **marker:** remove spamming warning from MapMarker ([react-native-maps#4644](react-native-maps#4644)) ([8825312](react-native-maps@8825312)), closes [react-native-maps#4536](react-native-maps#4536)

* ci: make reproducible exempt (react-native-maps#4681)

* feat: enable npm provenance (react-native-maps#4686)

* chore(release): 1.6.0-beta.3 [skip ci]

# [1.6.0-beta.3](react-native-maps/react-native-maps@v1.6.0-beta.2...v1.6.0-beta.3) (2023-04-20)

### Features

* enable npm provenance ([react-native-maps#4686](react-native-maps#4686)) ([3498c3f](react-native-maps@3498c3f))

* ci(push): add id-token write permissions (react-native-maps#4687)

Needed for npm provenance

* chore(release): 1.6.0-beta.3 [skip ci]

# [1.6.0-beta.3](react-native-maps/react-native-maps@v1.6.0-beta.2...v1.6.0-beta.3) (2023-04-20)

### Features

* enable npm provenance ([react-native-maps#4686](react-native-maps#4686)) ([3498c3f](react-native-maps@3498c3f))

* chore: update security policy (react-native-maps#4688)

* chore(release): 1.6.0 [skip ci]

# [1.6.0](react-native-maps/react-native-maps@v1.5.0...v1.6.0) (2023-04-20)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([react-native-maps#4468](react-native-maps#4468)) ([e48e1af](react-native-maps@e48e1af))
* **ios:** support for use_frameworks! :linkage => :static ([b0c2d42](react-native-maps@b0c2d42))
* **marker:** remove spamming warning from MapMarker ([react-native-maps#4644](react-native-maps#4644)) ([8825312](react-native-maps@8825312)), closes [react-native-maps#4536](react-native-maps#4536)
* **types:** missing PolygonPressEvent type export ([react-native-maps#4410](react-native-maps#4410)) ([d3557a3](react-native-maps@d3557a3))

### Features

* enable npm provenance ([react-native-maps#4686](react-native-maps#4686)) ([3498c3f](react-native-maps@3498c3f))
* **ios:** bump googlemaps to 7.4.0 ([react-native-maps#4679](react-native-maps#4679)) ([661cddb](react-native-maps@661cddb))

* build(android): bump gradle to 7.2

7.2 adds support for java 17 which is now the default version bundled with android studio

* docs(readme): onRegionChangeComplete called infinitely (react-native-maps#4574)

* fix(ios): followsUserLocation changes zoom level (react-native-maps#4696)

Resolves react-native-maps#4585

* chore(release): 1.6.1-beta.1 [skip ci]

## [1.6.1-beta.1](react-native-maps/react-native-maps@v1.6.0...v1.6.1-beta.1) (2023-04-21)

### Bug Fixes

* **ios:** followsUserLocation changes zoom level ([react-native-maps#4696](react-native-maps#4696)) ([3b9491e](react-native-maps@3b9491e)), closes [react-native-maps#4585](react-native-maps#4585)

* chore(react-native): bump to 0.70.9 (react-native-maps#4697)

* feat(android): bump android-maps-utils to 3.4.0 (react-native-maps#4699)

* chore(release): 1.7.0-beta.1 [skip ci]

# [1.7.0-beta.1](react-native-maps/react-native-maps@v1.6.1-beta.1...v1.7.0-beta.1) (2023-04-21)

### Features

* **android:** bump android-maps-utils to 3.4.0 ([react-native-maps#4699](react-native-maps#4699)) ([6b26c23](react-native-maps@6b26c23))

* chore(prettier): ignore ios/DerivedData (react-native-maps#4702)

* chore(release): 1.7.0 [skip ci]

# [1.7.0](react-native-maps/react-native-maps@v1.6.0...v1.7.0) (2023-04-23)

### Bug Fixes

* **ios:** followsUserLocation changes zoom level ([react-native-maps#4696](react-native-maps#4696)) ([3b9491e](react-native-maps@3b9491e)), closes [react-native-maps#4585](react-native-maps#4585)

### Features

* **android:** bump android-maps-utils to 3.4.0 ([react-native-maps#4699](react-native-maps#4699)) ([6b26c23](react-native-maps@6b26c23))

* fix(android): crash when removing feature belonging to collection (react-native-maps#4707)

Resolves react-native-maps#4706

* chore(release): 1.7.1 [skip ci]

## [1.7.1](react-native-maps/react-native-maps@v1.7.0...v1.7.1) (2023-04-23)

### Bug Fixes

* **android:** crash when removing feature belonging to collection ([react-native-maps#4707](react-native-maps#4707)) ([ae6fe90](react-native-maps@ae6fe90)), closes [react-native-maps#4706](react-native-maps#4706)

* chore: merge

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Simon-TechForm <73996878+Simon-TechForm@users.noreply.github.com>
Co-authored-by: semantic-release-bot <semantic-release-bot@martynus.net>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Maxim Artyukhov <artukhov@fastmail.com>
Co-authored-by: Andrew Scherkus <ascherkus@gmail.com>
Co-authored-by: sharvilak11 <sharvilakt@gmail.com>
Co-authored-by: Bilal Abdeen <Bilal-Abdeen@users.noreply.github.com>
osk-serhii pushed a commit to osk-serhii/react-native-maps that referenced this pull request Oct 3, 2023
## [1.5.1-beta.2](react-native-maps/react-native-maps@v1.5.1-beta.1...v1.5.1-beta.2) (2023-04-16)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](react-native-maps/react-native-maps#4468)) ([e48e1af](react-native-maps/react-native-maps@e48e1af))
osk-serhii pushed a commit to osk-serhii/react-native-maps that referenced this pull request Oct 3, 2023
# [1.6.0](react-native-maps/react-native-maps@v1.5.0...v1.6.0) (2023-04-20)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](react-native-maps/react-native-maps#4468)) ([e48e1af](react-native-maps/react-native-maps@e48e1af))
* **ios:** support for use_frameworks! :linkage => :static ([b0c2d42](react-native-maps/react-native-maps@b0c2d42))
* **marker:** remove spamming warning from MapMarker ([#4644](react-native-maps/react-native-maps#4644)) ([8825312](react-native-maps/react-native-maps@8825312)), closes [#4536](react-native-maps/react-native-maps#4536)
* **types:** missing PolygonPressEvent type export ([#4410](react-native-maps/react-native-maps#4410)) ([d3557a3](react-native-maps/react-native-maps@d3557a3))

### Features

* enable npm provenance ([#4686](react-native-maps/react-native-maps#4686)) ([3498c3f](react-native-maps/react-native-maps@3498c3f))
* **ios:** bump googlemaps to 7.4.0 ([#4679](react-native-maps/react-native-maps#4679)) ([661cddb](react-native-maps/react-native-maps@661cddb))
GoldenDragon0710 added a commit to GoldenDragon0710/google-map-react-native that referenced this pull request Nov 27, 2023
## [1.5.1-beta.2](react-native-maps/react-native-maps@v1.5.1-beta.1...v1.5.1-beta.2) (2023-04-16)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](react-native-maps/react-native-maps#4468)) ([e48e1af](react-native-maps/react-native-maps@e48e1af))
GoldenDragon0710 added a commit to GoldenDragon0710/google-map-react-native that referenced this pull request Nov 27, 2023
# [1.6.0](react-native-maps/react-native-maps@v1.5.0...v1.6.0) (2023-04-20)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](react-native-maps/react-native-maps#4468)) ([e48e1af](react-native-maps/react-native-maps@e48e1af))
* **ios:** support for use_frameworks! :linkage => :static ([b0c2d42](react-native-maps/react-native-maps@b0c2d42))
* **marker:** remove spamming warning from MapMarker ([#4644](react-native-maps/react-native-maps#4644)) ([8825312](react-native-maps/react-native-maps@8825312)), closes [#4536](react-native-maps/react-native-maps#4536)
* **types:** missing PolygonPressEvent type export ([#4410](react-native-maps/react-native-maps#4410)) ([d3557a3](react-native-maps/react-native-maps@d3557a3))

### Features

* enable npm provenance ([#4686](react-native-maps/react-native-maps#4686)) ([3498c3f](react-native-maps/react-native-maps@3498c3f))
* **ios:** bump googlemaps to 7.4.0 ([#4679](react-native-maps/react-native-maps#4679)) ([661cddb](react-native-maps/react-native-maps@661cddb))
PainStaker0331 added a commit to PainStaker0331/react-native-maps that referenced this pull request Mar 3, 2024
## [1.5.1-beta.2](react-native-maps/react-native-maps@v1.5.1-beta.1...v1.5.1-beta.2) (2023-04-16)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](react-native-maps/react-native-maps#4468)) ([e48e1af](react-native-maps/react-native-maps@e48e1af))
PainStaker0331 added a commit to PainStaker0331/react-native-maps that referenced this pull request Mar 3, 2024
# [1.6.0](react-native-maps/react-native-maps@v1.5.0...v1.6.0) (2023-04-20)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](react-native-maps/react-native-maps#4468)) ([e48e1af](react-native-maps/react-native-maps@e48e1af))
* **ios:** support for use_frameworks! :linkage => :static ([b0c2d42](react-native-maps/react-native-maps@b0c2d42))
* **marker:** remove spamming warning from MapMarker ([#4644](react-native-maps/react-native-maps#4644)) ([8825312](react-native-maps/react-native-maps@8825312)), closes [#4536](react-native-maps/react-native-maps#4536)
* **types:** missing PolygonPressEvent type export ([#4410](react-native-maps/react-native-maps#4410)) ([d3557a3](react-native-maps/react-native-maps@d3557a3))

### Features

* enable npm provenance ([#4686](react-native-maps/react-native-maps#4686)) ([3498c3f](react-native-maps/react-native-maps@3498c3f))
* **ios:** bump googlemaps to 7.4.0 ([#4679](react-native-maps/react-native-maps#4679)) ([661cddb](react-native-maps/react-native-maps@661cddb))
Super-CodeKing added a commit to Super-CodeKing/react_native_map that referenced this pull request Apr 26, 2024
## [1.5.1-beta.2](react-native-maps/react-native-maps@v1.5.1-beta.1...v1.5.1-beta.2) (2023-04-16)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](react-native-maps/react-native-maps#4468)) ([e48e1af](react-native-maps/react-native-maps@e48e1af))
Super-CodeKing added a commit to Super-CodeKing/react_native_map that referenced this pull request Apr 26, 2024
# [1.6.0](react-native-maps/react-native-maps@v1.5.0...v1.6.0) (2023-04-20)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](react-native-maps/react-native-maps#4468)) ([e48e1af](react-native-maps/react-native-maps@e48e1af))
* **ios:** support for use_frameworks! :linkage => :static ([b0c2d42](react-native-maps/react-native-maps@b0c2d42))
* **marker:** remove spamming warning from MapMarker ([#4644](react-native-maps/react-native-maps#4644)) ([8825312](react-native-maps/react-native-maps@8825312)), closes [#4536](react-native-maps/react-native-maps#4536)
* **types:** missing PolygonPressEvent type export ([#4410](react-native-maps/react-native-maps#4410)) ([d3557a3](react-native-maps/react-native-maps@d3557a3))

### Features

* enable npm provenance ([#4686](react-native-maps/react-native-maps#4686)) ([3498c3f](react-native-maps/react-native-maps@3498c3f))
* **ios:** bump googlemaps to 7.4.0 ([#4679](react-native-maps/react-native-maps#4679)) ([661cddb](react-native-maps/react-native-maps@661cddb))
fairskyDev0201 pushed a commit to fairskyDev0201/react-native-maps that referenced this pull request Apr 29, 2024
## [1.5.1-beta.2](react-native-maps/react-native-maps@v1.5.1-beta.1...v1.5.1-beta.2) (2023-04-16)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](react-native-maps/react-native-maps#4468)) ([e48e1af](react-native-maps/react-native-maps@e48e1af))
fairskyDev0201 pushed a commit to fairskyDev0201/react-native-maps that referenced this pull request Apr 29, 2024
# [1.6.0](react-native-maps/react-native-maps@v1.5.0...v1.6.0) (2023-04-20)

### Bug Fixes

* **ios:** crash on Apple Maps when loading large polylines ([#4468](react-native-maps/react-native-maps#4468)) ([e48e1af](react-native-maps/react-native-maps@e48e1af))
* **ios:** support for use_frameworks! :linkage => :static ([b0c2d42](react-native-maps/react-native-maps@b0c2d42))
* **marker:** remove spamming warning from MapMarker ([#4644](react-native-maps/react-native-maps#4644)) ([8825312](react-native-maps/react-native-maps@8825312)), closes [#4536](react-native-maps/react-native-maps#4536)
* **types:** missing PolygonPressEvent type export ([#4410](react-native-maps/react-native-maps#4410)) ([d3557a3](react-native-maps/react-native-maps@d3557a3))

### Features

* enable npm provenance ([#4686](react-native-maps/react-native-maps#4686)) ([3498c3f](react-native-maps/react-native-maps@3498c3f))
* **ios:** bump googlemaps to 7.4.0 ([#4679](react-native-maps/react-native-maps#4679)) ([661cddb](react-native-maps/react-native-maps@661cddb))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants