search

Karumi / Dexter

Android library that simplifies the process of requesting permissions at runtime.
http://karumi.com
Apache License 2.0
5.23k stars 671 forks source link

fix memory leak #206

Closed lzdon closed 6 years ago

lzdon commented 6 years ago

sorry , i forgot validation

lzdon commented 6 years ago

why always check failed ? I just add one line code

Serchinastico commented 6 years ago

Hey @lzdon

I'm taking a look at the failing test and I'd say the test is incomplete as it stands right now, looks like we are not setting what to do in DexterInstance::336:

int permissionState = androidPermissionService.checkSelfPermission(context, permission);

and therefore it's returning 0 (default value), however, by the name of the test I'd say it should return PERMISSION_DENIED = -1 to then try with the invisible activity to do the same check and failing to do so. Here is the patch to fix it so that we can merge your PR:

  @ -132,6 +129,7 @@ import static org.mockito.Mockito.when;
    }

    @Test public void onPermissionFailedByRuntimeExceptionThenNotifiesListener() {
+     givenPermissionIsChecked(ANY_PERMISSION, PackageManager.PERMISSION_DENIED);
      givenARuntimeExceptionIsThrownWhenPermissionIsChecked(ANY_PERMISSION);
      givenShouldShowRationaleForPermission(ANY_PERMISSION);
sklinefelter commented 6 years ago

197 Unfortunately I just tested this locally and still see LeakCanary indicating the listener leaks the activity.

I was able to reproduce by adding LeakCanary to the Dexter sample and rotating once the permissions dialog was shown.

My suspicion is it's related to Multiple-Permissions

Serchinastico commented 6 years ago

Hi @sklinefelter

Do you have the hprof file? I've been testing for a while and I can't reproduce the issue, what's more, at first LeakCanary detected something but in the end it just reported that The GC was being lazy (See https://github.com/square/leakcanary/issues/536).

sklinefelter commented 6 years ago

I do have the additional details and can reproduce consistently.

Steps to reproduce:

  1. Add LeakCanary to a basic SampleApplication
public class SampleApplication extends Application {

    @Override
    public void onCreate() {
        super.onCreate();
        if (LeakCanary.isInAnalyzerProcess(this)) {
            // This process is dedicated to LeakCanary for heap analysis.
            // You should not init your app in this process.
            return;
        }
        LeakCanary.install(this);
    }
}
  1. Adjust the AndroidManifest to have the ".SampleApplication"
  <application
      android:allowBackup="true"
      android:label="@string/app_name"
      android:icon="@mipmap/ic_logo_karumi"
      android:supportsRtl="true"
      android:theme="@style/AppTheme"
      android:name=".SampleApplication">
  1. Accept storage permssion (this gets past the "GC was being lazy" issue for me somehow)
  2. Open the permissions dialog
  3. Rotate the phone (sometimes all it takes is one, sometimes you have to rotate 3 or 4 times)

Eventually, the LeakCanary notification comes through

Here's the leak:

In com.karumi.dexter.sample:1.0:1.

  • com.karumi.dexter.sample.SampleActivity has leaked:
  • GC ROOT android.os.HandlerThread.contextClassLoader
  • references dalvik.system.PathClassLoader.runtimeInternalObjects
  • references array java.lang.Object[].[10]
  • references static com.karumi.dexter.Dexter.instance
  • references com.karumi.dexter.DexterInstance.listener
  • references com.karumi.dexter.MultiplePermissionListenerThreadDecorator.listener
  • references com.karumi.dexter.listener.multi.CompositeMultiplePermissionsListener.listeners
  • references java.util.Arrays$ArrayList.a
  • references array com.karumi.dexter.listener.multi.MultiplePermissionsListener[].[0]
  • references com.karumi.dexter.sample.SampleMultiplePermissionListener.activity
  • leaks com.karumi.dexter.sample.SampleActivity instance
  • Retaining: 1.4 kB.
  • Reference Key: 58047889-7492-429e-9f11-fcefe81f7ecc
  • Device: Google google Pixel sailfish
  • Android Version: 8.1.0 API: 27 LeakCanary: 1.5.4 74837f0
  • Durations: watch=5544ms, gc=119ms, heap dump=1804ms, analysis=49174ms
  • Details:
  • Instance of android.os.HandlerThread | static $class$methods = 1901801520 | static $class$vtable = null | static $class$numReferenceStaticFields = 0 | static $class$clinitThreadId = 0 | static $classOverhead = byte[420]@1898567041 (0x7129d581) | static $class$dexTypeIndex = 5111 | static $class$numReferenceInstanceFields = 2 | static $class$virtualMethodsOffset = 2 | static $class$classLoader = null | static $class$dexCache = java.lang.DexCache@1897028624 (0x71125c10) | static $class$iFields = 1900117996 | static $class$shadow$klass = java.lang.Class | static $class$dexClassDefIndex = 2731 | static $class$name = java.lang.String@1899490136 (0x7137eb58) | static $class$accessFlags = 524289 | static $class$classSize = 544 | static $class$sFields = 0 | static $class$primitiveType = 131072 | static $class$objectSize = 148 | static $class$objectSizeAllocFastPath = 152 | static $class$referenceInstanceOffsets = -1073741824 | static $class$componentType = null | static $class$ifTable = java.lang.Object[2]@1897090168 (0x71134c78) | static $class$classFlags = 0 | static $class$extData = null | static $class$shadow$monitor = 536870912 | static $class$copiedMethodsOffset = 9 | static $class$status = 11 | static $class$superClass = java.lang.Thread | mHandler = null | mLooper = android.os.Looper@315101744 (0x12c81230) | mPriority = 0 | mTid = 6342 | blocker = null | blockerLock = java.lang.Object@315504056 (0x12ce35b8) | contextClassLoader = dalvik.system.PathClassLoader@315150696 (0x12c8d168) | daemon = false | eetop = 0 | group = java.lang.ThreadGroup@1892864888 (0x70d2d378) | inheritableThreadLocals = null | inheritedAccessControlContext = java.security.AccessControlContext@315504064 (0x12ce35c0) | lock = java.lang.Object@315504072 (0x12ce35c8) | name = java.lang.String@315155176 (0x12c8e2e8) | nativeParkEventPointer = 0 | nativePeer = 505793973760 | parkBlocker = null | parkState = 1 | priority = 5 | single_step = false | stackSize = 0 | started = true | stillborn = false | target = null | threadLocalRandomProbe = 0 | threadLocalRandomSecondarySeed = 0 | threadLocalRandomSeed = 0 | threadLocals = java.lang.ThreadLocal$ThreadLocalMap@315504080 (0x12ce35d0) | threadQ = null | threadStatus = 0 | tid = 475 | uncaughtExceptionHandler = null | shadow$klass = android.os.HandlerThread | shadow$monitor = 0
  • Instance of dalvik.system.PathClassLoader | static $class$methods = 1895576376 | static $class$vtable = null | static $class$numReferenceStaticFields = 0 | static $class$clinitThreadId = 0 | static $classOverhead = byte[316]@1895263521 (0x70f76d21) | static $class$dexTypeIndex = 1264 | static $class$numReferenceInstanceFields = 0 | static $class$virtualMethodsOffset = 2 | static $class$classLoader = null | static $class$dexCache = java.lang.DexCache@1895182352 (0x70f63010) | static $class$iFields = 0 | static $class$shadow$klass = java.lang.Class | static $class$dexClassDefIndex = 1244 | static $class$name = java.lang.String@1899563328 (0x71390940) | static $class$accessFlags = 524289 | static $class$classSize = 440 | static $class$sFields = 0 | static $class$primitiveType = 131072 | static $class$objectSize = 44 | static $class$objectSizeAllocFastPath = 48 | static $class$referenceInstanceOffsets = 263 | static $class$componentType = null | static $class$ifTable = java.lang.Object[0]@1897064952 (0x7112e9f8) | static $class$classFlags = 32 | static $class$extData = null | static $class$shadow$monitor = 536870912 | static $class$copiedMethodsOffset = 2 | static $class$status = 11 | static $class$superClass = dalvik.system.BaseDexClassLoader | pathList = dalvik.system.DexPathList@315228296 (0x12ca0088) | allocator = 505980096432 | classTable = 505980941184 | packages = java.util.HashMap@315228216 (0x12ca0038) | parent = java.lang.BootClassLoader@1897088160 (0x711344a0) | proxyCache = java.util.HashMap@315228256 (0x12ca0060) | runtimeInternalObjects = java.lang.Object[247]@315150700 (0x12c8d16c) | shadow$klass = dalvik.system.PathClassLoader | shadow$monitor = 0
  • Array of java.lang.Object[] | [0] = com.squareup.leakcanary.DefaultLeakDirectoryProvider$1 | [1] = com.squareup.leakcanary.DefaultLeakDirectoryProvider$3 | [2] = com.squareup.leakcanary.internal.FutureResult | [3] = com.squareup.leakcanary.AndroidHeapDumper$1 | [4] = com.squareup.leakcanary.GcTrigger$1 | [5] = com.squareup.leakcanary.RefWatcher | [6] = com.squareup.leakcanary.RefWatcher$1 | [7] = com.squareup.leakcanary.AndroidWatchExecutor$3 | [8] = com.squareup.leakcanary.ExcludedRefs$ParamsBuilder | [9] = com.squareup.leakcanary.Retryable$Result[] | [10] = com.karumi.dexter.Dexter | [11] = com.squareup.leakcanary.internal.LeakCanarySingleThreadFactory | [12] = com.karumi.dexter.listener.PermissionRequest | [13] = com.squareup.leakcanary.ExcludedRefs | [14] = com.karumi.dexter.DexterBuilder | [15] = com.karumi.dexter.AndroidPermissionService | [16] = com.karumi.dexter.listener.single.SnackbarOnDeniedPermissionListener | [17] = com.squareup.leakcanary.internal.LeakCanaryInternals$1 | [18] = com.karumi.dexter.sample.SampleBackgroundThreadPermissionListener | [19] = com.karumi.dexter.sample.SampleMultiplePermissionListener | [20] = butterknife.Unbinder | [21] = com.squareup.leakcanary.AndroidExcludedRefs$40 | [22] = com.karumi.dexter.PermissionRationaleToken | [23] = com.squareup.leakcanary.AndroidExcludedRefs$41 | [24] = com.karumi.dexter.listener.multi.SnackbarOnAnyDeniedMultiplePermissionsListener$Builder$1 | [25] = com.squareup.leakcanary.AndroidExcludedRefs$30 | [26] = com.squareup.leakcanary.ServiceHeapDumpListener | [27] = com.squareup.leakcanary.AndroidExcludedRefs$31 | [28] = com.squareup.leakcanary.AndroidExcludedRefs$20 | [29] = android.support.v4.app.ActivityCompatApi23$RequestPermissionsRequestCodeValidator | [30] = com.karumi.dexter.DexterBuilder$Permission | [31] = com.squareup.leakcanary.AndroidExcludedRefs$32 | [32] = com.squareup.leakcanary.ActivityRefWatcher | [33] = com.squareup.leakcanary.AndroidExcludedRefs$21 | [34] = com.karumi.dexter.Thread | [35] = com.squareup.leakcanary.AndroidExcludedRefs$10 | [36] = butterknife.internal.DebouncingOnClickListener$1 | [37] = com.karumi.dexter.MultiplePermissionListenerThreadDecorator | [38] = com.karumi.dexter.sample.SampleActivity_ViewBinding$1 | [39] = com.squareup.leakcanary.AndroidExcludedRefs$33 | [40] = com.squareup.leakcanary.AndroidExcludedRefs$22 | [41] = com.squareup.leakcanary.AndroidExcludedRefs$11 | [42] = com.karumi.dexter.sample.SampleActivity_ViewBinding$2 | [43] = com.karumi.dexter.sample.SampleApplication | [44] = com.squareup.leakcanary.AndroidExcludedRefs$34 | [45] = butterknife.internal.DebouncingOnClickListener | [46] = com.karumi.dexter.listener.single.PermissionListener[] | [47] = com.karumi.dexter.listener.single.BasePermissionListener | [48] = com.squareup.leakcanary.AndroidExcludedRefs$23 | [49] = com.squareup.leakcanary.LeakCanary | [50] = com.squareup.leakcanary.AndroidExcludedRefs$12 | [51] = com.karumi.dexter.listener.PermissionRequestErrorListener | [52] = com.squareup.leakcanary.HeapDump$Listener$1 | [53] = com.karumi.dexter.sample.SampleActivity_ViewBinding$3 | [54] = com.squareup.leakcanary.AndroidExcludedRefs$35 | [55] = com.squareup.leakcanary.AndroidExcludedRefs | [56] = com.squareup.leakcanary.AndroidExcludedRefs$24 | [57] = com.squareup.leakcanary.AndroidExcludedRefs$13 | [58] = com.karumi.dexter.DexterBuilder$SinglePermissionListener | [59] = com.squareup.leakcanary.GcTrigger | [60] = com.karumi.dexter.listener.multi.MultiplePermissionsListener | [61] = com.karumi.dexter.sample.SampleActivity_ViewBinding$4 | [62] = com.squareup.leakcanary.AndroidExcludedRefs$36 | [63] = com.squareup.leakcanary.AndroidExcludedRefs$25 | [64] = android.support.v4.app.ActivityCompat | [65] = com.squareup.leakcanary.AndroidExcludedRefs$14 | [66] = android.support.v4.content.PermissionChecker | [67] = com.squareup.leakcanary.AndroidExcludedRefs$37 | [68] = com.squareup.leakcanary.ExcludedRefs$Builder | [69] = com.squareup.leakcanary.AndroidExcludedRefs$26 | [70] = com.squareup.leakcanary.AndroidExcludedRefs$15 | [71] = com.squareup.leakcanary.RefWatcherBuilder | [72] = com.squareup.leakcanary.AndroidExcludedRefs$38 | [73] = com.squareup.leakcanary.AndroidExcludedRefs$27 | [74] = com.squareup.leakcanary.AndroidExcludedRefs$16 | [75] = com.squareup.leakcanary.AndroidExcludedRefs$39 | [76] = com.squareup.leakcanary.AndroidExcludedRefs$28 | [77] = com.squareup.leakcanary.AndroidExcludedRefs$17 | [78] = com.squareup.leakcanary.AndroidExcludedRefs$1 | [79] = android.support.design.widget.Snackbar$Callback | [80] = com.karumi.dexter.DexterBuilder$MultiPermissionListener | [81] = butterknife.ButterKnife | [82] = android.support.v4.app.ActivityCompatApi23 | [83] = com.squareup.leakcanary.AndroidExcludedRefs$29 | [84] = com.squareup.leakcanary.AndroidExcludedRefs$18 | [85] = com.squareup.leakcanary.AndroidExcludedRefs$2 | [86] = com.squareup.leakcanary.Retryable$Result | [87] = com.karumi.dexter.listener.single.DialogOnDeniedPermissionListener$Builder | [88] = com.squareup.leakcanary.ActivityRefWatcher$1 | [89] = com.karumi.dexter.sample.SamplePermissionListener | [90] = com.squareup.leakcanary.AndroidExcludedRefs$19 | [91] = com.squareup.leakcanary.AndroidExcludedRefs$3 | [92] = butterknife.internal.Utils | [93] = com.squareup.leakcanary.AndroidWatchExecutor$2 | [94] = com.karumi.dexter.listener.multi.BaseMultiplePermissionsListener | [95] = com.squareup.leakcanary.WatchExecutor | [96] = com.squareup.leakcanary.AndroidExcludedRefs$4 | [97] = com.karumi.dexter.sample.SampleActivity_ViewBinding | [98] = com.karumi.dexter.listener.multi.CompositeMultiplePermissionsListener | [99] = com.squareup.leakcanary.HeapDumper | [100] = com.karumi.dexter.listener.single.PermissionListener | [101] = com.squareup.leakcanary.AndroidExcludedRefs$5 | [102] = com.karumi.dexter.listener.multi.SnackbarOnAnyDeniedMultiplePermissionsListener$Builder | [103] = com.squareup.leakcanary.KeyedWeakReference | [104] = com.karumi.dexter.sample.SampleErrorListener | [105] = com.squareup.leakcanary.AndroidExcludedRefs$6 | [106] = android.support.design.widget.BaseTransientBottomBar$BaseCallback | [107] = com.squareup.leakcanary.AndroidWatchExecutor | [108] = com.squareup.leakcanary.DebuggerControl | [109] = com.karumi.dexter.ThreadFactory | [110] = com.squareup.leakcanary.internal.LeakCanaryInternals | [111] = android.support.v4.app.ActivityCompat$OnRequestPermissionsResultCallback | [112] = com.squareup.leakcanary.AndroidExcludedRefs$7 | [113] = com.squareup.leakcanary.internal.HeapAnalyzerService | [114] = com.squareup.leakcanary.Preconditions | [115] = android.support.v4.content.ContextCompat | [116] = com.karumi.dexter.DexterInstance | [117] = com.squareup.leakcanary.AndroidExcludedRefs$8 | [118] = com.squareup.leakcanary.Retryable | [119] = com.squareup.leakcanary.AndroidExcludedRefs[] | [120] = com.karumi.dexter.DexterException | [121] = com.squareup.leakcanary.AndroidExcludedRefs$9 | [122] = com.squareup.leakcanary.LeakDirectoryProvider | [123] = com.squareup.leakcanary.DebuggerControl$1 | [124] = com.squareup.leakcanary.DisplayLeakService | [125] = com.karumi.dexter.MultiplePermiss...
sklinefelter commented 6 years ago

@Serchinastico is there anything else that you need on my end?

wesley-firewalla commented 6 years ago

is there any updates for this memory leak issue?

Serchinastico commented 6 years ago

Not really, I tested it several times but results weren't consistent and I want to make sure we get rid of the issue for once

Serchinastico commented 6 years ago

Ok, so it looks like there is a leak in onActivityDestroyed as well, I'm going to:

I'm including some other pending PRs with non backwards compatible changes to next version so expect to see these changes in v5.0.0

Thank you all for your feedback and collaboration 👏

Serchinastico commented 6 years ago

Continues in #210