[ICU] Avoid reading ICU data files in render process.

[ICU] Avoid reading ICU data files in render process.

Instead of reading icudtl.dat from app_chrome directory,
use a file descriptor passed from the browser process.

This allows us to tight permissions on data directory.

BUG=378381
R=avi@chromium.org, jshin@chromium.org, sky@chromium.org

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

[Feng Qian, 2014-06-05 21:49:15]

jshin@, this is my tentative fix of ICU problem.

The #if-else in icu_util.cc really scares me, the new method only handles
ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE case.

I have to move InitializeICU call from
content/app/content_main_runner.cc:Initialize to
chrome/app/chrome_main_delegate.cc because Android file descriptor files live in
chrome/ directory.

So far, I only tested this CL in Clank, and it works fine with 700 permission on
app_chrome. I haven't get chance to run tests and test on other platforms.

Please either take the CL or help me run tests on other platforms
(Win/Linux/CrOS). I have only Android build environment setup.

[jungshik at Google, 2014-06-05 23:12:57]

Thank you for the CL !

On 2014/06/05 21:49:15, Feng Qian wrote:
> jshin@, this is my tentative fix of ICU problem.
> 
> The #if-else in icu_util.cc really scares me, the new method only handles
> ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE case.

DATA_STATIC case should just work because icudata is linked statically and
InitializeICU is no-op in that case.
DATA_SHARED is only used on Windows (default is DATA_FILE on Windows, too) and
this CL does not touch that part so that we don't have to worry about it either.


> 
> I have to move InitializeICU call from
> content/app/content_main_runner.cc:Initialize to
> chrome/app/chrome_main_delegate.cc because Android file descriptor files live
in
> chrome/ directory.
> 
> So far, I only tested this CL in Clank, and it works fine with 700 permission
on
> app_chrome. I haven't get chance to run tests and test on other platforms.

Various test binaries may or may not fail to work on Android with a permission
change because calling InitializeICU() in base/test/test_suite.cc may fail to
load the ICU data. It'd not be a problem if test binaries do not spawn a child
process, but if they did, it'd be a problem, wouldn't it? 

> 
> Please either take the CL or help me run tests on other platforms
> (Win/Linux/CrOS). I have only Android build environment setup.

As it is now, this CL does not change anything on those platforms. So, I think
trybot should be sufficient to make sure nothing is wrong on other platforms. 
It occurred to me that we might consider this approach on other platforms as
well, but there's no rush for that, I believe.

[jungshik at Google, 2014-06-05 23:30:58]

Can you try if blink tests and other tests run ok (can find icu data) on
Android?  If they're fine, looks good.

https://codereview.chromium.org/317833006/diff/1/base/i18n/icu_util.cc
File base/i18n/icu_util.cc (right):

