Closed Bug 606074 Opened 9 years ago Closed 9 years ago
In Android, the child process cannot figure out what timezone it is in
Related: bug 544414
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.
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.
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.
(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.
This only re-maps the environment variable for the child, and leaves it alone for the parent.
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.)
(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.
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.
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.