886925 - initial INIParser.parse to find default profile name at startup takes 75ms on a galaxy nexus

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Nathan Froyd [:froydnj]]

In the profile from bug 886528, GeckoApp.getProfile shows up as a major consumer of time in BrowserApp.onCreate().  Tracing through this, we get to INIParser.parse.  This function spends nearly all of its time in BufferedReader.readLine and FileReader.<init>, but maybe there's something that can be done to speed it up.

Comment 1

[(dormant account)]

Seems like we should hardcode profile name.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Taras Glek (:taras) from comment #1)
> Seems like we should hardcode profile name.

We can't hardcode the profile. We use profiles with webapps and we are looking to add mutliple profile support soon.

Comment 3

[Wesley Johnston (:wesj)]

We have to support multiple profiles, but we may be able to do something like not support setting the default in profiles.ini? i.e. If the user passes in a profile dir or name, use this to search, otherwise look for default and use it (or use a SharedPref?)

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Related to bug 894887

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP v1: Make the INIParser static

We create new INIParsers in a few locations. This patch keeps a single INIParser as a static. There is only one profiles.ini file, so making the INIParser a static should be fine.  The patch does try to collacse the use of the INIParser as well, by making the parser an arg to two helper methods.

No idea if this has a benefit or not. Maybe Shilpan can try to profile with this patch.

Comment 6

[Shilpan Bhagat]

GeckoProfile.getProfilesINI method tracing without patch: https://dl.dropboxusercontent.com/u/11916346/without%20patch.png

GeckoProfile.getProfilesINI method tracing with patch: https://dl.dropboxusercontent.com/u/11916346/with%20patch.png

There are no big wins with this patch. Especially because creating new INIParsers does not take much time/cpu. Because: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/INIParser.java#24

We do gain some insignificant win from this in every subsequent call after the first one, since there will be less file initializations,  which take more time then creating a new INIParser.

Comment 7

[Shilpan Bhagat]

Attachment: [WIP] Replacing the use of INIParsers with SharedPreferences

Essentially the patch removes INI parsing and file IO related to it, stores the current profile name in shared preferences previous profiles (if not removed) can be accessed via their profile names.

Should be extremely fast, will be profiling it in the next comment. :)

Comment 8

[Shilpan Bhagat]

Attachment: Result of removing INI parsing, minimizinf file IO and using shared prefs

My previous assessment with INI parsing showed that it took geckoprofile's get method spent about ~90 ms trying to get the profile. With the above patch, it goes down to ~2.5 ms.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 783427 [details] [diff] [review]
[WIP] Replacing the use of INIParsers with SharedPreferences

I am always a bit skeptical of using SharedPrefs. I started to look at the patch, but my initial feelings are:
* Make sure we have WebApp and Sync working well
* It's good that you have the code in GeckoProfile
* What failure cases might we expect if we do not keep profiles.ini up to date, for Gecko's sake?

More detailed comments coming.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 783427 [details] [diff] [review]
[WIP] Replacing the use of INIParsers with SharedPreferences

>diff --git a/mobile/android/base/GeckoProfile.java b/mobile/android/base/GeckoProfile.java

>+    private static final String SHARED_PREFERENCES_FILE_NAME = "MozillaFennec";

Looks like we normally use the Java class name, except for the mobile/android/base/background code which uses "background"

let's use "Profiles"
SHARED_PREFERENCES_FILE_NAME -> PREFS_NAME

>+    private static final String CURRENT_GECKO_PROFILE = "CurrentGeckoProfile";

Looks like we use a mix of styles here too: "this_style" and "ThisStyle", but "this_style" seems to be the Android convention

Let's use "last_profile"
CURRENT_GECKO_PROFILE -> LAST_PROFILE

