Closed Bug 606074 Opened 9 years ago Closed 9 years ago

In Android, the child process cannot figure out what timezone it is in

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: jmaher, Assigned: crowderbt)

References

Details

Attachments

(1 file, 3 obsolete files)

It appears that when accessing the timezone from javascript on fennec/android, we don't report it correctly.  Here is a page that does some dates/timezone stuff:
http://lassey.us/date.html

blassey, can you fill in more information here...
Related: bug 544414
Blocks: 616377
Assignee: nobody → crowderbt
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Is this still an issue?
afaik, it is, we haven't done anything to address it.  I haven't looked at it yet, though.
Yes, this is still an issue.  I suspect localtime_r, still investigating
This is a bit kludgy and I'm worried that it's the first of a host of system properties or environment variables that we're not exporting to the child which should be.  Unfortunately, I don't know of a different approach, so here we go.
Attachment #504833 - Flags: review?(jones.chris.g)
Summary: we don't appear to be reporting the timezone correctly for android devices when accessed by javascript → In Android, the child process cannot figure out what timezone it is in
Ha, I just saw this bug today when checking my flight status :).
Comment on attachment 504833 [details] [diff] [review]
Export "TZ" variable (or system property) to child as TZ envvar

This patch makes me rather uneasy.  The patch implies we're saying one or both of
 (1) TZ isn't set in the chrome process or content process
 (2) TZ is set for the chrome/java process, but isn't being passed through to the child

If we're seeing (1), then apparently bionic is buggy, and I have no idea how localtime() (really tzset()) would work even in the chrome process.  By comparison, glibc, according to my reading of relevant man pages, is spec'd to look up the format in /etc/localtime (or whatever per distro) if TZ isn't in the env.  bionic ought to be do the same, using whatever android thingie if necessary.

If we're seeing (2), what's TZ set to?  If it's the tz spec directly, instead of an indirection to system property or file, then content processes aren't going to honor timezone changes and so this patch is incomplete (could do this in a followup).

Clearing review awaiting more info.
Attachment #504833 - Flags: review?(jones.chris.g)
Yes, in my testing, TZ is actually not set in either process.  The bionic libc actually checks the system property (using code much like mine, checking the system property after failing to find the envvar), it does not consult any file, as far as I can tell, from looking at the android sources.
Attachment #504833 - Flags: review?(jones.chris.g)
(In reply to comment #8)
> Yes, in my testing, TZ is actually not set in either process.  The bionic libc
> actually checks the system property (using code much like mine, checking the
> system property after failing to find the envvar), it does not consult any
> file, as far as I can tell, from looking at the android sources.

Hm ... so if that's true, it sounds like bionic is doing the right thing, and I don't understand what's broken in the content process.  Is the content process not able to read the system property?
That's exactly what I've concluded.  For some reason (perhaps because it is not invoked from Dalvik?  Or has no Java components?) the child process does not see system properties, afaict.
It would be good to know why __system_property_get() isn't working from content processes.  (I can't imagine why that would be tied to dalvik/java, but who knows ...)  That being broken might hurt us elsewhere.
Perhaps that (and the removal of this code) could happen in a low-priority follow-up bug?
If we did that, then this temporary code will also need to forward timezone changes, and isn't clear how to do that efficiently.  Or that would need to be another possibly-temporary followup.
(And it would be nice to know why __system_property_get() is broken so that we can decide which path is a better use of time.)
This is what we discussed on IRC:

We read the relevant environment variable, using the fd number it specifies to create a new mapping of the shared fd for the child, and edit the envvar to point to the new fd number.

I've tested this as early in the child process as possible, and we are able to conduct normal operations against the shared map (ie., __system_property_get() works) without any other changes.
Attachment #504833 - Attachment is obsolete: true
Attachment #505211 - Flags: review?(jones.chris.g)
Attachment #504833 - Flags: review?(jones.chris.g)
Attached patch more cautious/correct approach (obsolete) — Splinter Review
This only re-maps the environment variable for the child, and leaves it alone for the parent.
Attachment #505211 - Attachment is obsolete: true
Attachment #505222 - Flags: review?(jones.chris.g)
Attachment #505211 - Flags: review?(jones.chris.g)
Comment on attachment 505222 [details] [diff] [review]
more cautious/correct approach

The |fd != 5| check is wrong, because we don't know what this fd will in the chrome process.  Don't need to include <sys/system_properties.h>.   Please make the "5" a static constant with a link to the last magic fd, which lives in nsCrashReporter.cpp.  (Next one we allocate, I'll shore this system up a bit.)
Attachment #505222 - Flags: review?(jones.chris.g)
(In reply to comment #17)
> Comment on attachment 505222 [details] [diff] [review]
> more cautious/correct approach
> 
> The |fd != 5| check is wrong, because we don't know what this fd will in the
> chrome process.

And to clarify --- we don't need it at all.
Attached patch fixes appliedSplinter Review
Though I addressed cjones' concerns about the "fd == 5" paragraph on IRC, I agree that it mildly obfuscates the code for very little benefit, so I've made the code in that section unconditional, and added the constant as requested.

:cjones, it'd be good if you'd look over the comment/style again, but I won't hold landing on that.
Attachment #505222 - Attachment is obsolete: true
Attachment #505428 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/7812a04bcaa9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
For posterity, cjones did give me an "r=me with the other stuff fixed" on IRC.  :)
http://hg.mozilla.org/mozilla-central/rev/a5f2517c3b73 <--- removing the sys/system_properties.h include, whoops.
Verified fixed, using:
Mozilla/5.0 (Android; Linux armv7l; rv:2.0b13pre) Gecko/20110314
Firefox/4.0b13pre Fennec/4.0b6pre

I assumed that http://lassey.us/date.html would fail or give some other result than on desktop Firefox.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.