From 022ec9863b36842db7db696ebd771e7cc9e4105b Mon Sep 17 00:00:00 2001 From: Miguel Beltran Date: Fri, 5 Apr 2024 11:41:30 +0200 Subject: [PATCH 1/4] fix: recover state of ShareSuccessManager after errors --- .../plus/share/MethodCallHandler.kt | 12 +++++------ .../plus/share/ShareSuccessManager.kt | 20 +++++-------------- 2 files changed, 10 insertions(+), 22 deletions(-) diff --git a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt index 8329b826e9..e210d95f52 100644 --- a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt +++ b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt @@ -12,14 +12,18 @@ internal class MethodCallHandler( ) : MethodChannel.MethodCallHandler { override fun onMethodCall(call: MethodCall, result: MethodChannel.Result) { + expectMapArguments(call) + // The user used a *WithResult method val isResultRequested = call.method.endsWith("WithResult") // We don't attempt to return a result if the current API version doesn't support it val isWithResult = isResultRequested && Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP_MR1 + if (isWithResult) + manager.setCallback(result) + when (call.method) { "shareUri" -> { - expectMapArguments(call) share.share( call.argument("uri") as String, subject = null, @@ -30,9 +34,6 @@ internal class MethodCallHandler( } } "share", "shareWithResult" -> { - expectMapArguments(call) - if (isWithResult && !manager.setCallback(result)) return - // Android does not support showing the share sheet at a particular point on screen. share.share( call.argument("text") as String, @@ -49,9 +50,6 @@ internal class MethodCallHandler( } } "shareFiles", "shareFilesWithResult" -> { - expectMapArguments(call) - if (isWithResult && !manager.setCallback(result)) return - // Android does not support showing the share sheet at a particular point on screen. try { share.shareFiles( diff --git a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/ShareSuccessManager.kt b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/ShareSuccessManager.kt index d73758b486..6cf5de72b6 100644 --- a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/ShareSuccessManager.kt +++ b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/ShareSuccessManager.kt @@ -17,21 +17,11 @@ internal class ShareSuccessManager(private val context: Context) : ActivityResul * Set result callback that will wait for the share-sheet to close and get either * the componentname of the chosen option or an empty string on dismissal. */ - fun setCallback(callback: MethodChannel.Result): Boolean { - return if (isCalledBack.compareAndSet(true, false)) { - // Prepare all state for new share - SharePlusPendingIntent.result = "" - isCalledBack.set(false) - this.callback = callback - true - } else { - callback.error( - "Share callback error", - "prior share-sheet did not call back, did you await it? Maybe use non-result variant", - null, - ) - false - } + fun setCallback(callback: MethodChannel.Result) { + // Prepare all state for new share + SharePlusPendingIntent.result = "" + isCalledBack.set(false) + this.callback = callback } /** From b0e0d768d06074ce495dfae2dff9555e7cdb46dd Mon Sep 17 00:00:00 2001 From: Miguel Beltran Date: Fri, 5 Apr 2024 11:58:02 +0200 Subject: [PATCH 2/4] fix deadlock in ShareSuccessManager --- .../plus/share/MethodCallHandler.kt | 66 +++++++++++-------- .../plus/share/ShareSuccessManager.kt | 28 ++++++-- 2 files changed, 63 insertions(+), 31 deletions(-) diff --git a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt index e210d95f52..4f66e21ed1 100644 --- a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt +++ b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt @@ -7,39 +7,50 @@ import java.io.IOException /** Handles the method calls for the plugin. */ internal class MethodCallHandler( - private val share: Share, - private val manager: ShareSuccessManager + private val share: Share, private val manager: ShareSuccessManager ) : MethodChannel.MethodCallHandler { override fun onMethodCall(call: MethodCall, result: MethodChannel.Result) { - expectMapArguments(call) - // The user used a *WithResult method val isResultRequested = call.method.endsWith("WithResult") // We don't attempt to return a result if the current API version doesn't support it - val isWithResult = isResultRequested && Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP_MR1 + val isWithResult = + isResultRequested && Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP_MR1 - if (isWithResult) - manager.setCallback(result) + expectMapArguments(call) + if (isWithResult && !manager.setCallback(result)) return when (call.method) { "shareUri" -> { - share.share( - call.argument("uri") as String, - subject = null, - withResult= false - ) + try { + share.share( + call.argument("uri") as String, subject = null, withResult = false + ) + } catch (e: IOException) { + manager.clear() + result.error("Share failed", e.message, null) + } if (!isWithResult) { - result.success(null) + if (isResultRequested) { + result.success("dev.fluttercommunity.plus/share/unavailable") + } else { + result.success(null) + } } } + "share", "shareWithResult" -> { // Android does not support showing the share sheet at a particular point on screen. - share.share( - call.argument("text") as String, - call.argument("subject") as String?, - isWithResult, - ) + try { + share.share( + call.argument("text") as String, + call.argument("subject") as String?, + isWithResult, + ) + } catch (e: IOException) { + manager.clear() + result.error("Share failed", e.message, null) + } if (!isWithResult) { if (isResultRequested) { @@ -49,6 +60,7 @@ internal class MethodCallHandler( } } } + "shareFiles", "shareFilesWithResult" -> { // Android does not support showing the share sheet at a particular point on screen. try { @@ -59,18 +71,20 @@ internal class MethodCallHandler( call.argument("subject"), isWithResult, ) - - if (!isWithResult) { - if (isResultRequested) { - result.success("dev.fluttercommunity.plus/share/unavailable") - } else { - result.success(null) - } - } } catch (e: IOException) { + manager.clear() result.error("Share failed", e.message, null) } + + if (!isWithResult) { + if (isResultRequested) { + result.success("dev.fluttercommunity.plus/share/unavailable") + } else { + result.success(null) + } + } } + else -> result.notImplemented() } } diff --git a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/ShareSuccessManager.kt b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/ShareSuccessManager.kt index 6cf5de72b6..3fdb20dea1 100644 --- a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/ShareSuccessManager.kt +++ b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/ShareSuccessManager.kt @@ -17,11 +17,21 @@ internal class ShareSuccessManager(private val context: Context) : ActivityResul * Set result callback that will wait for the share-sheet to close and get either * the componentname of the chosen option or an empty string on dismissal. */ - fun setCallback(callback: MethodChannel.Result) { - // Prepare all state for new share - SharePlusPendingIntent.result = "" - isCalledBack.set(false) - this.callback = callback + fun setCallback(callback: MethodChannel.Result): Boolean { + return if (isCalledBack.compareAndSet(true, false)) { + // Prepare all state for new share + SharePlusPendingIntent.result = "" + isCalledBack.set(false) + this.callback = callback + true + } else { + callback.error( + "Share callback error", + "prior share-sheet did not call back, did you await it? Maybe use non-result variant", + null, + ) + false + } } /** @@ -31,6 +41,14 @@ internal class ShareSuccessManager(private val context: Context) : ActivityResul returnResult(RESULT_UNAVAILABLE) } + /** + * Must be called on error to avoid deadlocking. + */ + fun clear() { + isCalledBack.set(true) + callback = null + } + /** * Send the result to flutter by invoking the previously set callback. */ From 90c42adb3345480bcf274a0b2f1629f65b836bb3 Mon Sep 17 00:00:00 2001 From: Miguel Beltran Date: Fri, 5 Apr 2024 12:08:11 +0200 Subject: [PATCH 3/4] refactor --- .../plus/share/MethodCallHandler.kt | 74 ++++++++----------- 1 file changed, 29 insertions(+), 45 deletions(-) diff --git a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt index 4f66e21ed1..986a728047 100644 --- a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt +++ b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt @@ -7,63 +7,40 @@ import java.io.IOException /** Handles the method calls for the plugin. */ internal class MethodCallHandler( - private val share: Share, private val manager: ShareSuccessManager + private val share: Share, + private val manager: ShareSuccessManager, ) : MethodChannel.MethodCallHandler { override fun onMethodCall(call: MethodCall, result: MethodChannel.Result) { + expectMapArguments(call) + // The user used a *WithResult method val isResultRequested = call.method.endsWith("WithResult") // We don't attempt to return a result if the current API version doesn't support it val isWithResult = isResultRequested && Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP_MR1 - expectMapArguments(call) if (isWithResult && !manager.setCallback(result)) return - when (call.method) { - "shareUri" -> { - try { + try { + when (call.method) { + "shareUri" -> { share.share( call.argument("uri") as String, subject = null, withResult = false ) - } catch (e: IOException) { - manager.clear() - result.error("Share failed", e.message, null) + success(isWithResult, isResultRequested, result) } - if (!isWithResult) { - if (isResultRequested) { - result.success("dev.fluttercommunity.plus/share/unavailable") - } else { - result.success(null) - } - } - } - "share", "shareWithResult" -> { - // Android does not support showing the share sheet at a particular point on screen. - try { + "share", "shareWithResult" -> { share.share( call.argument("text") as String, call.argument("subject") as String?, isWithResult, ) - } catch (e: IOException) { - manager.clear() - result.error("Share failed", e.message, null) - } - - if (!isWithResult) { - if (isResultRequested) { - result.success("dev.fluttercommunity.plus/share/unavailable") - } else { - result.success(null) - } + success(isWithResult, isResultRequested, result) } - } - "shareFiles", "shareFilesWithResult" -> { - // Android does not support showing the share sheet at a particular point on screen. - try { + "shareFiles", "shareFilesWithResult" -> { share.shareFiles( call.argument>("paths")!!, call.argument?>("mimeTypes"), @@ -71,21 +48,28 @@ internal class MethodCallHandler( call.argument("subject"), isWithResult, ) - } catch (e: IOException) { - manager.clear() - result.error("Share failed", e.message, null) + success(isWithResult, isResultRequested, result) } - if (!isWithResult) { - if (isResultRequested) { - result.success("dev.fluttercommunity.plus/share/unavailable") - } else { - result.success(null) - } - } + else -> result.notImplemented() } + } catch (e: IOException) { + manager.clear() + result.error("Share failed", e.message, null) + } + } - else -> result.notImplemented() + private fun success( + isWithResult: Boolean, + isResultRequested: Boolean, + result: MethodChannel.Result + ) { + if (!isWithResult) { + if (isResultRequested) { + result.success("dev.fluttercommunity.plus/share/unavailable") + } else { + result.success(null) + } } } From 5defc458224ddbcd9d2e2155c7a2965b8de703be Mon Sep 17 00:00:00 2001 From: Miguel Beltran Date: Fri, 5 Apr 2024 14:09:44 +0200 Subject: [PATCH 4/4] catch Throwable and pass error details up --- .../dev/fluttercommunity/plus/share/MethodCallHandler.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt index 986a728047..a4bff43e0d 100644 --- a/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt +++ b/packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt @@ -53,9 +53,9 @@ internal class MethodCallHandler( else -> result.notImplemented() } - } catch (e: IOException) { + } catch (e: Throwable) { manager.clear() - result.error("Share failed", e.message, null) + result.error("Share failed", e.message, e) } }