Keep strong references to the FileSource/OfflineManager operations callbacks #14601

LukasPaczos · 2019-05-07T11:51:22Z

Fixes #14485 and fixes #14297.
Refs #14589 (comment).

This PR aims to remove weak references overuse during the offline database merge and resources path change operations. This could lead to the callback reference during any step of the process to be garbage collected if not kept on the client side and result in a no-op.

The initial idea of keeping weak references to the callbacks was to ensure that we are not outliving the hosting activity, which might've been passed with the callback reference. However, the callback is not required to be depending on the hosting activity and it can (and should) be wrapped in a weak reference on the client side if that's the case, which is out of our control.

In any way, debugging why the callback has outlived the hosting object is much easier than looking for a cause of a no-op, which is why I believe that this change is a general improvement.

TODO:

offline merge
resources cache path change
~~deprecate FileUtils methods~~ opted to keep them around

This is an initial proposal, please let me know what you think @Guardiola31337 @tobrun and I can move forward with the remaining to-do items if you think the approach is correct.

LukasPaczos · 2019-05-07T11:52:26Z

...rm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java

@@ -225,73 +229,50 @@ public void run() {
   */
  public void mergeOfflineRegions(@NonNull String path, @NonNull final MergeOfflineRegionsCallback callback) {
    final File src = new File(path);
-    new FileUtils.CheckFileReadPermissionTask(new FileUtils.OnCheckFileReadPermissionListener() {


I'm happy to keep the async tasks and convert them to use strong references, but I think that proposed, streamlined setup is actually easier to read.

pakoito · 2019-05-07T19:02:49Z

...p/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/MergeOfflineRegionsActivity.kt

+    private val activityWeakReference = WeakReference<OfflineManager.MergeOfflineRegionsCallback>(activityCallback)
+
+    override fun onMerge(offlineRegions: Array<out OfflineRegion>?) {
+      activityWeakReference.get()?.onMerge(offlineRegions)


What happens if this callback is called between onDestroy and when the activity is cleared from memory? It could be a couple of seconds until GC kicks in and releases the activity.

During this time you are trying to set styles on the map but the UI will not allow it, and it could lead to crashes like the classic Can not perform this action after onSaveInstanceState.

That mergeDb is also suspicious because in turn it calls mapView.getMapAsync.

Thanks @pakoito. In this case, since the callback interacts with the map, it's definitely cleaner to make sure to release the reference proactively in the #onDestroy instead of waiting for the GC.

That's correct @pakoito thanks for jumping in here and share your feedback! Really appreciate it 🙏

I like this approach! Although it'd mean some extra work (as discussed in #14589 (comment)), I think it's the only option that we could implement without using a framework that handles / solves these problems for you.

Let's make sure that all the nested callbacks and objects are also cleaned up when in onActivityDestroy (we'd also need to change the naming so it's not attached to activities) so we don't leak anything and we use application contexts (as we're working with the file system and the handler it's on the UI queue).

@Guardiola31337 this code references the example activity, so I don't think we need to make the naming universal. Also, I'm not seeing anything else we should release, or am I missing something?

this code references the example activity, so I don't think we need to make the naming universal

Yeap, you're right. Sorry, I overlooked it 😬

Also, I'm not seeing anything else we should release, or am I missing something?

I'm referring to mergeOfflineDatabaseFiles which creates MergeOfflineRegionsCallback internally

mapbox-gl-native/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java

Line 305 in 9a6eb2f

mergeOfflineRegions(fileSource, file.getAbsolutePath(), new MergeOfflineRegionsCallback() {

Wanted to make sure that there's no option to leak and we've tested (if you have already forget about it 😅).

Thanks for taking a look at this @LukasPaczos 🙏

Gotcha, np! If there are no other comments/concerns I'm going to go ahead and start the work on the remaining to-do items.

Gotcha, np! If there are no other comments/concerns I'm going to go ahead and start the work on the remaining to-do items.

Sounds good to me @LukasPaczos I don't know how this thread got out of order 🤔 almost missed ☝️ comment 😅

Github had a hard day yesterday, all is in order now :)

pakoito · 2019-05-08T11:31:22Z

...p/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/MergeOfflineRegionsActivity.kt

@@ -112,6 +133,7 @@ class MergeOfflineRegionsActivity : AppCompatActivity() {

  override fun onDestroy() {
    super.onDestroy()
+    mergeCallback.onActivityDestroy()


tobrun · 2019-05-09T06:42:48Z

...p/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/MergeOfflineRegionsActivity.kt

+   * Since we expect from the results of the offline merge callback to interact with the hosting activity,
+   * we need to ensure that we are not interacting with a destroyed activity.
+   */
+  private class MergeCallback(private var activityCallback: OfflineManager.MergeOfflineRegionsCallback?) : OfflineManager.MergeOfflineRegionsCallback {


Why not use a weak reference here? this would avoid the onActivityDestroy setup

#14601 (comment)

WeakReference causes races with lifecycle events. It does prevents leaks, that's good, and also keeps a whole category of potential errors caused by calling UI methods or trying to start activities or change fragments after onDestroy.

… reference

…during cache path change

LukasPaczos · 2019-05-09T13:15:42Z

This one's ready for a review.

Guardiola31337

Overall, this looks good to me 👍 Well done @LukasPaczos

I left a couple of minor comments (not blocking the PR though)

Guardiola31337 · 2019-05-09T14:15:18Z

...rm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java

@@ -205,6 +207,8 @@ public void run() {
   * Merge offline regions from a secondary database into the main offline database.
   * <p>
   * When the merge is completed, or fails, the {@link MergeOfflineRegionsCallback} will be invoked on the main thread.
+   * The callback reference is <b>strongly kept</b> throughout the process,
+   * so it needs to be wrapped in a weak reference or released on the client side if necessary.


Do we want to "recommend" to wrap it into a WeakReference after 👀 the issues that may cause https://github.com/mapbox/mapbox-gl-native/pull/14601/files#r282389769?

wrapped in a weak reference or released

Let's leave this up to the discretion of the implementation needs 🙃

Guardiola31337 · 2019-05-09T14:42:08Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/utils/FileUtils.java

-      if (listener != null) {
-        listener.onError();
-      }
+      listener.onError();


What do you think about the approach of using a local callback as shown in #14589? e.g.

mapbox-gl-native/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/utils/FileUtils.java

Lines 36 to 37 in ba73e50

OnCheckFileReadPermissionListener localListener = listener;

listener = null;

That way the callback is released as soon as possible no matter what is done afterwards in onError.

🤔

Does it make any real difference @Guardiola31337? The object is going to be viable to GC as soon as it goes out of any thread's scope, which is basically the same moment in time as this method returns.

If the listener only needs to be consumed once that would prevent potential leaks (e.g. in the rare scenario in which the onCancelled and onPostExecute are called simultaneously or if the code changes in the future) and also it wouldn't hurt 😅

Anyways, saw that the PR was merged ¯\_(ツ)_/¯

Thanks @LukasPaczos

LukasPaczos added the Android Mapbox Maps SDK for Android label May 7, 2019

LukasPaczos requested review from Guardiola31337 and tobrun May 7, 2019 11:51

LukasPaczos commented May 7, 2019

View reviewed changes

LukasPaczos force-pushed the lp-keep-strong-callback-references branch from 57540dc to fd41ebb Compare May 7, 2019 13:19

pakoito reviewed May 7, 2019

View reviewed changes

LukasPaczos force-pushed the lp-keep-strong-callback-references branch 2 times, most recently from a824f75 to 9a6eb2f Compare May 8, 2019 10:57

pakoito reviewed May 8, 2019

View reviewed changes

tobrun reviewed May 9, 2019

View reviewed changes

LukasPaczos added 3 commits May 9, 2019 15:09

[android] use strong references to the offline merge callbacks

01f3fb4

[android] FileUtils read/write permission check tasks retain a strong…

8cd56c7

… reference

[android] keep strong callback reference and use application context …

eba2f25

…during cache path change

LukasPaczos force-pushed the lp-keep-strong-callback-references branch from 9a6eb2f to eba2f25 Compare May 9, 2019 13:10

LukasPaczos marked this pull request as ready for review May 9, 2019 13:11

LukasPaczos added this to the release-nectar milestone May 9, 2019

Guardiola31337 approved these changes May 9, 2019

View reviewed changes

LukasPaczos merged commit 0f599ce into master May 10, 2019

LukasPaczos deleted the lp-keep-strong-callback-references branch May 10, 2019 12:15

LukasPaczos mentioned this pull request May 10, 2019

[android] Fix intermittent GC of offline callbacks #14589

Closed

Guardiola31337 mentioned this pull request May 10, 2019

Add option to load offline maps database for NavigationView mapbox/mapbox-navigation-android#1895

Merged

6 tasks

LukasPaczos mentioned this pull request May 14, 2019

Backport #14601 to mojito #14666

Merged

LukasPaczos modified the milestones: release-nectar, release-mojito May 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep strong references to the FileSource/OfflineManager operations callbacks #14601

Keep strong references to the FileSource/OfflineManager operations callbacks #14601

LukasPaczos commented May 7, 2019 •

edited

Loading

LukasPaczos May 7, 2019

pakoito May 7, 2019 •

edited

Loading

LukasPaczos May 8, 2019

Guardiola31337 May 8, 2019

LukasPaczos May 8, 2019

Guardiola31337 May 8, 2019

LukasPaczos May 8, 2019

Guardiola31337 May 8, 2019

LukasPaczos May 9, 2019

pakoito May 8, 2019

tobrun May 9, 2019 •

edited

Loading

pakoito May 9, 2019 •

edited

Loading

LukasPaczos commented May 9, 2019

Guardiola31337 left a comment

Guardiola31337 May 9, 2019

LukasPaczos May 10, 2019

Guardiola31337 May 9, 2019

LukasPaczos May 9, 2019

Guardiola31337 May 10, 2019

	OnCheckFileReadPermissionListener localListener = listener;
	listener = null;

Keep strong references to the FileSource/OfflineManager operations callbacks #14601

Keep strong references to the FileSource/OfflineManager operations callbacks #14601

Conversation

LukasPaczos commented May 7, 2019 • edited Loading

Choose a reason for hiding this comment

pakoito May 7, 2019 • edited Loading

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

tobrun May 9, 2019 • edited Loading

Choose a reason for hiding this comment

pakoito May 9, 2019 • edited Loading

Choose a reason for hiding this comment

LukasPaczos commented May 9, 2019

Guardiola31337 left a 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

LukasPaczos commented May 7, 2019 •

edited

Loading

pakoito May 7, 2019 •

edited

Loading

tobrun May 9, 2019 •

edited

Loading

pakoito May 9, 2019 •

edited

Loading