>+    private static SharedPreferences getStoredProfileName(Context context) {

The name of this method is not what it returns. getSharedPreferences seems more appropriate

>+    public boolean setAsCurrentProfile() {

Just put this code in createProfileDir where we save to the INI now

But you never seem to call this method. The means when you try to read the last profile, it fails to load any XML file and the value returned from findDefaultProfile is null and BrowserApp just hardcodes "default". This might be affecting your timing results, since the XML shared prefs file is never actually loaded and read.

>+        if(!(mContext instanceof WebAppImpl)) {
>+            SharedPreferences.Editor editor = getStoredProfileName(mContext).edit();
>+            editor.putString(CURRENT_GECKO_PROFILE, mName);
>+            editor.apply();

Sadly, editor.apply is not supported in Android 2.2, which we still support. You'll need to use commit and try to make sure the call is on a background thread.

>     private boolean remove() {

>+            } else {
>+                mDir.delete();

This is a directory and File.delete will fail for non-empty directories. Use the delete(File) utility in GeckoProfile to make sure the folder is truly deleted.

>     public static String findDefaultProfile(Context context) {

>+        if(context instanceof WebAppImpl) {

if (

But I wonder if we even need this check here? We don't currently need to check for a WebApp, why should we need to now? Let's try without it.

>+            sDefaultProfileName = getStoredProfileName(context).getString(CURRENT_GECKO_PROFILE, null);

We should only need this part.

>-    private static String saltProfileName(String name) {

We have talked about not salting folder names. I guess we can try it.

Let's fix this up a bit and try re-running the timing measurements once we see the XML file being saved.

Comment 11

[Shilpan Bhagat]

Attachment: Part1: Removing INI parsing, minimizing file IO and using shared prefs

This patch makes changes to the previous WIP based on the above comments. Profiling after these changes shows that we still spend about 24ms reading from disk as compared to 85ms using ini parser (on S3). 

We can decrease the time even further via symlink. Another patch is coming up which uses symlink.

Comment 12

[Shilpan Bhagat]

Attachment: Part 2: Using Symlink instead of Shared Prefs

Yep, big win here. Using symlink gets it down to 3ms!

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 792480 [details] [diff] [review]
Part 2: Using Symlink instead of Shared Prefs

Can you just folder part 1 and part 2 into a singe patch?

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+        GeckoProfile.setAsDefault(this, getProfile().getDir());

I wonder if it's possible to know this in GeckoProfile and just set it in there

>diff --git a/mobile/android/base/GeckoProfile.java b/mobile/android/base/GeckoProfile.java

>     private static final String PREFS_NAME = "Profiles";
>     private static final String LAST_PROFILE = "last_profile";

Remove these too, right?

>+    private static native int createSymLink(String filePath,String linkPath);

nit: add space in args list

>+        try {
>+            File currFile = new File(ensureMozillaDirectory(context), "active-profile");

rename currFile -> activeDir

>+            if(currFile.exists()) {

nit: if (

>+    public static void setAsDefault (Context context, File defaultFile) {

nit: remove space before args list

>+        try {
>+            // Unlink from previous file
>+            removeSymLink(ensureMozillaDirectory(context).getAbsolutePath() + "/active-profile");

store the result of ensureMozillaDirectory since you use it twice?

>+            // Set as current profile

current -> active

>diff --git a/mozglue/android/APKOpen.cpp b/mozglue/android/APKOpen.cpp

>+extern "C" NS_EXPORT jint JNICALL
>+Java_org_mozilla_gecko_GeckoProfile_createSymLink(JNIEnv* jenv, jclass clazz, jstring inputFilePath, jstring inputSymLinkPath)
>+{
>+    const char *inputFilePathC    = jenv->GetStringUTFChars(inputFilePath, 0);
>+    const char *inputSymLinkPathC = jenv->GetStringUTFChars(inputSymLinkPath, 0);
>+    int result = symlink(inputFilePathC, inputSymLinkPathC);
>+
>+    //release resources when done
>+    jenv->ReleaseStringUTFChars(inputFilePath, inputFilePathC);
>+    jenv->ReleaseStringUTFChars(inputSymLinkPath, inputSymLinkPathC);
>+
>+    return result;
>+}
>+
>+extern "C" NS_EXPORT jint JNICALL
>+Java_org_mozilla_gecko_GeckoProfile_removeSymLink(JNIEnv* jenv, jclass clazz, jstring inputSymLinkPath)
>+{
>+    const char *inputSymLinkPathC = jenv->GetStringUTFChars(inputSymLinkPath, 0);
>+    int result = unlink(inputSymLinkPathC);
>+
>+    //release resources when done
>+    jenv->ReleaseStringUTFChars(inputSymLinkPath, inputSymLinkPathC);
>+
>+    return result;
>+}
>+

Have Kats or Blassey give feedback on these parts?

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

If we start using a symlinked profile, I think we should start *always* passing the --profile arg to GeckoThread. Not just for guest mode:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoThread.java#143

Comment 15

[Shilpan Bhagat]

Attachment: Patch: Using symlink instead of iniparser in GeckoProfile

Made changes as mentioned above. Need feedback on the native methods.

Comment 16

[Shilpan Bhagat]

> 
> >+        GeckoProfile.setAsDefault(this, getProfile().getDir());
> 
> I wonder if it's possible to know this in GeckoProfile and just set it in
> there
> 

I tried setting it up every time we create a new profile, however, creating a new profile does not correspond to defaulting it. For eg: We create a guest profile on launch, and then the default profile. If we defaulting it everytime a profile gets created, the guest profile will be defaulted and the "default" profile will never be created. I found this way to be more meaningful since we can control which profile we default. (Thinking about future scenarios)

Comment 17

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

Comment on attachment 792967 [details] [diff] [review]
Patch: Using symlink instead of iniparser in GeckoProfile

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

I just looked at the native function calls and ignored everything else. Generally they look fine but I think more error-handling is needed.

::: mobile/android/base/GeckoProfile.java
@@ +447,5 @@
> +            // Unlink from previous file
> +            removeSymLink(ensureMozillaDirectory(context).getAbsolutePath() + "/active-profile");
> +
> +            // Set as active profile
> +            createSymLink(defaultFile.getAbsolutePath(), ensureMozillaDirectory(context).getAbsolutePath() + "/active-profile");

If either of these calls return nonzero then you should at the very least log the error, and probably crash. If the symlink call fails then I don't know what gecko will do - will it create a new folder at /active-profile and populate it with a profile? If so, could a subsequent run of symlink succeed and "hide" the /active-profile folder with the symlinked one? Random failures in symlink may manifest as extremely strange behaviour to the user, which will be hard to diagnose without proper logging or reporting.

::: mozglue/android/APKOpen.cpp
@@ +302,5 @@
> +extern "C" NS_EXPORT jint JNICALL
> +Java_org_mozilla_gecko_GeckoProfile_createSymLink(JNIEnv* jenv, jclass clazz, jstring inputFilePath, jstring inputSymLinkPath)
> +{
> +    const char *inputFilePathC    = jenv->GetStringUTFChars(inputFilePath, 0);
> +    const char *inputSymLinkPathC = jenv->GetStringUTFChars(inputSymLinkPath, 0);

Need to check return values for nullness. Also please pass NULL instead of 0 for the last parameter, since it's a jboolean* argument.

@@ +315,5 @@
> +
> +extern "C" NS_EXPORT jint JNICALL
> +Java_org_mozilla_gecko_GeckoProfile_removeSymLink(JNIEnv* jenv, jclass clazz, jstring inputSymLinkPath)
> +{
> +    const char *inputSymLinkPathC = jenv->GetStringUTFChars(inputSymLinkPath, 0);

Ditto

Comment 18

[Shilpan Bhagat]

Attachment: Patch: Using symlink instead of iniparser in GeckoProfile

Made changes as mentioned. Also throws when symlinking fails. In anycase, symlink does not create a new folder in case the target filePath is missing. It simply throws an error. This patch should also take care of all those edge cases.

Comment 19

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

Comment on attachment 793107 [details] [diff] [review]
Patch: Using symlink instead of iniparser in GeckoProfile

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

::: mobile/android/base/GeckoProfile.java
@@ +457,5 @@
> +            if (result == -1) {
> +                if (activeFile.exists()) {
> +                    throw new IOException("File or directory already exists at " + symlinkPath);
> +                } else {
> +                    throw new IOException("Unable to symlink active profile at " + symlinkPath);

These exceptions are going to get caught by the catch block three lines down. Is that intentional?

::: mozglue/android/APKOpen.cpp
@@ +304,5 @@
> +{
> +    const char* inputFilePathC    = jenv->GetStringUTFChars(inputFilePath, NULL);
> +    const char* inputSymLinkPathC = jenv->GetStringUTFChars(inputSymLinkPath, NULL);
> +    if (inputFilePathC == NULL || inputSymLinkPathC == NULL)
> +      return -1;

if inputSymLinkPathC is null you still need to do a ReleaseStringUTFChars on inputFilePathC before returning.

Comment 20

[Shilpan Bhagat]

Attachment: Patch: Using symlink instead of iniparser in GeckoProfile

This approach makes more sense to me. What do you think Kats?

Comment 21

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

Comment on attachment 793157 [details] [diff] [review]
Patch: Using symlink instead of iniparser in GeckoProfile

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

Doing it this way (i.e. throwing from the JNI code) is fine by me, but you still need to do proper error handling for nulls and such.

::: mozglue/android/APKOpen.cpp
@@ +302,5 @@
> +extern "C" NS_EXPORT jint JNICALL
> +Java_org_mozilla_gecko_GeckoProfile_createSymLink(JNIEnv* jenv, jclass clazz, jstring inputFilePath, jstring inputSymLinkPath)
> +{
> +    const char* inputFilePathC    = jenv->GetStringUTFChars(inputFilePath, NULL);
> +    const char* inputSymLinkPathC = jenv->GetStringUTFChars(inputSymLinkPath, NULL);

You still need to have null checks here.

@@ +311,5 @@
> +    jenv->ReleaseStringUTFChars(inputFilePath, inputFilePathC);
> +    jenv->ReleaseStringUTFChars(inputSymLinkPath, inputSymLinkPathC);
> +
> +    if (result == -1)
> +      JNI_Throw(jenv, "java/io/IOException", strerror(errno));

This should be jenv->ThrowNew(jenv->FindClass("java/io/IOException"), sterror(errno));

Also, this reminds me, both functions should be 2-space indented.

@@ +313,5 @@
> +
> +    if (result == -1)
> +      JNI_Throw(jenv, "java/io/IOException", strerror(errno));
> +
> +    return result;

No need to return this if you're doing the error handling here. Just change the return type to void.

Comment 22

[Shilpan Bhagat]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> Comment on attachment 793157 [details] [diff] [review]
> Patch: Using symlink instead of iniparser in GeckoProfile
> 
> Review of attachment 793157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Doing it this way (i.e. throwing from the JNI code) is fine by me, but you
> still need to do proper error handling for nulls and such.
> 
> ::: mozglue/android/APKOpen.cpp
> @@ +302,5 @@
> > +extern "C" NS_EXPORT jint JNICALL
> > +Java_org_mozilla_gecko_GeckoProfile_createSymLink(JNIEnv* jenv, jclass clazz, jstring inputFilePath, jstring inputSymLinkPath)
> > +{
> > +    const char* inputFilePathC    = jenv->GetStringUTFChars(inputFilePath, NULL);
> > +    const char* inputSymLinkPathC = jenv->GetStringUTFChars(inputSymLinkPath, NULL);
> 
> You still need to have null checks here.
>
Doesn't the symlink method take care of that for us? Afaik it will return -1 and set errno which we can throw. Do we still need null checks? And if we do, shouldn't we throw something? 
> @@ +311,5 @@
> > +    jenv->ReleaseStringUTFChars(inputFilePath, inputFilePathC);
> > +    jenv->ReleaseStringUTFChars(inputSymLinkPath, inputSymLinkPathC);
> > +
> > +    if (result == -1)
> > +      JNI_Throw(jenv, "java/io/IOException", strerror(errno));
> 
> This should be jenv->ThrowNew(jenv->FindClass("java/io/IOException"),
> sterror(errno));
> 
Any reason why not use JNI_Throw method we defined for throwing? 
http://mxr.mozilla.org/mozilla-central/source/mozglue/android/APKOpen.cpp#111
> @@ +313,5 @@
> > +
> > +    if (result == -1)
> > +      JNI_Throw(jenv, "java/io/IOException", strerror(errno));
> > +
> > +    return result;
> 
> No need to return this if you're doing the error handling here. Just change
> the return type to void.
Yeah, I was thinking there could be times when we would want to provide an alternate approach when symlink fails since throwing here does not crash the app. But I agree this does feel hacky. C++ does this though.

Comment 23

[Shilpan Bhagat]

On the comments above

Comment 24

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

(In reply to Shilpan Bhagat from comment #22)
> > You still need to have null checks here.
> >
> Doesn't the symlink method take care of that for us? Afaik it will return -1
> and set errno which we can throw. Do we still need null checks? And if we
> do, shouldn't we throw something? 

I didn't think it would but I guess it makes more sense if it does. Still worth testing to make sure this doesn't blow up or anything.

> > This should be jenv->ThrowNew(jenv->FindClass("java/io/IOException"),
> > sterror(errno));
> > 
> Any reason why not use JNI_Throw method we defined for throwing? 
> http://mxr.mozilla.org/mozilla-central/source/mozglue/android/APKOpen.cpp#111

Ah, my bad. Yeah, using JNI_Throw is fine.

> > No need to return this if you're doing the error handling here. Just change
> > the return type to void.
> Yeah, I was thinking there could be times when we would want to provide an
> alternate approach when symlink fails since throwing here does not crash the
> app. But I agree this does feel hacky. C++ does this though.

Ah, I see. I would still rather take it out, because there shouldn't be any other C++ code calling this JNI function directly, and any Java code that calls can put its error handling in the catch block for the exception that gets thrown.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 793157 [details] [diff] [review]
Patch: Using symlink instead of iniparser in GeckoProfile

>diff --git a/mobile/android/base/GeckoThread.java b/mobile/android/base/GeckoThread.java

>     private String addCustomProfileArg(String args) {
>         String profile = "";
>         if (GeckoAppShell.getGeckoInterface() != null) {
>-            if (GeckoAppShell.getGeckoInterface().getProfile().inGuestMode()) {
>-                try {
>-                    profile = " -profile " + GeckoAppShell.getGeckoInterface().getProfile().getDir().getCanonicalPath();
>-                } catch (IOException ioe) { Log.e(LOGTAG, "error getting guest profile path", ioe); }
>-            } else if (GeckoApp.sIsUsingCustomProfile) {
>-                profile = " -P " + GeckoAppShell.getGeckoInterface().getProfile().getName();
>-            }
>+            try {
>+                profile = " -profile " + GeckoAppShell.getGeckoInterface().getProfile().getDir().getCanonicalPath();
>+            } catch (IOException ioe) { Log.e(LOGTAG, "error getting profile path", ioe); }


You are removing the code that handles GeckoApp.sIsUsingCustomProfile and that worries me. I would like nothing more than to remove that code, but until we know for sure that we can, lets re-org the method a bit:

            if (GeckoApp.sIsUsingCustomProfile) {
                profile = " -P " + GeckoAppShell.getGeckoInterface().getProfile().getName();
            } else {
                try {
                    profile = " -profile " + GeckoAppShell.getGeckoInterface().getProfile().getDir().getCanonicalPath();
                } catch (IOException ioe) {
                    Log.e(LOGTAG, "error getting profile path", ioe);
                }
            }

Make sense? r+ with that change and do a Try server run before landing!

Comment 26

[Shilpan Bhagat]

https://tbpl.mozilla.org/?tree=Try&rev=50108803eca5

push to try on this patch turns up all green. I will make these changes and put another patch with that soon. They seem to be trivial.

Comment 27

[Shilpan Bhagat]

Attachment: Patch: Using symlink instead of iniparser in GeckoProfile

Carry forward, with the mentioned changes.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply cleanly to fx-team. Please rebase and request checkin again.

Comment 29

[Shilpan Bhagat]

Attachment: Patch: Using symlink instead of iniparser in GeckoProfile

Should not break anymore.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/193e7c0052a9

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/193e7c0052a9

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

Backed out:
https://hg.mozilla.org/mozilla-central/rev/dc7b76fcf7e4

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

Some profile related logcat output from my Nightly update:

W/GeckoProfile(18881): requested profile directory missing: null
D/GeckoProfile(18881): Created new profile dir at /data/data/org.mozilla.fennec/files/mozilla/default
D/GeckoHealthRec(18881): Initializing profile cache.
D/GeckoHealthRec(18881): Using times.json for profile creation time.
D/GeckoHealthRec(18881): Incorporating environment: times.json profile creation = 1377713907362
I/GeckoThread(18881): RunGecko - args =  -profile /data/data/org.mozilla.fennec/files/mozilla/default --guest

Two things:
1. We create a new unsalted "default" profile instead of using my existing salted "default" profile
2. We pass the "--guest" arg to Gecko, even though the main profile is not "guest" and we are not passing the "guest" profile to Gecko. The menu menu shows "Enter Guest Session" so I am not guest browsing.

I think #1 is the cause of many bugs. #2 needs to be figured out separately.

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

To fix #1 we need to kepp the profile.ini reading code and use it if the symlink does not exist.
* symlink = fast path
* profile.ini = slow path

Once the proper default folder is found, we create the symlink and the next time we use the symlink (fast path).

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking this because of bug 910378 and users losing data on Nightly.

Comment 36

[Shilpan Bhagat]

Attachment: Patch: Using symlink instead of iniparser in GeckoProfile

Now has iniparser for legacy reasons. This was a really bad mixup. Sorry about that.

Renamed "active-profile" to "activeProfile" for users who were affected by nightly.

Comment 37

[Richard Newman [:rnewman]]

N.B., any code that touches GeckoProfile needs to test:

* Sync
* Guest mode
* FHR document generation, recording, and non-UI generation (submission).

And, in order to repro today's bug: you need a populated profile *before* upgrading to the code under test.

Comment 38

[Richard Newman [:rnewman]]

Clearing target milestone after backout.

Comment 39

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 796956 [details] [diff] [review]
Patch: Using symlink instead of iniparser in GeckoProfile

>diff --git a/mobile/android/base/GeckoProfile.java b/mobile/android/base/GeckoProfile.java

> public final class GeckoProfile {
>     private static final String LOGTAG = "GeckoProfile";
>     // Used to "lock" the guest profile, so that we'll always restart in it
>     private static final String LOCK_FILE_NAME = ".active_lock";

Let's create:
      private static final String SYMLINK_NAME = "active-profile.1";

(the ".1" is a poor man's versioning system)

>     public static String findDefaultProfile(Context context) {

>+            // Open profiles.ini to find the correct path if a symlink does not exist.
>+            INIParser parser = getProfilesINI(context);
>+            for (Enumeration<INISection> e = parser.getSections().elements(); e.hasMoreElements();) {
>+                INISection section = e.nextElement();
>+                if (section.getIntProperty("Default") == 1) {
>+                    sDefaultProfileName = section.getStringProperty("Name");
>+
>+                    // Rename salted directory to match the name.
>+                    File currDir = new File(section.getStringProperty("Path"));
>+                    File newDir = new File(ensureMozillaDirectory(context), sDefaultProfileName);
>+                    currDir.renameTo(newDir);
>+
>+                    return sDefaultProfileName;

I'm not 100% sure renaming the folder is safe to do at this point. We need to think about that. It might be easier to leave it alone and workaround having the "salt." prefix

>+    private File findProfileDir(File mozillaDir) {
>+        File nameDir = new File(mozillaDir, mName);
>+        if (! nameDir.exists()) {
>+            return null;
>+         }
>+         return nameDir;

It makes more sense to me to test for positive:
          if (nameDir.exists()) {
              return namrDir;
           }
           return null;

>diff --git a/mobile/android/base/GeckoThread.java b/mobile/android/base/GeckoThread.java

>     private String addCustomProfileArg(String args) {

>-            if (GeckoAppShell.getGeckoInterface().getProfile().inGuestMode()) {
>+            if (GeckoApp.sIsUsingCustomProfile) {
>+                profile = " -P " + GeckoAppShell.getGeckoInterface().getProfile().getName();
>+            } else {
>                 try {
>                     profile = " -profile " + GeckoAppShell.getGeckoInterface().getProfile().getDir().getCanonicalPath();
>                 } catch (IOException ioe) { Log.e(LOGTAG, "error getting guest profile path", ioe); }
> 
>                 if (args == null || !args.contains(BrowserApp.GUEST_BROWSING_ARG)) {
>                     guest = " " + BrowserApp.GUEST_BROWSING_ARG;
>                 }

One of the bugs from the original patch was it always sent "--guest" here. I think it's because we removed the "inGuestMode" check at the top. You should add it back:

                  if (GeckoAppShell.getGeckoInterface().getProfile().inGuestMode()) {
                      if (args == null || !args.contains(BrowserApp.GUEST_BROWSING_ARG)) {
                          guest = " " + BrowserApp.GUEST_BROWSING_ARG;
                      }
                  }

Comment 40

[Wesley Johnston (:wesj)]

Shilpan, just curious if you'd had any time to look at this?

Comment 41

[Shilpan Bhagat]

Attachment: Patch: Using symlink instead of iniparser in GeckoProfile (w/ legacy iniParser)

This patch takes care of the above mentioned comments, renaming is not required. Sync, bookmarks and health report seems to work fine with this patch.

Sorry, for taking so long. I was waiting for my account to open up again and then school caught up. :( 

Will be more readily available now.

Comment 42

[Shilpan Bhagat]

Oh and guest mode works fine too!

Comment 43

[Shilpan Bhagat]

(In reply to Wesley Johnston (:wesj) from comment #40)
> Shilpan, just curious if you'd had any time to look at this?

Clearing needinfo, I will continue working on this from now

Comment 44

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 810369 [details] [diff] [review]
Patch: Using symlink instead of iniparser in GeckoProfile (w/ legacy iniParser)

I was supposed to send this to Brian a while ago. That said, I did want to wait for a new merge before landing it, if it gets an r+.

Comment 45

[Lukas Blakk [:lsblakk] use ?needinfo]

Not clear what the risk/reward is here and the nightly issue in bug 910378 is resolved so no longer tracking here.

Comment 46

[Brian Nicholson (:bnicholson)]

Comment on attachment 810369 [details] [diff] [review]
Patch: Using symlink instead of iniparser in GeckoProfile (w/ legacy iniParser)

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

::: mobile/android/base/GeckoProfile.java
@@ +29,5 @@
>      private static final String LOCK_FILE_NAME = ".active_lock";
>  
>      private static HashMap<String, GeckoProfile> sProfileCache = new HashMap<String, GeckoProfile>();
>      private static String sDefaultProfileName = null;
> +    private static File mMozDir;

s/mMozDir/sMozDir/

@@ +479,4 @@
>              }
> +
> +            // Set as active profile
> +            createSymLink(defaultFile.getAbsolutePath(), symlinkPath);

Looks like we're unconditionally removing and recreating the existing link, even if it's the same as the one we're given. How about we see if defaultFile.getCanonicalFile().getAbsolutePath == activeFile.getCanonicalFile().getAbsolutePath(), and if so, return without doing anything?

Comment 47

[microrffr]

I think the profile salt is a security feature. As I understand it, this change allows access to the profile under a constant path. Does this need security review?

Comment 48

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.