842262 - location of the ProfLD directory on Android has been unexpectedly changed [was: firefox android not loading fonts in the user profile directory anymore]

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Mathieu Pellerin]

For the last few days, something regressed in Firefox Android which broke loading of font in the firefox user profile directory (a feature introduced in bug 619521)

Step to reproduce:
1. Download the Khmer font add-on (https://addons.mozilla.org/en-us/android/addon/khmer-fonts-package/)
2. Open a web page with Khmer script (for e.g. http://khmerization.blogspot.com/)

Actual Result:
No Khmer displayed.

Expected Result:
Khmer :)

Comment 1

[Jonathan Kew [:jfkthame]]

Bisecting with m-c nightlies, this regressed in the Nightly build of 2013-02-13.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=36525224b14e&tochange=161a347bda5b

No obvious smoking gun in the pushlog there, AFAICS ... will try to reproduce and bisect further with local builds.

Comment 2

[Jonathan Kew [:jfkthame]]

The first bad revision is:
changeset:   121603:86b8a6ff664e
user:        Martin Stransky <[e-mail]>
date:        Mon Feb 11 15:47:21 2013 +0100
summary:     Bug 239254 - [Linux] Support disk cache on a local path, r=michal.novotny

I haven't really studied what the patch from bug 239254 is doing, but I guess maybe it is moving the font files from where the add-on puts them in the profile to some other location, where the font-loading code no longer sees them.

Nominating this for tracking, since it regresses the locally-added-fonts feature, being used to support Khmer (for example), in such a way as to make it unusable.

Comment 3

[Mathieu Pellerin]

Jonathan, would probably be worth adding a testcase for this feature to avoid undetected breakage like this one in the future.

Comment 4

[Jonathan Kew [:jfkthame]]

Mathieu, the issue may actually be a flaw in the Khmer fonts add-on. I notice that it uses the directory service with the "ProfD" key to get the profile directory:

  https://addons.mozilla.org/en-us/android/files/browse/183588/file/bootstrap.js#L12

This would correspond to NS_APP_USER_PROFILE_50_DIR in the C++ source. However, the fonts code actually looks in the "profile local" directory using the NS_APP_USER_PROFILE_LOCAL_50_DIR constant:

  http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFT2FontList.cpp#1095

(See http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsAppDirectoryServiceDefs.h#69 for the definitions of these constants and the corresponding strings.)

My guess is that until bug 239254, the "profile" and "profile local" directories were returning the same path on Android, but this is no longer the case. I don't know whether that was an intended change (will ask in that bug), but I suspect that if you update the add-on to use the "ProfLD" key, it'll start working again.

Comment 5

[Mathieu Pellerin]

Jonathan, ok, good catch.

Generally speaking, what is the difference btw the "profile" directory vs "profile local" directory? I had (obviously) no idea of this prof vs prof local deal.

Regarding the add-on itself, am I right to assume that I still have to access the add-on's directory via the "profile" directory, but then place the font(s) into a Fonts subdirectory within the "profile local" directory? I.e., would the add-on directory be accessible via the "profile local" path?

Comment 6

[Mathieu Pellerin]

Alright, tweaked the add-on. Whatever decision will be taken as part of bug 239254, it won't matter as it's now putting fonts through NS_APP_USER_PROFILE_LOCAL_50_DIR.

Btw, for the record, add-ons are placed in NS_APP_USER_PROFILE_50_DIR :)

Comment 7

[Jonathan Kew [:jfkthame]]

OK, I'm going to morph this bug to the more general "location of the ProfLD directory on Android has been changed", as that's the actual issue that exposed the inconsistency between the add-on and platform code.

Although the specific case of the Khmer fonts add-on can be resolved by tweaking that add-on (though I guess this may leave you with an "orphaned" copy of the font in the ProfD directory, wasting storage space?), the wider issue is that changing the ProfLD directory path may cause various features to lose access to previously-stored data. (See bug 239254 comment 83.)

cc'ing a bunch of people from bug 239254, as they're probably the best ones to address this issue.

Comment 8

[Jonathan Kew [:jfkthame]]

FTR, from bug 239254 comment 81:

By adding logging in a local build, I confirmed that prior to [bug 239254], ProfD and ProfLD resolved to the same directory:

I/Gecko   (17221): profile dir:       [/data/data/org.mozilla.fennec_jkew/files/mozilla/syhz4rjf.default]
I/Gecko   (17221): profile-local dir: [/data/data/org.mozilla.fennec_jkew/files/mozilla/syhz4rjf.default]

but after this landing, ProfLD now resolves to a directory under files/.cache:

I/Gecko   (17329): profile dir:       [/data/data/org.mozilla.fennec_jkew/files/mozilla/syhz4rjf.default]
I/Gecko   (17329): profile-local dir: [/data/data/org.mozilla.fennec_jkew/files/.cache/mozilla/syhz4rjf.default]

And from [:gcp] in bug 239254 comment 83:

This is going to make everything that was storing stuff in ProfLD to lose it's data, isn't it? So it will make existing SafeBrowsing databases invisible to the app, for example.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

>Generally speaking, what is the difference btw the "profile" directory vs "profile 
>local" directory

The profile directory is the one where the data that you can't regenerate is stored. The profile local directory is the one where caches etc go. If you move to a new machine, you shouldn't lose any data if your profile local gets inaccessible.

Profile might be on a server, profile local is always the local machine.

Profile data needs to be endian-agnostic, profile local may assume that the local machine doesn't change.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jonathan Kew (:jfkthame) from comment #8)
> So it will make existing SafeBrowsing databases
> invisible to the app, for example.

Isn't the SafeBrowsing database just "cached" content and will be re-downloaded if it goes missing? If so, then ProfLD seems like the right place for it, although we should probably just copy over the old file to avoid re-downloading.

OTOH, I understand that the ProfLD / ProfD difference doesn't really make sense on Android. We surely could make nsXREDirProvider not do that for Android.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

>Isn't the SafeBrowsing database just "cached" content and will be re-downloaded if 
>it goes missing? If so, then ProfLD seems like the right place for it, although we 
>should probably just copy over the old file to avoid re-downloading.

That's correct  (but move, not copy, seems preferable, due to the low amount of storage on some Android devices we support). I'm not saying it should move to ProfD (it's not endian-safe), I'm just pointing out the consequences of this bug.

If Firefox for Android had more users than it has right now, we couldn't just blow away all users caches without notifying the server operator (Google), but right now it doesn't really matter either way.

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Tim Taubert [:ttaubert] from comment #10)
> (In reply to Jonathan Kew (:jfkthame) from comment #8)
> > So it will make existing SafeBrowsing databases
> > invisible to the app, for example.
> 
> Isn't the SafeBrowsing database just "cached" content and will be
> re-downloaded if it goes missing? If so, then ProfLD seems like the right
> place for it, although we should probably just copy over the old file to
> avoid re-downloading.

Presumably so. Although more generally, if the location of ProfLD is changed, we'd want to purge stuff from the old directory so that we don't waste storage space forevermore with the no-longer-accessible cached data.

But I don't see how we could safely do that in the case where ProfD and ProfLD used to be the same directory, and then one of them is separated out (as happened here), because we don't necessarily know which items were being referred to through each key.

So such a "move" could only be executed cleanly if each component that makes use of ProfLD is aware of the change and takes responsibility for moving its own data.

> 
> OTOH, I understand that the ProfLD / ProfD difference doesn't really make
> sense on Android. We surely could make nsXREDirProvider not do that for
> Android.

That sounds fine to me (to the extent I understand all this).

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: keep ProfD == ProfLD on Android

That should do, I think?

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 715973 [details] [diff] [review]
keep ProfD == ProfLD on Android

Oops, that patch needs some fixing.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

I gess you need something like:

+#if defined(ANDROID)
+  rv = NS_NewNativeLocalFile(nsDependentCString(homeDir), true,
+                             getter_AddRefs(localDir));
+#else
   if (aLocal) {
      [...]
   }
+#endif

or set aLocal to FALSE

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Martin Stránský from comment #15)
> or set aLocal to FALSE

Yeah, that's better.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: keep ProfD == ProfLD on Android

Comment 18

[Mathieu Pellerin]

This will need to be uplifted to Aurora.

Comment 19

[bhavana bajaj [:bajaj]]

Marking the assignee to Tim as i see a WIP patch from him ..Feel free to reassign if needed.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

"WIP" :) Michal, review ping?

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/9f29eaf2b309

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f29eaf2b309

Comment 23

[Mathieu Pellerin]

Gentle reminder: this fix has to be applied to firefox v21 too as the issue was caused by a commit applied shortly before the end of the v21 cycle.

Comment 24

[Jonathan Kew [:jfkthame]]

Comment on attachment 715974 [details] [diff] [review]
keep ProfD == ProfLD on Android

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 239254
User impact if declined: loss of access to some resources stored/cached in the profile on Android, e.g. added fonts
Testing completed (on m-c, etc.): in Nightly since 03/03
Risk to taking this patch (and alternatives if risky): minimal
String or UUID changes made by this patch: none

Comment 25

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/daf58c4defdb