Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core][Darwin] SDK objects should hold weak pointers to the core style objects #15539

Merged
merged 16 commits into from
Sep 10, 2019

Conversation

pozdnyakov
Copy link
Contributor

@pozdnyakov pozdnyakov commented Sep 2, 2019

Thus, we can avoid crashes caused by dangling raw{Source|Layer} pointer after new style is parsed.

Fixes #15333

  • Add integration test

@pozdnyakov pozdnyakov force-pushed the mikhail_weak_pointer_for_style branch 2 times, most recently from d08776d to 5a76003 Compare September 4, 2019 12:41
@pozdnyakov pozdnyakov self-assigned this Sep 4, 2019
@pozdnyakov pozdnyakov added bug Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS labels Sep 4, 2019
@pozdnyakov pozdnyakov marked this pull request as ready for review September 4, 2019 14:28
@pozdnyakov pozdnyakov requested a review from a team September 4, 2019 14:28
Copy link
Contributor

@julianrex julianrex left a comment

Choose a reason for hiding this comment

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

Looks good - a few minor comments. I will test as we discussed.

We should update

-[MGLImageSource description]
-[MGLShapeSource description]

as they could trigger the exception, which we don't want in this case.

platform/darwin/src/MGLSource_Private.h Show resolved Hide resolved
platform/darwin/src/MGLSource_Private.h Outdated Show resolved Hide resolved
platform/darwin/src/MGLStyleLayer_Private.h Outdated Show resolved Hide resolved
platform/darwin/src/MGLSource.mm Show resolved Hide resolved
@pozdnyakov
Copy link
Contributor Author

@julianrex Thanks for your comments! considered in the latest commit.

Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

lgtm! for core changes.

@@ -6,6 +6,8 @@
#include <mbgl/style/types.hpp>
#include <mbgl/style/conversion.hpp>

#include <mapbox/weak.hpp>

#include <cassert>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit unrelated to this PR: looks like cassert and stdexcept can be removed

@julianrex julianrex force-pushed the mikhail_weak_pointer_for_style branch from 66382ce to a804570 Compare September 8, 2019 02:17
@julianrex julianrex added the needs changelog Indicates PR needs a changelog entry prior to merging. label Sep 8, 2019
@julianrex
Copy link
Contributor

@pozdnyakov FYI updated with some tweaks, and an additional test.

@julianrex julianrex requested review from captainbarbosa and removed request for 1ec5 September 9, 2019 03:56
@julianrex julianrex removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Sep 9, 2019
@julianrex
Copy link
Contributor

@captainbarbosa can you review the ios/macos bits please?

@julianrex julianrex added this to the release-ristretto milestone Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ios] Crashes caused by dangling rawSource pointer after new style is parsed.
4 participants