726382 - Fennec native code has too many different chunks of code dealing with reading profile info

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fold GeckoDirProvider into GeckoProfile

There's a lot of duplicated and inefficient code to read stuff from the profile. This should be cleaned up. I have a patch that folds GeckoDirProvider into a new GeckoProfile class. A different GeckoProfile object is created on demand to track each user profile the code deals with, so the abstraction level is cleaner. A lot of duplicate code is also folded into this class under more semantically meaningful method names. Most of the original GeckoDirProvider code is kept the same, but in some places slightly tuned for speed.

Tagging wesj for review since he wrote GeckoDirProvider. Note also that this patch depends on the one from bug 726381 which removes a bunch of cruft.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Try run that included this change was green: https://tbpl.mozilla.org/?tree=Try&rev=8df1f33bb1c1

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 596412 [details] [diff] [review]
Fold GeckoDirProvider into GeckoProfile

Unmarking for review as I will need to update this patch to deal with profile migration as well.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fold GeckoDirProvider into GeckoProfile

Reworked this a little to keep the code that moves the profiles between SD card and internal storage. Verified that that code works by exercising it on a Gingerbread emulator and verifying the directories get moved with adb shell.

Started a try run with this patch as well at https://tbpl.mozilla.org/?tree=Try&rev=31845d3b0868

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 596914 [details] [diff] [review]
Fold GeckoDirProvider into GeckoProfile

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

::: mobile/android/base/GeckoProfile.java
@@ +90,5 @@
> +        mName = profileName;
> +    }
> +
> +    public File getDir() {
> +        if (mDir == null) {

I'd prefer:
if (mDir != null)
   return mDir;

// do stuff
return mDir;

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 596914 [details] [diff] [review]
Fold GeckoDirProvider into GeckoProfile

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

I like the cleanup and the apis. Some general comments and questions. I assume most of the code isn't changed much if at all. If blassey is ok with introducing the sync readFile api's, seems fine.

::: mobile/android/base/GeckoApp.java
@@ +1647,5 @@
>          }
> +        // fall back to default profile if we didn't load a specific one
> +        if (mProfile == null) {
> +            mProfile = GeckoProfile.get(this);
> +        }

Can we move this into getProfile (i.e. lazy init?)

::: mobile/android/base/GeckoProfile.java
@@ +1,1 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-

Might as well use MPL2 now: http://www.mozilla.org/MPL/headers/

@@ +80,5 @@
> +            if (profile == null) {
> +                profile = new GeckoProfile(context, profileName);
> +                sProfileCache.put(profileName, profile);
> +            }
> +            return profile;

Do we need to include the context's hashCode in this to differentiate further? (I realize this is my old code...)

@@ +101,5 @@
> +                    Log.d(LOGTAG, "Found profile dir: " + mDir.getAbsolutePath());
> +                }
> +            } catch (IOException ioe) {
> +                Log.e(LOGTAG, "Error getting profile dir", ioe);
> +            }

Can we not just eat this up here? I like forcing callers to think about what they're getting back. For the passwords and browser db providers, not getting something back means we pass null into SQLiteBridge right now, which is bad... I need to fix that.

@@ +114,5 @@
> +        Log.d(LOGTAG, "zerdatime " + SystemClock.uptimeMillis() + " - finish check sessionstore.bak exists");
> +        return hasSession;
> +    }
> +
> +    public String readFile(String filename) {

I really think we should make this api async and avoid giving ourselves a footgun. I didn't do that with GeckoDirProvider because it was being called from a ContentProvider and I wasn't really sure of the right way to make that happen.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Wesley Johnston (:wesj) from comment #5)
> ::: mobile/android/base/GeckoApp.java
> @@ +1647,5 @@
> >          }
> > +        // fall back to default profile if we didn't load a specific one
> > +        if (mProfile == null) {
> > +            mProfile = GeckoProfile.get(this);
> > +        }
> 
> Can we move this into getProfile (i.e. lazy init?)
> 

Sure. I'll have to change two other lines that use mProfile directly within GeckoApp to use getProfile() instead, then.

> ::: mobile/android/base/GeckoProfile.java
> @@ +1,1 @@
> > +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> 
> Might as well use MPL2 now: http://www.mozilla.org/MPL/headers/
> 

ok, will do.

> @@ +80,5 @@
> > +            if (profile == null) {
> > +                profile = new GeckoProfile(context, profileName);
> > +                sProfileCache.put(profileName, profile);
> > +            }
> > +            return profile;
> 
> Do we need to include the context's hashCode in this to differentiate
> further? (I realize this is my old code...)
> 

