Linux Sandbox: split the GPU policies to their own file.

Linux Sandbox: split the GPU policies to their own files.

We split the GPU policies to their own independant files in order to make
sandbox_seccomp_bpf_linux.cc more sane.

Moreover, we clean up the logic so that the "WarmUp" process is an
implementation of SandboxBPFBasePolicy::PreSandboxHook().

More work remains to clean-up the GPU policies, but this is an important
first step.

In addition, we put all Linux sandboxing related files in content/common in
a new sandbox_linux directory.

BUG=325535
R=jorgelo@chromium.org, mseaborn@chromium.org, piman@chromium.org, rsesek@chromium.org

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

[jln (very slow on Chromium), 2013-12-12 01:32:18]

Jorge, can you please take a look ?
Robert: if you have some time, please take a look as well!

Again, this is an intermediate change, mostly moving code. Please add remarks on
old code as a TODO in crbug.com/325535.

The meat is in the new file,
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc

Again, I tried to touch as little code as possible to make the review easier.
It's mostly unchanged, except that the "WarmUp" and starting of the broker
process will happen in a clear PreSandboxHook() virtual method .

I'm also moving everything to content/common/sandbox_linux/.

[jln (very slow on Chromium), 2013-12-12 01:33:32]

Note: this has been quickly tested on a "Daisy" device: about:gpu says
"sandboxed true".

[jln (very slow on Chromium), 2013-12-12 01:44:26]

I had to bump the "move detection" threshold for this CL. The review tool is
confused about a few minor changes that it sees as unrelated files (the OWNERs
file is an example). Ignore this.

[jln (very slow on Chromium), 2013-12-12 19:31:26]

I'll actually fold the next CL into this to split
sandbox_bpf_gpu_policy_linux.cc in two parts.

[Robert Sesek, 2013-12-12 21:33:47]

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
File content/common/sandbox_linux/sandbox_bpf_base_policy_linux.h (right):

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_base_policy_linux.h:35: virtual bool
PreSandboxHook();
nit: blank line before comment and after method

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
File content/common/sandbox_linux/sandbox_bpf_cros_arm_gpu_policy_linux.cc
(right):

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_cros_arm_gpu_policy_linux.cc:204:
dlopen("/usr/lib/libmali.so", RTLD_NOW|RTLD_GLOBAL|RTLD_NODELETE);
Maybe pull these flags out into a constant and pass them around that way?

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
File content/common/sandbox_linux/sandbox_bpf_cros_arm_gpu_policy_linux.h
(right):

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_cros_arm_gpu_policy_linux.h:15:
explicit CrosArmGpuProcessPolicy(bool allow_shmat)
Don't inline ctors and dtors:
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_cros_arm_gpu_policy_linux.h:31: //
CONTENT_COMMON_SANDBOX_LINUX_SANDBOX_BPF_CROS_ARM_GPU_POLICY_LINUX_H_
nit: put on previous line, even if it overflows 80

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
File content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:78: is_enabled =
is_enabled &&
Could switch to &=

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
File content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h (right):

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h:21:
GpuProcessPolicy() : broker_process_(NULL) {}
Same out-of-line the dtor/ctor.

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h:36: bool
(*broker_sandboxer_callback)(void),
Could/should this be a base::Callback?

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h:38: const
std::vector<std::string>& write_whitelist_extra);
nit: blank line after

[Jorge Lucangeli Obes, 2013-12-12 21:37:21]

https://chromiumcodereview.appspot.com/99133015/diff/80001/components/nacl.gyp
File components/nacl.gyp (right):

