Refactor CrashDump*Manager to use a shared CrashDumpObserver.

Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton.

CrashDumpManager watches for generated microdumps when child processes
die, and CrashMicroDumpManager is responsible for aborting the WebView
browser process. Both share similarities that can be combined, and doing
so allows us to eventually support generating a minidump and then dying
in WebView.

BUG=633979
Committed: https://crrev.com/982ab4cee9fef4a7af0321b47d3dbfe5813b7975
Cr-Commit-Position: refs/heads/master@{#413431}

[Tobias Sargeant, 2016-08-01 15:01:04]

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

[commit-bot: I haz the power, 2016-08-01 15:01:21]

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, 2016-08-01 15:16:47]

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

[commit-bot: I haz the power, 2016-08-01 15:16:48]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Tobias Sargeant, 2016-08-01 16:58:00]

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

[commit-bot: I haz the power, 2016-08-01 16:58:23]

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, 2016-08-01 17:18:21]

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

[commit-bot: I haz the power, 2016-08-01 17:18:22]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Tobias Sargeant, 2016-08-02 12:49:39]

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

[commit-bot: I haz the power, 2016-08-02 12:49:50]

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, 2016-08-02 12:52:21]

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

[commit-bot: I haz the power, 2016-08-02 12:52:22]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Tobias Sargeant, 2016-08-02 15:21:29]

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

[commit-bot: I haz the power, 2016-08-02 15:21:49]

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, 2016-08-02 15:56:34]

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

[commit-bot: I haz the power, 2016-08-02 15:56:35]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Tobias Sargeant, 2016-08-03 09:27:07]

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

[commit-bot: I haz the power, 2016-08-03 09:27:11]

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, 2016-08-03 09:30:40]

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

[commit-bot: I haz the power, 2016-08-03 09:30:41]

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

[Tobias Sargeant, 2016-08-03 09:47:38]

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

[commit-bot: I haz the power, 2016-08-03 09:47:49]

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, 2016-08-03 10:25:09]

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

[commit-bot: I haz the power, 2016-08-03 10:25:09]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Tobias Sargeant, 2016-08-03 11:04:40]

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

[commit-bot: I haz the power, 2016-08-03 11:04:56]

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, 2016-08-03 12:44:47]

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

[commit-bot: I haz the power, 2016-08-03 12:44:48]

Dry run: This issue passed the CQ dry run.

[Tobias Sargeant, 2016-08-03 13:15:57]

Description was changed from

==========
Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton.

CrashDumpManager watches for generated microdumps when child processes
die, and CrashMicroDumpManager is responsible for aborting the WebView
browser process. Both share similarities that can be combined, and doing
so allows us to eventually support generating a minidump and then dying
in WebView.

BUG=
==========

to

==========
Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton.

CrashDumpManager watches for generated microdumps when child processes
die, and CrashMicroDumpManager is responsible for aborting the WebView
browser process. Both share similarities that can be combined, and doing
so allows us to eventually support generating a minidump and then dying
in WebView.

BUG=633979
==========

[Tobias Sargeant, 2016-08-03 13:15:58]

tobiasjs@chromium.org changed reviewers:
+ torne@chromium.org

[Torne, 2016-08-03 16:33:48]

General approach here looks good. The one thing I wonder about is making it a
singleton instead of just letting the embedder create it as it currently does..
could probably argue either way?

https://codereview.chromium.org/2200693002/diff/120001/content/shell/browser/...
File content/shell/browser/shell_content_browser_client.cc (left):

https://codereview.chromium.org/2200693002/diff/120001/content/shell/browser/...
content/shell/browser/shell_content_browser_client.cc:301: if
(breakpad::IsCrashReporterEnabled()) {
Where did this condition (and the equivalent in the other files)go? I can't spot
it in the new code, maybe I'm missing something?

[Tobias Sargeant, 2016-08-03 16:58:57]

On 2016/08/03 16:33:48, Torne wrote:
> General approach here looks good. The one thing I wonder about is making it a
> singleton instead of just letting the embedder create it as it currently
does..
> could probably argue either way?

The two classes that got refactored differed on this front, so I picked one.
Now the observer is a singleton, but the embedder gets to decide if the observer
does any real work.

Conceivably the embedder could also create the observer, which might be cleaner
because there would be tighter control over the thread on which the create
happens.

>
https://codereview.chromium.org/2200693002/diff/120001/content/shell/browser/...
> File content/shell/browser/shell_content_browser_client.cc (left):
> 
>
https://codereview.chromium.org/2200693002/diff/120001/content/shell/browser/...
> content/shell/browser/shell_content_browser_client.cc:301: if
> (breakpad::IsCrashReporterEnabled()) {
> Where did this condition (and the equivalent in the other files)go? I can't
spot
> it in the new code, maybe I'm missing something?

They disappear because the observer needs to be notified of all child process
starts.

e.g. in aw_content_browser_client.cc there wasn't a check originally, because
we're
registering a different type of client. If that logic can't be avoided (I've
made
something of an assumption that the creation test and this test are morally
equivalent)
then it would have to be pushed down into the client subclass, I guess.
So the control over this becomes whether or not the embedder registers an
observer
client.

[Tobias Sargeant, 2016-08-04 12:57:35]

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

[commit-bot: I haz the power, 2016-08-04 12:57:55]

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, 2016-08-04 13:07:36]

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