I wasn't entirely sure about this, but I don't think so. I believe the context will always be the same within a given android application, so if the context is different then the entire cached table will be different too (i.e. in the static space of a different application).

> @@ +101,5 @@
> > +                    Log.d(LOGTAG, "Found profile dir: " + mDir.getAbsolutePath());
> > +                }
> > +            } catch (IOException ioe) {
> > +                Log.e(LOGTAG, "Error getting profile dir", ioe);
> > +            }
> 
> Can we not just eat this up here? I like forcing callers to think about what
> they're getting back. For the passwords and browser db providers, not
> getting something back means we pass null into SQLiteBridge right now, which
> is bad... I need to fix that.
> 

Don't those pieces of code already check for null? I'd prefer to just document the possible null return value rather than throwing the IOException here; it seems like this is called often enough that the boilerplate needed to handle the IOException would just be annoying.

> @@ +114,5 @@
> > +    public String readFile(String filename) {
> 
> I really think we should make this api async and avoid giving ourselves a
> footgun. I didn't do that with GeckoDirProvider because it was being called
> from a ContentProvider and I wasn't really sure of the right way to make
> that happen.

I can file another bug for it. It's a little bit outside the scope of this bug though, since this is mostly a cleanup/refactoring patch.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fold GeckoDirProvider into GeckoProfile

Updated patch. This updates getProfile() to do the lazy init, updates the license to MPL 2.0 (as best I could figure out how to).

More importantly, it rebases on top of a more recent m-i which includes bnicholson's 5be23e1bd1e7 which requires some significant changes to the patch. The AboutHomeContent changes (specifically readLastTabs) and related GeckoProfile changes (readSessionFile, readFile) should be re-reviewed.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Also cc'ing bnicholson as an FYI since by the sounds of IRC chatter his patch increased startup time or something, and if that code needs to change it might be easier (or harder) to do it after this patch lands.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 597194 [details] [diff] [review]
Fold GeckoDirProvider into GeckoProfile

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

Mostly just comments, most of which may be dumb misunderstandings on my part. Since this is mostly just moving things around (although I think you've changed how file reading is done in a few places), I really REALLY wish we could put together some tests first so that we can know behavior isn't changing before and after.

::: mobile/android/base/AboutHomeContent.java
@@ -382,5 @@
> -        }
> -        if (fileStream == null)
> -            return null;
> -
> -        return readStringFromStream(fileStream);

This is now only used by readFromZipFile. Is there some reason you aren't moving it as well?

::: mobile/android/base/GeckoProfile.java
@@ +90,5 @@
> +            } else {
> +                Log.d(LOGTAG, "Found profile dir: " + mDir.getAbsolutePath());
> +            }
> +        } catch (IOException ioe) {
> +            Log.e(LOGTAG, "Error getting profile dir", ioe);

I think we wanted callers to have to deal with errors here rather than hoping they checked the return values.

@@ +113,5 @@
> +        File sessionFile = null;
> +        if (! geckoReady) {
> +            // we might have crashed, in which case sessionstore.js has tabs from last time
> +            sessionFile = new File(dir, "sessionstore.js");
> +            if (! sessionFile.exists()) {

We've been trying to avoid this exists call if we don't need it. The readFile below should throw if the file doesn't exist and we can catch the problem there I assume.

@@ +145,5 @@
> +            try {
> +                return readFile(target);
> +            } catch (IOException ioe) {
> +                Log.d(LOGTAG, "Unable to read " + target.getAbsolutePath());
> +            }

I don't like throwing this away and returning null. I have a feeling callers aren't going to check for null and we'll just wind up with problems. Can we just let the IOException trickle through?

@@ +150,5 @@
> +        }
> +        return null;
> +    }
> +
> +    private String readFile(File target) throws IOException {

All of these read and move file functions should be async. At the very least, can you file that?

@@ +208,5 @@
> +    }
> +
> +    private void moveProfilesFrom(File oldFilesDir) {
> +        if (oldFilesDir == null) {
> +            return;

Why would this be null? Are we wallpapering over a bug here?

@@ +234,5 @@
> +        // if we get here, we know that oldMozDir != currentMozDir, so we have some stuff to move
> +        moveDirContents(oldMozDir, currentMozDir);
> +    }
> +
> +    private void moveDirContents(File src, File dst) {

The old version of this has a quick "renameTo" path. Is there a reason we don't want that here?

@@ +238,5 @@
> +    private void moveDirContents(File src, File dst) {
> +        File[] files = src.listFiles();
> +        if (files == null) {
> +            src.delete();
> +            return;

Do you need to create dst in here somewhere?

@@ +265,5 @@
> +        }
> +        src.delete();
> +    }
> +
> +    private boolean moveFile(File src, File dst) {

The old version of this handled dst being a directory, as well as providing a quick "renameTo" path. Is there a reason we don't want them here?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to Wesley Johnston (:wesj) from comment #9)
> Mostly just comments, most of which may be dumb misunderstandings on my
> part. Since this is mostly just moving things around (although I think
> you've changed how file reading is done in a few places), I really REALLY
> wish we could put together some tests first so that we can know behavior
> isn't changing before and after.

I also wish we could put together some tests for this but as far as I know none of the existing frameworks allow for testing this, particularly the "move to SD card" functionality. I did test it manually as much as I was able though.

> > -
> > -        return readStringFromStream(fileStream);
> 
> This is now only used by readFromZipFile. Is there some reason you aren't
> moving it as well?
> 

I wanted to, but thought it could be done as a follow-up to avoid having this change snowball out of control.

> 
> I think we wanted callers to have to deal with errors here rather than
> hoping they checked the return values.
> 

The existing code already checks for null; I verified all the call sites to make sure of it.

> 
> We've been trying to avoid this exists call if we don't need it. The
> readFile below should throw if the file doesn't exist and we can catch the
> problem there I assume.
> 

Done.

> 
> I don't like throwing this away and returning null. I have a feeling callers
> aren't going to check for null and we'll just wind up with problems. Can we
> just let the IOException trickle through?
> 

Done, even though I prefer returning null rather than throwing an exception.

> 
> All of these read and move file functions should be async. At the very
> least, can you file that?
> 

Filed as bug 730644.

> 
> Why would this be null? Are we wallpapering over a bug here?
> 

No, if there is no external storage (e.g. if the SD card is taken out) then that could be legitimately null.

> 
> The old version of this has a quick "renameTo" path. Is there a reason we
> don't want that here?
> 
> Do you need to create dst in here somewhere?
> 
> The old version of this handled dst being a directory, as well as providing
> a quick "renameTo" path. Is there a reason we don't want them here?

The new code still keeps the renameTo as a fast path, and only falls back to manually moving the file or creating the directory if that fails. The diff might be a bit confusing because of the way I rearranged the code; the old code started with an attempt at a renameTo that would always fail whereas mine avoids that failure and just renames the things inside the folder. Whenever moveDirContents is invoked, the "dst" file always exists, so I don't need to create it inside that function itself.

Landed on inbound with above fixes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/92362d0c00e4

Comment 11

[Wesley Johnston (:wesj)]

Sorry for the review delay. Thanks kats!

Comment 12

[Richard Newman [:rnewman]]

A quick scan of 

https://hg.mozilla.org/integration/mozilla-inbound/file/92362d0c00e4/mobile/android/base/GeckoProfile.java

suggests that there should by synchronization to protect access to mDir and mMozDir (that is, getDir and ensureMozillaDirectory should be synchronized).

createProfileDir almost certainly is vulnerable to races. I'm running out the door, so this is just a quick drive-by; sorry for not providing details.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Richard Newman [:rnewman] from comment #12)
> A quick scan of 
> 
> https://hg.mozilla.org/integration/mozilla-inbound/file/92362d0c00e4/mobile/
> android/base/GeckoProfile.java
> 
> suggests that there should by synchronization to protect access to mDir and
> mMozDir (that is, getDir and ensureMozillaDirectory should be synchronized).
> 
> createProfileDir almost certainly is vulnerable to races. I'm running out
> the door, so this is just a quick drive-by; sorry for not providing details.

For some reason I remember convincing myself that there was no way the race would actually manifest. But you're right, there should still be synchronization in that class. I'll upload a follow-up shortly.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fix synchronization

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/92362d0c00e4

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 600818 [details] [diff] [review]
Fix synchronization

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

… it would actually be worse than the comment describes; this could result in two separate salted profile directories being created, as I think margaret has seen in the wild.

This looks good. I'll try to find time to do a brief concurrency examination of some of Fennec's code at some point, see if I can spot any more.

Comment 17

[Richard Newman [:rnewman]]

Saw you were away, kats, so I landed this on inbound to narrow the window of confusing half-landing :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef47598bb1e0

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/ef47598bb1e0