permissions-dispatcher / PermissionsDispatcher

A declarative API to handle Android runtime permissions.
https://github.com/permissions-dispatcher/PermissionsDispatcher
Apache License 2.0
11.21k stars 1.44k forks source link

permissionRequest proceed doesn't work due to KtxPermissionRequest.requestPermission being null #734

Closed SamYStudiO closed 2 years ago

SamYStudiO commented 3 years ago

Overview

Environment

PermissionsDispatcher KTX

This doesn't work cause KtxPermissionRequest.requestPermission weakreference got lost and is null

 private fun onLocationShowRationale(permissionRequest: PermissionRequest) {
        findNavController().navigate(
            RoundFormFragmentDirections.actionGlobalAlertDialog(
                messageRes = R.string.NSLocationWhenInUseUsageDescription,
                negativeButtonRes = R.string.global_cancel,
                positiveButtonRes = R.string.dialog_gps_permissions_activate,
                requestCode = REQUEST_CODE_PERMISSION_RATIONAL
            )
        )
        setDialogPositiveClickListener {requestCode, _ ->
            when (requestCode) {
                REQUEST_CODE_PERMISSION_RATIONAL -> {
                    permissionRequest.proceed()
                }
            }
        }
        setDialogNegativeClickListener {requestCode, _ ->
            when (requestCode) {
                REQUEST_CODE_PERMISSION_RATIONAL -> {
                    permissionRequest.cancel()
                    findNavController().popBackStack()
                }
            }
        }
    }

But this works

 private fun onLocationShowRationale(permissionRequest: PermissionRequest) {
        permissionRequest.proceed()
    }

Any help appreciated Thx!

SamYStudiO commented 3 years ago

I guess this func is being garbage collected since nobody keep a reference, so if you delay your permissionRequest.proceed() then it may be null https://github.com/permissions-dispatcher/PermissionsDispatcher/blob/c8876729dfd3cd2158d57098e56e02514792fd62/ktx/src/main/java/permissions/dispatcher/ktx/PermissionsRequesterImpl.kt#L27-L32

hotchemi commented 2 years ago

@SamYStudiO thx, could you give us more info for this case? in my understanding this could happen when PermissionsRequester instance is already released from memory?

SamYStudiO commented 2 years ago

@hotchemi hello exactly there seems to have nothing keeping a reference to requestFun specified in my previous answer. so if you delay permissionRequest.proceed() with a confirmation dialog for example, that function may have been garbage collected when effectively calling proceed.

hotchemi commented 2 years ago

@SamYStudiO thx, one thing I haven't understood is in your case PermissionsRequester is also destroyed? If so I'm wondering we should not keep a reference to functions, otherwise, it causes the memory leak again in theory 🤔 in ktx-sample, it works as expected and do you think we can do a similar approach?

https://github.com/permissions-dispatcher/PermissionsDispatcher/blob/af245e244ddb11c7656f6a66ae8b3c89412bbcb3/ktx-sample/src/main/java/permissions/dispatcher/ktx/sample/MainFragment.kt#L82-L83

SamYStudiO commented 2 years ago

I'm not sure why ktx-sample works, i guess for this one GC hasn't been done yet, in my case https://github.com/permissions-dispatcher/PermissionsDispatcher/blob/c8876729dfd3cd2158d57098e56e02514792fd62/ktx/src/main/java/permissions/dispatcher/ktx/KtxPermissionRequest.kt#L11 requestPermission.get() is null if i delay it with my custom dialog implementation and wait my dialog to return a positive value

hotchemi commented 2 years ago

can we get an example that we can repro the issue? The reason ktx-sample works is requestPermission and permissionDenied are still referenced from PermissionsRequester AFAIK.

SamYStudiO commented 2 years ago

it's a huge projet, i may have a look to extract, but i'm pretty sure problem is requestFun having no strong reference anywhere and so beeing destroy before we may use it https://github.com/permissions-dispatcher/PermissionsDispatcher/blob/c8876729dfd3cd2158d57098e56e02514792fd62/ktx/src/main/java/permissions/dispatcher/ktx/PermissionsRequesterImpl.kt#L27-L32

hotchemi commented 2 years ago

tried https://github.com/permissions-dispatcher/PermissionsDispatcher/compare/prototype/gc_for_PermissionRequest?expand=1 but PermissionRequest worked as expected. My advice here is to keep PermissionsRequester alive until everything related to permission handling is finished. this is out expected use case for the API.

SamYStudiO commented 2 years ago

Hello check ktx-sample from my fork to reproduce the pb https://github.com/SamYStudiO/PermissionsDispatcher Issue is reproductible when using DialogFragment instead of AlertDialog directly If https://github.com/permissions-dispatcher/PermissionsDispatcher/blob/c8876729dfd3cd2158d57098e56e02514792fd62/ktx/src/main/java/permissions/dispatcher/ktx/KtxPermissionRequest.kt#L7 is not a weakreference pb is solved, need to make sure this is not a leak though

hotchemi commented 2 years ago

thx, I'll take a look

hotchemi commented 2 years ago

I tried your repo but could not repro the issue, unfortunately.. 🤔 could you give more environment info? I tried on Nexus5 API level 30 emulator.

SamYStudiO commented 2 years ago

This is indeed random from that project since this has a low memory impact. Calling System.gc in not enough since this is just an indication we want to run gc faster, but this will not run gc immediatly and if it's not required. I made some change to have more memory consumption by creating a huge bitmap from the app and this time this will showcase the pb each time. You may adjust bitmap size and proceed delay if required Just update or download my repo again.

hotchemi commented 2 years ago

thx, I suppose https://github.com/permissions-dispatcher/PermissionsDispatcher/pull/742 would resolve the issue.

hotchemi commented 2 years ago

ktx-1.1.2 has been released.