Closed Bug 1238066 Opened 4 years ago Closed 4 years ago

Add LeakCanary to local builds

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

All
Android
defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

(Depends on 6 open bugs)

Details

Attachments

(1 file)

LeakCanary is a memory leak detection library for Android and Java:
https://github.com/square/leakcanary

We just want to use this in local builds (maybe in Nightly?).

Square offers two libraries: The actual lib and a no-op lib that can be used to compile release versions without the need to change the code using LeakCanary:
* com.squareup.leakcanary:leakcanary-android:1.3.1
* com.squareup.leakcanary:leakcanary-android-no-op:1.3.1

By default LeakCanary will detect Activity leaks. For everything else we'll need to manually add references to RefWatcher if they are interesting to monitor.
I recently did this locally (just in the gradle build because that's easy) and it worked without issues (I didn't find any leaks though).
(In reply to Sebastian Kaspari (:sebastian) from comment #1)
> I recently did this locally (just in the gradle build because that's easy)
> and it worked without issues (I didn't find any leaks though).

Somebody (mcomella?  mhaigh?) did this before.  Post the patch and let's get it into Gradle, at least.  We can try to get the no-op thing working in moz.build if it's useful.
(In reply to Nick Alexander :nalexander from comment #2)
> (In reply to Sebastian Kaspari (:sebastian) from comment #1)
> > I recently did this locally (just in the gradle build because that's easy)
> > and it worked without issues (I didn't find any leaks though).
> 
> Somebody (mcomella?  mhaigh?) did this before.  Post the patch and let's get
> it into Gradle, at least.  We can try to get the no-op thing working in
> moz.build if it's useful.

I had a gist that did this long ago:
https://gist.github.com/leibovic/38c21068e4ac58e5c751
This patch adds LeakCanary to gradle builds and the no-op library to
mach-based builds.

Review commit: https://reviewboard.mozilla.org/r/31731/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31731/
Attachment #8710373 - Flags: review?(nalexander)
I tested this patch with a manually created leak.

I mean eventually we could add LeakCanary to mach based builds too (There are a bunch of classes though..) and hide it behind a build flag.

By default LeakCanary only detects Activity leaks. If we want to use it for more then we should expose the RefWatcher instance like described here:
https://github.com/square/leakcanary#how-do-i-use-it
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Depends on: 1241460
Comment on attachment 8710373 [details]
MozReview Request: Bug 1238066 - Add LeakCanary to local (gradle) builds. r?nalexander

https://reviewboard.mozilla.org/r/31731/#review28719

I'd like to see a green try build, but other than that, rock on!

I will have to rebase a bunch of Gradle work on top of this, but whatever :)

::: mobile/android/app/base/build.gradle:121
(Diff revision 1)
> +    // Gradle based debug builds include LeakCanary. Mach based builds only include the no-op

It's not quite true that it's debug only.  As written, it's *all* Gradle builds.  (Unless LeakCanary itself differentiates, which it can and might.)  Let's do not be clever about debug/release in our build.gradle files for now, since it's easier.

::: mobile/android/config/proguard/leakcanary-keeps.cfg:6
(Diff revision 1)
> +# For Android 6.0 compatibility

nit: full sentences with periods, please.

::: mobile/android/thirdparty/com/squareup/leakcanary/LeakCanary.java:1
(Diff revision 1)
> +package com.squareup.leakcanary;

nit: Include license.  I think this should be MPL, since it's our version.
Attachment #8710373 - Flags: review?(nalexander) → review+
https://hg.mozilla.org/mozilla-central/rev/2ff2a6523bc0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1257933
Depends on: 1257934
Depends on: 1257936
Depends on: 1257667
Depends on: 1257941
See Also: → 1259099
Depends on: 1260284
Depends on: 1260271
Depends on: 1262092
Depends on: 1262153
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 47 → mozilla47
You need to log in before you can comment on or make changes to this bug.