Reland: Non-SFI NaCl: Allow CLOCK_SYSTEM_TRACE on Chrome OS

Reland: Non-SFI NaCl: Allow CLOCK_SYSTEM_TRACE on Chrome OS

chrome://tracing uses TimeTicks::NowFromSystemTraceTime in
base/time/time_posix.cc.

The previous patch is reverted because it did not check this
test is running on Chrome OS.

TEST=nacl_loader_unittests  # on i686 and ARM Chrome OS
TEST=trybot (including linux_chromeos)
BUG=378063
R=jln@chromium.org

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

[hamaji, 2014-06-05 17:19:14]

Sorry for this. The patch set 1 is the original change. Only the unittest has
been modified.

Just allowing clock_gettime(CLOCK_SYSTEM_TRACE) failure in OS_CHROMEOS would be
a choice. But I'd check clock_gettime(CLOCK_SYSTEM_TRACE) is working if
IsOnChromeOS is true to ensure someone won't change ALLOWED to ENOSYS for
CLOCK_SYSTEM_TRACE.

[jln (very slow on Chromium), 2014-06-05 17:53:29]

lgtm, but please run "git cl format".

Also I don't think we run these tests on a real Chrome OS machine for now. But
we (hopefully) should soon, so it's still worth it.

Did you manage to test this on a kernel where the feature is present?

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:432: :
is_running_on_chromeos_(base::SysInfo::IsRunningOnChromeOS()) {}
Nit: 2 extra spaces

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:436: OVERRIDE {
Nit: shouldn't it be two spaces more? before OVERRIDE

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:438: new
nacl::nonsfi::NaClNonSfiBPFSandboxPolicy());
nit: two extra spaces

[jln (very slow on Chromium), 2014-06-05 17:54:28]

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:470: #endif
Add "// if defined(OS_CHROMEOS)"

[jln (very slow on Chromium), 2014-06-05 17:56:15]

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:97: #if defined(OS_CHROMEOS)
If you prefer, we could do this unconditionally.

I don't think we care much about non-ChromeOS Linux since this feature won't run
there. It's also probably not a big security risk, even if that clockid gets
reused for something else.

[hamaji, 2014-06-05 18:21:02]

Yes, I'm always running nacl_loader_unittests on ARM Chrome OS device for my
seccomp changes. This is what I tried to mean by my TEST= line.

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_sandbox.cc (right):

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox.cc:97: #if defined(OS_CHROMEOS)
On 2014/06/05 17:56:15, jln wrote:
> If you prefer, we could do this unconditionally.
> 
> I don't think we care much about non-ChromeOS Linux since this feature won't
run
> there. It's also probably not a big security risk, even if that clockid gets
> reused for something else.

Hmm. I'd prefer having this as removing this would not reduce complexity of the
unittest much (the complex code is in defined(OS_CHROMEOS).

Also, I'd like not to make unnecessary logic change in a retrial of a reverted
change. If you don't like this #if, please let me remove this in a separate
change.

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right):

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:432: :
is_running_on_chromeos_(base::SysInfo::IsRunningOnChromeOS()) {}
On 2014/06/05 17:53:29, jln wrote:
> Nit: 2 extra spaces

Done.

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:436: OVERRIDE {
On 2014/06/05 17:53:29, jln wrote:
> Nit: shouldn't it be two spaces more? before OVERRIDE

Actually, this line had 80 columns and git cl format concatinated the two lines.

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:438: new
nacl::nonsfi::NaClNonSfiBPFSandboxPolicy());
On 2014/06/05 17:53:29, jln wrote:
> nit: two extra spaces

Done.

https://codereview.chromium.org/317913002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:470: #endif
On 2014/06/05 17:54:28, jln wrote:
> Add "// if defined(OS_CHROMEOS)"

Done. It seems "// defined(OS_CHROMEOS)" is more popular in Chrome repo so I
removed if from your suggestion.

https://codereview.chromium.org/317913002/diff/80001/base/time/time_posix.cc
File base/time/time_posix.cc (right):

https://codereview.chromium.org/317913002/diff/80001/base/time/time_posix.cc#...
base/time/time_posix.cc:347: #else  // !(defined(OS_CHROMEOS) &&
!defined(__native_client__))
git cl format added this

[hamaji, 2014-06-06 05:28:05]

Let me bypass OWNER check as the semantics of this patch was not changed since
the previous version. If there are any further feedbacks, I'll address them in a
separate change (maybe with resolving the TODO in time_posix.cc)

[jln (very slow on Chromium), 2014-06-06 05:51:13]

On 2014/06/06 05:28:05, hamaji wrote:
> Let me bypass OWNER check as the semantics of this patch was not changed since
> the previous version. If there are any further feedbacks, I'll address them in
a
> separate change (maybe with resolving the TODO in time_posix.cc)

Yes, it's definitely fine to TBR ajwong for base/

[hamaji, 2014-06-06 06:02:16]

Committed patchset #5 manually as r275347 (tree was closed).