[commit-bot: I haz the power, 2016-08-04 13:07:37]

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

[Tobias Sargeant, 2016-08-04 13:08:53]

tobiasjs@chromium.org changed reviewers:
+ alokp@chromium.org, bauerb@chromium.org, mkwst@chromium.org,
rsesek@chromium.org

[Tobias Sargeant, 2016-08-04 13:14:07]

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

[commit-bot: I haz the power, 2016-08-04 13:14:23]

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, 2016-08-04 13:26:04]

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

[commit-bot: I haz the power, 2016-08-04 13:26:05]

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

[Tobias Sargeant, 2016-08-04 13:29:35]

Patchset #9 (id:160001) has been deleted

[Tobias Sargeant, 2016-08-04 13:29:54]

Patchset #8 (id:140001) has been deleted

[Tobias Sargeant, 2016-08-04 13:30:22]

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

[commit-bot: I haz the power, 2016-08-04 13:30:34]

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

[Tobias Sargeant, 2016-08-04 13:32:32]

Patchset #3 (id:40001) has been deleted

[Tobias Sargeant, 2016-08-04 13:33:28]

Patchset #4 (id:80001) has been deleted

[Tobias Sargeant, 2016-08-04 13:33:41]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-08-04 13:44:41]

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

[commit-bot: I haz the power, 2016-08-04 13:44:42]

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

[Tobias Sargeant, 2016-08-04 13:53:30]

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

[commit-bot: I haz the power, 2016-08-04 13:54:07]

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

[Tobias Sargeant, 2016-08-04 13:54:14]

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

[commit-bot: I haz the power, 2016-08-04 13:54:22]

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

[Tobias Sargeant, 2016-08-04 15:12:32]

PTAL. The plan, post refactoring, is to make webview able to generate minidumps
for silent upload.

[commit-bot: I haz the power, 2016-08-04 15:37:49]

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

