Closed Bug 937945 Opened 6 years ago Closed 6 years ago

Don't store activity Context in GeckoProfile

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: bnicholson, Assigned: bnicholson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Attachment #831201 - Flags: review?(rnewman)
Duplicate of this bug: 902060
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!
Attachment #831201 - Flags: review?(rnewman) → feedback+
Attachment #831201 - Attachment is obsolete: true
Attachment #831710 - Flags: review?(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`?
Attachment #831710 - Flags: review?(rnewman) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/000ac7f90a40
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.