Closed Bug 842262 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Networking: Cache, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox20 --- unaffected
firefox21 + fixed

People

(Reporter: nirvn.asia, Assigned: ttaubert)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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 :)
OS: Windows 7 → Android
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.
Keywords: regression
Hardware: x86 → ARM
The first bad revision is:
changeset:   121603:86b8a6ff664e
user:        Martin Stransky <stransky@redhat.com>
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.
Blocks: 239254
Component: Layout: Text → Graphics: Text
Summary: firefox android not compiling fonts in the user profile directory anymore → firefox android not loading fonts in the user profile directory anymore
Jonathan, would probably be worth adding a testcase for this feature to avoid undetected breakage like this one in the future.
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.
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?
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 :)
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.
Component: Graphics: Text → Networking: Cache
Summary: firefox android not loading fonts in the user profile directory anymore → location of the ProfLD directory on Android has been unexpectedly changed [was: firefox android not loading fonts in the user profile directory anymore]
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.
>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.
(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.
>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.
(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).
Attached patch keep ProfD == ProfLD on Android (obsolete) — Splinter Review
That should do, I think?
Attachment #715973 - Flags: review?(michal.novotny)
Comment on attachment 715973 [details] [diff] [review]
keep ProfD == ProfLD on Android

Oops, that patch needs some fixing.
Attachment #715973 - Flags: review?(michal.novotny)
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
(In reply to Martin Stránský from comment #15)
> or set aLocal to FALSE

Yeah, that's better.
Attachment #715973 - Attachment is obsolete: true
Attachment #715974 - Flags: review?(michal.novotny)
This will need to be uplifted to Aurora.
Marking the assignee to Tim as i see a WIP patch from him ..Feel free to reassign if needed.
Assignee: nobody → ttaubert
"WIP" :) Michal, review ping?
Attachment #715974 - Flags: review?(mh+mozilla)
Attachment #715974 - Flags: review?(mh+mozilla) → review+
Attachment #715974 - Flags: review?(michal.novotny)
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/9f29eaf2b309
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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 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
Attachment #715974 - Flags: approval-mozilla-aurora?
Attachment #715974 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.