Broadcast NotifyTimezoneChange to all RenderProcessHosts when the system time zone changes

Broadcast NotifyTimezoneChange to all RenderProcessHosts when the system time
zone changes.

On Mac, this listens for NSSystemTimeZoneDidChangeNotification notifications.
The Mac implementation also contains a sandbox hole to allow time zone
information (/etc/localtime and /usr/share/zoneinfo) to be read from within
sandboxed renderer processes.

On Windows, this listens for WM_TIMECHANGE messages.

On Linux (but not Chrome OS or Android), this watches for changes to
/etc/localtime, /etc/timezone, and /etc/TZ. There isn't a better standard for
watching for time zone changes on Linux. The actual mechanism is
libc-specific, but this should work with common libc implementations. Time
zone watching is suppressed if the TZ environment variable is set. Linux
(including Chrome OS and Android) already contains a sandbox workaround
(ProxyLocaltimeCallToBrowser) that allows renderer processes to get time zone
information from the browser.

On Android, this uses a Java bridge to listen for
Intent.ACTION_TIMEZONE_CHANGED.

This unifies the existing time zone change notification from Chrome OS with
the other platform implementations.

On Mac, Linux, Chrome OS, and Android renderers should pick up the new time
zone name and UTC offset. On Windows, renderers may only be able to pick up
the new UTC offset only, and the sandbox may blocks them from picking up the
new time zone name, although in my test on Windows Server 2012, they were able
to pick up both the UTC offset and the time zone name upon change.

This is a continuation of https://codereview.chromium.org/183763041/ (Mac) and
https://codereview.chromium.org/193763002/ (Windows).

BUG=288697
TEST=http://crbug.com/288697#c12: load the page, change the system time zone,
     and then click "recheck" to ensure that the renderer picks up the new
     time zone. Don't reload the page, which is likely to give you a new
     renderer process, use the "recheck" link on the page.

R=bulach@chromium.org, cpu@chromium.org, jeremy@chromium.org, jln@chromium.org, jochen@chromium.org, pastarmovj@chromium.org, rsesek@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267226

[Mark Mentovai, 2014-04-28 20:48:40]

Reviewer selection:
 - jochen for content and overall non-platform-specific stuff
 - rsesek for Mac and
 - jeremy for the Mac sandbox profile
 - cpu for Windows
 - jln for Linux
 - pastarmovj for Chrome OS
 - bulach for Android

[Robert Sesek, 2014-04-28 20:53:38]

https://codereview.chromium.org/251613002/diff/220001/content/browser/time_zo...
File content/browser/time_zone_monitor.h (right):

https://codereview.chromium.org/251613002/diff/220001/content/browser/time_zo...
content/browser/time_zone_monitor.h:33: static TimeZoneMonitor* Create();
I think the preference now is to return a scoped_ptr<T> from Create methods so
that the object can't ever accidentally leak.

https://codereview.chromium.org/251613002/diff/220001/content/browser/time_zo...
content/browser/time_zone_monitor.h:35: TimeZoneMonitor();
Should the ctor be public if you have a Create method?

https://codereview.chromium.org/251613002/diff/220001/content/browser/time_zo...
File content/browser/time_zone_monitor_mac.mm (right):

https://codereview.chromium.org/251613002/diff/220001/content/browser/time_zo...
content/browser/time_zone_monitor_mac.mm:35: }  // namespace content
Fold these two namespaces (this and the one that starts on line 37) into one.

[Mark Mentovai, 2014-04-28 21:08:37]

I made the other two changes.

https://codereview.chromium.org/251613002/diff/220001/content/browser/time_zo...
File content/browser/time_zone_monitor.h (right):

https://codereview.chromium.org/251613002/diff/220001/content/browser/time_zo...
content/browser/time_zone_monitor.h:35: TimeZoneMonitor();
rsesek wrote:
> Should the ctor be public if you have a Create method?

Create’s a static method from the base class. It doesn’t have access to anything
that’s not public.

[Robert Sesek, 2014-04-28 21:14:48]

time_zone_monitor_mac.mm lgtm

