mandoline: Fix ICU initialization.

mandoline: Fix ICU initialization.

We need to complete the initialization of ICU before we raise the
sandbox. That means we can't pass a file descriptor to the ICU data file
across mojo pipes. Due to how Android handles resources files, we also
can't pass a file descriptor around inside the same process. So pass a
raw pointer to a memory mapped file during the sandbox warm-up phase,
and make a new option to initialize ICU from this raw pointer.

But that just uncovers a bigger issue: we don't always call what was the
sandbox warming code. If it's general initialization that needs to be
called before we run MojoMain(), we need to include initialization code
in most all main.cc implementations. This bakes a base initialize call
into the mojo application library.

This fixes a crash in the page cycler set, which happens when a page
tries to do date operations, which fail because of missing locale data.
This should let us raise the sandbox on the page cycler.

BUG=546644
Committed: https://crrev.com/e5735ff0a8ddb61672d964a44ebc8a8ebe2b6242
Cr-Commit-Position: refs/heads/master@{#358904}

[Elliot Glaysher, 2015-10-29 20:20:41]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[Elliot Glaysher, 2015-10-29 20:20:41]

This fixes the page cycler with multiprocess and the sandbox on.

[Elliot Glaysher, 2015-10-29 21:04:04]

(Hold off on this temporarily; we aren't calling init in all the places we
should be.)

[Elliot Glaysher, 2015-10-29 22:21:06]

Description was changed from

==========
mandoline: Fix ICU initialization.

We need to complete the initialization of ICU before we raise the
sandbox. That means we can't pass a file descriptor to the ICU data file
across mojo pipes. Instead, during the sandbox warm-up phase, pass a
file descriptor to the loaded process.

This fixes a crash in the page cycler set, which happens when a page
tries to do date operations, which fail because of missing locale data.

BUG=546644
==========

to

==========
mandoline: Fix ICU initialization.

We need to complete the initialization of ICU before we raise the
sandbox. That means we can't pass a file descriptor to the ICU data file
across mojo pipes. Instead, during the sandbox warm-up phase, pass a
file descriptor to the loaded process.

But that just uncovers a bigger issue: we don't always call what was the
sandbox warming code. If it's general initialization that needs to be
called before we run MojoMain(), we need to include initialization code
in most all main.cc implementations. This makes an initialize base and an
initialize base and ICU implementation that can be included in different
translation units, and makes the various runners use them.

This fixes a crash in the page cycler set, which happens when a page
tries to do date operations, which fail because of missing locale data.

BUG=546644
==========

[jam, 2015-10-30 00:53:09]

jam@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2015-10-30 00:53:09]

It seems for the time being all of our apps use base, and it would be preferable
to avoid having them have to remember to include one of two headers (depending
on if they use ICU) to ensure things are properly initialized.

What's the harm in automatically doing this in the C++ client library
(ApplicationRunner?)? That would also do ICU (no harm in also initializing it
always, it is used by parts of base after all)

If in the future we have any apps that want to do custom initialization before
the sandbox is engaged, then we can bring back a symbol like this.

[Elliot Glaysher, 2015-11-04 20:43:19]

ptal

Is there a cross platform way to duplicate a base::PlatformFile? If so, this
patch could be made better by duplicating the file descriptor to the memory
mapped file instead of loading it multiple times.

[Elliot Glaysher, 2015-11-05 00:09:58]

ping sky

[sky, 2015-11-05 03:32:14]

On 2015/11/05 00:09:58, Elliot Glaysher wrote:
> ping sky

Sorry, I thought John was reviewing. Does File::Duplicate() do what you want?

[sky, 2015-11-05 03:40:17]

I'm LGTMing. I would prefer if this was optional.

https://codereview.chromium.org/1425853003/diff/430001/base/i18n/icu_util.h
File base/i18n/icu_util.h (right):

https://codereview.chromium.org/1425853003/diff/430001/base/i18n/icu_util.h#n...
base/i18n/icu_util.h:23: BASE_I18N_EXPORT PlatformFile OpenIcuDataFile();
Can this return a File? I believe File owns the PlatformFile, which would avoid
accidental leaks.

https://codereview.chromium.org/1425853003/diff/430001/mojo/runner/child_proc...
File mojo/runner/child_process.cc (right):

https://codereview.chromium.org/1425853003/diff/430001/mojo/runner/child_proc...
mojo/runner/child_process.cc:299: base::i18n::InitializeICU();
Is there a way this could be optional? Seems sad to bake icu initialization into
all mojo apps.

[Elliot Glaysher, 2015-11-05 18:41:06]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-11-05 18:42:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425853003/450001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425853003/450001

[commit-bot: I haz the power, 2015-11-05 18:42:03]

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

[commit-bot: I haz the power, 2015-11-05 18:42:04]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Elliot Glaysher, 2015-11-05 18:42:57]

Description was changed from

==========
mandoline: Fix ICU initialization.

We need to complete the initialization of ICU before we raise the
sandbox. That means we can't pass a file descriptor to the ICU data file
across mojo pipes. Instead, during the sandbox warm-up phase, pass a
file descriptor to the loaded process.

But that just uncovers a bigger issue: we don't always call what was the
sandbox warming code. If it's general initialization that needs to be
called before we run MojoMain(), we need to include initialization code
in most all main.cc implementations. This makes an initialize base and an
initialize base and ICU implementation that can be included in different
translation units, and makes the various runners use them.

This fixes a crash in the page cycler set, which happens when a page
tries to do date operations, which fail because of missing locale data.

BUG=546644
==========

to

==========
mandoline: Fix ICU initialization.

