Update the ICU's default timezone in render

Update the ICU's default timezone in browser/renderer

When a timezone is changed by a user at the OS level,
the ICU's default timezone has to be updated in both browser
and renderer processes.

This CL propagates the new timezone id to renderers as well as updating
it in the browser process.

A v8-side change is still necessary to get Intl.DateTimeFormat to
work in a new timezone.

BUG=406382, 379595
TEST=Manual test (browser process). 1. Go to history page 2. Change the OS tz 3. Reload the history page and see if the timestamp is in a new timezone.

Committed: https://crrev.com/0a9aa635da90c5a1674cb5cb9d26394ac473a923
Cr-Commit-Position: refs/heads/master@{#327529}

[jungshik at Google, 2015-04-03 19:36:16]

This does not work yet.

[jungshik at Google, 2015-04-10 00:23:43]

jshin@chromium.org changed reviewers:
+ jochen@chromium.org, mark@chromium.org

[jungshik at Google, 2015-04-10 00:23:44]

Mark and Jochen, can you take a look? This does not solve the bug (v8's
Intl.DateTimeFormat bug; it requires a v8-side change), yet. It sends down the
ICU tzid to renderer so that ICU's default tz id is set to a new value in
renderer. With that, we can do something for v8's Intl.DateTimeFormat
(invalidate cache, recreate DateTimeFormat object, etc). 

Initially, I thought the ICU tz id has to be passed down to v8, but it turned
out not necessary because updating the ICU's deflt tz in renderer should
suffice. 

In the meantime, this CL fixes the timestamp in History page. 

However, there are a couple of questions/issues below. If you have any insight
on them, I'd like to hear. 

Thanks

https://codereview.chromium.org/697203006/diff/60001/content/browser/time_zon...
File content/browser/time_zone_monitor.cc (right):

https://codereview.chromium.org/697203006/diff/60001/content/browser/time_zon...
content/browser/time_zone_monitor.cc:36: VLOG(0) << "timezone reset to " <<
zone_id_str;
I expected this function to be called only once per tz change, but I got this
log msg 3 times for each tz change I made on Linux. I wonder why. 

BTW, I'll increase the verbosity level.

https://codereview.chromium.org/697203006/diff/60001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/697203006/diff/60001/content/renderer/render_...
content/renderer/render_thread_impl.cc:1705:
icu::TimeZone::adoptDefault(defaultZone);
adoptDefault() is not thread-safe. If this is called in every thread of
Renderer process, that's a bit problematic (well, we're setting to the same
value, but still it's wasteful to do that multiple times even if there's no
side-effect. TSAN will bark, I guess.)  I wonder if there's a way to call this
only in ONE thread per renderer.

[jochen (gone - plz use gerrit), 2015-04-10 12:17:25]

https://codereview.chromium.org/697203006/diff/60001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/697203006/diff/60001/content/renderer/render_...
content/renderer/render_thread_impl.cc:1705:
icu::TimeZone::adoptDefault(defaultZone);
On 2015/04/10 at 00:23:44, Jungshik at google wrote:
> adoptDefault() is not thread-safe. If this is called in every thread of
> Renderer process, that's a bit problematic (well, we're setting to the same
value, but still it's wasteful to do that multiple times even if there's no
side-effect. TSAN will bark, I guess.)  I wonder if there's a way to call this
only in ONE thread per renderer.

can we make it thread safe?

[Mark Mentovai, 2015-04-10 13:19:00]

https://codereview.chromium.org/697203006/diff/60001/content/browser/time_zon...
File content/browser/time_zone_monitor.cc (right):

https://codereview.chromium.org/697203006/diff/60001/content/browser/time_zon...
content/browser/time_zone_monitor.cc:36: VLOG(0) << "timezone reset to " <<
zone_id_str;
Jungshik at google wrote:
> I expected this function to be called only once per tz change, but I got this
> log msg 3 times for each tz change I made on Linux. I wonder why. 

Linux doesn’t have a standard way to notify of system time zone changes, so we
watch three files in /etc that should cover the majority of cases. Some systems
have more than one of those files (the one here has two of them), so they get
more than one notification. See content/browser/time_zone_monitor_linux.cc.

[jungshik at Google, 2015-04-10 18:47:36]