https://codereview.chromium.org/317833006/diff/1/base/i18n/icu_util.cc#newcode60
base/i18n/icu_util.cc:60: bool InitializeICU(int data_fd) {
This should be no-op if DATA_STATIC is used.

https://codereview.chromium.org/317833006/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/317833006/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2645: {
nit: I wonder why you enclosed this block with a pair of braces. I don't see any
lock (or am I missing something?)

https://codereview.chromium.org/317833006/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2652:
app_data_path.AppendASCII("icudtl.dat");
nit: |icudata_path| would be better, wouldn't it?

[jungshik at Google, 2014-06-05 23:40:38]

https://codereview.chromium.org/317833006/diff/1/chrome/app/chrome_main_deleg...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/317833006/diff/1/chrome/app/chrome_main_deleg...
chrome/app/chrome_main_delegate.cc:681: #endif
What about other implementations of content_main_delegate such as
shell_main_delegate?

[Feng Qian, 2014-06-06 17:40:31]

https://codereview.chromium.org/317833006/diff/1/base/i18n/icu_util.cc
File base/i18n/icu_util.cc (right):

https://codereview.chromium.org/317833006/diff/1/base/i18n/icu_util.cc#newcode60
base/i18n/icu_util.cc:60: bool InitializeICU(int data_fd) {
What about DATA_SHARED case? Would it be the same as InitializeICU()?

On 2014/06/05 23:30:57, Jungshik Shin wrote:
> This should be no-op if DATA_STATIC is used.

https://codereview.chromium.org/317833006/diff/1/chrome/app/chrome_main_deleg...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/317833006/diff/1/chrome/app/chrome_main_deleg...
chrome/app/chrome_main_delegate.cc:681: #endif
I can move logic into content_main_runner.cc.

On 2014/06/05 23:40:38, Jungshik Shin wrote:
> What about other implementations of content_main_delegate such as
> shell_main_delegate?

https://codereview.chromium.org/317833006/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/317833006/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2645: {
I intended to scope variable names, but can remove it and rename variables.

On 2014/06/05 23:30:58, Jungshik Shin wrote:
> nit: I wonder why you enclosed this block with a pair of braces. I don't see
any
> lock (or am I missing something?)

https://codereview.chromium.org/317833006/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2652:
app_data_path.AppendASCII("icudtl.dat");
sure

On 2014/06/05 23:30:57, Jungshik Shin wrote:
> nit: |icudata_path| would be better, wouldn't it?

[jungshik at Google, 2014-06-06 18:55:36]

On 2014/06/06 17:40:31, Feng Qian wrote:
> https://codereview.chromium.org/317833006/diff/1/base/i18n/icu_util.cc
> File base/i18n/icu_util.cc (right):
> 
>
https://codereview.chromium.org/317833006/diff/1/base/i18n/icu_util.cc#newcode60
> base/i18n/icu_util.cc:60: bool InitializeICU(int data_fd) {
> What about DATA_SHARED case? Would it be the same as InitializeICU()?

Sorry that I didn't mention it earlier. DATA_SHARED is NOT used on
Android/Linux/Mac. It's only used on Windows. So, we don't have to worry about
it. 

> 
> On 2014/06/05 23:30:57, Jungshik Shin wrote:
> > This should be no-op if DATA_STATIC is used.
> 
>
https://codereview.chromium.org/317833006/diff/1/chrome/app/chrome_main_deleg...
> File chrome/app/chrome_main_delegate.cc (right):
> 
>
https://codereview.chromium.org/317833006/diff/1/chrome/app/chrome_main_deleg...
> chrome/app/chrome_main_delegate.cc:681: #endif
> I can move logic into content_main_runner.cc.

If possible, that'll be nice. 

> 
> On 2014/06/05 23:40:38, Jungshik Shin wrote:
> > What about other implementations of content_main_delegate such as
> > shell_main_delegate?
> 
>
https://codereview.chromium.org/317833006/diff/1/chrome/browser/chrome_conten...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/317833006/diff/1/chrome/browser/chrome_conten...
> chrome/browser/chrome_content_browser_client.cc:2645: {
> I intended to scope variable names, but can remove it and rename variables.

Oh. I see. Then, it's fine. 
 
> On 2014/06/05 23:30:58, Jungshik Shin wrote:
> > nit: I wonder why you enclosed this block with a pair of braces. I don't see
> any
> > lock (or am I missing something?)
> 
>
https://codereview.chromium.org/317833006/diff/1/chrome/browser/chrome_conten...
> chrome/browser/chrome_content_browser_client.cc:2652:
> app_data_path.AppendASCII("icudtl.dat");
> sure
> 
> On 2014/06/05 23:30:57, Jungshik Shin wrote:
> > nit: |icudata_path| would be better, wouldn't it?

[jungshik at Google, 2014-06-06 23:07:50]

LGTM if you take care of nits and component-build link issue. 
Thanks

https://codereview.chromium.org/317833006/diff/20001/base/i18n/icu_util.cc
File base/i18n/icu_util.cc (right):

https://codereview.chromium.org/317833006/diff/20001/base/i18n/icu_util.cc#ne...
base/i18n/icu_util.cc:66: #if (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_STATIC)
nit: intentation

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

https://codereview.chromium.org/317833006/diff/20001/base/i18n/icu_util.h#new...
base/i18n/icu_util.h:20: BASE_I18N_EXPORT bool InitializeICU(int data_fd);
Should we avoid function overloading and use InitializeICUwithFileDescriptor,
instead?

[Avi (use Gerrit), 2014-06-09 16:13:11]

content/ lgtm

[sky, 2014-06-09 16:15:18]

chrome LGTM

[Feng Qian, 2014-06-09 16:26:51]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-06-09 16:27:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/feng@chromium.org/317833006/60001

[commit-bot: I haz the power, 2014-06-10 08:33:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-10 11:50:57]

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

[commit-bot: I haz the power, 2014-06-10 11:50:58]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[Feng Qian, 2014-06-10 15:46:12]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-06-10 15:49:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/feng@chromium.org/317833006/60001

[commit-bot: I haz the power, 2014-06-10 16:22:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-10 17:01:16]

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

[commit-bot: I haz the power, 2014-06-10 17:01:18]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[Feng Qian, 2014-06-10 18:40:55]

Committed patchset #4 manually as r276100 (presubmit successful).