[commit-bot: I haz the power, 2016-08-04 15:37:50]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-08-04 16:01:32]

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
File android_webview/browser/aw_browser_terminator.cc (right):

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_terminator.cc:23:
child_process_id_to_pipe_.clear();
Is this actually necessary if you're in the destructor anyway?

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_terminator.cc:34: LOG(WARNING) << "tobiasjs:
mapping: " << child_process_id << " to "
Nit: You might not want to immortalize yourself in here that much :)

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_terminator.cc:48:
DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
Is there a particular reason you need the FILE thread for this as opposed to,
say, the blocking pool?

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
File android_webview/browser/aw_browser_terminator.h (right):

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_terminator.h:10: #include
"base/sync_socket.h"
Could you forward-declare this instead of including the header?

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
File components/crash/content/browser/crash_dump_manager_android.cc (right):

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.cc:44: if
(!breakpad::IsCrashReporterEnabled()) {
Nit: no braces for oneline bodies.

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
File components/crash/content/browser/crash_dump_observer_android.cc (right):

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_observer_android.cc:133:
base::Bind(&CrashDumpObserver::OnChildExit, weak_factory_.GetWeakPtr(),
Actually, do you even need the weak pointer? This class is a singleton, so it
will be destroyed only after the message loops have stopped running anyway,
right? (Otherwise you'd need to make sure you destroy this on the FILE thread).

[Robert Sesek, 2016-08-04 16:59:01]

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
File android_webview/browser/aw_browser_terminator.cc (right):

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_terminator.cc:29:
DCHECK(!ContainsKey(child_process_id_to_pipe_, child_process_id));
IWYU: base/stl_util.h

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_terminator.cc:29:
DCHECK(!ContainsKey(child_process_id_to_pipe_, child_process_id));
Should this be just a DCHECK? This code will continue in release mode...

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
File components/crash/content/browser/crash_dump_manager_android.cc (right):

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.cc:174:
CrashDumpManager::ProcessMinidump(minidump_path, pid, process_type,
I think you can remove the "CrashDumpManager::" since ProcessMindiump is no
longer static

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
File components/crash/content/browser/crash_dump_manager_android.h (right):

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.h:1: 
nit: spurious blank line

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.h:30: int
mapping_index);
What is |mapping_index| ? Should be documented.

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.h:32: 
// breakpad::CrashDumpObserver::Client implementation:

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
File components/crash/content/browser/crash_dump_observer_android.cc (right):

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_observer_android.cc:54: client) !=
std::end(registered_clients_))
nit: needs braces since the condition is multi-line

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
File components/crash/content/browser/crash_dump_observer_android.h (right):

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_observer_android.h:31: Client() {}
For pure virtual interfaces, you don't need to provide a ctor, and if you do not
intend to delete through the interface pointer, move the destructor into
protected:

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_observer_android.h:80:
std::list<Client*> registered_clients_;
Could you use base::ObserverListThreadSafe instead? (Note that it notifies by
PostTask rather than calling into the observer directly.

[Tobias Sargeant, 2016-08-05 14:52:51]

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

[commit-bot: I haz the power, 2016-08-05 14:53:07]

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

[Tobias Sargeant, 2016-08-05 14:59:22]

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
File android_webview/browser/aw_browser_terminator.cc (right):

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_terminator.cc:23:
child_process_id_to_pipe_.clear();
On 2016/08/04 16:01:32, Bernhard Bauer wrote:
> Is this actually necessary if you're in the destructor anyway?

Agree, it's not necessary.

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_terminator.cc:29:
DCHECK(!ContainsKey(child_process_id_to_pipe_, child_process_id));
On 2016/08/04 16:59:01, Robert Sesek wrote:
> Should this be just a DCHECK? This code will continue in release mode...

The danger of overwriting one pid->pipe mapping (and thus maybe not terminating
because we expect data on the wrong pipe) is probably not sufficiently high to
justify terminating the process immediately. And then if the two choices are
between overwriting the mapping and not doing so, I think there are cases where
either would be right. I think the DCHECK is sufficient, but I am on the fence
whether to overwrite or return immediately in release mode.

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_terminator.cc:29:
DCHECK(!ContainsKey(child_process_id_to_pipe_, child_process_id));
On 2016/08/04 16:59:01, Robert Sesek wrote:
> IWYU: base/stl_util.h

Done.

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_terminator.cc:34: LOG(WARNING) << "tobiasjs:
mapping: " << child_process_id << " to "
On 2016/08/04 16:01:32, Bernhard Bauer wrote:
> Nit: You might not want to immortalize yourself in here that much :)

Definitely not!

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
File android_webview/browser/aw_browser_terminator.h (right):

https://codereview.chromium.org/2200693002/diff/200001/android_webview/browse...
android_webview/browser/aw_browser_terminator.h:10: #include
"base/sync_socket.h"
On 2016/08/04 16:01:32, Bernhard Bauer wrote:
> Could you forward-declare this instead of including the header?

Done.

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
File components/crash/content/browser/crash_dump_manager_android.cc (right):

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.cc:44: if
(!breakpad::IsCrashReporterEnabled()) {
On 2016/08/04 16:01:32, Bernhard Bauer wrote:
> Nit: no braces for oneline bodies.

Done.

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.cc:174:
CrashDumpManager::ProcessMinidump(minidump_path, pid, process_type,
On 2016/08/04 16:59:01, Robert Sesek wrote:
> I think you can remove the "CrashDumpManager::" since ProcessMindiump is no
> longer static

Done.

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
File components/crash/content/browser/crash_dump_manager_android.h (right):

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.h:1: 
On 2016/08/04 16:59:01, Robert Sesek wrote:
> nit: spurious blank line

Done.

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.h:30: int
mapping_index);
On 2016/08/04 16:59:01, Robert Sesek wrote:
> What is |mapping_index| ? Should be documented.

Done (combination of a better name, and a comment on the associated member).

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.h:32: 
On 2016/08/04 16:59:01, Robert Sesek wrote:
> // breakpad::CrashDumpObserver::Client implementation:

Done.

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
File components/crash/content/browser/crash_dump_observer_android.cc (right):

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_observer_android.cc:54: client) !=
std::end(registered_clients_))
On 2016/08/04 16:59:01, Robert Sesek wrote:
> nit: needs braces since the condition is multi-line