On 2015/04/10 13:19:00, Mark Mentovai wrote:
>
https://codereview.chromium.org/697203006/diff/60001/content/browser/time_zon...
> File content/browser/time_zone_monitor.cc (right):
> 
>
https://codereview.chromium.org/697203006/diff/60001/content/browser/time_zon...
> content/browser/time_zone_monitor.cc:36: VLOG(0) << "timezone reset to " <<
> zone_id_str;
> Jungshik at google wrote:
> > I expected this function to be called only once per tz change, but I got
this
> > log msg 3 times for each tz change I made on Linux. I wonder why. 
> 
> Linux doesn’t have a standard way to notify of system time zone changes, so we
> watch three files in /etc that should cover the majority of cases. Some
systems
> have more than one of those files (the one here has two of them), so they get
> more than one notification. See content/browser/time_zone_monitor_linux.cc.

Aha... I've seen that code, but it didn't dawn on me that it'd cause this issue.
Thanks. 
 
What do you think of comparing the current tz and a new detected tz and sending
a message to renderers only when they're different? The tz detection still has
to be done for every one of them, but at least we can save a msg down to
renderers. Or, are you ok with multiple msgs sent down?

[Mark Mentovai, 2015-04-10 18:49:52]

Multiple messages didn’t seem like a major problem at the time, since there
wasn’t much machinery springing into action in each process on each update. The
more machinery you add (as you’re doing now), the more expensive each no-op
update becomes. I don’t have a problem with verifying that there’s actually a
change before broadcasting a notification.

[jungshik at Google, 2015-04-10 18:51:55]

On 2015/04/10 12:17:25, jochen wrote:
>
https://codereview.chromium.org/697203006/diff/60001/content/renderer/render_...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/697203006/diff/60001/content/renderer/render_...
> content/renderer/render_thread_impl.cc:1705:
> icu::TimeZone::adoptDefault(defaultZone);
> On 2015/04/10 at 00:23:44, Jungshik at google wrote:
> > adoptDefault() is not thread-safe. If this is called in every thread of
> > Renderer process, that's a bit problematic (well, we're setting to the same
> value, but still it's wasteful to do that multiple times even if there's no
> side-effect. TSAN will bark, I guess.)  I wonder if there's a way to call this
> only in ONE thread per renderer.
> 
> can we make it thread safe?

You mean adoptDefault()? I guess it's possible with a patch to ICU (and we can
upstream it). Alternatively, we can lock on our side and compare the current tz
and a value passed in from the browser process and call adoptDefault() only when
they're different. Is the latter acceptable? I'm more inclined to do the latter.
 
Tz change events are extremely rare so that locking on outside wouldn't be a big
deal, would it?

[Mark Mentovai, 2015-04-10 19:00:16]

I’d rather avoid any locking if possible.

[jochen (gone - plz use gerrit), 2015-04-13 12:59:41]

On 2015/04/10 at 18:51:55, jshin wrote:
> On 2015/04/10 12:17:25, jochen wrote:
> >
https://codereview.chromium.org/697203006/diff/60001/content/renderer/render_...
> > File content/renderer/render_thread_impl.cc (right):
> > 
> >
https://codereview.chromium.org/697203006/diff/60001/content/renderer/render_...
> > content/renderer/render_thread_impl.cc:1705:
> > icu::TimeZone::adoptDefault(defaultZone);
> > On 2015/04/10 at 00:23:44, Jungshik at google wrote:
> > > adoptDefault() is not thread-safe. If this is called in every thread of
> > > Renderer process, that's a bit problematic (well, we're setting to the
same
> > value, but still it's wasteful to do that multiple times even if there's no
> > side-effect. TSAN will bark, I guess.)  I wonder if there's a way to call
this
> > only in ONE thread per renderer.
> > 
> > can we make it thread safe?
> 
> You mean adoptDefault()? I guess it's possible with a patch to ICU (and we can
upstream it). Alternatively, we can lock on our side and compare the current tz
and a value passed in from the browser process and call adoptDefault() only when
they're different. Is the latter acceptable? I'm more inclined to do the latter.
 
> Tz change events are extremely rare so that locking on outside wouldn't be a
big deal, would it?

is it possibly to make the pointer atomic so it's safe to read/write from
multiple threads?

[jungshik at Google, 2015-04-24 22:40:54]

https://codereview.chromium.org/697203006/diff/60001/content/renderer/render_...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/697203006/diff/60001/content/renderer/render_...
content/renderer/render_thread_impl.cc:1705:
icu::TimeZone::adoptDefault(defaultZone);
On 2015/04/10 12:17:24, jochen wrote:
> On 2015/04/10 at 00:23:44, Jungshik at google wrote:
> > adoptDefault() is not thread-safe. If this is called in every thread of
> > Renderer process, that's a bit problematic (well, we're setting to the same
> value, but still it's wasteful to do that multiple times even if there's no
> side-effect. TSAN will bark, I guess.)  I wonder if there's a way to call this
> only in ONE thread per renderer.
> 
> can we make it thread safe?

