Closed Bug 809942 Opened 9 years ago Closed 9 years ago

ToolkitProfile service can't change default profile

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 20

People

(Reporter: wesj, Assigned: mfinkle)

Details

Attachments

(1 file, 1 obsolete file)

mfinkle has been trying to update his profiles addon:

http://people.mozilla.com/%7Emfinkle/fennec/addons/

but using the toolkit profile service doesn't seem to be working anymore. We should be honoring things that set Default in profiles.ini. Its possible that we're using a cache or something that overwrites the changes. Need more investigation.
Attached patch WIP patch (obsolete) — Splinter Review
This patch adds a public method to get the default profile name. The default profile is defined as "Default=1" in profiles.ini, not just "default".

The patch then uses the method to implement App.getDefaultProfileName

This seems to fix the problem. Without using this, the "default" profile was always ebing used.

I tested this with WebApps too and they seem to be working with the patch.
Assignee: nobody → mark.finkle
Attachment #682497 - Flags: feedback?(wjohnston)
Comment on attachment 682497 [details] [diff] [review]
WIP patch

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

I think this is fine. Just worried about performance impact.

::: mobile/android/base/App.java.in
@@ +25,5 @@
>          return "@MOZ_CHILD_PROCESS_NAME@";
>      }
>  
>      protected String getDefaultProfileName() {
> +        String profile = GeckoProfile.findDefaultProfile(this);

So this does File I/O now. I wonder if we can cache the default (which would causes this to be incorrect if the default was changed by JS and Fennec not restarted yet). Can we do something like look at the timestamp on the file and use that determine whether to use the cache?

::: mobile/android/base/GeckoProfile.java
@@ +57,5 @@
> +        synchronized (sProfileCache) {
> +            GeckoProfile profile = sProfileCache.get(profileName);
> +            if (profile != null)
> +                return profile;
> +        }

Oh wow. Good catch.
Attachment #682497 - Flags: feedback?(wjohnston) → feedback+
(In reply to Wesley Johnston (:wesj) from comment #2)
> Comment on attachment 682497 [details] [diff] [review]
> WIP patch
> 
> Review of attachment 682497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this is fine. Just worried about performance impact.
> 
> ::: mobile/android/base/App.java.in
> @@ +25,5 @@
> >          return "@MOZ_CHILD_PROCESS_NAME@";
> >      }
> >  
> >      protected String getDefaultProfileName() {
> > +        String profile = GeckoProfile.findDefaultProfile(this);
> 
> So this does File I/O now. I wonder if we can cache the default (which would
> causes this to be incorrect if the default was changed by JS and Fennec not
> restarted yet). Can we do something like look at the timestamp on the file
> and use that determine whether to use the cache?

I think it swaps the I/O so there should only be the same number of I/O. That said, I am adding a sDefaultProfileName cache to GetProfile, which will enforce this better.
Attached patch patchSplinter Review
This patch removes the logging and adds the sDefaultProfileName cache
Attachment #682497 - Attachment is obsolete: true
Attachment #684127 - Flags: review?(wjohnston)
Comment on attachment 684127 [details] [diff] [review]
patch

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

::: mobile/android/base/GeckoProfile.java
@@ +119,5 @@
>          if (!TextUtils.isEmpty(profilePath)) {
>              File dir = new File(profilePath);
>              if (dir.exists() && dir.isDirectory()) {
>                  if (mDir != null) {
> +                    Log.i(LOGTAG, "profile dir changed from " + mDir + " to " + dir);

I wonder if we should remove this. Seems like a potential privacy leak.
Attachment #684127 - Flags: review?(wjohnston) → review+
Removed the Log.i

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b4caaea44f
Target Milestone: --- → Firefox 20
https://hg.mozilla.org/mozilla-central/rev/b5b4caaea44f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.