https://codereview.chromium.org/251613002/diff/220001/content/browser/time_zo...
File content/browser/time_zone_monitor.h (right):

https://codereview.chromium.org/251613002/diff/220001/content/browser/time_zo...
content/browser/time_zone_monitor.h:35: TimeZoneMonitor();
On 2014/04/28 21:08:38, Mark Mentovai wrote:
> rsesek wrote:
> > Should the ctor be public if you have a Create method?
> 
> Create’s a static method from the base class. It doesn’t have access to
anything
> that’s not public.

Right, but is it meaningful to actually construct a TimeZoneMonitor base class?
It seems like you should restrict the public API to what people should actually
call.

[Mark Mentovai, 2014-04-28 21:20:48]

The base class constructor is protected now, but the subclass constructors need
to remain public. But they’re not exposed to anyone via a header (except for the
Android one, which requires a header for the JNI interface).

[Robert Sesek, 2014-04-28 21:28:09]

LGTM

[cpu_(ooo_6.6-7.5), 2014-04-29 01:15:10]

lgtm

nit below.

https://codereview.chromium.org/251613002/diff/260001/content/browser/time_zo...
File content/browser/time_zone_monitor_win.cc (right):

https://codereview.chromium.org/251613002/diff/260001/content/browser/time_zo...
content/browser/time_zone_monitor_win.cc:31: DCHECK_EQ(hwnd,
gfx::SingletonHwnd::GetInstance()->hwnd());
If it was me I would remove this dcheck (and logging.h include).

[Mark Mentovai, 2014-04-29 01:58:45]

Thanks for the review!

https://codereview.chromium.org/251613002/diff/260001/content/browser/time_zo...
File content/browser/time_zone_monitor_win.cc (right):

https://codereview.chromium.org/251613002/diff/260001/content/browser/time_zo...
content/browser/time_zone_monitor_win.cc:31: DCHECK_EQ(hwnd,
gfx::SingletonHwnd::GetInstance()->hwnd());
cpu wrote:
> If it was me I would remove this dcheck (and logging.h include).

Done.

[jochen (gone - plz use gerrit), 2014-04-29 07:54:47]

lgtm

[pastarmovj, 2014-04-29 08:09:18]

lgtm

[bulach, 2014-04-29 08:33:43]

lgtm for android, with some small suggestions.

thanks for the detailed explanation, and apologies in advance if my question
below about sending the tz directly doesn't make sense.

https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...
File content/browser/time_zone_monitor.h (right):

https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...
content/browser/time_zone_monitor.h:20: // Sandboxing also may prevent renderer
processes from reading the time
question: is it because the renderers are calling directly the platform-specific
local time function? would it be possible to:
- abstract renderers in all platforms to use something like our own
"LocaltimeCall"
- send the new timezone directly here, to keep the renderers fully
platform-agnostic?

https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...
content/browser/time_zone_monitor.h:43: void NotifyRenderers();
nit: depending on the above, this would take the new tz to be broadcast

https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...
File content/browser/time_zone_monitor_android.cc (right):