https://chromiumcodereview.appspot.com/99133015/diff/80001/components/nacl.gy...
components/nacl.gyp:194:
'../content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc',
Do we want to keep the sandbox_ prefixes inside the sandbox_linux directory?

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
File content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:133: void
set_broker_process(BrokerProcess* broker_process) {
Is this the correct style for the setter?

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:139: // and will
serve requests to access files over an IPC. The client of this
I would say "over IPC" or "over an IPC channel".

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:184:
DCHECK(!chromeos_arm_gpu);
Maybe add a comment reminding the reader "This policy is only used on
x86_64/i386." The previous comment to this effect is in a different function.

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:304: // Add arm
specific files to whitelist in the broker.
ARM-specific

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
File content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h (left):

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h:6: #define
CONTENT_SHELL_BROWSER_SHELL_NET_LOG_H_
WTF =P

[Robert Sesek, 2013-12-12 21:41:42]

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
File content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:133: void
set_broker_process(BrokerProcess* broker_process) {
On 2013/12/12 21:37:21, Jorge Lucangeli Obes wrote:
> Is this the correct style for the setter?

Yes, simple accessors use unix_hacker_style and get inlined.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

[jln (very slow on Chromium), 2013-12-12 22:15:14]

Thanks, PTAL!

https://chromiumcodereview.appspot.com/99133015/diff/80001/components/nacl.gyp
File components/nacl.gyp (right):

https://chromiumcodereview.appspot.com/99133015/diff/80001/components/nacl.gy...
components/nacl.gyp:194:
'../content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc',
On 2013/12/12 21:37:21, Jorge Lucangeli Obes wrote:
> Do we want to keep the sandbox_ prefixes inside the sandbox_linux directory?

Probably not. Let's see what piman@ says, I'll add him for owner review.

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
File content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:133: void
set_broker_process(BrokerProcess* broker_process) {
On 2013/12/12 21:37:21, Jorge Lucangeli Obes wrote:
> Is this the correct style for the setter?

Done.

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:133: void
set_broker_process(BrokerProcess* broker_process) {
On 2013/12/12 21:41:43, rsesek wrote:
> On 2013/12/12 21:37:21, Jorge Lucangeli Obes wrote:
> > Is this the correct style for the setter?
> 
> Yes, simple accessors use unix_hacker_style and get inlined.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:139: // and will
serve requests to access files over an IPC. The client of this
On 2013/12/12 21:37:21, Jorge Lucangeli Obes wrote:
> I would say "over IPC" or "over an IPC channel".

Done.

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:184:
DCHECK(!chromeos_arm_gpu);
On 2013/12/12 21:37:21, Jorge Lucangeli Obes wrote:
> Maybe add a comment reminding the reader "This policy is only used on
> x86_64/i386." The previous comment to this effect is in a different function.

Done, but not that it's also used on ARM desktop. Even though ARM Desktop is not
really a thing.

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:304: // Add arm
specific files to whitelist in the broker.
On 2013/12/12 21:37:21, Jorge Lucangeli Obes wrote:
> ARM-specific

Done.

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
File content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h (left):

https://chromiumcodereview.appspot.com/99133015/diff/80001/content/common/san...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h:6: #define
CONTENT_SHELL_BROWSER_SHELL_NET_LOG_H_
On 2013/12/12 21:37:21, Jorge Lucangeli Obes wrote:
> WTF =P

ack.

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
File content/common/sandbox_linux/sandbox_bpf_base_policy_linux.h (right):

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_base_policy_linux.h:35: virtual bool
PreSandboxHook();
On 2013/12/12 21:33:48, rsesek wrote:
> nit: blank line before comment and after method

Done.

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
File content/common/sandbox_linux/sandbox_bpf_cros_arm_gpu_policy_linux.cc
(right):

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_cros_arm_gpu_policy_linux.cc:204:
dlopen("/usr/lib/libmali.so", RTLD_NOW|RTLD_GLOBAL|RTLD_NODELETE);
On 2013/12/12 21:33:48, rsesek wrote:
> Maybe pull these flags out into a constant and pass them around that way?

Done.

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
File content/common/sandbox_linux/sandbox_bpf_cros_arm_gpu_policy_linux.h
(right):

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_cros_arm_gpu_policy_linux.h:15:
explicit CrosArmGpuProcessPolicy(bool allow_shmat)
On 2013/12/12 21:33:48, rsesek wrote:
> Don't inline ctors and dtors:
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

Done.

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_cros_arm_gpu_policy_linux.h:31: //
CONTENT_COMMON_SANDBOX_LINUX_SANDBOX_BPF_CROS_ARM_GPU_POLICY_LINUX_H_
On 2013/12/12 21:33:48, rsesek wrote:
> nit: put on previous line, even if it overflows 80

Done.

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
File content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.cc:78: is_enabled =
is_enabled &&
On 2013/12/12 21:33:48, rsesek wrote:
> Could switch to &=

Done.

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
File content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h (right):

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h:21:
GpuProcessPolicy() : broker_process_(NULL) {}
On 2013/12/12 21:33:48, rsesek wrote:
> Same out-of-line the dtor/ctor.

Done.

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h:36: bool
(*broker_sandboxer_callback)(void),
On 2013/12/12 21:33:48, rsesek wrote:
> Could/should this be a base::Callback?

We could do that. That will require changing the BrokerProcess class. Feel free
to add this to crbug.com/325535 as a TODO if you feel strongly about it.

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h:38: const
std::vector<std::string>& write_whitelist_extra);
On 2013/12/12 21:33:48, rsesek wrote:
> nit: blank line after

Done.

[jln (very slow on Chromium), 2013-12-12 23:38:00]

Mark: can you approve components/nacl.gyp ?

Also, what do you think of
https://chromiumcodereview.appspot.com/99133015/diff2/140001:220001/content/c...
to keep the transitive closure of link dependencies small ?

[Jorge Lucangeli Obes, 2013-12-12 23:53:13]

lgtm

[jln (very slow on Chromium), 2013-12-12 23:56:37]

Antoine: could you approve this CL as content/ OWNER?

I'm moving the Linux sandbox files to a new content/common/sandbox_linux
directory as part of a larger cleanup.

Also, would renaming things such as
"content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h" to
"content/common/sandbox_linux/bpf_gpu_policy_linux.h" (i.e. remove the redundant
"sandbox" in the name) look ok ?

[Mark Seaborn, 2013-12-13 00:03:47]

https://chromiumcodereview.appspot.com/99133015/diff/220001/content/common/sa...
File content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/99133015/diff/220001/content/common/sa...
content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:297: #if
!defined(NACL_LINUX)
This is abusing the NACL_LINUX macro somewhat.  This is a macro that is defined
as 0 or 1 depending on whether you're on Linux.  It's intended to be used as
"#if NACL_LINUX" or "if (NACL_LINUX)".

Could you instead #define something like IN_NACL_HELPER for nacl_helper's .cc
files?  That's less likely to go wrong in future, and closer to how the
NACL_WIN64 macro works.

https://chromiumcodereview.appspot.com/99133015/diff/220001/content/common/sa...
content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:319: bool
StartBPFSandbox(const CommandLine& command_line,
I suspect this could go inside "#if !defined(IN_NACL_HELPER)" too, then all of
GetGpuProcessSandbox() could go inside the #if as well.

[piman, 2013-12-13 00:03:58]

On 2013/12/12 23:56:37, jln wrote:
> Antoine: could you approve this CL as content/ OWNER?
> 
> I'm moving the Linux sandbox files to a new content/common/sandbox_linux
> directory as part of a larger cleanup.

LGTM though next time if possible, please separate the move CL from the
functional changes.

> 
> Also, would renaming things such as
> "content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h" to
> "content/common/sandbox_linux/bpf_gpu_policy_linux.h" (i.e. remove the
redundant
> "sandbox" in the name) look ok ?

Sure.

[jln (very slow on Chromium), 2013-12-13 00:20:42]

Thanks Mark. PTAL!

https://chromiumcodereview.appspot.com/99133015/diff/220001/content/common/sa...
File content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/99133015/diff/220001/content/common/sa...
content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:297: #if
!defined(NACL_LINUX)
On 2013/12/13 00:03:48, Mark Seaborn wrote:
> This is abusing the NACL_LINUX macro somewhat.  This is a macro that is
defined
> as 0 or 1 depending on whether you're on Linux.  It's intended to be used as
> "#if NACL_LINUX" or "if (NACL_LINUX)".
> 
> Could you instead #define something like IN_NACL_HELPER for nacl_helper's .cc
> files?  That's less likely to go wrong in future, and closer to how the
> NACL_WIN64 macro works.

Done.

https://chromiumcodereview.appspot.com/99133015/diff/220001/content/common/sa...
content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:319: bool
StartBPFSandbox(const CommandLine& command_line,
On 2013/12/13 00:03:48, Mark Seaborn wrote:
> I suspect this could go inside "#if !defined(IN_NACL_HELPER)" too, then all of
> GetGpuProcessSandbox() could go inside the #if as well.

Done.

[Jorge Lucangeli Obes, 2013-12-13 00:24:39]

Tested on leon (x86_64 Intel) and GPU is sandboxed.

https://chromiumcodereview.appspot.com/99133015/diff/160002/content/common/sa...
File content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/99133015/diff/160002/content/common/sa...
content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:342:
ignore_result(IsChromeOS);
Don't you need to actually call the functions here?

[jln (very slow on Chromium), 2013-12-13 00:26:22]

Thanks for the test Jorge!

https://chromiumcodereview.appspot.com/99133015/diff/160002/content/common/sa...
File content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/99133015/diff/160002/content/common/sa...
content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:342:
ignore_result(IsChromeOS);
On 2013/12/13 00:24:40, Jorge Lucangeli Obes wrote:
> Don't you need to actually call the functions here?

No, using the address is fine as a "usage". Otherwise function pointers wouldn't
work.

It's cleaner to not actually call them.

[Mark Seaborn, 2013-12-13 01:08:47]

LGTM for NaCl parts

https://chromiumcodereview.appspot.com/99133015/diff/320001/components/nacl.gyp
File components/nacl.gyp (right):

https://chromiumcodereview.appspot.com/99133015/diff/320001/components/nacl.g...
components/nacl.gyp:184: # Allow .cc files to know if they're being compiled as
part
Nit: indent by 2 more spaces to match other lines

https://chromiumcodereview.appspot.com/99133015/diff/320001/content/common/sa...
File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/99133015/diff/320001/content/common/sa...
content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:211:
dlopen("/usr/lib/libmali.so", dlopen_flag);
You should probably be checking for errors here, if only to warn

https://chromiumcodereview.appspot.com/99133015/diff/320001/content/common/sa...
File content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/99133015/diff/320001/content/common/sa...
content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:340: return false;
Redundant when there's another return later?  It looks odd.

[jln (very slow on Chromium), 2013-12-13 01:31:31]

Thanks Mark!

https://codereview.chromium.org/99133015/diff/320001/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/99133015/diff/320001/components/nacl.gyp#newc...
components/nacl.gyp:184: # Allow .cc files to know if they're being compiled as
part
On 2013/12/13 01:08:47, Mark Seaborn wrote:
> Nit: indent by 2 more spaces to match other lines

Done.

https://codereview.chromium.org/99133015/diff/320001/content/common/sandbox_l...
File content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc (right):

https://codereview.chromium.org/99133015/diff/320001/content/common/sandbox_l...
content/common/sandbox_linux/bpf_cros_arm_gpu_policy_linux.cc:211:
dlopen("/usr/lib/libmali.so", dlopen_flag);
On 2013/12/13 01:08:47, Mark Seaborn wrote:
> You should probably be checking for errors here, if only to warn

Agreed. This is old code that I'm moving. I mentioned that to jorgelo@. It's
even worse than it seems, because this will run even on non Mali machines and
thus is expected to fail.

https://codereview.chromium.org/99133015/diff/320001/content/common/sandbox_l...
File content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc (right):

https://codereview.chromium.org/99133015/diff/320001/content/common/sandbox_l...
content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:340: return false;
On 2013/12/13 01:08:47, Mark Seaborn wrote:
> Redundant when there's another return later?  It looks odd.

It was to make it absolutely clear that what's below is dead code. But I think
the risk of the compiler not figuring it out is small. Removed.

[Robert Sesek, 2013-12-13 01:39:15]

LGTM

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
File content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h (right):

https://chromiumcodereview.appspot.com/99133015/diff/120001/content/common/sa...
content/common/sandbox_linux/sandbox_bpf_gpu_policy_linux.h:36: bool
(*broker_sandboxer_callback)(void),
On 2013/12/12 22:15:14, jln wrote:
> On 2013/12/12 21:33:48, rsesek wrote:
> > Could/should this be a base::Callback?
> 
> We could do that. That will require changing the BrokerProcess class. Feel
free
> to add this to crbug.com/325535 as a TODO if you feel strongly about it.

Don't feel strongly, was just thinking aloud.

[jln (very slow on Chromium), 2013-12-13 01:46:05]

Committed patchset #13 manually as r240509 (presubmit successful).

[scottmg, 2013-12-13 06:19:47]

A revert of this CL has been created in
https://codereview.chromium.org/114483003/ by scottmg@chromium.org.

The reason for reverting is: Appears to have caused failures on linux dbg test
1.