937945 - Don't store activity Context in GeckoProfile

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

GeckoProfile holds onto the Context given to it when it's created, and that Context is usually an Activity (GeckoApp). This can lead to memory leaks as described in bug 891634. The Context is used only to get the files dir, so we can drop the Context reference entirely and replace it with a File.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Don't store Context in GeckoProfile

This patch changes getProfilesINI to accept a File instead of a Context, then replaces all mContext references to sFilesDir. Also includes some other minor cleanup.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 831201 [details] [diff] [review]
Don't store Context in GeckoProfile

Review of attachment 831201 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoProfile.java
@@ +49,5 @@
>  
>      private boolean mInGuestMode = false;
>      private static GeckoProfile mGuestProfile = null;
>  
> +    private static File sFilesDir;

Perhaps store `mozillaDir` instead, rather than creating new File instances everywhere it gets used?

@@ +51,5 @@
>      private static GeckoProfile mGuestProfile = null;
>  
> +    private static File sFilesDir;
> +
> +    static private INIParser getProfilesINI(File filesDir) {

private static, plz.

And perhaps have a fast path that takes mozillaDir as an argument instead? All but one caller already have it.

@@ +129,5 @@
>              return profile;
>          }
>      }
>  
> +    private synchronized File getMozillaDirectory() throws IOException {

I'd probably keep the name of this as `ensureMozillaDirectory`: it has side-effects, and its postcondition is "here's a valid, created directory, or I threw a checked exception". That sounds like an ensure* to me.

@@ +133,5 @@
> +    private synchronized File getMozillaDirectory() throws IOException {
> +        File mozDir = new File(sFilesDir, "mozilla");
> +        if (! mozDir.exists()) {
> +            if (! mozDir.mkdirs()) {
> +                throw new IOException("Unable to create mozilla directory at " + mozDir.getAbsolutePath());

if (mozDir.exists() || mozDir.mkdirs()) {
    return mozDir;
}
throw new IOException(...);

@@ +498,5 @@
>      }
>  
>      private File findProfileDir(File mozillaDir) {
>          // Open profiles.ini to find the correct path
> +        INIParser parser = getProfilesINI(sFilesDir);

Hey! I already have mozillaDir!

@@ +526,5 @@
>          return salt.toString();
>      }
>  
>      private File createProfileDir(File mozillaDir) throws IOException {
> +        INIParser parser = getProfilesINI(sFilesDir);

Hey! I already have mozillaDir!

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Don't store Context in GeckoProfile, v2

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 831710 [details] [diff] [review]
Don't store Context in GeckoProfile, v2

Review of attachment 831710 [details] [diff] [review]:
-----------------------------------------------------------------

I think this makes sense!

::: mobile/android/base/GeckoProfile.java
@@ +34,1 @@
>      private File mDir;

Can we rename this to `mProfileDir`?

Comment 6

[Brian Nicholson (:bnicholson)]

(In reply to Richard Newman [:rnewman] from comment #5)
> Comment on attachment 831710 [details] [diff] [review]
> Don't store Context in GeckoProfile, v2
> 
> Review of attachment 831710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this makes sense!
> 
> ::: mobile/android/base/GeckoProfile.java
> @@ +34,1 @@
> >      private File mDir;
> 
> Can we rename this to `mProfileDir`?

Yep, done.

Comment 7

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/000ac7f90a40

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/000ac7f90a40