We need to complete the initialization of ICU before we raise the
sandbox. That means we can't pass a file descriptor to the ICU data file
across mojo pipes. Instead, during the sandbox warm-up phase, pass a
file descriptor to the loaded process.

But that just uncovers a bigger issue: we don't always call what was the
sandbox warming code. If it's general initialization that needs to be
called before we run MojoMain(), we need to include initialization code
in most all main.cc implementations. This bakes a base initialize call
into the mojo application library.

This fixes a crash in the page cycler set, which happens when a page
tries to do date operations, which fail because of missing locale data.
This should let us raise the sandbox on the page cycler.

BUG=546644
==========

[sky, 2015-11-05 18:43:54]

LGTM !

[Elliot Glaysher, 2015-11-05 18:44:37]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-11-05 18:45:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425853003/450001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425853003/450001

[commit-bot: I haz the power, 2015-11-05 18:56:35]

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

[commit-bot: I haz the power, 2015-11-05 18:56:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Elliot Glaysher, 2015-11-05 19:22:29]

ping jam for base owners review

[jam, 2015-11-05 19:29:03]

lgtm

https://codereview.chromium.org/1425853003/diff/450001/base/i18n/icu_util.h
File base/i18n/icu_util.h (right):

https://codereview.chromium.org/1425853003/diff/450001/base/i18n/icu_util.h#n...
base/i18n/icu_util.h:23: BASE_I18N_EXPORT PlatformFile OpenIcuDataFile();
nit: ICU for consistency with the rest of the file

[Elliot Glaysher, 2015-11-05 19:59:20]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-11-05 20:00:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425853003/450001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425853003/450001

[commit-bot: I haz the power, 2015-11-06 00:07:56]

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

[commit-bot: I haz the power, 2015-11-06 00:07:57]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Elliot Glaysher, 2015-11-06 18:15:38]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-11-06 18:15:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425853003/450001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425853003/450001

[commit-bot: I haz the power, 2015-11-06 18:21:45]

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

[commit-bot: I haz the power, 2015-11-06 18:21:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Elliot Glaysher, 2015-11-06 18:41:36]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-11-06 18:41:36]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1425853003/#ps470001
(title: "Merge tot")

[commit-bot: I haz the power, 2015-11-06 18:42:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425853003/470001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425853003/470001

[Elliot Glaysher, 2015-11-06 20:55:15]

The CQ bit was unchecked by erg@chromium.org

[Elliot Glaysher, 2015-11-09 22:43:13]

sky: this is a big enough change that you should probably rereview parts of this
patch.

base/i18n: This changes things so we can initialize ICU from raw memory. We
can't use file descriptors because of how android handles file descriptors to
assets in an apk. The methods we were using to get a file descriptor were
questionably wrong on non-Android platforms and I've hidden them behind ifdefs.

This should also deal with the component build fairly cleanly. In the component
build, this new initialize one copy of icu from another is a noop since there's
only one copy of icu linked in. I've tested this locally; there doesn't appear
to be a bot for this configuration yet.

mojo/runner: This requires us to call 'base::i18n::InitializeICU()' in
child_procces.cc so that icu is initialized and we can pass the data to the copy
of ICU in the loaded mojo file. I previously removed this call on your request,
but we now hard need it with this approach.

As a plus side, this way won't duplicate memory. Yay.

[sky, 2015-11-10 00:53:50]

SLGTM - I'm not a base owner, so I didn't look there.

[Elliot Glaysher, 2015-11-10 18:15:25]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-11-10 18:15:26]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1425853003/#ps570001
(title: "Rebase to to")

[commit-bot: I haz the power, 2015-11-10 18:15:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425853003/570001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425853003/570001

[commit-bot: I haz the power, 2015-11-10 21:23:07]

Committed patchset #31 (id:570001)

[commit-bot: I haz the power, 2015-11-10 21:24:20]

Patchset 31 (id:??) landed as
https://crrev.com/e5735ff0a8ddb61672d964a44ebc8a8ebe2b6242
Cr-Commit-Position: refs/heads/master@{#358904}

[msw, 2015-11-10 22:54:07]

On 2015/11/10 21:24:20, commit-bot: I haz the power wrote:
> Patchset 31 (id:??) landed as
> https://crrev.com/e5735ff0a8ddb61672d964a44ebc8a8ebe2b6242
> Cr-Commit-Position: refs/heads/master@{#358904}

This may be causing crashes on Chromium Mojo Linux Perf:
http://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Linux%20...
All tests crashed on all 3 runs since this went in.

[msw, 2015-11-10 23:27:35]

On 2015/11/10 22:54:07, msw wrote:
> On 2015/11/10 21:24:20, commit-bot: I haz the power wrote:
> > Patchset 31 (id:??) landed as
> > https://crrev.com/e5735ff0a8ddb61672d964a44ebc8a8ebe2b6242
> > Cr-Commit-Position: refs/heads/master@{#358904}
> 
> This may be causing crashes on Chromium Mojo Linux Perf:
>
http://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Linux%20...
> All tests crashed on all 3 runs since this went in.

Hmm, I wonder if this is also causing the new Android apptest and
mojo_runner_unittests failures:
http://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Android/...

[Elliot Glaysher, 2015-11-10 23:35:02]

A revert of this CL (patchset #31 id:570001) has been created in
https://codereview.chromium.org/1429263005/ by erg@chromium.org.

The reason for reverting is: Failures on Chromium Mojo Linux Perf which appears
to be because the perf bots don't have an up to date kernel and on Chromium Mojo
Android which has some weird crash..