Currently, the scripts in src/scripts have multiple implementations
for handling when common.sh fails to load, some of which are buggy.
To simplify the boilerplate, these scripts now just exit if common.sh
fails to load. The shell itself will print the following message if
common.sh is not found:
/usr/lib/crosutils/common.sh: No such file or directory
BUG=chromium-os:32442
TEST=Run these scripts with and without common.sh installed.
Change-Id: Ie54420b6c649774f9cb039c14c80f4cf6c6ebc07
Reviewed-on: https://gerrit.chromium.org/gerrit/27058
Reviewed-by: David James <davidjames@chromium.org>
Tested-by: David James <davidjames@chromium.org>
Commit-Ready: David James <davidjames@chromium.org>
Change the kernel loglevel from 1 to 0 to avoid showing console
messages on the screen during shutdown.
An example is the "Power down." message that is shown when shutting
down. This appears on the screen because the kernel prints the
message with KERN_EMERG (level 0). As such, 0 < 1, and the message
appears on the screen.
BUG=chromium-os:28602
TEST=Tested on lumpy, saw no messages when shutting down.
Change-Id: Id3842c2203f6cc4bf3bc9165d8537f440fffba61
Reviewed-on: https://gerrit.chromium.org/gerrit/22104
Reviewed-by: Olof Johansson <olofj@chromium.org>
Tested-by: Sean Paul <seanpaul@chromium.org>
Commit-Ready: Sean Paul <seanpaul@chromium.org>
Currently, if set -e spots a nonzero exit we basically have
no real debug information- it just stops immediately without stating
where or why. This forces our scripts to be stupidly verbose so
we can track roughly where they were, thus when they fail we can
use that information to localize the rough exit point.
Instead we should be traping that set -e induced exit and
outputing necessary debug information to run it down. This includes
outputing the relevant stack trace, or at least what we can get of
it.
The 'die' function is now enhanced to automatically dump the trace
that lead to it. For most consumers this is desired- however for
commandline parsing induced dies ("--board is missing" for example),
the trace is noise. For those cases, a 'die_notrace' function was
added that retains the original non-backtrace behaviour.
Example output via instrumenting cros_generate_breakpad_symbols
w/ the failing command '/bin/false' (nonzero exit code).
Before:
./cros_generate_breakpad_symbols monkeys --board=x86-alex
<no output at all, just exit code 1>
With this CL:
./cros_generate_breakpad_symbols monkeys --board=x86-alex
ERROR : script called: ./cros_generate_breakpad_symbols 'monkeys' '--board=x86-alex'
ERROR : Backtrace: (most recent call is last)
ERROR : file cros_generate_breakpad_symbols, line 207, called: main 'monkeys' '--board=x86-alex'
ERROR : file cros_generate_breakpad_symbols, line 163, called: die_err_trap '/bin/false' '1'
ERROR :
ERROR : Command failed:
ERROR : Command '/bin/false' exited with nonzero code: 1
BUG=chromium-os:30598
TEST=inject a failing command into a script, verify the output.
TEST=inject a 'command not found', verify the output
TEST=cbuildbot x86-generic-full --remote
TEST=cbuildbot arm-tegra2-full --remote
TEST=cbuildbot chromiumos-sdk --remote
Change-Id: I517ffde4d1bb7e2310a74f5a6455b53ba2dea86c
Reviewed-on: https://gerrit.chromium.org/gerrit/17225
Reviewed-by: Brian Harring <ferringb@chromium.org>
Tested-by: Brian Harring <ferringb@chromium.org>
Commit-Ready: Brian Harring <ferringb@chromium.org>
This reverts commit bc856858be86b1ae7c4dc33b256f43baac51636d
The thing is that ${FLAGS_board} is not set when build_kernel_image.sh runs, so the check never kicks in.
Change-Id: I501cb979c7aef8d2f7061da2b6cf2daedfe65004
Reviewed-on: https://gerrit.chromium.org/gerrit/20977
Tested-by: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Commit-Ready: Vadim Bendebury <vbendeb@chromium.org>
For some still unknown reason writes to location 0xcf9 do not cause
the Link to reboot, they cause it to shut down instead. While this
will have to be investigated and fixed, this change modifies the code
to have the kernel use the keyboard controller (implemented by the EC
on Link) to restart the system.
Once the 0xcf9 problem is resolved, this change could be reverted.
BUG=chrome-os-partner:8397
TEST=manual
. typing 'reboot' at the shell causes the system to restart. It
was shutting down before this change.
Change-Id: I515c87c8ffb57c444bfc3e7074d584e7cbefa87f
Signed-off-by: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-on: https://gerrit.chromium.org/gerrit/18333
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Rather than trying to use an old/stale common.sh, use the common.sh
from the invocation point- if invoked via /usr/lib/crosutils, use that
common.sh. If invoked via src/scripts/, use that, etc.
Trying to intermix it just introduces potential for bugs and invalidly
freezes common.sh api, thus the efforts to revert this and ultimately
revert the existing of a crosutils ebuild.
BUG=chromium-os:27201
TEST=cbuildbot x86-generic-full
Change-Id: I4c6c5fbade3d28c71752bd4c44dccad49af52ec0
Reviewed-on: https://gerrit.chromium.org/gerrit/18303
Reviewed-by: David James <davidjames@chromium.org>
Commit-Ready: Brian Harring <ferringb@chromium.org>
Tested-by: Brian Harring <ferringb@chromium.org>
There was no reason for it to be platform-specific. Original CL
that added it to ARM (it was already there for x86) is:
<http://gerrit.chromium.org/gerrit/1467>
This change is in preparation for moving platform-specific bits
into ebuilds.
BUG=chromium-os:24808
TEST=Validated that kern_guid gets set properly.
Change-Id: I5544ad3730e05128c0a9b0a4a3a8aee80ef31df5
Reviewed-on: https://gerrit.chromium.org/gerrit/13821
Tested-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Mandeep Singh Baines <msb@chromium.org>
Commit-Ready: Doug Anderson <dianders@chromium.org>
This disables text cursor on ttys by default to get rid of a black
rectangle showing up during but on splash screen.
BUG=chromium-os:22879
TEST=manual boot on Alex
Change-Id: I06c50034c03488e49f88e01152cdceeee4f49a24
Signed-off-by: Pawel Osciak <posciak@chromium.org>
Reviewed-on: https://gerrit.chromium.org/gerrit/11730
Reviewed-by: Olof Johansson <olofj@chromium.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
This causes the script to treat amd64 the same as x86.
BUG=chromium-os:21284
TEST=./build_image for amd64-generic shouldn't generate an invalid
architecture error
Change-Id: I8f40a827684dde1258158d470a38623a8c936bca
Reviewed-on: http://gerrit.chromium.org/gerrit/9963
Reviewed-by: Vince Laviano <vlaviano@chromium.org>
Tested-by: Sonny Rao <sonnyrao@chromium.org>
Commit-Ready: Sonny Rao <sonnyrao@chromium.org>
Tegra2 systems require a minimum vmalloc bootarg to successfully
reserve the graphics carveout memory. If the vmalloc is insufficient
a crash can result prior to any serial output reported. The minimum
vmalloc is >= carveout size + framebuffer - 32MB, thus for a 256MB
graphics carveout (with 10MB framebuffer), vmalloc>=234MB. This
arg is not added in the u-boot configs to avoid tightly coupling
the u-boot and kernel and is discussed in greater depth at
http://gerrit.chromium.org/gerrit/#change,8293.
BUG=chrome-os-partner:5197,chrome-os-partner:5902
TEST=Manually observe /proc/cmdline reflects change
Change-Id: I66b35b266c7542771f2d4fc497dd4429587529f8
Reviewed-on: http://gerrit.chromium.org/gerrit/8373
Commit-Ready: Katie Roberts-Hoffman <katierh@chromium.org>
Reviewed-by: Katie Roberts-Hoffman <katierh@chromium.org>
Tested-by: Katie Roberts-Hoffman <katierh@chromium.org>
This will prevent the recovery kernel from having a different salt from the
rootfs it corresponds to.
BUG=chromium-os:20766
TESTED_ON=kaen
TEST=Adhoc
Build a recovery image and do recovery from it.
Change-Id: I96f735e527d807247e09e17aac1ed5b51367f0ef
Signed-off-by: Elly Jones <ellyjones@chromium.org>
Reviewed-on: http://gerrit.chromium.org/gerrit/8288
BUG=chromium-os:12138
TEST=Adhoc
TESTED_ON=Kaen
Build an image. Look for "Generating root fs hash tree (salt <foo>)." in the
output. Boot the image, grep for 'salt=' in dmesg. All should be well.
Change-Id: Ia7d8504033ff1e62ba451129be5796702809cc7c
Signed-off-by: Elly Jones <ellyjones@chromium.org>
Reviewed-on: http://gerrit.chromium.org/gerrit/7335
BUG=chromium-os:12138
TEST=Adhoc
TESTED_ON=Kaen
Build an image. Look for "Generating root fs hash tree (salt <foo>)." in the
output. Boot the image, grep for 'salt=' in dmesg. All should be well.
Change-Id: If9dbefbd8a875d06ff45cd54704f166c2511c3b7
Signed-off-by: Elly Jones <ellyjones@chromium.org>
Reviewed-on: http://gerrit.chromium.org/gerrit/7174
This give users the choice to have rootfs formatted with squashfs.
When --squash_image is specified, the rootfs will be formatted to squashfs.
Users can also use --squash_sort_file to specify the file priority when
squashfs is created.
BUG=None
TEST=Manually tested "--squash_image", and the image can be installed
from USB stick. Also tried "--squash_sort_file=sort-prio.list", and files
in squashfs are sorted.
Change-Id: I5fd818ac9d1203598926efa82e94fa105cd86ebc
Reviewed-on: http://gerrit.chromium.org/gerrit/5664
Tested-by: Da Zheng <zhengda@chromium.org>
Reviewed-by: Da Zheng <zhengda@chromium.org>
This reverts commit 451f36e4a8.
Last time I removed the --crosbug12352_arm_kernel_signing flag, buildbot
failed. The reason seemed to be that buildbot still passing this flag to
build_image. However, I cannot find anywhere in the log that indicates
buildbot did pass this flag to build_image. So I think the last failure
should be transient and it is good to obsolete this flag.
BUG=chromium-os:12352
TEST=build_image
TEST=load_kernel_test -b 2 /path/to/image /path/to/recovery_key.vbpubk
Change-Id: Ic757eb2dc4304e7205b483063335f8816b536433
Reviewed-on: http://gerrit.chromium.org/gerrit/4794
Reviewed-by: Che-Liang Chiou <clchiou@chromium.org>
Tested-by: Che-Liang Chiou <clchiou@chromium.org>
The problem here is that most were doing their exiting w/in a subshell;
exit within a subshell kills the subshell, not the parent. Not all scripts
were using set -e (which would pick up the failing subshell); as such
just rewriting them to remove the potential via eliminateing the subshelling.
Beyond that, removed a couple of custom (working, although non-standard)
approaches, and removed a duplicate common.sh sourc'ing w/in mk_memento_images.sh
TEST=force 'find_common_sh' to fail, note the scripts fails to exit
BUG=none
Change-Id: Ia1108a091a6399ad6aedd3cade4a107f4411686c
Reviewed-on: http://gerrit.chromium.org/gerrit/3905
Reviewed-by: Brian Harring <ferringb@chromium.org>
Tested-by: Brian Harring <ferringb@chromium.org>
Since now the arm firmware can parse %U as x86 bios, and kernel can
parse PARTNROFF=%d, we are able to generate kernel command line with
such construct.
BUG=chromium-os:14022,15683
TEST=manual
1. Build image with root filesystem verification turns off
2. Boot successfully
3. Run 'cat /proc/cmdline' and validate its output
4. Run 'rootdev' and validate its output
Change-Id: I11de0a30928efe9d9b0149feb3389a2f30063516
Reviewed-on: http://gerrit.chromium.org/gerrit/1104
Tested-by: Che-Liang Chiou <clchiou@chromium.org>
Reviewed-by: <nsanders@google.com>
Reviewed-by: Che-Liang Chiou <clchiou@chromium.org>
This commit is a part of transition to enable ARM kernel signing. It is
at first an option that is enabled manually, and then (in this commit)
enabled by default. After more tests, the scripts that generate unsigned
ARM kernel partition will probably be removed.
BUG=chromium-os:12352
TEST=./build_image && load_kernel_test -b 2 /path/to/chromiumos_image.bin /usr/share/vboot/devkeys/recovery_key.vbpubk
Change-Id: I7d4ecc566f9c5cc0106a7af59255fc63fdfe017a
Tested-by: Che-Liang Chiou <clchiou@chromium.org>
Reviewed-by: Tom Wai-Hong Tam <waihong@chromium.org>
Reviewed-by: Che-Liang Chiou <clchiou@chromium.org>
Reviewed-by: Rong Chang <rongchang@chromium.org>
Tested-by: Tom Wai-Hong Tam <waihong@chromium.org>
Reviewed-on: http://gerrit.chromium.org/gerrit/1319
Tested-by: Nick Sanders <nsanders@chromium.org>
This commit is a part of transition to enable ARM kernel signing. It is
at first an option that is enabled manually, and then (in this commit)
enabled by default. After more tests, the scripts that generate unsigned
ARM kernel partition will probably be removed.
BUG=chromium-os:12352
TEST=./build_image && load_kernel_test -b 2 /path/to/chromiumos_image.bin /usr/share/vboot/devkeys/recovery_key.vbpubk
Change-Id: I6d48d1603cd7c96514892bcbbf8994b2d4cc2a08
Reviewed-on: http://gerrit.chromium.org/gerrit/512
Tested-by: Che-Liang Chiou <clchiou@chromium.org>
Reviewed-by: Tom Wai-Hong Tam <waihong@chromium.org>
verity_depth must be 0. No other values are supported. So changing the
default to 0.
Needs to go in before:
http://codereview.chromium.org/6901005
BUG=chromium-os:14357
TEST=Ran build_image, mod_image_for_test.sh, mod_image_for_recovery.sh
chromeos-install, and image_to_live.sh.
Change-Id: I6208327d9ce68c9ba56d78e99bc145e34bee9d1d
R=wad@chromium.org,scottz@chromium.org,gauravsh@chromium.org
Review URL: http://codereview.chromium.org/6902061
It makes debugging system boot issues harder, so let's re-enable console
output for a while. It will be quieted down again later.
Change-Id: I7c543c09818c0152470c9f5e54d36c614d05bf3a
BUG=chrome-os-partner:3199
TEST=Boot a system, check for console output before login screen
Review URL: http://codereview.chromium.org/6801045
The use of the mux can cause issues with some EC 8042 controllers and
result in loss of keyboard/trackpad between suspend/resume/reboot cycles.
We will never have more than keyboard+trackpad attached through the 8042
on our devices so using the mux is unnecessary.
BUG=chrome-os-partner:2700
TEST=manual verification that mux is not probed at boot:
Original:
serio: i8042 KBD port at 0x60,0x64 irq 1
serio: i8042 AUX0 port at 0x60,0x64 irq 12
serio: i8042 AUX1 port at 0x60,0x64 irq 12
serio: i8042 AUX2 port at 0x60,0x64 irq 12
serio: i8042 AUX3 port at 0x60,0x64 irq 12
New:
serio: i8042 KBD port at 0x60,0x64 irq 1
serio: i8042 AUX port at 0x60,0x64 irq 12
Change-Id: I942ab86eb71941ab072ad6a17d15b067ca94439d
Review URL: http://codereview.chromium.org/6679031
For the kernel nmi to be activated and function properly it
must be enabled in the kernel command line. This CL adds
`nmi_watchdog=panic,lapic' (which is equivalent of
nmi_watchdog=2) to the command line. The reason '2' was used
and not '1' is that nmi_watchdog=1 is known to break audio
functionality on chromeos devices.
Note that only x86 platforms are affected by this change,
ARM platforms will be added later.
This CL is required by the autotest CL
http://codereview.chromium.org/6596002
Change-Id: Ie8a9ba3f0de6d236cbe098e402b0240aa64ddcd0
BUG=chromium-os:12463, chromium-os:12464
TEST=see below
. build new image for test and install it on the target
. restart the target
. observe the contents of /proc/sys/kernel/nmi_watchdog (it
should read '1' after reboot)
. run autotest as follows
(chroot) ~/trunk/src/scripts $ ./run_remote_tests.sh \
--board=x86-mario --remote=172.22.75.163 \
platform_KernelErrorPaths kernel_BootMessagesServer \
platform_HighResTimers
. observe the results:
INFO : Test results:
---------------------------------------------------------
kernel_BootMessagesServer PASS
kernel_BootMessagesServer/kernel_BootMessagesServer PASS
coldboot_active_mb 50
coldboot_anonpages_mb 36
coldboot_buffers_mb 3
coldboot_cached_mb 166
coldboot_inactive_mb 156
coldboot_memfree_mb 1649
platform_HighResTimers PASS
platform_HighResTimers/platform_HighResTimers PASS
platform_KernelErrorPaths PASS
platform_KernelErrorPaths/platform_KernelErrorPaths PASS
---------------------------------------------------------
Total PASS: 6/6 (100%)
No crashes detected during testing.
Elapsed time: 4m28s
Review URL: http://codereview.chromium.org/6597001
For now arm kernel partitions are not signed. This CL is a transitionsl.
That is, the added flag should be removed after arm verified boot is stable.
To properly create an arm kernel partition, we also need another CL for
vbutil_kernel utility that turns off x86-only modifications on kernel
image. See CL:6538015.
BUG=chromium-os:3790,chromium-os:12352
TEST=see below
Build images for x86 and arm successfully, and notice that load_kernel_test
passes for x86 and signed arm image.
$ build_image --board=tegra2_seaboard --crosbug12352_arm_kernel_signing
$ build_image --board=tegra2_seaboard --nocrosbug12352_arm_kernel_signing
$ build_image --board=x86-generic
Review URL: http://codereview.chromium.org/6538014
Change-Id: I1be381bae2fc367a0603ac2ec67ee70fc9a257e4
Using %U+1 will ensure that we avoid device enumeration issues during recovery mode
boots.
TEST=build_image+dev recovery kernel and boot to it on new cros fw / fixed enumeration problem
used build with mp recovery kernel on cr-48 -> installed then booted fine
used dev recovery kernel on a legacy machine; installed fine, booted fine
tested with cr-48, mp recovery kernel, and noenable_rootfs_verification to ensure /dev/sd%D%P still worked as normal.
Change-Id: I5b1277a47536738a78c18988fd912cc05ebddd4b
BUG=chromium-os:5470
Review URL: http://codereview.chromium.org/6549034
from within the chroot.
It also fixes a number of style issues.
It changes the meaning of cros_workon "list-all" to list all available
packages, and adds "list-live" to list all live packages.
It changes things that load chromeos-common.sh from the installer to
load it from /usr/lib/installer.
BUG=chromium-os:4230
TEST=synced, rebuilt chroot, made packages, made images, built chrome
from source, and wrote an image to a USB stick.
Review URL: http://codereview.chromium.org/6240018
Change-Id: I90c34420af1a64020402bafef8e9e77f56837c02
Not printing on the uart makes a huge difference in boot time performance. Since it's easy to re-enable again for those debugging boot time issues, let's disable it by default. It's how x86 does it as well.
Change-Id: Idb4af8d98434e70ee1389b3579c220da34be9c3b
BUG=chromium-os:11351
TEST=Build image and boot system
Review URL: http://codereview.chromium.org/6290011
This reverts commit ea621903e9.
TEST=grep dmesg for clock & nmi to make sure things look ok.
Run example program to make sure result of clock_getres call tv_nsec = 1 again
BUG=chromium-os:10720
Change-Id: Ife23fc7d08420788d4c165957d739ec2ce974969
Review URL: http://codereview.chromium.org/6274006
BUG=10425
TEST=Tested before and after.
Change-Id: Ic3e66805945f379a79562338b16845bb5dbd674f
Before: Machine hangs till power-cycle.
After: Machine reboots and we get a crash dump.
Review URL: http://codereview.chromium.org/5893005
On ARM platform read kernel from device passed in from u-boot
instead of hardcoding it to 0
BUG=none
TEST=emerged on seaboard, booted from SD card and emmc
Change-Id: Ia4506ed9f85d94eb37a9ac57430e1490d106c403
Review URL: http://codereview.chromium.org/5612008
Patch from Allen Martin <amartin@nvidia.com>.
This just matches H2C and H2O
Changes are:
-earlyprintk: serial isn't supported, so remove
-console=ttyS0: allow /dev/console to function, after serial console support is removed.
+quiet: sets loglevel=2, quiets early boot
+console=tty2: this will output console onto vt2.
+loglevel=1: this reduces output to vt2 past what quiet specifies.
BUG=chromium-os:8084
TEST="boot factory install shim, regular image. Note /dev/console works and outputs to tty2"
Change-Id: I2d77bf1de5870c7e610859f063d5a587acd56051
Review URL: http://codereview.chromium.org/4167001
We need to set these options in order to link the TPM driver in the kernel before we fix the PNP table in the firmware. This is the least disruptive change that makes the TPM available earlier in the boot sequence.
Change-Id: I729cd7c153507200e177895bae01951e97b70968
BUG=none
TEST=rebuilt kernel, reinstalled, booted, verified that tcsd still runs
Review URL: http://codereview.chromium.org/3969001
dm_verity will wait for a device to be ready. This is needed in the factory installer so that
dmsetup doesn't fail early before the usb device is visible.
TEST=built images and booted them (x86-generic & factory installer)
Prior to commit, need to ensure dev_wait doesn't break unverified boot behavior
BUG=chromium-os:7451
Change-Id: I5b838b94e6a17dd0778331121311cdfe180991ce
Review URL: http://codereview.chromium.org/3936001
- This unbreaks the devserver's update server for ARM.
- This also unforks some of build_image.
BUG=None
TEST=Installed to tegra2_seaboard from USB without --skip_vblock,
updated tegra2_seaboard.
Review URL: http://codereview.chromium.org/3493004
Change-Id: I6e66344de51609393407934f78aa20f49974efef
- Build a "kernel image" which contains a uboot script and a uboot kernel
image.
- Fix some sd* assumptions.
- Remove cruft that has never done anything usefull from update_bootloaders
BUG=none
TEST=Built, booted, and updated on tegra2_dev-board
Review URL: http://codereview.chromium.org/3396011
Change-Id: I00ecf57faa5fe64c8e33dd4c042f1dbed806c10a
Change-Id: I14cd9dc365093c0450210d7853ad5f67ffa0ddd0
BUG=chromium-os:5183
TEST=1) manually built a dev install shim and verified it's only bootable when dev switch is ON
Review URL: http://codereview.chromium.org/3153001
With the newest Chrome OS BIOS and bootstub, this will be expanded to the
booted kernel partition's UniqueGuid, so that the kernel device can be
determined with certainty, since the BIOS and kernel may enumerate drives
differently.
You can identify the booted kernel partition at runtime with something like
this:
sudo cgpt find -1 -u \
$(cat /proc/cmdline | sed 's/.*kern_guid=\([0-9a-f-]\+\).*/\1/')
Review URL: http://codereview.chromium.org/3035020
This change adds
- --rootfs_hash_pad to specify the MBs reserved for the pad
- the implementation of the above flag
- check if total fs size + pad size exceeds the partition size
- hash appending in make_image_bootable()
Fixes:
- a style for ROOT_FS_HASH usage
- bad mount|grep
- bad bash subst for root devices in all boot paths
- fixed a typo in the update_bootloaders table creation
- disables verified usb for now
Adding the padding argument ensures that the generated hash tree for the root filesystem is appended to the image. Assuming the rootfs is _never_ mounted read-write
again, that hash tree will be valid and vboot will be able to proceed.
BUG=chromium-os:2693
TEST=manual build_image
Review URL: http://codereview.chromium.org/3043011
Change-Id: I67d9b0f91cacdefa309c0cc2dd7fed1d2eddd7a7
The use_vboot and vboot_ flags were confusing from a functionality perspective
since verified boot as a feature encompasses firmware and kernel functionality.
The firmware bits are always enabled, but use_vboot enabled the image-integrity
portion of vboot. It is not called
--enable_rootfs_verification
and all options for the kernel functionality is under --verity_* given that
verity/dm-verity is the current working name for the module and userspace tool.
TEST=ran x86-generic build_image & tegra2-dev-board build_image and checked the resulting boot.config files (with and without --enable_rootfs_verification).
BUG=chromium-os:2693
Review URL: http://codereview.chromium.org/2917008