Done.

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_observer_android.cc:133:
base::Bind(&CrashDumpObserver::OnChildExit, weak_factory_.GetWeakPtr(),
On 2016/08/04 16:01:32, Bernhard Bauer wrote:
> Actually, do you even need the weak pointer? This class is a singleton, so it
> will be destroyed only after the message loops have stopped running anyway,
> right? (Otherwise you'd need to make sure you destroy this on the FILE
thread).

I think you're right. And being able to simplify that is a good argument for
keeping it a singleton, I think.

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
File components/crash/content/browser/crash_dump_observer_android.h (right):

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_observer_android.h:31: Client() {}
On 2016/08/04 16:59:01, Robert Sesek wrote:
> For pure virtual interfaces, you don't need to provide a ctor, and if you do
not
> intend to delete through the interface pointer, move the destructor into
> protected:

Done.

https://codereview.chromium.org/2200693002/diff/200001/components/crash/conte...
components/crash/content/browser/crash_dump_observer_android.h:80:
std::list<Client*> registered_clients_;
On 2016/08/04 16:59:01, Robert Sesek wrote:
> Could you use base::ObserverListThreadSafe instead? (Note that it notifies by
> PostTask rather than calling into the observer directly.

I looked at this, and I'm not sure that it's compatible with either using a
thread pool, as Bernhard suggested, or (straightforwardly) compatible with the
need to register on one thread, and receive callbacks on another. I can't use
ObserverList either, because it's not threadsafe.

However, thinking about thread safety more in light of your comment, I realised
I had to make some other changes.

[Bernhard Bauer, 2016-08-05 15:43:39]

LGTM

https://codereview.chromium.org/2200693002/diff/220001/components/crash/conte...
File components/crash/content/browser/crash_dump_manager_android.h (right):

https://codereview.chromium.org/2200693002/diff/220001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.h:68: // The id used
to identify the file descriptor in the set of file
Nit: empty line before comment please.

https://codereview.chromium.org/2200693002/diff/220001/components/crash/conte...
File components/crash/content/browser/crash_dump_observer_android.cc (right):

https://codereview.chromium.org/2200693002/diff/220001/components/crash/conte...
components/crash/content/browser/crash_dump_observer_android.cc:50: LOG(WARNING)
<< "tobiasjs: " << __PRETTY_FUNCTION__ << " not deleted "
Ahem :)

[Robert Sesek, 2016-08-09 16:51:53]

LGTM

[alokp, 2016-08-09 16:54:41]

alokp@chromium.org changed reviewers:
+ sanfin@chromium.org

[alokp, 2016-08-09 16:54:42]

+sanfin to review chromecast since it touches android code

[Simeon, 2016-08-12 21:55:01]

On 2016/08/09 16:54:42, alokp wrote:
> +sanfin to review chromecast since it touches android code

lgtm

[Torne, 2016-08-16 11:59:08]

lgtm

[Tobias Sargeant, 2016-08-17 15:18:30]

On 2016/08/09 16:54:42, alokp wrote:
> +sanfin to review chromecast since it touches android code

Alok, could you please also stamp, because I don't think Simeon is listed as an
owner?

[halliwell, 2016-08-17 15:41:57]

On 2016/08/17 15:18:30, Tobias Sargeant wrote:
> On 2016/08/09 16:54:42, alokp wrote:
> > +sanfin to review chromecast since it touches android code
> 
> Alok, could you please also stamp, because I don't think Simeon is listed as
an
> owner?

chromecast/ lgtm

[Mike West, 2016-08-19 09:30:37]

mkwst@chromium.org changed reviewers:
+ halliwell@chromium.org

[Mike West, 2016-08-19 09:30:38]

LGTM.

[Tobias Sargeant, 2016-08-19 09:32:50]

The CQ bit was checked by tobiasjs@chromium.org

[Tobias Sargeant, 2016-08-19 09:32:51]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
sanfin@chromium.org, rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2200693002/#ps260001
(title: "add missing base::; remove file thread DCHECKs")

[commit-bot: I haz the power, 2016-08-19 09:33:13]

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, 2016-08-19 12:48:26]

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

[commit-bot: I haz the power, 2016-08-19 12:48:28]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Tobias Sargeant, 2016-08-19 13:02:05]

The CQ bit was checked by tobiasjs@chromium.org

[commit-bot: I haz the power, 2016-08-19 13:02:29]

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, 2016-08-19 15:21:13]

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

[commit-bot: I haz the power, 2016-08-19 15:21:15]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Tobias Sargeant, 2016-08-22 10:00:19]

The CQ bit was checked by tobiasjs@chromium.org

[Tobias Sargeant, 2016-08-22 10:00:20]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
torne@chromium.org, sanfin@chromium.org, halliwell@chromium.org,
rsesek@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2200693002/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-22 10:00:29]

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, 2016-08-22 12:13:46]

