mash: Store chrome --mash crash key metadata in shared memory

mash: Store chrome --mash crash key metadata in shared memory

Chrome uses "crash keys" to provide additional metadata for crash dump
uploads (e.g. the last active URL). Change chrome mash processes to store
their keys in a breakpad SimpleStringDictionary, which is just a fixed-
size table of key/value pairs. The dictionary is kept in a block of
shared memory named "/chrome_crash_keys_<pid>" such that crash_reporter
can find it.

The shared memory buffer is cleaned up by Chrome during normal shutdown
and by crash_reporter during crashes.

The OS-level change to crash reporter is:
https://chromium-review.googlesource.com/c/444846/

BUG=692594
TEST=Added to chrome_app_unittests, also manually tested crashing ash
and verified the uploaded crash report has valid crash keys on the crash
dashboard

[James Cook, 2017-02-27 19:29:38]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[James Cook, 2017-02-27 19:30:10]

jamescook@chromium.org changed reviewers:
+ vapier@chromium.org

[James Cook, 2017-02-27 19:30:10]

vapier, please take a look.

[commit-bot: I haz the power, 2017-02-27 19:30:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 21:13:14]

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

[commit-bot: I haz the power, 2017-02-27 21:13:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2017-02-28 17:56:57]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 17:57:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 19:21:51]

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

[commit-bot: I haz the power, 2017-02-28 19:21:52]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-02-28 19:22:59]

Ping vapier?

[James Cook, 2017-03-02 01:03:34]

jamescook@chromium.org changed reviewers:
+ hashimoto@chromium.org

[James Cook, 2017-03-02 01:03:34]

hashimoto, can you take a look? Thanks.

[hashimoto, 2017-03-02 07:42:23]

I'd like to take a look at sample crash report made with this code.
Could you give me a crash ID?

https://codereview.chromium.org/2718123003/diff/20001/ash/common/accelerators...
File ash/common/accelerators/debug_commands.cc (right):

https://codereview.chromium.org/2718123003/diff/20001/ash/common/accelerators...
ash/common/accelerators/debug_commands.cc:154:
base::debug::SetCrashKeyValue("url-chunk", "crash-url-chunk-for-testing");
This line depends on chrome/common/crash_keys.cc to register kActiveURL
(="url-chunk") as a crash key, when running as part of chrome, right?
IIUC ash shouldn't depend on chrome so please avoid doing this.

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
File chrome/app/mash/mash_crash_keys.cc (right):

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys.cc:41:
g_crash_key_dictionary->SetKeyValue(key.data(), value.data());
base/strings/string_piece.h says StrinbPiece::data() shouldn't be used when a
NUL terminated string is expected.
https://cs.chromium.org/chromium/src/base/strings/string_piece.h?q=file:base/...

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys.cc:45:
g_crash_key_dictionary->RemoveKey(key.data());
ditto.

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys.cc:60: g_shared_memory_fd =
shm_open(shared_memory_name.c_str(), oflag, mode);
Can't we just use utilities in base/memory/shared_memory_helper.h and
base/memory/shared_memory.h?

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys.cc:64: int result =
ftruncate(g_shared_memory_fd, kSharedMemorySize);
Please use HANDLE_EINTR.
ftruncate may return EINTR.

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys.cc:72: // Memory filled with 0 is a valid empty
SimpleStringDictionary.
Please use placement new, so that you don't have to have this assumption.

g_crash_key_dictionary = new (buffer) SimpleStringDictionary();

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys.cc:77: void RegisterCrashKeys() {
If you want to allow processes which cannot depend on chrome code to register
these crash keys, please move the corresponding part of
RegisterChromeCrashKeys() in chrome/common/crash_keys.cc to somewhere those
processes can depend on.

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys.cc:81: {"url-chunk", crash_keys::kLargeSize},
Please use the constants defined in the appropriate header.

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
File chrome/app/mash/mash_crash_keys.h (right):

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys.h:11: #error Unsupported platform
Why do we need this when mash_crash_keys.cc is already excluded by GN if
!is_chromeos?

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys.h:16: // Initializes mash support for crash keys
(see base::debug::crash_logging.h).
nit: "base::debug::crash_logging.h" -> "base/debug/crash_logging.h"?

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
File chrome/app/mash/mash_crash_keys_unittest.cc (right):

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys_unittest.cc:32:
base::debug::SetCrashKeyValue("url-chunk", "http://example.com");
Please use the constant name kActiveUrl.

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys_unittest.cc:34:
base::debug::ClearCrashKey("url-chunk");
ditto.

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_ru...
File chrome/app/mash/mash_runner.cc (left):

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_ru...
chrome/app/mash/mash_runner.cc:261: // service processes is handled by the
OS-level crash_reporter.
IMO we still need this comment here to describe how stack dumping works on
Chrome OS.

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_ru...
File chrome/app/mash/mash_runner.cc (right):

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_ru...
chrome/app/mash/mash_runner.cc:157: mash_crash_keys::Initialize();
How about putting this code in MashMain or RunMain to be consistent with other
initialization code?

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_ru...
chrome/app/mash/mash_runner.cc:161: MashRunner::~MashRunner() {
ditto.
Other shutdown code is in RunMain().

[hashimoto, 2017-03-02 09:49:23]

A few more comments:
- IIUC breakpad's SimpleStringDictionary is suitable for IPC only when used
between processes built at the same time (i.e. Chrome browser process and
renderer process).  Since chrome and crash_reporter belong to different
repositories and built separately, any difference of breakpad version, compiler,
or build option can break this feature.
- Linux kernel may recycle the PID after a process exits. So a new process with
the same PID can overwrite the shared memory when crash happens repeatedly.

[James Cook, 2017-03-07 17:59:56]

On 2017/03/02 09:49:23, hashimoto wrote:
> A few more comments:
> - IIUC breakpad's SimpleStringDictionary is suitable for IPC only when used
> between processes built at the same time (i.e. Chrome browser process and
> renderer process).  Since chrome and crash_reporter belong to different
> repositories and built separately, any difference of breakpad version,
compiler,
> or build option can break this feature.
> - Linux kernel may recycle the PID after a process exits. So a new process
with
> the same PID can overwrite the shared memory when crash happens repeatedly.

Thank you for the review. I'm going to put this on hold for now, because I have
some other things to finish for Q1. I hope to pick it up next quarter. That
should also give rsesek@ and the crash team some time to finish their
refactoring of crash key registration.

[vapier, 2017-03-11 06:15:00]

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
File chrome/app/mash/mash_crash_keys.cc (right):

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys.cc:56: const int oflag = O_CREAT | O_RDWR;
i don't think this exec's, so should use O_CLOEXEC everywhere unless you need it

https://codereview.chromium.org/2718123003/diff/20001/chrome/app/mash/mash_cr...
chrome/app/mash/mash_crash_keys.cc:59: const mode_t mode = S_IRUSR | S_IWUSR;
S_xxx constants aren't readable.  just use an octal constant like 0600.