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

Don't invoke nativeRender when surface has been destroyed #14821

Closed
wants to merge 2 commits into from

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Jun 3, 2019

Closes #14592
Closes #14252
Closes #14463

This PR fixes a native crash when an app is being backgrounded and we were still scheduling renders (the mapview surface has been destroyed). Concrete steps to repriduce:

  • schedule a render on the main thread
  • the surface gets destroyed on main thread
  • we run nativeRender on render thread while the surface was already destroyed on main thread

Additionally to fixing the crash, I'm committing a configuration that allows to use a release build of the testapp as a debug build.

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Jun 3, 2019
@tobrun tobrun added this to the release-oolong milestone Jun 3, 2019
@tobrun tobrun requested a review from LukasPaczos June 3, 2019 11:59
@tobrun tobrun self-assigned this Jun 3, 2019
@LukasPaczos
Copy link
Contributor

This is a great find!
It does fix the issue reproduced by the test, but I still have my doubts about the use of the hasSurface flag. Looking through the code again I think that we should use a lock/mutex on the hasSurface variable instead of using an AtomicBoolean.

The atomicity of the variable doesn't completely guard us when the surface is destroyed after the if-check in the onDrawFrame and before we call/during the call to nativeRender.

Do you agree? If so, do you think we could incorporate those changes here?

Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

GLSurfaceViewMapRenderer#onSurfaceCreated, GLSurfaceViewMapRenderer#onSurfaceDestroyed and GLSurfaceViewMapRenderer#onSurfaceChanged should also be synchronised.

@tobrun
Copy link
Member Author

tobrun commented Jun 4, 2019

GLSurfaceViewMapRenderer#onSurfaceCreated, GLSurfaceViewMapRenderer#onSurfaceDestroyed and GLSurfaceViewMapRenderer#onSurfaceChanged should also be synchronised.

Added those in 0f877c9 though the flag that we are guarding isn't set or get in those blocks. I'm actually not seeing why we would need them. Could you add an example use-case where it would fail without synchronization of those blocks?

@tobrun
Copy link
Member Author

tobrun commented Jun 4, 2019

obsolete with changes made in #14841

@tobrun tobrun closed this Jun 4, 2019
@tobrun tobrun deleted the tvn-sigsegv11 branch June 25, 2019 11:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
2 participants