Add LeakCanary to local builds

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sebastian, Assigned: sebastian)

Tracking

(Depends on: 6 bugs)

unspecified
Firefox 47
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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.

Comment 3

3 years ago
(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
(Assignee)

Comment 4

3 years ago
Created attachment 8710373 [details]
MozReview Request: Bug 1238066 - Add LeakCanary to local (gradle) builds. r?nalexander

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)
(Assignee)

Comment 5

3 years ago
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)

Updated

3 years ago
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
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+

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ff2a6523bc0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
(Assignee)

Updated

3 years ago
Depends on: 1257933
(Assignee)

Updated

3 years ago
Depends on: 1257934
(Assignee)

Updated

3 years ago
Depends on: 1257936
(Assignee)

Updated

3 years ago
Depends on: 1257667
(Assignee)

Updated

3 years ago
Depends on: 1257941
(Assignee)

Updated

3 years ago
See Also: → bug 1259099
(Assignee)

Updated

3 years ago
Depends on: 1260284
(Assignee)

Updated

3 years ago
Depends on: 1260271
(Assignee)

Updated

3 years ago
Depends on: 1262092
(Assignee)

Updated

3 years ago
Depends on: 1262153
You need to log in before you can comment on or make changes to this bug.