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

[core] Fix layer impl cast in render layer implementations #15398

Merged
merged 2 commits into from
Aug 16, 2019
Merged

[core] Fix layer impl cast in render layer implementations #15398

merged 2 commits into from
Aug 16, 2019

Conversation

pozdnyakov
Copy link
Contributor

This PR puts impl() functions definitions into a nameless namespace
to provide internal linkage and to make sure that appropriate cast function
is invoked.

The problem was discovered by sanitize bot https://circleci.com/gh/mapbox/mapbox-gl-native/316959

@pozdnyakov pozdnyakov self-assigned this Aug 16, 2019
@pozdnyakov pozdnyakov added bug Core The cross-platform C++ core, aka mbgl labels Aug 16, 2019
@tmpsantos tmpsantos added the needs backport Indicates PR needs to be cherrypicked into a previous release branch. label Aug 16, 2019
@tmpsantos
Copy link
Contributor

/cc @zugaldia @chloekraw @julianrex @LukasPaczos this needs backporting, should be a straightforward cherry-pick

@pozdnyakov
[core] Fix layer impl cast in render layer implementations
9e236a9 
This PR puts `impl()` functions definitions into a nameless namespace
to provide internal linkage and to make sure that appropriate cast function
is invoked.
@chloekraw
Copy link
Contributor

@tmpsantos @pozdnyakov, thanks. Please add a changelog entry and request me for review on this changelog entry before merging.

@chloekraw chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 16, 2019
@pozdnyakov pozdnyakov requested review from a team and chloekraw August 16, 2019 14:18
@pozdnyakov
Copy link
Contributor Author

@tmpsantos @pozdnyakov, thanks. Please add a changelog entry and request me for review on this changelog entry before merging.

Done in the latest commit, please take a look.

chloekraw
chloekraw reviewed Aug 16, 2019
View reviewed changes
Copy link
Contributor

@chloekraw chloekraw left a comment

Choose a reason for hiding this comment

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

@pozdnyakov thanks! comments are in

platform/android/CHANGELOG.md Outdated
@@ -10,6 +10,7 @@ Mapbox welcomes participation and contributions from everyone. If you'd like to
- Add fallback support to local ideograph font families [#15255](https://github.com/mapbox/mapbox-gl-native/pull/15255)

### Bug fixes
- Fixed possible crash caused by invoking of the wrong cast layer implementation function [#15398](https://github.com/mapbox/mapbox-gl-native/pull/15398).
Copy link
Contributor

Choose a reason for hiding this comment

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

add "a" before "possible crash"

Copy link
Contributor

Choose a reason for hiding this comment

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

since the crash is only "possible", the verb clause should be "that could be caused by"

Copy link
Contributor

Choose a reason for hiding this comment

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

delete "of" --> "invoking the wrong"

Copy link
Contributor

@chloekraw chloekraw Aug 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think the last four words sound better as: "layer casting implementation function." but let me know if this is incorrect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

layer implementation casting function - it casts Layer::Impl instances.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah, that’s perfect, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

platform/ios/CHANGELOG.md Outdated
@@ -19,6 +19,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT

### Other changes

* Fixed possible crash caused by invoking of the wrong cast layer implementation function. ([#15398](https://github.com/mapbox/mapbox-gl-native/pull/15398))
Copy link
Contributor

Choose a reason for hiding this comment

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

same comments as in android changelog

@chloekraw chloekraw removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 16, 2019
@pozdnyakov pozdnyakov merged commit 13fd3ad into master Aug 16, 2019
@pozdnyakov pozdnyakov deleted the mikhail_fix_layer_impl_cast branch August 16, 2019 15:33
LukasPaczos added a commit that referenced this pull request Aug 16, 2019
@LukasPaczos
[android] move #15398 out of v8.3.0-alpha.3 changelog
33c3d16
@LukasPaczos LukasPaczos mentioned this pull request Aug 16, 2019
Merged
chloekraw pushed a commit that referenced this pull request Aug 16, 2019
@LukasPaczos @chloekraw
[android] move #15398 out of v8.3.0-alpha.3 changelog (#15401)
98dbed2
@chloekraw
Copy link
Contributor

chloekraw commented Aug 18, 2019 •
edited
Loading

this needs backporting, should be a straightforward cherry-pick

@pozdnyakov @tmpsantos could you clarify, which commit was this regression introduced in and which releases require a backport?

@pozdnyakov
Copy link
Contributor Author

@pozdnyakov @tmpsantos could you clarify, which commit was this regression introduced in and which releases require a backport?

It's 4b21560 . Also, the issue would affect only debug build configurations, where these casting functions do not get inlined.

@LukasPaczos
Copy link
Member

Also, the issue would affect only debug build configurations, where these casting functions do not get inlined.

On Android, we're shipping only release builds of GL-native in public artifacts so this wouldn't need patching if I understand correctly.

@pozdnyakov
Copy link
Contributor Author

On Android, we're shipping only release builds of GL-native in public artifacts so this wouldn't need patching if I understand correctly.

IMO still better to patch, in case someone is building SDK from source and with modified build flags.

@LukasPaczos
Copy link
Member

This wouldn't require patch releases though, only CPs to release branches, right?

@LukasPaczos
Copy link
Member

Capturing from a chat with @pozdnyakov, we tested arm-v7, arm-v8 and x86 builds and it looks like release builds that we publish are stable and do not crash. Therefore, we might not need to push additional patch releases, only CP the fix to release branches for developers that build from source.

That said, I'll let you @pozdnyakov @chloekraw to make the final call. If we'd like to be super-safe, we can schedule the patch releases as well.

/cc @tobrun

@pozdnyakov
Copy link
Contributor Author

Here is the branch, reproducing the issue https://github.com/mapbox/mapbox-gl-native/tree/mikhail_reproduce_layer_impl_cast_problem

@friedbunny friedbunny added this to the release-queso milestone Aug 19, 2019
@LukasPaczos LukasPaczos mentioned this pull request Aug 23, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tmpsantos tmpsantos tmpsantos approved these changes

@chloekraw chloekraw chloekraw approved these changes

Assignees

@pozdnyakov pozdnyakov

Labels
bug Core The cross-platform C++ core, aka mbgl needs backport Indicates PR needs to be cherrypicked into a previous release branch.
Projects
None yet
Milestone
release-queso
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pozdnyakov @tmpsantos @chloekraw @LukasPaczos @friedbunny