https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...
content/browser/time_zone_monitor_android.cc:14:
impl_.Reset(Java_TimeZoneMonitor_getInstance(
how about making getInstance() call start() if needed, to avoid this extra JNI
roundtrip?
(or the other way around, make start() return the instance)

https://codereview.chromium.org/251613002/diff/270001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TimeZoneMonitor.java
(right):

https://codereview.chromium.org/251613002/diff/270001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TimeZoneMonitor.java:20:
class TimeZoneMonitor extends BroadcastReceiver {
nit: it's a bit more common to use an anonymous inner class rather than expose
at the top-level, something like:

private final mBroadcastReceiver = new BroadcastReceiver() {
      @Override
      public void onReceive(Context context, Intent intent) {
        if (!intent.getAction().equals(Intent.ACTION_TIMEZONE_CHANGED)) {
            Log.e(TAG, "unexpected intent");
            return;
        } 
        nativeTimeZoneChangedFromJava(mNativePtr);
      }
}

then use this for register / unregister

https://codereview.chromium.org/251613002/diff/270001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TimeZoneMonitor.java:28:
protected TimeZoneMonitor(Context context) {
nit: private?

https://codereview.chromium.org/251613002/diff/270001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TimeZoneMonitor.java:33:
static TimeZoneMonitor getInstance(Context appContext) {
nit: as above, perhaps fold this with start to avoid two jni calls

[jeremy, 2014-04-29 11:50:51]

https://codereview.chromium.org/251613002/diff/270001/content/renderer/render...
File content/renderer/renderer.sb (right):

https://codereview.chromium.org/251613002/diff/270001/content/renderer/render...
content/renderer/renderer.sb:33: ; http://crbug.com/288697
I assume there is no way to add code to sandbox warmup so we don't need to add
stuff to the sandbox profile? I'd prefer that approach if feasible...

IIUC this is just a temporary hole and we can close it once things are plumbed
through from the browser correctly?  If so can you specifically mention this in
the comment here?

[Mark Mentovai, 2014-04-29 13:00:39]

https://codereview.chromium.org/251613002/diff/270001/content/renderer/render...
File content/renderer/renderer.sb (right):

https://codereview.chromium.org/251613002/diff/270001/content/renderer/render...
content/renderer/renderer.sb:33: ; http://crbug.com/288697
jeremy wrote:
> I assume there is no way to add code to sandbox warmup so we don't need to add
> stuff to the sandbox profile? I'd prefer that approach if feasible...

The time zone is already warmed up at renderer startup. The problem is that if
the time zone changes, renderer processes need to read the new time zone
specification, because the one that they’ve cached will have become invalid.

> IIUC this is just a temporary hole and we can close it once things are plumbed
> through from the browser correctly?  If so can you specifically mention this
in
> the comment here?

Not so. I don’t have any plans to plumb this through to the browser on any
additional platforms, and even on Linux where this currently does happen, Julien
considers it a hack and has said he’d like to remove it.

localtime is normally a fast library call. If we took the approach of plumbing
it through to the browser, each localtime call would become a blocking IPC.
There are also likely numerous calls to localtime, mostly in third_party code,
and we’d have to hack away at all of it to replace localtime in these cases.

I looked at solutions involving bringing the new time zone definition into the
renderer when it changes, but couldn’t arrive at anything good. The standardized
formats for the TZ environment variable aren’t rich enough to provide a full
time zone specification, and the nonstandard format observed by Mac and glibc
allows a full time zone specification to be read from a file, but that’s not
useful in renderer processes.

[jeremy, 2014-04-29 13:02:36]

LGTM

Thanks for the explanation!

[Mark Mentovai, 2014-04-29 16:01:05]

https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...
File content/browser/time_zone_monitor.h (right):

https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...
content/browser/time_zone_monitor.h:20: // Sandboxing also may prevent renderer
processes from reading the time
bulach wrote:
> question: is it because the renderers are calling directly the
platform-specific
> local time function? would it be possible to:
> - abstract renderers in all platforms to use something like our own
> "LocaltimeCall"
> - send the new timezone directly here, to keep the renderers fully
> platform-agnostic?

That’s pretty difficult to get right. The renderers would need their own time
zone specification data, since as I pointed out, it’s otherwise impossible for
them to read the system’s time zone specification files (/usr/share/zoneinfo).
It’s possible that we can use time zone data from ICU, but then we’ve got to be
extra careful to maintain and update that data, and we’re in an awkward
situation of mapping the system time zone in the browser process to an ICU time
zone to be used in the renderers, and mapping by name seems very fragile. As I
mentioned to Jeremy, the localtime calls in the renderers are found in
third_party code (even in ICU, hope there’s no circular dependency) so there’d
be a lot of local patching to replace these with our own custom localtime. And
if we got any of this wrong—and I’m sure there will be corner cases that we
won’t get right—we’d be in a situation where Chrome reports a different time
than the rest of the system. We could even get into a situation where some parts
of Chrome use Chrome’s idea of the local time zone (local times in web content)
where other parts use the system’s idea of the local time zone (navigation
history).

None of this sounds very good, and I really do prefer to continue using the
system’s time zone specifications to avoid these embarrassing mismatches.

https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...
File content/browser/time_zone_monitor_android.cc (right):

https://codereview.chromium.org/251613002/diff/270001/content/browser/time_zo...
content/browser/time_zone_monitor_android.cc:14:
impl_.Reset(Java_TimeZoneMonitor_getInstance(
bulach wrote:
> how about making getInstance() call start() if needed, to avoid this extra JNI
> roundtrip?
> (or the other way around, make start() return the instance)

I did this, and then I folded start() into the constructor to simplify it even
further.

https://codereview.chromium.org/251613002/diff/270001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/TimeZoneMonitor.java
(right):

https://codereview.chromium.org/251613002/diff/270001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/TimeZoneMonitor.java:20:
class TimeZoneMonitor extends BroadcastReceiver {
bulach wrote:
> nit: it's a bit more common to use an anonymous inner class rather than expose
> at the top-level, something like:

Done.

I used BatteryStatusManager, a recent addition to this directory, as my
template. Should that file be changed too, or should the CL that added it be
re-reviewed?

[jln (very slow on Chromium), 2014-04-29 22:57:47]

lgtm

I really wish we could get things plumbed between the render and the browser
correctly.

The hack that we use on Linux to make localtime() work is a very sore point.

https://codereview.chromium.org/251613002/diff/290001/content/browser/time_zo...
File content/browser/time_zone_monitor_linux.cc (right):

https://codereview.chromium.org/251613002/diff/290001/content/browser/time_zo...
content/browser/time_zone_monitor_linux.cc:53: owner_(owner) {
Do you want to DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) in the
constructor and the destructor as well?

[Mark Mentovai, 2014-04-30 03:52:23]

jln wrote:
> I really wish we could get things plumbed between the render and the browser
> correctly.
> 
> The hack that we use on Linux to make localtime() work is a very sore point.

Yeah, it would be good if we could come up with an elegant way to do this. I
discussed a lot of the problems with the possible approaches in my last couple
of messages on this review.

I specifically did call out how undesirable it is to make localtime into a sync
IPC to another process, which is unfortunately exactly what
ProxyLocaltimeCallToBrowser is.

https://codereview.chromium.org/251613002/diff/290001/content/browser/time_zo...
File content/browser/time_zone_monitor_linux.cc (right):

https://codereview.chromium.org/251613002/diff/290001/content/browser/time_zo...
content/browser/time_zone_monitor_linux.cc:53: owner_(owner) {
jln wrote:
> Do you want to DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) in the
> constructor and the destructor as well?

Sure, done.

[Mark Mentovai, 2014-04-30 03:55:41]

https://codereview.chromium.org/251613002/diff/290001/content/browser/time_zo...
File content/browser/time_zone_monitor_linux.cc (right):

https://codereview.chromium.org/251613002/diff/290001/content/browser/time_zo...
content/browser/time_zone_monitor_linux.cc:53: owner_(owner) {
jln wrote:
> Do you want to DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) in the
> constructor and the destructor as well?

Well, I couldn’t do the destructor, because this class is reference-counted,
references may be held on the FILE thread, and we can’t be positive that the
last reference will be released on any thread in particular.

[jln (DO NOT USE THIS), 2014-04-30 05:46:35]

https://codereview.chromium.org/251613002/diff/290001/content/browser/time_zo...
File content/browser/time_zone_monitor_linux.cc (right):

https://codereview.chromium.org/251613002/diff/290001/content/browser/time_zo...
content/browser/time_zone_monitor_linux.cc:53: owner_(owner) {
On 2014/04/30 03:55:41, Mark Mentovai wrote:
> jln wrote:
> > Do you want to DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) in the
> > constructor and the destructor as well?
> 
> Well, I couldn’t do the destructor, because this class is reference-counted,
> references may be held on the FILE thread, and we can’t be positive that the
> last reference will be released on any thread in particular.

Ahh right, good point! And of course it's still fine: the destructor can indeed
run on any thread, as long as nothing is accessing owner_ or file_path_watchers_
which is guaranteed by the very fact that the destructor is being run.

[Mark Mentovai, 2014-04-30 15:49:29]

Committed patchset #14 manually as r267226 (presubmit successful).