Enable Crashpad for Android [chromium/src : master]
Joshua Peraza (Gerrit)
Hi Richard, Tobias.
This CL is getting very near ready for review. The only remaining CQ failures are for AwDebugTest.testDumpDoesNotContainNonWhitelistedKey*.
I'm wondering what the purpose of the crash keys white list was and whether we still need it.
Thanks!
7 comments:
File android_webview/browser/aw_browser_terminator.cc:
I removed this in one of my earlier passes and intended to come back around and check with someone f […]
AwBrowserTerminator is back and now observes crash signals from child processes.
With Crashpad, when a child process crashes it sends a message to a CrashHandlerHost running in the browser. The CrashHandlerHost then sends the signal out to any observers before launching Crashpad's handler to dump the child.
If AwBrowserTerminator observes that a child has exited abnormally without having received a crash signal for the child, it is assumed to have been killed.
File android_webview/common/crash_reporter/aw_crash_reporter_client.cc:
std::string* version,
std::string* channel) {
*product_name = "AndroidWebView";
*version = PRODUCT_VERSION;
*channel = version_info::GetChannelString(version_info::GetChannel());
}Note that since WebView forwards the minidumps to a central service on the device, we want to ensure […]
I don't anticipate needing to modify anything further in WebView's uploader, so this should continue to behave as before. I'm returning false here so that Crashpad doesn't accidentally start uploading reports without consent sometime in the future.
File android_webview/lib/aw_main_delegate.cc:
We want to be able to tell the difference on the backend, though..
It turns out breakpad never did this either. process_type was used to check whether it should initialize itself for the browser process or not, but then it would just hardcode the ptype to "browser". I also checked and it looks like we have no 'webview' reports with minidumps. I think this means some portion of your reports marked as 'browser' are actually single process webview.
I think it'd probably be better to fix this post-transition.
File chrome/browser/metrics/oom/out_of_memory_reporter.cc:
OOM on Android usually means "the platform sent SIGKILL to the process", not "a memory allocation fa […]
Thanks! That was super helpful information.
File chromecast/app/android/crash_handler.cc:
Should this be Crashpad or Crash Reports?
File chromecast/browser/cast_browser_main_parts.cc:
should this be breakpad?
Done
File content/shell/android/BUILD.gn:
Patch Set #15, Line 174:
ared_libraries = [ ":libcontent_shell_content_view" ]FIXME
Done
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza uploaded patch set #49 to this change.
Enable Crashpad for Android
Overview:
This CL disables Breakpad for Chrome, Content Shell, WebView, and
Chromecast on Android and replaces it with Crashpad. When a crash
signal is received, the browser forks+execs a Crashpad handler process
either for itself or on behalf of a crashing child to create a crash
dump. The transition to Crashpad requires several attached changes
to be submit with it to cope with changes in how child process crashes
and minidumps are written.
build/android/gyp/apkbuilder.py
chrome/android/chrome_public_apk_tmpl.gni
chrome/android/static_initializers.gni
and various BUILD.gn files
- The crashpad handler is a standalone executable, packaged like a
loadable module and named libcrashpad_handler.so in order to not
be ignored by Android's APK installer.
- components/minidump_uploader now has some native code needing to
be linked into various .sos.
components/crash/
- Remove CrashDumpManager. Minidump creation is handled entirely by
Crashpad. Metrics recorded by CrashDumpManager have been moved to
ChildProcessCrashObserver.
- Update CrashDumpObserver to not take ownership of its observers,
allowing more flexibility for observer implementations.
- Remove OnChildStart from CrashDumpObserver as it is no longer
necessary.
components/minidump_uploader/
- Add mime_write_crash_reports.cc to MIME encode minidumps.
Uploaders expect crash reports to already be MIME encoded since
Breakpad was doing that in a signal handler call-back.
CrashFileManager now automatically calls into native code to do
the encoding and write to a directory of crash reports whenever it
checks for reports without logcats.
chrome/app/*.{cc,h}
chrome/browser/*.cc
content/shell/app/*.{cc,h}
content/shell/browser/*.cc
- Initialize Crashpad instead of Breakpad, with minor cleanup and
adjustment for changes to CrashDumpObserver.
android_webview/
- Update AwBrowserTerminator to observe child process crashes via
CrashHandlerHost instead of a kAndroidWebViewCrashSignalDescriptor.
CrashHandlerHost synchronously notifies observers when a child
process receives a crash signal. AwBrowserTerminator can
differentiate between child process crashes and kills be observing
whether the child received a crashing signal before it exited.
- Move aw_microdump_crash_reporter.{cc,h} to
aw_crash_reporter_client.{cc,h} with minor cleanup and tweaks to
initialize Crashpad.
chromecast/
- There are now two directories that crash report uploaders should
be aware of: "Crashpad" contains a database of raw minidumps
produced by Crashpad, and "Crash Reports" contains MIME encoded
minidumps. MIME encoding is performed by a CrashReportMimeWriter
in CastCrashUploader.java:checkForCrashDumps().
chrome/browser/page_load_metrics/ and
components/data_reduction_proxy/
- Make DataReductionProxyPingbackClientImpl observe CrashHandlerHost
instead of CrashDumpManager. Foreground OOMs are differentiated
from OTHER_CRASHes by observing whether the child received a crash
signal. Child process IDs are used to identify crashers instead
of render process host IDs to avoid looking up the render process
host ID from a process ID.
chrome/browser/metrics/chrome_stability_metrics_provider.{cc,h}
- Observe CrashHandlerHost instead of CrashDumpManager to detect
renderer crashes.
- A scoped_refptr to the SequencedTaskRunner that created the
metrics provider is kept because helper_ must be accessed on this
sequence. CrashDumpManager also implicitly did this via an
ObserverListThreadSafe.
chrome/browser/metrics/oom/
- Encapsulate observing render process OOM kills in a
RenderProcessKilledObserver. This class deduces that a renderer
was killed if it exited without having received a crash signal.
Bug: crashpad:30
Cq-Include-Trybots: master.tryserver.chromium.android:android_compile_x64_dbg;master.tryserver.chromium.android:android_compile_x86_dbg
Change-Id: I0efa451585f60287853c47f860f09ced581c8958
---
M android_webview/BUILD.gn
M android_webview/browser/DEPS
M android_webview/browser/aw_browser_main_parts.cc
M android_webview/browser/aw_browser_terminator.cc
M android_webview/browser/aw_browser_terminator.h
M android_webview/browser/aw_content_browser_client.cc
M android_webview/browser/aw_debug.cc
M android_webview/common/aw_paths.cc
A android_webview/common/crash_reporter/aw_crash_reporter_client.cc
A android_webview/common/crash_reporter/aw_crash_reporter_client.h
D android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc
D android_webview/common/crash_reporter/aw_microdump_crash_reporter.h
M android_webview/common/crash_reporter/crash_keys.cc
M android_webview/java/DEPS
M android_webview/java/src/org/chromium/android_webview/AwDebug.java
M android_webview/javatests/src/org/chromium/android_webview/test/AwDebugTest.java
M android_webview/lib/aw_main_delegate.cc
M android_webview/test/BUILD.gn
M build/android/gyp/apkbuilder.py
M chrome/BUILD.gn
M chrome/android/BUILD.gn
M chrome/android/chrome_public_apk_tmpl.gni
M chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
M chrome/android/static_initializers.gni
M chrome/app/chrome_crash_reporter_client.cc
M chrome/app/chrome_crash_reporter_client.h
M chrome/app/chrome_main_delegate.cc
M chrome/browser/chrome_browser_main_android.cc
M chrome/browser/chrome_content_browser_client.cc
M chrome/browser/metrics/chrome_stability_metrics_provider.cc
M chrome/browser/metrics/chrome_stability_metrics_provider.h
M chrome/browser/metrics/oom/out_of_memory_reporter.cc
M chrome/browser/metrics/oom/out_of_memory_reporter.h
M chrome/browser/metrics/oom/out_of_memory_reporter_unittest.cc
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
M chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
M chrome/common/chrome_paths.cc
M chromecast/android/BUILD.gn
M chromecast/app/BUILD.gn
M chromecast/app/android/DEPS
M chromecast/app/android/cast_crash_reporter_client_android.cc
M chromecast/app/android/cast_crash_reporter_client_android.h
M chromecast/app/android/crash_handler.cc
M chromecast/app/android/crash_handler.h
M chromecast/app/cast_main_delegate.cc
M chromecast/browser/android/BUILD.gn
M chromecast/browser/android/DEPS
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashHandler.java
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploaderFactory.java
M chromecast/browser/cast_browser_main_parts.cc
M chromecast/browser/cast_content_browser_client.cc
M chromecast/browser/cast_content_browser_client.h
M components/crash/android/BUILD.gn
A components/crash/android/java/src/org/chromium/components/crash/browser/ChildProcessCrashObserver.java
M components/crash/content/app/BUILD.gn
M components/crash/content/app/crash_reporter_client.cc
M components/crash/content/app/crash_reporter_client.h
M components/crash/content/app/crashpad_linux.cc
M components/crash/content/browser/BUILD.gn
M components/crash/content/browser/child_process_crash_observer_android.cc
M components/crash/content/browser/child_process_crash_observer_android.h
M components/crash/content/browser/crash_dump_observer_android.cc
M components/crash/content/browser/crash_dump_observer_android.h
M components/crash/content/browser/crash_handler_host_linux.cc
M components/crash/content/browser/crash_handler_host_linux.h
M components/crash/core/common/BUILD.gn
M components/data_reduction_proxy/content/browser/data_reduction_proxy_pingback_client_impl.cc
M components/data_reduction_proxy/content/browser/data_reduction_proxy_pingback_client_impl.h
M components/data_reduction_proxy/content/browser/data_reduction_proxy_pingback_client_impl_unittest.cc
M components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc
M components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h
M components/minidump_uploader/BUILD.gn
M components/minidump_uploader/DEPS
M components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java
A components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashReportMimeWriter.java
A components/minidump_uploader/mime_write_crash_reports.cc
M content/shell/android/BUILD.gn
M content/shell/app/shell_crash_reporter_client.cc
M content/shell/app/shell_crash_reporter_client.h
M content/shell/app/shell_main_delegate.cc
M content/shell/browser/shell_browser_main_parts.cc
M content/shell/browser/shell_content_browser_client.cc
84 files changed, 1,132 insertions(+), 1,120 deletions(-)
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Jochen Eisinger (Gerrit)
can you please also update https://chromium.googlesource.com/chromium/src/+/master/docs/testing/using_breakpad_with_content_shell.md?
And if you could add Android support to https://cs.chromium.org/chromium/src/content/shell/tools/breakpad_integration_test.py that'd be super extra great :)
Tobias Sargeant (Gerrit)
The purpose of the whitelist is to avoid WebView uploading information that is not covered by the Usage and Diagnostics checkbox, that is covered by Chrome's crash uploading consent (for example, we can't log URLs, but I think there are other examples). It really needs to stay there.
Patch Set 48:
(7 comments)
Hi Richard, Tobias.
This CL is getting very near ready for review. The only remaining CQ failures are for AwDebugTest.testDumpDoesNotContainNonWhitelistedKey*.
I'm wondering what the purpose of the crash keys white list was and whether we still need it.
Thanks!
Tobias Sargeant (Gerrit)
1 comment:
File android_webview/browser/aw_browser_terminator.cc:
Patch Set #50, Line 71:
crash_reporterIf this isn't done, then the microdump generated as a result of LOG(FATAL) will be the one that's captured by crash. That's uninteresting to us (it's the renderer crash before it that we care about). So not doing this will obscure all manually reported renderer crashes in multiprocess. It's possible that we can fix this in back-end processing, but it's extra work and there's also the possibility that the earlier microdump is truncated because of the limited logcat space.
(crashpad is still going to generate microdumps, isn't it?)
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza uploaded patch set #58 to this change.
Enable Crashpad for Android
Overview:
This CL disables Breakpad for Chrome, Content Shell, WebView, and
Chromecast on Android and replaces it with Crashpad. When a crash
signal is received, the browser forks+execs a Crashpad handler process
either for itself or on behalf of a crashing child to create a crash
dump.
build/android/gyp/apkbuilder.py
chrome/android/chrome_public_apk_tmpl.gni
chrome/android/static_initializers.gni
and various BUILD.gn files
- The crashpad handler is a standalone executable, packaged like a
loadable module and named libcrashpad_handler.so in order to not
be ignored by Android's APK installer.
- components/minidump_uploader now has some native code needing to
be linked into various .sos.
components/crash/
- Remove CrashDumpManager. Minidump creation is handled entirely by
Crashpad.
M chrome/browser/metrics/oom/out_of_memory_reporter_unittest.cc
D components/crash/content/browser/crash_dump_manager_android.cc
D components/crash/content/browser/crash_dump_manager_android.h
D components/crash/content/browser/crash_dump_manager_android_unittest.cc
M components/crash/content/browser/crash_dump_observer_android.cc
M components/crash/content/browser/crash_dump_observer_android.h
M components/crash/content/browser/crash_handler_host_linux.cc
M components/crash/content/browser/crash_handler_host_linux.h
M components/crash/content/browser/crash_metrics_reporter_android.cc
M components/crash/content/browser/crash_metrics_reporter_android.h
M components/crash/content/browser/crash_metrics_reporter_android_unittest.cc
M components/crash/core/common/BUILD.gn
M components/data_reduction_proxy/content/browser/data_reduction_proxy_pingback_client_impl_unittest.cc
M components/minidump_uploader/BUILD.gn
M components/minidump_uploader/DEPS
M components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java
A components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashReportMimeWriter.java
A components/minidump_uploader/mime_write_crash_reports.cc
M content/shell/android/BUILD.gn
M content/shell/app/shell_crash_reporter_client.cc
M content/shell/app/shell_crash_reporter_client.h
M content/shell/app/shell_main_delegate.cc
M content/shell/browser/shell_browser_main_parts.cc
M content/shell/browser/shell_content_browser_client.cc
M content/shell/tools/breakpad_integration_test.py
M docs/README.md
R docs/testing/using_crashpad_with_content_shell.md
82 files changed, 1,072 insertions(+), 1,431 deletions(-)
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza would like Robert Sesek, Luke Halliwell and Gustav Sennton to review this change.
M base/android/build_info.cc
M base/android/build_info.h
M base/android/java_exception_reporter.cc
M base/android/java_exception_reporter.h
M base/android/jni_android.cc
85 files changed, 1,096 insertions(+), 1,470 deletions(-)
Joshua Peraza (Gerrit)
Tobias: This is now updated with support for sanitization and crash-key whitelisting.
Jochen: I've updated breakpad_integration_test.py with Android support and documentation on testing with Crashpad.
Also adding:
halliwell for chromecast/
rsesek for components/crash/
gsennton for components/minidump_uploader/
Thanks!
1 comment:
File android_webview/browser/aw_browser_terminator.cc:
Patch Set #50, Line 71:
Keeps this logIf this isn't done, then the microdump generated as a result of LOG(FATAL) will be the one that's ca […]
As discussed offline, I think it'd be okay to go ahead without microdumps.
Then, without crash dump suppression, we'd be getting extra minidumps for this intentional crash, but that seems okay to me since they should be easy to ignore. We're generally reluctant to introduce ways to disable Crashpad unless they're necessary. I think it'd be especially prudent to not suppress minidump generation here while Crashpad is new, in case it encounters problems tracing the crashing renderer and is unable to produce a dump.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Luke Halliwell (Gerrit)
sanfin - can you review chromecast android changes?
Robert Sesek (Gerrit)
Very exciting!! 🎉
What's going to happen to the crash client ID? On Mac/Win, we reset it because Crashpad's database controls it. Does https://cs.chromium.org/chromium/src/components/crash/core/common/crash_keys.cc?l=26&rcl=5b9084f802a4976d61d9cfcce611f6b76faa251c need to be updated?
5 comments:
File android_webview/browser/aw_browser_terminator.h:
Patch Set #61, Line 34:
std::map<base::ProcessId, int> child_pid_to_crash_signal_;This seems duplicative of ChildProcessCrashObserver. Can this use that?
File android_webview/common/crash_reporter/aw_crash_reporter_client.h:
Comments for the top-level declarations, please.
File chrome/android/static_initializers.gni:
Patch Set #61, Line 16:
expected_static_initializer_count = 7What are the new static initializers?
File components/crash/content/browser/child_process_crash_observer_android.h:
Patch Set #61, Line 17:
class ChildProcessCrashObserverClass-level comment here would be helpful. How/why is this distinct from CrashDumpObserver?
Patch Set #61, Line 33:
std::map<base::ProcessId, int> child_pid_to_crash_signal_;What's the thread safety of this? There don't appear to be guarantees about the thread on which the observer methods run, so can this be mutated from multiple threads?
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Thanks!
I've also updated crash_keys.cc to pass on the metrics client id as was done with mac and windows.
Patch set 63:Commit-Queue +1
5 comments:
This seems duplicative of ChildProcessCrashObserver. […]
I've moved this to CrashDumpObserver and include the crash signal in TerminationInfo so that it can be used by both AwBrowserTerminator and ChildProcessCrashObserver.
File android_webview/common/crash_reporter/aw_crash_reporter_client.h:
Comments for the top-level declarations, please.
Patch Set #61, Line 16:
expected_static_initializer_count = 7What are the new static initializers?
There are no real, additional static initializers. The increased count comes from crashpad's handler executable's .ctors section which begins with 0xffffffff and terminates with 0x00000000.
File components/crash/content/browser/child_process_crash_observer_android.h:
Patch Set #61, Line 17:
class ChildProcessCrashObserver
Class-level comment here would be helpful. […]
Done, and I've also renamed CrashDumpObserver to ChildExitObserver to further distinguish the two classes.
What's the thread safety of this? There don't appear to be guarantees about the thread on which the […]
Now protected with crash_signals_lock_ over in ChildExitObserver.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza uploaded patch set #65 to this change.
Enable Crashpad for Android
Overview:
This CL disables Breakpad for Chrome, Content Shell, WebView, and
Chromecast on Android and replaces it with Crashpad. When a crash
signal is received, the browser forks+execs a Crashpad handler process
either for itself or on behalf of a crashing child to create a crash
dump.
build/android/gyp/apkbuilder.py
chrome/android/chrome_public_apk_tmpl.gni
chrome/android/static_initializers.gni
and various BUILD.gn files
- The crashpad handler is a standalone executable, packaged like a
loadable module and named libcrashpad_handler.so in order to not
be ignored by Android's APK installer.
- components/minidump_uploader now has some native code needing to
be linked into various .sos.
components/crash/
- Remove CrashDumpManager. Minidump creation is handled entirely by
Crashpad.
- Rename CrashDumpObserver to ChildExitObserver and update it to not
take ownership of its observers, allowing more flexibility for
observer implementations.
- Remove OnChildStart from ChildExitObserver as it is no longer
necessary.
- ChildExitObserver observers CrashHandlerHost to be notified when
child processes receive crash signals.
components/minidump_uploader/
- Add mime_write_crash_reports.cc to MIME encode minidumps.
Uploaders expect crash reports to already be MIME encoded since
Breakpad was doing that in a signal handler call-back.
CrashFileManager now automatically calls into native code to do
the encoding and write to a directory of crash reports whenever it
checks for reports without logcats.
chrome/app/*.{cc,h}
chrome/browser/*.cc
content/shell/app/*.{cc,h}
content/shell/browser/*.cc
- Initialize Crashpad instead of Breakpad, with minor cleanup and
adjustment for changes to CrashDumpObserver.
android_webview/
- AwBrowserTerminator now observes child process crashes via
ChildExitObserver.
M components/crash/content/app/crash_reporter_client.cc
M components/crash/content/app/crash_reporter_client.h
M components/crash/content/app/crashpad_linux.cc
M components/crash/content/browser/BUILD.gn
R components/crash/content/browser/child_exit_observer_android.cc
R components/crash/content/browser/child_exit_observer_android.h
M components/crash/content/browser/child_process_crash_observer_android.cc
M components/crash/content/browser/child_process_crash_observer_android.h
D components/crash/content/browser/crash_dump_manager_android.cc
D components/crash/content/browser/crash_dump_manager_android.h
D components/crash/content/browser/crash_dump_manager_android_unittest.cc
M components/crash/content/browser/crash_handler_host_linux.cc
M components/crash/content/browser/crash_handler_host_linux.h
M components/crash/content/browser/crash_metrics_reporter_android.cc
M components/crash/content/browser/crash_metrics_reporter_android.h
M components/crash/content/browser/crash_metrics_reporter_android_unittest.cc
M components/crash/core/common/BUILD.gn
M components/crash/core/common/crash_keys.cc
M components/data_reduction_proxy/content/browser/data_reduction_proxy_pingback_client_impl_unittest.cc
M components/minidump_uploader/BUILD.gn
M components/minidump_uploader/DEPS
M components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java
A components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashReportMimeWriter.java
A components/minidump_uploader/mime_write_crash_reports.cc
M content/shell/android/BUILD.gn
M content/shell/app/shell_crash_reporter_client.cc
M content/shell/app/shell_crash_reporter_client.h
M content/shell/app/shell_main_delegate.cc
M content/shell/browser/shell_browser_main_parts.cc
M content/shell/browser/shell_content_browser_client.cc
M content/shell/tools/breakpad_integration_test.py
M docs/README.md
R docs/testing/using_crashpad_with_content_shell.md
88 files changed, 1,142 insertions(+), 1,524 deletions(-)
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Gustav Sennton (Gerrit)
Apologies, I won't have time to review this today, and am OOO all of next week.
The only comment I have right now is (probably the most obvious one): is there anyway we could split this CL into smaller ones?
Joshua Peraza (Gerrit)
Patch Set 66:
Apologies, I won't have time to review this today, and am OOO all of next week.
The only comment I have right now is (probably the most obvious one): is there anyway we could split this CL into smaller ones?
There are a few small pieces that can be shaved off into another CL, but I think the vast majority needs to be submit together. The root changes are things under components/crash/. The changes to android_webview/, chrome/, chromecast/, and content/shell/ can be reviewed independently of each other (and each contains similar changes), but need to be submit together with the changes to components/crash/ (with components/minidump_uploader/ being some glue between them).
Patch set 67:Commit-Queue +1
Robert Sesek (Gerrit)
2 comments:
File components/crash/content/browser/child_exit_observer_android.h:
Patch Set #67, Line 143:
std::map<int, base::ProcessHandle> process_host_id_to_pid_;Are there threading restrictions on this map and browser_child_process_info_ ?
File components/crash/content/browser/child_exit_observer_android.h:
Maybe add a:
bool is_crashed() const { return crash_signo != kInvalidSigno; }
... since that seems to be a really common test, which would improve readability a bit.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Sherman (Gerrit)
I'm not sure what code you wanted me to review. It's honestly been a long while since I've worked on this code at all, so it's probably a good idea to have an additional primary reviewer for any code where you need my OWNERS approval.
I also strongly agree with Gustav that this should be split up into several CLs. The crash uploading code is both very important, and somewhat fragile. I'm hopeful that perhaps you or others on the Crashpad team can take ownership of it and bring it into much better shape. However, given its current state, I think it's very likely that we'll miss something in such a large single CL. I am quite confident that you should be able to break up this work into smaller logical pieces, perhaps guarding functionality behind a flag while you land the individual pieces. (OTOH, if your primary reviewer(s) prefer the large one-shot CL, I can do my best to adapt.)
10 comments:
File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:
File[] minidumpsSansLogcat = crashFileManager.getMinidumpsSansLogcat();
if (minidumpsSansLogcat.length > 0) {
AsyncTask.THREAD_POOL_EXECUTOR.execute(
new LogcatExtractionRunnable(minidumpsSansLogcat[0]));
}It seems that this code attempts to upload only the first minidump in the list. Let's document why that is.
Let's say more about the inputs and outputs here. What input state does this class expect, and what is its output? That is, where does it fit into the overall puzzle of uploading crash reports?
Patch Set #67, Line 19:
public void tryToUploadMinidump();nit: Please place this on a separate line (unless the Java style guide says not to), and document it.
/**
* Registers a callback for uploading minidumps. May be called at most once.
*
* @param callback The callback to trigger when a new minidump is generated by a child process.
*/
public static void registerUploadCallback(UploadMinidumpCallback callback) {
assert sCallback == null;
sCallback = callback;
}It looks like this class is replacing the CrashDumpManager.java class. However, (1) I don't see CrashDumpManager.java removed in this CL, and (2) it looks like this function drops some checks from that code – notably the thread assertions. Are the thread assertions no longer relevant? If not, why not?
/**
* Notifies any registered observer that a child process has exited due to an apparent crash.
*/This comment seems like it came from somewhere else, and should be updated – or am I somehow completely misreading it?
Patch Set #67, Line 33:
BreakpadThis comment should be updated. Probably, a lot of the comments in this class documentation will also need to be revised somewhat.
File crashpadDir = getCrashpadDirectory();
if (crashpadDir.exists() && ensureCrashDirExists()) {
File crashDir = getCrashDirectory();
CrashReportMimeWriter.mimeWriteCrashReports(crashpadDir, crashDir);
}It looks like this method now has a side-effect of copying over files from the Crashpad directory? That feels somewhat out of place – it seems like that copy should happen elsewhere. But, if it can't happen elsewhere, we should at least update the documentation to explicitly mention this side-effect.
/**
*/Looks like there is a missing comment here?
Patch Set #67, Line 17:
void mimeWriteCrashReportsnit: I don't understand what it means to "mime write" something, which is how I parse this method name. Assuming my guess at the meaning is correct: Perhaps this could be named something like "copyCrashReportsWithMimeInfo"? Not necessarily such a long name – something shorter but equally clear would certainly be preferable!
Patch Set #67, Line 17:
mimeWriteCrashReportsPlease add documentation.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Patch Set 67:
(10 comments)
I'm not sure what code you wanted me to review. It's honestly been a long while since I've worked on this code at all, so it's probably a good idea to have an additional primary reviewer for any code where you need my OWNERS approval.
I also strongly agree with Gustav that this should be split up into several CLs. The crash uploading code is both very important, and somewhat fragile. I'm hopeful that perhaps you or others on the Crashpad team can take ownership of it and bring it into much better shape. However, given its current state, I think it's very likely that we'll miss something in such a large single CL. I am quite confident that you should be able to break up this work into smaller logical pieces, perhaps guarding functionality behind a flag while you land the individual pieces. (OTOH, if your primary reviewer(s) prefer the large one-shot CL, I can do my best to adapt.)
Thanks for the comments! I had added you for components/minidump_uploader.
I'll see what I can do about breaking this up.
11 comments:
File minidump = crashFileManager.getMinidumpSansLogcatForPid(pid);
if (minidump != null) {
AsyncTask.THREAD_POOL_EXECUTOR.execute(
It seems that this code attempts to upload only the first minidump in the list. […]
I was doing this to be similar to the previous behavior, but now I realize it's racy if multiple child process' are crashing simultaneously. I've updated ChildProcessCrashObserver to call this on a sequence (I didn't see any way to use sequences from Java), as well as pass the pid of the crashing child so this code can identify which dump it's supposed to be getting a logcat for. I was a little hesitant about doing that since it could break with pid namespacing (or other possible errors in writing the expected pid to the filename), but AFAIK that doesn't happen on Android. If for some reason this does break, these dumps will still get uploaded on startup here:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java?rcl=cf6b7ebf9197301063debaf7dbb4d57422057049&l=488I also considered having this continue to just attach a logcat to the most recent dump, adding a new state to CrashFileManager to protect the dump from being processed by other calls to this method, but that seemed more complicated and could end up attaching logcats to the wrong dumps.
Let's say more about the inputs and outputs here. […]
Done
Patch Set #67, Line 19:
ed to handle the newly created dumnit: Please place this on a separate line (unless the Java style guide says not to), and document it […]
Clang format does this.
*/
public interface ChildCrashedCallback { public void childCrashed(int pid); }
/**
* The globally registered callback for responding to child process crashes, or null if no
* callback has been registered yet.
*/
private static ChildCrashedCallback sCallback;It looks like this class is replacing the CrashDumpManager.java class. […]
The thread assertions in tryToUploadMinidump() are removed because the method no longer touches the file system. I suspect the thread assertion in registerUploadCallback() was to ensure there are no races setting sCallback, so I've added that back in.
CrashDumpManger.java is removed.
* Registers a callback for responding to child process crashes. May be called at most once, and
* only on the UI thread.
*This comment seems like it came from somewhere else, and should be updated – or am I somehow complet […]
I've updated the rest of the class to hopefully be more consistent with this comment. The class notifies an observer of child process crashes, and uploading minidumps is just what that callback happens to do.
File components/crash/content/browser/child_exit_observer_android.h:
Are there threading restrictions on this map and browser_child_process_info_ ?
Added comments noting that both are only accessed on the UI thread.
File components/crash/content/browser/child_exit_observer_android.h:
Patch Set #66, Line 46:
bool is_crashed() const { return crash_signo != kInvalidSigno; }Maybe add a: […]
Done
File crashDir = getCrashDirectory();
CrashReportMimeWriter.rewriteMinidumpsAsMIMEs(crashpadDir, crashDir);
}
}It looks like this method now has a side-effect of copying over files from the Crashpad directory? T […]
I agree it seems an odd place, but I don't think there's any better place without much larger changes to the uploading code since it expects the files to already be encoded as MIME messages. Breakpad was rewriting the minidumps as MIME files in its signal handler, which is not a place we want to be doing that. Documentation updated.
/**
*
Looks like there is a missing comment here?
Done
Please add documentation.
Done
nit: I don't understand what it means to "mime write" something, which is how I parse this method na […]
Done
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza uploaded patch set #71 to this change.
Enable Crashpad for Android
Overview:
This CL disables Breakpad for Chrome, Content Shell, WebView, and
Chromecast on Android and replaces it with Crashpad. When a crash
signal is received, the browser forks+execs a Crashpad handler process
either for itself or on behalf of a crashing child to create a crash
dump.
build/android/gyp/apkbuilder.py
chrome/android/chrome_public_apk_tmpl.gni
chrome/android/static_initializers.gni
and various BUILD.gn files
- The crashpad handler is a standalone executable, packaged like a
loadable module and named libcrashpad_handler.so in order to not
be ignored by Android's APK installer.
- components/minidump_uploader now has some native code needing to
be linked into various .sos.
components/crash/
- Remove CrashDumpManager. Minidump creation is handled entirely by
Crashpad.
- Remove OnChildStart from ChildExitObserver as it is no longer
necessary.
- ChildExitObserver observers CrashHandlerHost to be notified when
child processes receive crash signals.
components/minidump_uploader/
- Uploaders expect crash reports to already be MIME encoded since
Breakpad was doing that in a signal handler call-back.
CrashFileManager now automatically calls into native code to do
the encoding and write to a directory of crash reports whenever it
checks for reports without logcats.
chrome/app/*.{cc,h}
chrome/browser/*.cc
content/shell/app/*.{cc,h}
content/shell/browser/*.cc
- Initialize Crashpad instead of Breakpad, with minor cleanup and
adjustment for changes to CrashDumpObserver.
android_webview/
- AwBrowserTerminator now observes child process crashes via
ChildExitObserver rather than its own pipe.
chromecast/
- There are now two directories that crash report uploaders should
be aware of: "Crashpad" contains a database of raw minidumps
produced by Crashpad, and "Crash Reports" contains MIME encoded
minidumps. MIME encoding is performed by a CrashReportMimeWriter
in CastCrashUploader.java:checkForCrashDumps().
Bug: crashpad:30
Cq-Include-Trybots: master.tryserver.chromium.android:android_compile_x64_dbg;master.tryserver.chromium.android:android_compile_x86_dbg
Change-Id: I0efa451585f60287853c47f860f09ced581c8958
---
M android_webview/BUILD.gn
M android_webview/browser/DEPS
M android_webview/browser/aw_browser_main_parts.cc
M android_webview/browser/aw_browser_terminator.cc
M android_webview/browser/aw_browser_terminator.h
M android_webview/browser/aw_content_browser_client.cc
M android_webview/browser/aw_debug.cc
M android_webview/common/aw_paths.cc
M android_webview/common/crash_reporter/crash_keys.cc
M android_webview/java/DEPS
M android_webview/java/src/org/chromium/android_webview/AwDebug.java
M android_webview/javatests/src/org/chromium/android_webview/test/AwDebugTest.java
M android_webview/lib/aw_main_delegate.cc
M android_webview/test/BUILD.gn
M build/android/gyp/apkbuilder.py
D components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
M components/crash/content/app/BUILD.gn
M components/crash/content/app/crash_reporter_client.cc
M components/crash/content/app/crash_reporter_client.h
M components/crash/content/browser/BUILD.gn
R components/crash/content/browser/child_exit_observer_android.cc
R components/crash/content/browser/child_exit_observer_android.h
M components/crash/content/browser/child_process_crash_observer_android.cc
M components/crash/content/browser/child_process_crash_observer_android.h
D components/crash/content/browser/crash_dump_manager_android.cc
D components/crash/content/browser/crash_dump_manager_android.h
D components/crash/content/browser/crash_dump_manager_android_unittest.cc
M components/crash/content/browser/crash_handler_host_linux.cc
M components/crash/content/browser/crash_handler_host_linux.h
M components/crash/content/browser/crash_metrics_reporter_android.cc
M components/crash/content/browser/crash_metrics_reporter_android.h
M components/crash/content/browser/crash_metrics_reporter_android_unittest.cc
M components/crash/core/common/BUILD.gn
M components/crash/core/common/crash_keys.cc
M components/data_reduction_proxy/content/browser/data_reduction_proxy_pingback_client_impl_unittest.cc
M components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java
M content/shell/android/BUILD.gn
M content/shell/app/shell_crash_reporter_client.cc
M content/shell/app/shell_crash_reporter_client.h
M content/shell/app/shell_main_delegate.cc
M content/shell/browser/shell_browser_main_parts.cc
M content/shell/browser/shell_content_browser_client.cc
71 files changed, 607 insertions(+), 1,261 deletions(-)
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza removed Gustav Sennton from this change.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza removed Simeon Anfinrud from this change.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza uploaded patch set #83 to this change.
Enable Crashpad for Android
Overview:
This CL disables Breakpad for Chrome, Content Shell, WebView, and
Chromecast on Android and replaces it with Crashpad. When a crash
signal is received, the browser forks+execs a Crashpad handler process
either for itself or on behalf of a crashing child to create a crash
dump.
chrome/android/monochrome/scripts/monochrome_apk_checker.py
- libcrashpad_handler.so is a helper executable for non-monochrome
APKs. Monochrome has this code linked into libmonochrome.so.
components/crash/
- Remove CrashDumpManager. Minidump creation is handled entirely by
Crashpad.
- Remove OnChildStart from ChildExitObserver as it is no longer
necessary.
- ChildExitObserver observers CrashHandlerHost to be notified when
child processes receive crash signals.
components/minidump_uploader/
- Uploaders expect crash reports to already be MIME encoded since
Breakpad was doing that in a signal handler call-back.
CrashFileManager now automatically calls into native code to do
the encoding and write to a directory of crash reports whenever it
checks for reports without logcats.
chrome/app/*.{cc,h}
chrome/browser/*.cc
content/shell/app/*.{cc,h}
content/shell/browser/*.cc
- Initialize Crashpad instead of Breakpad, with minor cleanup and
adjustment for changes to CrashDumpObserver.
android_webview/
- AwBrowserTerminator now observes child process crashes via
ChildExitObserver rather than its own pipe.
chromecast/
- There are now two directories that crash report uploaders should
be aware of: "Crashpad" contains a database of raw minidumps
produced by Crashpad, and "Crash Reports" contains MIME encoded
minidumps. MIME encoding is performed by a CrashReportMimeWriter
in CastCrashUploader.java:checkForCrashDumps().
Bug: crashpad:30
Cq-Include-Trybots: master.tryserver.chromium.android:android_compile_x64_dbg;master.tryserver.chromium.android:android_compile_x86_dbg
Change-Id: I0efa451585f60287853c47f860f09ced581c8958
---
M android_webview/BUILD.gn
M android_webview/browser/DEPS
M android_webview/browser/aw_browser_main_parts.cc
M android_webview/browser/aw_browser_terminator.cc
M android_webview/browser/aw_browser_terminator.h
M android_webview/browser/aw_content_browser_client.cc
M android_webview/browser/aw_debug.cc
M android_webview/common/aw_paths.cc
M android_webview/common/crash_reporter/aw_crash_reporter_client.cc
M android_webview/common/crash_reporter/aw_crash_reporter_client.h
M android_webview/common/crash_reporter/crash_keys.cc
M android_webview/java/src/org/chromium/android_webview/AwDebug.java
M android_webview/javatests/src/org/chromium/android_webview/test/AwDebugTest.java
M android_webview/lib/aw_main_delegate.cc
M chrome/android/BUILD.gn
M chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
M chrome/android/monochrome/scripts/monochrome_apk_checker.py
M chrome/app/chrome_crash_reporter_client.cc
M chrome/app/chrome_crash_reporter_client.h
M chrome/app/chrome_main_delegate.cc
M chrome/browser/chrome_browser_main_android.cc
M chrome/browser/chrome_content_browser_client.cc
M chrome/browser/crash_upload_list/crash_upload_list.cc
M chrome/browser/metrics/oom/out_of_memory_reporter_unittest.cc
M chrome/common/chrome_paths.cc
M chromecast/android/BUILD.gn
M chromecast/app/BUILD.gn
M chromecast/app/android/DEPS
M chromecast/app/android/cast_crash_reporter_client_android.cc
M chromecast/app/android/cast_crash_reporter_client_android.h
M chromecast/app/android/crash_handler.cc
M chromecast/app/android/crash_handler.h
M chromecast/app/cast_main_delegate.cc
M chromecast/browser/android/BUILD.gn
M chromecast/browser/android/DEPS
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashHandler.java
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploaderFactory.java
M chromecast/browser/cast_browser_main_parts.cc
M chromecast/browser/cast_content_browser_client.cc
M chromecast/browser/cast_content_browser_client.h
M components/crash/android/BUILD.gn
D components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
D components/crash/android/javatests/src/org/chromium/components/crash/browser/CrashDumpManagerTest.java
M components/crash/content/app/BUILD.gn
M components/crash/content/app/crash_reporter_client.cc
M components/crash/content/app/crash_reporter_client.h
M components/crash/content/browser/BUILD.gn
M components/crash/content/browser/child_exit_observer_android.cc
M components/crash/content/browser/child_exit_observer_android.h
M components/crash/content/browser/child_process_crash_observer_android.cc
M components/crash/content/browser/child_process_crash_observer_android.h
D components/crash/content/browser/crash_dump_manager_android.cc
D components/crash/content/browser/crash_dump_manager_android.h
D components/crash/content/browser/crash_dump_manager_android_unittest.cc
M components/crash/content/browser/crash_metrics_reporter_android.cc
M components/crash/content/browser/crash_metrics_reporter_android.h
M components/crash/content/browser/crash_metrics_reporter_android_unittest.cc
M components/crash/core/common/BUILD.gn
M components/crash/core/common/crash_keys.cc
M components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java
M content/shell/app/shell_main_delegate.cc
M content/shell/browser/shell_browser_main_parts.cc
M content/shell/browser/shell_content_browser_client.cc
64 files changed, 354 insertions(+), 1,408 deletions(-)
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
agrieve (Gerrit)
Looks like this is 100kb increase. Is this overlap with the packaging change or is this an additional 100kb?
Trybot: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-binary-size/63698
Brand-new HTML size diff: https://storage.googleapis.com/chrome-supersize/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchromium-binary-size-trybot-results%2Fandroid-binary-size%2F2018%2F09%2F27%2F63698.ndjson&diff_mode=on
Joshua Peraza (Gerrit)
Patch Set 85:
Looks like this is 100kb increase. Is this overlap with the packaging change or is this an additional 100kb?
Trybot: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-binary-size/63698
Brand-new HTML size diff: https://storage.googleapis.com/chrome-supersize/viewer.html?load_url=https%3A%2F%2Fstorage.googleapis.com%2Fchromium-binary-size-trybot-results%2Fandroid-binary-size%2F2018%2F09%2F27%2F63698.ndjson&diff_mode=on
It's overlapping with the packaging change.
3 comments:
File android_webview/browser/aw_browser_terminator.cc:
Make a call to CrashMetricsReporter::ChildExited
File components/crash/content/browser/child_exit_observer_android.cc:
}
case content::NOTIFICATION_RENDERER_PROCESS_CREATED: {
// The child process pid isn't available when process is gone, keep a
// mapping between process_host_id and pid, so we can find it later.
process_host_id_to_pid_[rph->GetID()] = rph->GetProcess().Handle();
Avoid multiple notifications
Patch Set #67, Line 33:
the PIDThis comment should be updated. […]
Done
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza uploaded patch set #96 to this change.
Enable Crashpad for Android
Overview:
This CL disables Breakpad for Chrome, Content Shell, WebView, and
Chromecast on Android and replaces it with Crashpad. When a crash
signal is received, the browser forks+execs a Crashpad handler process
either for itself or on behalf of a crashing child to create a crash
dump.
components/crash/
- Remove CrashDumpManager. Minidump creation is handled entirely by
Crashpad.
- Remove OnChildStart from ChildExitObserver as it is no longer
necessary.
- ChildExitObserver observers CrashHandlerHost to be notified when
child processes receive crash signals.
- De-duplicate calls to ChildExitObserver::Client::OnChildExit when
NOTIFICATION_RENDER_PROCESS_{CLOSED, TERMINATED} are both sent.
components/minidump_uploader/
- Uploaders expect crash reports to already be MIME encoded since
Breakpad was doing that in a signal handler call-back.
CrashFileManager now automatically calls into native code to do
the encoding and write to a directory of crash reports whenever it
checks for reports without logcats.
chrome/app/*.{cc,h}
chrome/browser/*.cc
content/shell/app/*.{cc,h}
content/shell/browser/*.cc
- Initialize Crashpad instead of Breakpad, with minor cleanup and
adjustment for changes to CrashDumpObserver.
chrome/browser/metrics/oom/out_of_memory_reporter_unittest.cc
- Simulate crashes/exits with NOTIFICATION_RENDER_PROCESS_{CREATED,
CLOSED} and signals sent to ChildExitObserver instead of a crash
dump.
android_webview/
- AwBrowserTerminator now observes child process crashes via
ChildExitObserver rather than its own pipe.
chromecast/
- There are now two directories that crash report uploaders should
be aware of: "Crashpad" contains a database of raw minidumps
produced by Crashpad, and "Crash Reports" contains MIME encoded
minidumps. MIME encoding is performed by a CrashReportMimeWriter
in CastCrashUploader.java:checkForCrashDumps().
Bug: crashpad:30
Cq-Include-Trybots: master.tryserver.chromium.android:android_compile_x64_dbg;master.tryserver.chromium.android:android_compile_x86_dbg
Change-Id: I0efa451585f60287853c47f860f09ced581c8958
---
M android_webview/BUILD.gn
M android_webview/browser/DEPS
M android_webview/browser/aw_browser_main_parts.cc
M android_webview/browser/aw_browser_terminator.cc
M android_webview/browser/aw_browser_terminator.h
M android_webview/browser/aw_content_browser_client.cc
M android_webview/browser/aw_debug.cc
M android_webview/common/aw_paths.cc
M android_webview/common/crash_reporter/aw_crash_reporter_client.cc
M android_webview/common/crash_reporter/aw_crash_reporter_client.h
M android_webview/common/crash_reporter/crash_keys.cc
M android_webview/java/src/org/chromium/android_webview/AwDebug.java
M android_webview/javatests/src/org/chromium/android_webview/test/AwDebugTest.java
M android_webview/lib/aw_main_delegate.cc
M chrome/android/BUILD.gn
M chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
M chrome/app/chrome_crash_reporter_client.cc
M chrome/app/chrome_crash_reporter_client.h
M chrome/app/chrome_main_delegate.cc
M chrome/browser/chrome_browser_main_android.cc
M chrome/browser/chrome_content_browser_client.cc
M chrome/browser/crash_upload_list/crash_upload_list.cc
M chrome/browser/metrics/oom/out_of_memory_reporter_unittest.cc
M chrome/common/chrome_paths.cc
M chromecast/android/BUILD.gn
M chromecast/app/BUILD.gn
M chromecast/app/android/DEPS
M chromecast/app/android/cast_crash_reporter_client_android.cc
M chromecast/app/android/cast_crash_reporter_client_android.h
M chromecast/app/android/crash_handler.cc
M chromecast/app/android/crash_handler.h
M chromecast/app/cast_main_delegate.cc
M chromecast/browser/android/BUILD.gn
M chromecast/browser/android/DEPS
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashHandler.java
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploaderFactory.java
M chromecast/browser/cast_browser_main_parts.cc
M chromecast/browser/cast_content_browser_client.cc
M chromecast/browser/cast_content_browser_client.h
M components/crash/android/BUILD.gn
D components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
D components/crash/android/javatests/src/org/chromium/components/crash/browser/CrashDumpManagerTest.java
M components/crash/content/app/BUILD.gn
M components/crash/content/app/crash_reporter_client.cc
M components/crash/content/app/crash_reporter_client.h
M components/crash/content/browser/BUILD.gn
M components/crash/content/browser/child_exit_observer_android.cc
M components/crash/content/browser/child_exit_observer_android.h
M components/crash/content/browser/child_process_crash_observer_android.cc
M components/crash/content/browser/child_process_crash_observer_android.h
D components/crash/content/browser/crash_dump_manager_android.cc
D components/crash/content/browser/crash_dump_manager_android.h
D components/crash/content/browser/crash_dump_manager_android_unittest.cc
M components/crash/content/browser/crash_metrics_reporter_android.cc
M components/crash/content/browser/crash_metrics_reporter_android.h
M components/crash/content/browser/crash_metrics_reporter_android_unittest.cc
M components/crash/core/common/BUILD.gn
M components/crash/core/common/crash_keys.cc
M components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java
M components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/CrashTestRule.java
M content/shell/app/shell_main_delegate.cc
M content/shell/browser/shell_browser_main_parts.cc
M content/shell/browser/shell_content_browser_client.cc
64 files changed, 381 insertions(+), 1,417 deletions(-)
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Joshua Peraza uploaded patch set #97 to this change.
Enable Crashpad for Android
Overview:
This CL disables Breakpad for Chrome, Content Shell, WebView, and
Chromecast on Android and replaces it with Crashpad. When a crash
signal is received, the browser forks+execs a Crashpad handler process
either for itself or on behalf of a crashing child to create a crash
dump.
components/crash/
- Remove CrashDumpManager. Minidump creation is handled entirely by
Crashpad.
- Remove OnChildStart from ChildExitObserver as it is no longer
necessary.
- ChildExitObserver observers CrashHandlerHost to be notified when
child processes receive crash signals.
- De-duplicate calls to ChildExitObserver::Client::OnChildExit when
NOTIFICATION_RENDER_PROCESS_{CLOSED, TERMINATED} are both sent.
components/minidump_uploader/
- Uploaders expect crash reports to already be MIME encoded since
Breakpad was doing that in a signal handler call-back.
CrashFileManager now automatically calls into native code to do
the encoding and write to a directory of crash reports whenever it
checks for reports without logcats.
chrome/app/*.{cc,h}
chrome/browser/*.cc
content/shell/app/*.{cc,h}
content/shell/browser/*.cc
- Initialize Crashpad instead of Breakpad, with minor cleanup and
adjustment for changes to CrashDumpObserver.
chrome/browser/metrics/oom/out_of_memory_reporter_unittest.cc
- Simulate crashes/exits with NOTIFICATION_RENDER_PROCESS_{CREATED,
CLOSED} and signals sent to ChildExitObserver instead of a crash
dump.
android_webview/
- AwBrowserTerminator now observes child process crashes via
ChildExitObserver rather than its own pipe.
- Crashpad always produces minidumps, and not microdumps.
- Disabling Crashpad is not yet supported.
chromecast/
- There are now two directories that crash report uploaders should
be aware of: "Crashpad" contains a database of raw minidumps
produced by Crashpad, and "Crash Reports" contains MIME encoded
minidumps. MIME encoding is performed by a CrashReportMimeWriter
in CastCrashUploader.java:checkForCrashDumps().
Bug: crashpad:30
Cq-Include-Trybots: master.tryserver.chromium.android:android_compile_x64_dbg;master.tryserver.chromium.android:android_compile_x86_dbg
Change-Id: I0efa451585f60287853c47f860f09ced581c8958
---
M android_webview/BUILD.gn
M android_webview/browser/DEPS
M android_webview/browser/aw_browser_main_parts.cc
M android_webview/browser/aw_browser_terminator.cc
M android_webview/browser/aw_browser_terminator.h
M android_webview/browser/aw_content_browser_client.cc
M android_webview/browser/aw_debug.cc
M android_webview/common/aw_paths.cc
M android_webview/common/crash_reporter/aw_crash_reporter_client.cc
M android_webview/common/crash_reporter/aw_crash_reporter_client.h
M android_webview/common/crash_reporter/crash_keys.cc
M android_webview/java/src/org/chromium/android_webview/AwDebug.java
M android_webview/javatests/src/org/chromium/android_webview/test/AwDebugTest.java
M android_webview/lib/aw_main_delegate.cc
M chrome/android/BUILD.gn
M chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
M chrome/app/chrome_crash_reporter_client.cc
M chrome/app/chrome_crash_reporter_client.h
M chrome/app/chrome_main_delegate.cc
M chrome/browser/chrome_browser_main_android.cc
M chrome/browser/chrome_content_browser_client.cc
M chrome/browser/crash_upload_list/crash_upload_list.cc
M chrome/browser/metrics/oom/out_of_memory_reporter_unittest.cc
M chrome/common/chrome_paths.cc
M chromecast/android/BUILD.gn
M chromecast/app/BUILD.gn
M chromecast/app/android/DEPS
M chromecast/app/android/cast_crash_reporter_client_android.cc
M chromecast/app/android/cast_crash_reporter_client_android.h
M chromecast/app/android/crash_handler.cc
M chromecast/app/android/crash_handler.h
M chromecast/app/cast_main_delegate.cc
M chromecast/browser/android/BUILD.gn
M chromecast/browser/android/DEPS
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashHandler.java
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploaderFactory.java
M chromecast/browser/cast_browser_main_parts.cc
M chromecast/browser/cast_content_browser_client.cc
M chromecast/browser/cast_content_browser_client.h
M components/crash/android/BUILD.gn
D components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
D components/crash/android/javatests/src/org/chromium/components/crash/browser/CrashDumpManagerTest.java
M components/crash/content/app/BUILD.gn
M components/crash/content/app/crash_reporter_client.cc
M components/crash/content/app/crash_reporter_client.h
M components/crash/content/browser/BUILD.gn
M components/crash/content/browser/child_exit_observer_android.cc
M components/crash/content/browser/child_exit_observer_android.h
M components/crash/content/browser/child_process_crash_observer_android.cc
M components/crash/content/browser/child_process_crash_observer_android.h
D components/crash/content/browser/crash_dump_manager_android.cc
D components/crash/content/browser/crash_dump_manager_android.h
D components/crash/content/browser/crash_dump_manager_android_unittest.cc
M components/crash/content/browser/crash_metrics_reporter_android.cc
M components/crash/content/browser/crash_metrics_reporter_android.h
M components/crash/content/browser/crash_metrics_reporter_android_unittest.cc
M components/crash/core/common/BUILD.gn
M components/crash/core/common/crash_keys.cc
M components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java
M components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/CrashTestRule.java
M content/shell/app/shell_main_delegate.cc
M content/shell/browser/shell_browser_main_parts.cc
M content/shell/browser/shell_content_browser_client.cc
64 files changed, 381 insertions(+), 1,417 deletions(-)
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Hi, all!
This is slimmed down and ready for review again.
Refreshing reviewers,
torne for android_webview/
yfriedman for chrome/
sanfin for chromecast/
mark for components/crash/
isherman for components/minidump_uploader/
beverloo for content/shell/
Thanks!
2 comments:
Make a call to CrashMetricsReporter::ChildExited
}
case content::NOTIFICATION_RENDERER_PROCESS_CREATED: {
// The child process pid isn't available when process is gone, keep a
// mapping between process_host_id and pid, so we can find it later.
process_host_id_to_pid_[rph->GetID()] = rph->GetProcess().Handle();
Avoid multiple notifications
Done
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
components/crash LGTM
Patch set 97:Code-Review +1
Peter Beverloo (Gerrit)
//content/shell lgtm
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Simeon Anfinrud (Gerrit)
2 comments:
File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java:
Patch Set #97, Line 101:
rewriteMinidumpsAsMIMEswhat is this doing?
File chromecast/browser/cast_browser_main_parts.cc:
Patch Set #97, Line 638:
reports_dircan you explain the difference between GetCrashDumpLocation() and GetCrashReportsLocation()?
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
2 comments:
Patch Set #97, Line 101:
rewriteMinidumpsAsMIMEswhat is this doing?
After writing a minidump, Breakpad would immediately re-write the file, in its signal handler, into a MIME message. Crashpad just writes the raw minidump at crash time.
crashDumpDirectory is a (directory structure) database of minidumps written by Crashpad, managed by a crashpad::CrashReportDatabase. This method takes each minidump from crashDumpDirectory, re-writes it as a MIME message like Breakpad did, and leaves it in crashReportDirectory, deleting the original minidump from the database.
File chromecast/browser/cast_browser_main_parts.cc:
Patch Set #97, Line 638:
reports_dircan you explain the difference between GetCrashDumpLocation() and GetCrashReportsLocation()?
GetCrashDumpLocation() is the directory containing a crashpad database. GetCrashReportsLocation() is the directory of MIME encoded crash reports, ready for upload, the same as GetCrashDumpLocation() was before this change.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Tobias Sargeant (Gerrit)
2 comments:
File android_webview/browser/aw_browser_terminator.cc:
Patch Set #97, Line 69:
TODO(jperaza)Do you know how long it will take you to resolve this TODO? Not resolving it will mean that we will create and upload two crash reports for every renderer crash for WebView.
File android_webview/java/src/org/chromium/android_webview/AwDebug.java:
Patch Set #97, Line 28:
public static void dumpWithoutCrashing()This is not the same as Chrome's dumpWithoutCrashing, because the crash goes to the application, not to the crash processing pipeline.
You can't change this signature without potentially breaking applications that use reflection to call it. For example, AGSA uses it to generate a crash dump into a provided file, so that they can collect thread information in the case of deadlock.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
2 comments:
Patch Set #97, Line 69:
TODO(jperaza)
Do you know how long it will take you to resolve this TODO? Not resolving it will mean that we will […]
I've been thinking more about this and can probably put it into this CL or a quick followup (a day-ish to implement and roll into chromium).
I'm thinking of adding CrashWithoutDumping(string message) to ensure this is only used for cases like this. Sound good?
File android_webview/java/src/org/chromium/android_webview/AwDebug.java:
Patch Set #97, Line 28:
public static void dumpWithoutCrashing()
This is not the same as Chrome's dumpWithoutCrashing, because the crash goes to the application, not […]
Ah, thanks!
Does this need to continue to only write to truncated, pre-existing files?
Specifically, I'm thinking of doing a:Files.move(getLastCrashDump(), dumpFile, REPLACE_EXISTING)
but I could do a copy to the truncated file descriptor if that's still needed.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Ilya Sherman (Gerrit)
LGTM for //components/minidump_uploader. Thanks for splitting up this CL into more manageable pieces!
Patch set 97:Code-Review +1
1 comment:
This method begins by reading all minidumps from Crashpad's database and
* rewriting them as MIME files in the Crash Reports directory.nit: Please add this documentation above as well.
Tobias Sargeant (Gerrit)
1 comment:
Patch Set #97, Line 28:
public static void dumpWithoutCrashing()
Ah, thanks! […]
I don't think we can rely on the caller re-opening the File in question, so probably copying the contents is safest. It would be better not to also upload the dump, though.
Yaron Friedman (Gerrit)
+wnwen to review as well of chrome android side
3 comments:
File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:
Patch Set #97, Line 441:
minidumpI think logging an error when it's not not found could be helpful for future debugging
File chrome/browser/crash_upload_list/crash_upload_list.cc:
Patch Set #97, Line 31:
cache_dir.Append("Crash Reports")why is this not using the path below? I'm confused how this is consistent with chrome_paths.cc changes below. Oh is that we write crashes to crashpad dir for it to process and then it spits out in "Crash Reports"?
File components/crash/content/browser/crash_metrics_reporter_android.cc:
Patch Set #97, Line 121:
android_oom_killis this actually accurate? it sounds like all of the minidump handling was moved to crashpad but we use whether a minidump exists to know whether it was a legitimate crash or android just killing a child process to reclaim memory. We can't distinguish between the later and symbolization failing. If this just reports on whether the process was force closed this would break the actionability of our renderer CPM metrics.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
3 comments:
Patch Set #97, Line 69:
TODO(jperaza)
I've been thinking more about this and can probably put it into this CL or a quick followup (a day-i […]
Done
File chrome/browser/crash_upload_list/crash_upload_list.cc:
Patch Set #97, Line 31:
cache_dir.Append("Crash Reports")
why is this not using the path below? I'm confused how this is consistent with chrome_paths. […]
Yep. The "Crashpad" directory holds raw minidumps, which is what DIR_CRASH_DUMPS means everywhere else and is consistent with what macOS and Windows do. The "Crash Reports" directory holds reports that have been processed and are ready for upload.
If/when we revisit upload on Android, we can get rid of the "Crash Reports" directory and use a CrashUploadListCrashpad() instead.
File components/crash/content/browser/crash_metrics_reporter_android.cc:
Patch Set #97, Line 121:
android_oom_kill
is this actually accurate? it sounds like all of the minidump handling was moved to crashpad but we […]
"crashed" is true when the child process has received a signal initiating a crash dump.
When a child receives a crash signal, it sends a message to a CrashHandlerHost running in the browser process. The CrashHandlerHost sends the signal and child pid to any observers, including ChildExitObserver, before launching Crashpad to dump the child.
My understanding was that kEmptyDump indicated that Breakpad never wrote to the file (presumably) because the child never received a crash signal. This should be the same as has_valid_dump was before, except that it avoids the issue of Breakpad/Crashpad failing to produce a dump for reasons unrelated to the status of the child process.
Peter Wen (Gerrit)
Horray! Really looking forward to crashpad in android
1 comment:
Patch Set #97, Line 121:
android_oom_kill
"crashed" is true when the child process has received a signal initiating a crash dump. […]
Yep, so this is actually a higher signal than what we used to have with the minidump. In an ideal world we'd want to record this signal even when the browser crashes, is that at all possible?
Joshua Peraza (Gerrit)
3 comments:
Patch Set #97, Line 441:
minidumpI think logging an error when it's not not found could be helpful for future debugging
Patch Set #97, Line 121:
android_oom_kill
Yep, so this is actually a higher signal than what we used to have with the minidump. […]
That depends on your definition of possible. :D
AFAIK OOM kills can't be handled by the application. However, the browser could record a bit to a file at startup. During a crash (or normal shutdown), the browser's signal handler or Crashpad can clear that bit. At the next startup, if the bit is still set, we can guess that the browser was OOM killed (though we can't be sure). We'd need a separate process (some system service? long-lived crashpad handler process?) to differentiate things like power failures. I don't whether Android provides ways to better glean information about OOMs.
This method begins by reading all minidumps from Crashpad's database and
* rewriting them as MIME files in the Crash Reports directory.nit: Please add this documentation above as well.
Done
Richard Coles (Gerrit)
1 comment:
Patch Set #97, Line 121:
android_oom_kill
That depends on your definition of possible. :D […]
IIRC Chrome on android already maintains a bit similar to that: we set it when we come into the foreground, and clear it when we go into the background. If the process goes away without a crash dump being generated while the bit is set, we assume we crashed in a way that failed to generate a dump and this is bad. If it happens while the bit isn't set, we assume we were OOM killed. Neither of these conclusions is always true: it's possible we crash while in the background, and it's possible (though rare) to be OOM killed while in the foreground.
I don't think ANdroid in general provides any better way.
Peter Wen (Gerrit)
1 comment:
Patch Set #97, Line 121:
android_oom_kill
IIRC Chrome on android already maintains a bit similar to that: we set it when we come into the fore […]
You can see a more detailed trace of the current clean_exit_beacon bit and how Android uses it in my draft doc here: go/background-uma (will send it out once I'm done).
I'm looking for a signal to count the number of browser crashes that actually *do* generate a minidump (or handled by crashpad after this CL), whether in the foreground or background (of course, distinguishing foreground and background is a secondary objective). So that is different from what the current clean_exit_beacon provides.
We already do this for renderer and GPU, but use the code here in the browser process to count, see the block starting line#254. I'm hoping that crashpad being in a separate process might allow us to do something similar for the browser process.
I like Joshua's suggestion, just the other way around, browser sets a bit on startup, maybe crash_not_handled=true, in local_state, and crashpad clears that bit, crash_not_handled=false, when it handles a crash. Thus if clean_exit_beacon is false (i.e. a crash occurred in foreground), and crash_not_handled=false, we know that the previous crash was indeed handled by Crashpad and a minidump generated (most likely).
WDYT?
Yaron Friedman (Gerrit)
last q's/comments from me.
6 comments:
File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:
private void onFinishNativeInitialization() {
if (mNativeInitializationComplete) return;
mNativeInitializaI was doing this to be similar to the previous behavior, but now I realize it's racy if multiple chi […]
FYI: we're adding sequences to java this quarter. The current way to do it would be AsyncTask.SERIAL_EXECUTOR but it has pretty poor performance at times due to contention.
File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:
Patch Set #97, Line 441:
minidump
Done
don't see it (probably just not uploaded?)
File chrome/browser/chrome_browser_main_android.cc:
Seems this code can be moved elsewhere too. I don't believe the process_launcher thread is created in the same place anymore. For android it's done roughly here: https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelperImpl.java?q=LauncherThread+file:java+-file:test&dr=CSs&l=232 which is before native library is even initialized. See also https://cs.chromium.org/chromium/src/content/browser/child_process_launcher_helper.cc?type=cs&g=0&l=195
I guess we still know that we wouldn't have launched a child process by this point.
Does this still need to happen before any processes are launched for crashpad?
File chrome/browser/chrome_content_browser_client.cc:
hmm. IIRC crash was the only reason this is in the chrome/ layer. It seems like we could remove this embedder hook if we finish removing breakpad but I guess this is still used for linux.
Patch Set #97, Line 1905:
enable_crash_reporterdoes this change at all the semantics of when reporting is done to crash vs just enabled locally? I don't see any changes to permission related to reporting so that seems fine but I'm just curious (and haven't traced) whether this means that for local builds, you'd still exercise crash machinery, get a crash report in logcat, but not upload anything while for official builds it would upload?
See before I thought breakpad reproting was only enabled for "#if defined(GOOGLE_CHROME_BUILD)" in chrome_browser_main_android.cc and don't know whether this being unconditionally true behaves differently.
File components/crash/content/browser/crash_metrics_reporter_android.cc:
Patch Set #97, Line 121:
android_oom_kill
You can see a more detailed trace of the current clean_exit_beacon bit and how Android uses it in my […]
thanks for the explanation, Joshua. Sounds like this is fine then. Peter's comments are orthogonal and we can sort out later. :)
Joshua Peraza (Gerrit)
Thanks!
6 comments:
File android_webview/java/src/org/chromium/android_webview/AwDebug.java:
Patch Set #97, Line 28:
* needs to be protected from the unwantI don't think we can rely on the caller re-opening the File in question, so probably copying the con […]
This now uses an AwDebugCrashReporterClient to set different sanitization settings and to write crash dumps to a separate database location (<cacheDir>/WebView/Debug) which avoids accidental upload.
The tools/refactoring needed are for this are added in:
https://chromium-review.googlesource.com/c/chromium/src/+/1269099
File chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:
Patch Set #97, Line 441:
wser lau
don't see it (probably just not uploaded?)
Yep, uploaded now!
File chrome/browser/chrome_browser_main_android.cc:
Seems this code can be moved elsewhere too. […]
Yes, but it almost didn't need to. We no longer need to set up the dump file before the child starts, but ChildExitObserver is now using NOTIFICATION_RENDER_PROCESS_CREATED to determine when it's received both NOTIFICATION_RENDER_PROCESS_TERMINATED and NOTIFICATION_RENDER_PROCESS_CLOSED.
If we can find another way of de-duping those notifications, we'd only need to create a ChildExitObserver before any children crash.
I've partially re-included the comment.
File chrome/browser/chrome_content_browser_client.cc:
hmm. IIRC crash was the only reason this is in the chrome/ layer. […]
You're referring to BrowserChildProcessStarted()? It's already removed.
does this change at all the semantics of when reporting is done to crash vs just enabled locally? I […]
Yep, previously local crash dumping was only done for offical builds. Now crash dumping is always done locally including attaching logcats, whether it's an official build or not. The individual uploaders determine whether it is appropriate to upload, and should be disabled for non-official builds.
File components/crash/content/browser/crash_metrics_reporter_android.cc:
thanks for the explanation, Joshua. Sounds like this is fine then. […]
Done
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Richard Coles (Gerrit)
Sorry if these were already covered but I haven't been paying close attention to the very long discussion here :)
3 comments:
File android_webview/browser/aw_browser_terminator.cc:
Patch Set #100, Line 109:
if (info.normal_termination) {What's "normal termination" here? It's not clear how this altered code preserves the previous behaviour - we used to always take some action, no?
File android_webview/common/crash_reporter/aw_crash_reporter_client.cc:
Is the new code restricting crash reporting to a fraction somewhere? I can't see it. We need this because 100% crash uploading from stable is too much data.
Does the new code preserve the difference between "webview" and "browser" anywhere? It has historically been useful for us to be able to quickly tell which crashes are single-process.
Yaron Friedman (Gerrit)
ok, this lg but given that we branch on Thursday and this a scary change with potential mysteries to be addressed if our metrics change, can we wait for the cut and have this roll out with m72?
1 comment:
You're referring to BrowserChildProcessStarted()? It's already removed.
No I get that. I meant that we many not need this whole embedder API (ContentBrowserClient::GetAdditionalMappedFilesForChildProcess) as I believe all the other concepts are knowable for content so we could just simplify things
Joshua Peraza (Gerrit)
Patch Set 100:
(1 comment)
ok, this lg but given that we branch on Thursday and this a scary change with potential mysteries to be addressed if our metrics change, can we wait for the cut and have this roll out with m72?
SGTM!
4 comments:
Patch Set #100, Line 109:
if (info.normal_termination) {
What's "normal termination" here? It's not clear how this altered code preserves the previous behavi […]
It's a non-crashing, intentional shutdown.
It was also used previously on line 142, above and to the left.
The stuff done between OnChildExit() and that early return are:
1. Looking up the right pipe, which isn't needed anymore since we're now setting info.crash_signo (the value previously read from the pipe).
2. Calling OnRenderProcessGone(), done above.
3. Calling CrashDumpManager::ProcessMinidumpFromChild(). The only bit that needed to be preserved from CrashDumpManager was calling CrashMetricsReporter::ChildProcessExited(), done above.And with minidump processing and pipe reading out of the way, there's no more I/O, so no more need for task posting! :)
File android_webview/common/crash_reporter/aw_crash_reporter_client.cc:
Is the new code restricting crash reporting to a fraction somewhere? I can't see it. […]
It's not. I can move this to WebView's uploader (AwBrowserProcess.java:handleMinidumps()), but...
Previously, this would have also disabled calling CrashMetricsReporter::CrashDumpProcessed()
(via AwBrowserTerminator::OnChildExit() -> CrashDumpManager::ProcessMinidumpFileFromChild() -> CrashDumpProcessed())I think we probably DO want to record metrics for these clients, even if we want to sample the minidumps?
Does the new code preserve the difference between "webview" and "browser" anywhere? It has historica […]
Done, in the latest patchset, by adding CrashReporterClient::GetBrowserProcessType() to override the ptype annotation.
This also wasn't done by Breakpad for minidumps before. It would check for either of these values to initialize as a browser process, but then set ptype to "browser". All reports with ptype="webview" were from microdumps.
File chrome/browser/chrome_content_browser_client.cc:
No I get that. […]
Ack
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Yaron Friedman (Gerrit)
Richard Coles (Gerrit)
3 comments:
Patch Set #100, Line 109:
if (info.normal_termination) {
It's a non-crashing, intentional shutdown. […]
Ah, okay. So basically calling exit() which isn't a thing that we really do anyway (in theory). Sorry, I missed it due to the diff not aligning :)
File android_webview/common/crash_reporter/aw_crash_reporter_client.cc:
It's not. I can move this to WebView's uploader (AwBrowserProcess.java:handleMinidumps()), but... […]
Yes, we would want to record the metrics. This code was implemented before webview *had* metrics so it was probably an oversight :)
We haven't moved this to Finch yet so we still need a manual knob to turn for it.
Done, in the latest patchset, by adding CrashReporterClient::GetBrowserProcessType() to override the […]
Oops. Yeah, we had various intentional or accidental differences between the data in minidumps vs microdumps before :|
Joshua Peraza (Gerrit)
1 comment:
Yes, we would want to record the metrics. […]
Done
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Richard Coles (Gerrit)
I'll have to defer to Toby for webview approval I think as I'm not sure about one last point, but otherwise this looks okay to me:
1 comment:
File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:
Patch Set #105, Line 262:
final int kMinidumpUploadPercentage = 100;Is this the only place where we actually handle the minidumps? This gets called at startup for the dumps from the previous run (which is the only choice for browser crashes) - is that how we also handle renderer crashes, or do we do those "as they happen"?
Toby, does sampling here seem reasonable to you? I'm not familiar enough with the overall dumping flow.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
1 comment:
Patch Set #105, Line 262:
final int kMinidumpUploadPercentage = 100;
Is this the only place where we actually handle the minidumps? This gets called at startup for the d […]
Renderer crashes are handled as they happen, but also by this method:
AwBrowserTerminator::OnRenderProcessGoneDetail() -> AwBrowserProcess.java:triggerMinidumpUploading() -> handleMinidumpsAndSetMetricsConsent() -> handleMinidumps()
Richard Coles (Gerrit)
A
1 comment:
Patch Set #105, Line 262:
final int kMinidumpUploadPercentage = 100;
Renderer crashes are handled as they happen, but also by this method: […]
Aha, thanks Joshua, I missed that.
So, yeah. The behaviour change here is that:
- we'll still generate dumps all the time and we'll just delete most of them instead of
- uploading
- we'll reroll the sampling rate each time a renderer crashes, and upload a random x% of renderer crashes, instead of uploading all renderer crashes for x% of browser-process lifetimes.
The latter doesn't seem like a problem to me (in fact it may be better?), but the former means we're doing a bunch more work and also writing more data to disk which is a concern for android in general. Toby, what do you think?
Richard Coles (Gerrit)
1 comment:
Done
I've thought about this further and discussed with Toby and we're concerned that this doesn't go far enough as-is.
Previously we only generated a dump 1% (the number we reduce this to for stable) of the time. Generating a dump 100% of the time, and then just throwing it away 99% of the time, will significantly increase the amount of disk writes we do, which is a significant cause for concern for Android device vendors. How difficult would it be to actually avoid generating the dump to disk based on the ratio as the breakpad implementation does?
(also, i'm not sure offhand whether the old behaviour considered the user consent bit at this point or not: if we were previously not generating dumps at all when the consent bit was off, the we should ideally also keep *that* behaviour, and even if the old behaviour wasn't doing that.. it would actually be ideal to do that anyway).
Ilya Sherman (Gerrit)
1 comment:
I've thought about this further and discussed with Toby and we're concerned that this doesn't go far […]
FWIW, we intentionally do not consider the user consent bit when deciding whether or not to capture a dump. That way, users can decide to share a crash report after it happens, i.e. they can consent at the relevant moment, and have the crash data available locally. Maybe we want different tradeoffs on Android specifically; but in general, at least temporarily storing the data locally is desirable.
Richard Coles (Gerrit)
1 comment:
FWIW, we intentionally do not consider the user consent bit when deciding whether or not to capture […]
There's no way to access the crash dumps on Android unless you have a rooted device (application data directories are intentionally not accessible to the user), and WebView doesn't even have the chrome://crashes UI to see a list of them either. Having the crash dumps get generated anyway might be occasionally useful to chromium developers but I don't think it's useful to users.
Regardless, the sampling ratio no longer applying at dump generation time is a regression for us on webview that we'd prefer to avoid.
Ilya Sherman (Gerrit)
1 comment:
There's no way to access the crash dumps on Android unless you have a rooted device (application dat […]
Ah, I had been thinking of Chrome rather than WebView. I think the behavior you described for WebView makes sense; and also the behavior that I described should be kept for Chrome, where we do have a chrome://crashes UI.
Richard Coles (Gerrit)
1 comment:
Ah, I had been thinking of Chrome rather than WebView. […]
I'm not sure you can do anything useful for chrome either here - you still can't access the actual dumps without a rooted device. Is there some way to actually manually upload a dump from chrome://crashes ?
In any case: as long as we don't regress the behaviour here it's fine with me; if Chrome was already recording all the dumps to disk then it's not a problem if it still does, just as long as webview samples as it does now.
Ilya Sherman (Gerrit)
1 comment:
I'm not sure you can do anything useful for chrome either here - you still can't access the actual dumps without a rooted device. Is there some way to actually manually upload a dump from chrome://crashes ?
Yes, Chrome allows users to manually upload dumps from chrome://crashes =)
Richard Coles (Gerrit)
1 comment:
> I'm not sure you can do anything useful for chrome either here - you still can't access the actual […]
It doesn't seem to for me - if I have automatic reporting enabled they're already uploaded and it just gives me a link to file a bug about it, and if I disable automatic reporting then new crashes don't even appear in chrome://crashes at all (and the page has a notice at the top saying so).
Mark Mentovai (Gerrit)
1 comment:
It doesn't seem to for me - if I have automatic reporting enabled they're already uploaded and it just gives me a link to file a bug about it, and if I disable automatic reporting then new crashes don't even appear in chrome://crashes at all (and the page has a notice at the top saying so).
It works on Crashpad platforms, so on desktop Mac and Windows, yes, it’s doing what Ilya describes.
Until we land this, Android is still a Breakpad platform, and we don’t have “send report now” for Breakpad. (And its chreme://crashes page is much less complete.)
Richard Coles (Gerrit)
1 comment:
> It doesn't seem to for me - if I have automatic reporting enabled they're already uploaded and it […]
Ah, understood - thanks Mark! So yeah, leaving it for Chrome makes sense, but WebView doesn't have any entry point for this UI so there's not much value for us.
Joshua Peraza (Gerrit)
1 comment:
Ah, understood - thanks Mark! So yeah, leaving it for Chrome makes sense, but WebView doesn't have a […]
This is now done here:
https://chromium-review.googlesource.com/c/chromium/src/+/1298651
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
1 comment:
This is now done here: […]
Regarding WebView, torne wrote:
> There's no way to access the crash dumps on Android unless you have a
> rooted device (application data directories are intentionally not
> accessible to the user), and WebView doesn't even have the
> chrome://crashes UI to see a list of them either. Having the crash dumps
> get generated anyway might be occasionally useful to chromium developers
> but I don't think it's useful to users.
For WebView (among other things), the design I’m percolating right now is to put Crashpad into a new “one-shot” mode. Crash reports won’t persist in the database, but will be immediately handed off to the uploader, which will have one shot to upload them (if consent was given, obviously). After that, they’re gone.
Won’t really work well for Android (without some Chrome-side special-casing, anyway) until Crashpad owns uploading on its own.
Simeon Anfinrud (Gerrit)
Patch set 109:Code-Review +1
1 comment:
File chromecast/app/android/crash_handler.h:
Patch Set #109, Line 29:
GetCrashReportsLocationIt appears that crashes get dumped to the crash dump location, and then get rewritten and moved to the crash reports location. Can we have a comment here explaining the difference?
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Thanks!
Patch set 110:Commit-Queue +1
1 comment:
File chromecast/app/android/crash_handler.h:
Patch Set #109, Line 29:
messages allowing uploIt appears that crashes get dumped to the crash dump location, and then get rewritten and moved to t […]
Done
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
+halliwell for
chromecast/app/BUILD.gn
chromecast/app/cast_main_delegate.cc
chromecast/browser/cast_browser_main_parts.cc
chromecast/browser/cast_content_browser_client.cc
chromecast/browser/cast_content_browser_client.h
Thanks!
Luke Halliwell (Gerrit)
Patch set 110:Code-Review +1
Joshua Peraza (Gerrit)
+jam for:
chrome/app/chrome_main_delegate.cc
chrome/common/chrome_paths.cc
Thanks!
Tobias Sargeant (Gerrit)
android_webview/ LTGM, as far as that's possible. The webview team will watch out for crash reporting behaviour changes.
Patch set 111:Code-Review +1
Joshua Peraza (Gerrit)
+jam for:
chrome/app/chrome_main_delegate.cc
chrome/common/chrome_paths.cc
Thanks!
Joshua Peraza (Gerrit)
Joshua Peraza removed Mark Mentovai from this change.
To view, visit change 989401. To unsubscribe, or for help writing mail filters, visit settings.
John Abd-El-Malek (Gerrit)
Peter Wen (Gerrit)
Thanks for moving this CL along and reaching consensus, Joshua!
Joshua Peraza (Gerrit)
Patch Set 111: Code-Review+1
Thanks for moving this CL along and reaching consensus, Joshua!
Thanks, and thanks for all the help everyone! Everything's ready now, but with the branch coming up next week, I think it would be best to submit this then for maximum warm-up time.
Mark Mentovai (Gerrit)
Patch Set 111:
Patch Set 111: Code-Review+1
Thanks for moving this CL along and reaching consensus, Joshua!
Thanks, and thanks for all the help everyone! Everything's ready now, but with the branch coming up next week, I think it would be best to submit this then for maximum warm-up time.
That’s a good plan. Thanks, Joshua!
Joshua Peraza (Gerrit)
Patch set 115:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Rebase" https://chromium-review.googlesource.com/c/989401/115
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/989401/115
Bot data: {"action": "start", "triggered_at": "2018-12-03T00:02:10.0Z", "cq_cfg_revision": "70cab9d2b640236dbe7514876ab171e09605f26a", "revision": "14195eb1bbcbc3fd1a9edf6d4708726c6db768bd"}
Commit Bot (Gerrit)
Commit Bot merged this change.
Enable Crashpad for Android
Overview:
This CL disables Breakpad for Chrome, Content Shell, WebView, and
Chromecast on Android and replaces it with Crashpad. When a crash
signal is received, the browser forks+execs a Crashpad handler process
either for itself or on behalf of a crashing child to create a crash
dump.
components/crash/
- Remove CrashDumpManager. Minidump creation is handled entirely by
Crashpad.
- Remove OnChildStart from ChildExitObserver as it is no longer
necessary.
- ChildExitObserver observers CrashHandlerHost to be notified when
child processes receive crash signals.
- De-duplicate calls to ChildExitObserver::Client::OnChildExit when
NOTIFICATION_RENDER_PROCESS_{CLOSED, TERMINATED} are both sent.
components/minidump_uploader/
- Uploaders expect crash reports to already be MIME encoded since
Breakpad was doing that in a signal handler call-back.
CrashFileManager now automatically calls into native code to do
the encoding and write to a directory of crash reports whenever it
checks for reports without logcats.
chrome/app/*.{cc,h}
chrome/browser/*.cc
content/shell/app/*.{cc,h}
content/shell/browser/*.cc
- Initialize Crashpad instead of Breakpad, with minor cleanup and
adjustment for changes to CrashDumpObserver.
chrome/browser/metrics/oom/out_of_memory_reporter_unittest.cc
- Simulate crashes/exits with NOTIFICATION_RENDER_PROCESS_{CREATED,
CLOSED} and signals sent to ChildExitObserver instead of a crash
dump.
android_webview/
- AwBrowserTerminator now observes child process crashes via
ChildExitObserver rather than its own pipe.
- Crashpad always produces minidumps, and not microdumps.
- Disabling Crashpad is not yet supported.
chromecast/
- There are now two directories that crash report uploaders should
be aware of: "Crashpad" contains a database of raw minidumps
produced by Crashpad, and "Crash Reports" contains MIME encoded
minidumps. MIME encoding is performed by a CrashReportMimeWriter
in CastCrashUploader.java:checkForCrashDumps().
Bug: crashpad:30
Cq-Include-Trybots: master.tryserver.chromium.android:android_compile_x64_dbg;master.tryserver.chromium.android:android_compile_x86_dbg
Change-Id: I0efa451585f60287853c47f860f09ced581c8958
Reviewed-on: https://chromium-review.googlesource.com/c/989401
Commit-Queue: Joshua Peraza <jpe...@chromium.org>
Reviewed-by: Tobias Sargeant <tobi...@chromium.org>
Reviewed-by: John Abd-El-Malek <j...@chromium.org>
Reviewed-by: Peter Wen <wn...@chromium.org>
Reviewed-by: Luke Halliwell <hall...@chromium.org>
Reviewed-by: Simeon Anfinrud <san...@chromium.org>
Reviewed-by: Yaron Friedman <yfri...@chromium.org>
Reviewed-by: Mark Mentovai <ma...@chromium.org>
Reviewed-by: Peter Beverloo <pe...@chromium.org>
Reviewed-by: Ilya Sherman <ishe...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612987}
---
M android_webview/BUILD.gn
M android_webview/browser/DEPS
M android_webview/browser/aw_browser_main_parts.cc
M android_webview/browser/aw_browser_terminator.cc
M android_webview/browser/aw_browser_terminator.h
M android_webview/browser/aw_content_browser_client.cc
M android_webview/browser/aw_debug.cc
M android_webview/common/aw_paths.cc
M android_webview/common/crash_reporter/aw_crash_reporter_client.cc
M android_webview/common/crash_reporter/aw_crash_reporter_client.h
M android_webview/common/crash_reporter/crash_keys.cc
M android_webview/lib/aw_main_delegate.cc
M chrome/android/BUILD.gn
M chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
M chrome/app/chrome_crash_reporter_client.cc
M chrome/app/chrome_crash_reporter_client.h
M chrome/app/chrome_main_delegate.cc
M chrome/browser/chrome_browser_main_android.cc
M chrome/browser/chrome_content_browser_client.cc
M chrome/browser/crash_upload_list/crash_upload_list.cc
M chrome/browser/metrics/oom/out_of_memory_reporter_unittest.cc
M chrome/common/chrome_paths.cc
M chromecast/android/BUILD.gn
M chromecast/app/BUILD.gn
M chromecast/app/android/DEPS
M chromecast/app/android/cast_crash_reporter_client_android.cc
M chromecast/app/android/cast_crash_reporter_client_android.h
M chromecast/app/android/crash_handler.cc
M chromecast/app/android/crash_handler.h
M chromecast/app/cast_main_delegate.cc
M chromecast/browser/android/BUILD.gn
M chromecast/browser/android/DEPS
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashHandler.java
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploader.java
M chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastCrashUploaderFactory.java
M chromecast/browser/cast_browser_main_parts.cc
M chromecast/browser/cast_content_browser_client.cc
M chromecast/browser/cast_content_browser_client.h
M components/crash/android/BUILD.gn
D components/crash/android/java/src/org/chromium/components/crash/browser/CrashDumpManager.java
D components/crash/android/javatests/src/org/chromium/components/crash/browser/CrashDumpManagerTest.java
M components/crash/content/app/BUILD.gn
M components/crash/content/app/crash_reporter_client.cc
M components/crash/content/app/crash_reporter_client.h
M components/crash/content/app/crashpad.cc
M components/crash/content/app/crashpad.h
M components/crash/content/app/crashpad_linux.cc
M components/crash/content/browser/BUILD.gn
M components/crash/content/browser/child_exit_observer_android.cc
M components/crash/content/browser/child_exit_observer_android.h
M components/crash/content/browser/child_process_crash_observer_android.cc
M components/crash/content/browser/child_process_crash_observer_android.h
D components/crash/content/browser/crash_dump_manager_android.cc
D components/crash/content/browser/crash_dump_manager_android.h
D components/crash/content/browser/crash_dump_manager_android_unittest.cc
M components/crash/content/browser/crash_metrics_reporter_android.cc
M components/crash/content/browser/crash_metrics_reporter_android.h
M components/crash/content/browser/crash_metrics_reporter_android_unittest.cc
M components/crash/core/common/BUILD.gn
M components/crash/core/common/crash_keys.cc
M components/minidump_uploader/BUILD.gn
M components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/CrashFileManager.java
M components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/CrashTestRule.java
M components/minidump_uploader/rewrite_minidumps_as_mimes.cc
A components/minidump_uploader/rewrite_minidumps_as_mimes.h
M content/shell/app/shell_main_delegate.cc
M content/shell/browser/shell_browser_main_parts.cc
M content/shell/browser/shell_content_browser_client.cc
68 files changed, 534 insertions(+), 1,371 deletions(-)
Joshua Peraza (Gerrit)
This broke the android-rel builder when trying to find Crashpad's third_party license files. It's fixed here: https://chromium-review.googlesource.com/c/chromium/src/+/1358002
calamity (Gerrit)
calamity has created a revert of this change.