It turned out that I was mistaken about this part. It's called only once per
renderer process afaict. So, I think we don't have to do anything here.

https://codereview.chromium.org/697203006/diff/160001/content/browser/time_zo...
File content/browser/time_zone_monitor.cc (right):

https://codereview.chromium.org/697203006/diff/160001/content/browser/time_zo...
content/browser/time_zone_monitor.cc:35: if (*current_zone == *new_zone) {
This will be true and a msg will not go down to renderers even after a tz change
if |TimeZone::createDefault()| is never called in the browser process before.
That's because createDefault() will call detectHostTimeZone() internally in that
case.
 
One (hacky) solution is to call TimeZone::createDefault() early in the browser
process (as we do in every renderer process before sandbox kicks in;
http://goo.gl/zULKAB ). It'll slow down the init time slightly, but it might be
in noise.

[jungshik at Google, 2015-04-24 22:46:58]

Can you take another look? Thanks.

https://codereview.chromium.org/697203006/diff/160001/content/browser/time_zo...
File content/browser/time_zone_monitor.cc (right):

https://codereview.chromium.org/697203006/diff/160001/content/browser/time_zo...
content/browser/time_zone_monitor.cc:35: if (*current_zone == *new_zone) {
On 2015/04/24 22:40:54, Jungshik at google wrote:
> This will be true and a msg will not go down to renderers even after a tz
change
> if |TimeZone::createDefault()| is never called in the browser process before.
> That's because createDefault() will call detectHostTimeZone() internally in
that
> case.
>  
> One (hacky) solution is to call TimeZone::createDefault() early in the browser
> process (as we do in every renderer process before sandbox kicks in;
> http://goo.gl/zULKAB ). It'll slow down the init time slightly, but it might
be
> in noise. 

I did this in PS #10.

[jochen (gone - plz use gerrit), 2015-04-27 19:21:59]

lgtm

https://codereview.chromium.org/697203006/diff/200001/content/browser/time_zo...
File content/browser/time_zone_monitor.cc (right):

https://codereview.chromium.org/697203006/diff/200001/content/browser/time_zo...
content/browser/time_zone_monitor.cc:36: VLOG(0) << "timezone already updated";
can we make this VLOG(1) ?

https://codereview.chromium.org/697203006/diff/200001/content/browser/time_zo...
content/browser/time_zone_monitor.cc:46: VLOG(0) << "timezone reset to " <<
zone_id_str;
same here

[jungshik at Google, 2015-04-27 22:14:40]

On 2015/04/27 19:21:59, jochen wrote:
> lgtm
> 
>
https://codereview.chromium.org/697203006/diff/200001/content/browser/time_zo...
> File content/browser/time_zone_monitor.cc (right):
> 
>
https://codereview.chromium.org/697203006/diff/200001/content/browser/time_zo...
> content/browser/time_zone_monitor.cc:36: VLOG(0) << "timezone already
updated";
> can we make this VLOG(1) ?
> 
>
https://codereview.chromium.org/697203006/diff/200001/content/browser/time_zo...
> content/browser/time_zone_monitor.cc:46: VLOG(0) << "timezone reset to " <<
> zone_id_str;
> same here

Thanks. Good catch !  I meant to change them to VLOG(1), but kept VLOG(0) during
my testing.

[Mark Mentovai, 2015-04-28 12:46:57]

LGTM

[jungshik at Google, 2015-04-28 19:48:14]

tsepez, can you owner-review/approve a change in IPC message? Thanks. 

Thank you, Mark and Jochen for the review.

[jungshik at Google, 2015-04-28 19:49:43]

jshin@chromium.org changed reviewers:
+ tsepez@chromium.org

[jungshik at Google, 2015-04-28 19:49:44]

tsepez, can you owner-review/approve a change in IPC message? Thanks.

[Tom Sepez, 2015-04-28 19:53:57]

Messages LGTM.

[jungshik at Google, 2015-04-28 20:06:57]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2015-04-28 20:06:57]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/697203006/#ps220001
(title: "VLOG(1) instead of 0")

[commit-bot: I haz the power, 2015-04-28 20:07:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/697203006/220001

[commit-bot: I haz the power, 2015-04-28 20:10:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-28 20:10:20]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jungshik at Google, 2015-04-29 17:02:27]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2015-04-29 17:02:28]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
tsepez@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/697203006/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-29 17:03:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/697203006/240001

[commit-bot: I haz the power, 2015-04-29 18:35:49]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-04-29 18:36:48]

Patchset 13 (id:??) landed as
https://crrev.com/0a9aa635da90c5a1674cb5cb9d26394ac473a923
Cr-Commit-Position: refs/heads/master@{#327529}