Description was changed from

==========
Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton.

CrashDumpManager watches for generated microdumps when child processes
die, and CrashMicroDumpManager is responsible for aborting the WebView
browser process. Both share similarities that can be combined, and doing
so allows us to eventually support generating a minidump and then dying
in WebView.

BUG=633979
==========

to

==========
Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton.

CrashDumpManager watches for generated microdumps when child processes
die, and CrashMicroDumpManager is responsible for aborting the WebView
browser process. Both share similarities that can be combined, and doing
so allows us to eventually support generating a minidump and then dying
in WebView.

BUG=633979
==========

[commit-bot: I haz the power, 2016-08-22 12:13:48]

Committed patchset #10 (id:280001)

[commit-bot: I haz the power, 2016-08-22 12:14:57]

Description was changed from

==========
Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton.

CrashDumpManager watches for generated microdumps when child processes
die, and CrashMicroDumpManager is responsible for aborting the WebView
browser process. Both share similarities that can be combined, and doing
so allows us to eventually support generating a minidump and then dying
in WebView.

BUG=633979
==========

to

==========
Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton.

CrashDumpManager watches for generated microdumps when child processes
die, and CrashMicroDumpManager is responsible for aborting the WebView
browser process. Both share similarities that can be combined, and doing
so allows us to eventually support generating a minidump and then dying
in WebView.

BUG=633979

Committed: https://crrev.com/982ab4cee9fef4a7af0321b47d3dbfe5813b7975
Cr-Commit-Position: refs/heads/master@{#413431}
==========

[commit-bot: I haz the power, 2016-08-22 12:14:59]

Patchset 10 (id:??) landed as
https://crrev.com/982ab4cee9fef4a7af0321b47d3dbfe5813b7975
Cr-Commit-Position: refs/heads/master@{#413431}

[boliu, 2016-09-01 22:59:35]

boliu@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2016-09-01 22:59:37]

Fix: https://codereview.chromium.org/2304733002/

https://codereview.chromium.org/2200693002/diff/280001/components/crash/conte...
File components/crash/content/browser/crash_dump_manager_android.cc (left):

https://codereview.chromium.org/2200693002/diff/280001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.cc:53:
BrowserChildProcessObserver::Add(this);
this got dropped

https://codereview.chromium.org/2200693002/diff/280001/components/crash/conte...
File components/crash/content/browser/crash_dump_manager_android.cc (right):

https://codereview.chromium.org/2200693002/diff/280001/components/crash/conte...
components/crash/content/browser/crash_dump_manager_android.cc:42: //if
(!breakpad::IsCrashReporterEnabled())
commented out code?

[boliu, 2016-09-02 01:21:16]

So during code review for https://codereview.chromium.org/2304733002/, we found
some other issues, not major, but not very idiomatic. I'm gonna revert on trunk
and branch.

[Tobias Sargeant, 2016-09-02 15:42:00]

Description was changed from

==========
Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton.

CrashDumpManager watches for generated microdumps when child processes
die, and CrashMicroDumpManager is responsible for aborting the WebView
browser process. Both share similarities that can be combined, and doing
so allows us to eventually support generating a minidump and then dying
in WebView.

BUG=633979

Committed: https://crrev.com/982ab4cee9fef4a7af0321b47d3dbfe5813b7975
Cr-Commit-Position: refs/heads/master@{#413431}
==========

to

==========
Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton.

CrashDumpManager watches for generated microdumps when child processes
die, and CrashMicroDumpManager is responsible for aborting the WebView
browser process. Both share similarities that can be combined, and doing
so allows us to eventually support generating a minidump and then dying
in WebView.

BUG=633979

Committed: https://crrev.com/982ab4cee9fef4a7af0321b47d3dbfe5813b7975
Cr-Commit-Position: refs/heads/master@{#413431}
==========

[Tobias Sargeant, 2016-09-16 12:40:32]

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

[commit-bot: I haz the power, 2016-09-16 12:40:45]

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, 2016-09-16 12:55:42]

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

[commit-bot: I haz the power, 2016-09-16 12:55:43]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Tobias Sargeant, 2016-09-16 13:45:47]

Patchset #11 (id:300001) has been deleted

[Tobias Sargeant, 2016-09-16 13:45:59]

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

[commit-bot: I haz the power, 2016-09-16 13:46:22]

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, 2016-09-16 14:59:42]

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

[commit-bot: I haz the power, 2016-09-16 14:59:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Tobias Sargeant, 2016-09-20 15:23:55]

tobiasjs@chromium.org changed reviewers:
+ thestig@chromium.org