Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2677563006:
android: Report java exception in child (Closed)
Descriptionandroid: Report java exception in child
Enable JavaExceptionReporter for child processes so that unhandled java
exceptions generate crash reports.
Note this also requires fixing InitNonBrowserCrashReporterForAndroid so
that SetDumpWithoutCrashingFunction is called in child processes.
Tested with chrome://gpu-java-crash added in r447908, manually
enabling breakpad microdumps with local build, and adding
--enable-crash-reporter-for-testing. Verified microdump is generated
with the java stack information in logcat
BUG=680775
Review-Url: https://codereview.chromium.org/2677563006
Cr-Commit-Position: refs/heads/master@{#449000}
Committed: https://chromium.googlesource.com/chromium/src/+/e76f62c07f2dfbddef18d4011c576f678fc1f54b
Patch Set 1 #
Total comments: 2
Patch Set 2 : remove braces #
Messages
Total messages: 25 (12 generated)
boliu@chromium.org changed reviewers: + mariakhomenko@chromium.org, rsesek@chromium.org
ptal +rsesek for components/crash +mariakhomenko for non-owner chrome review
lgtm to the best of my knowledge this logic does what you say, but I have no knowledge of what gotchas this code might have. I hope you've been able to test this.
Description was changed from ========== android: Report java exception in child Enable JavaExceptionReporter for child processes so that unhandled java exceptions generate crash reports. Note this also requires fixing InitNonBrowserCrashReporterForAndroid so that SetDumpWithoutCrashingFunction is called in child processes. BUG=680775 ========== to ========== android: Report java exception in child Enable JavaExceptionReporter for child processes so that unhandled java exceptions generate crash reports. Note this also requires fixing InitNonBrowserCrashReporterForAndroid so that SetDumpWithoutCrashingFunction is called in child processes. Tested with chrome://gpu-java-crash added in r447908, and manually enabling microdumps with local build. BUG=680775 ==========
Description was changed from ========== android: Report java exception in child Enable JavaExceptionReporter for child processes so that unhandled java exceptions generate crash reports. Note this also requires fixing InitNonBrowserCrashReporterForAndroid so that SetDumpWithoutCrashingFunction is called in child processes. Tested with chrome://gpu-java-crash added in r447908, and manually enabling microdumps with local build. BUG=680775 ========== to ========== android: Report java exception in child Enable JavaExceptionReporter for child processes so that unhandled java exceptions generate crash reports. Note this also requires fixing InitNonBrowserCrashReporterForAndroid so that SetDumpWithoutCrashingFunction is called in child processes. Tested with chrome://gpu-java-crash added in r447908, and manually enabling breakpad microdumps with local build. Verified microdump is generated with the java stack information. BUG=680775 ==========
Description was changed from ========== android: Report java exception in child Enable JavaExceptionReporter for child processes so that unhandled java exceptions generate crash reports. Note this also requires fixing InitNonBrowserCrashReporterForAndroid so that SetDumpWithoutCrashingFunction is called in child processes. Tested with chrome://gpu-java-crash added in r447908, and manually enabling breakpad microdumps with local build. Verified microdump is generated with the java stack information. BUG=680775 ========== to ========== android: Report java exception in child Enable JavaExceptionReporter for child processes so that unhandled java exceptions generate crash reports. Note this also requires fixing InitNonBrowserCrashReporterForAndroid so that SetDumpWithoutCrashingFunction is called in child processes. Tested with chrome://gpu-java-crash added in r447908, manually enabling breakpad microdumps with local build, and adding --enable-crash-reporter-for-testing. Verified microdump is generated with the java stack information in logcat BUG=680775 ==========
Description was changed from ========== android: Report java exception in child Enable JavaExceptionReporter for child processes so that unhandled java exceptions generate crash reports. Note this also requires fixing InitNonBrowserCrashReporterForAndroid so that SetDumpWithoutCrashingFunction is called in child processes. Tested with chrome://gpu-java-crash added in r447908, manually enabling breakpad microdumps with local build, and adding --enable-crash-reporter-for-testing. Verified microdump is generated with the java stack information in logcat BUG=680775 ========== to ========== android: Report java exception in child Enable JavaExceptionReporter for child processes so that unhandled java exceptions generate crash reports. Note this also requires fixing InitNonBrowserCrashReporterForAndroid so that SetDumpWithoutCrashingFunction is called in child processes. Tested with chrome://gpu-java-crash added in r447908, manually enabling breakpad microdumps with local build, and adding --enable-crash-reporter-for-testing. Verified microdump is generated with the java stack information in logcat BUG=680775 ==========
On 2017/02/04 00:11:34, Maria wrote: > lgtm to the best of my knowledge this logic does what you say, but I have no > knowledge of what gotchas this code might have. I hope you've been able to test > this. I did test that microdump got generated in logcat, and added that to description I'm not sure about the actually generating a report to disk and uploading part. I don't see anything in chrome://crashes from these crashes in my local chromium build, and I'm not sure if there's an easy way to force that on, so to everything end to end..
rsesek: ping!
LGTM. Sorry, was OOO Friday MTV-time for travel and Monday for vacation.
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
boliu@chromium.org changed reviewers: + grt@chromium.org
oh oops, +grt to stamp chrome/app again
lgtm w/ a nit https://codereview.chromium.org/2677563006/diff/1/chrome/app/chrome_main_dele... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2677563006/diff/1/chrome/app/chrome_main_dele... chrome/app/chrome_main_delegate.cc:891: if (process_type.empty()) { nit: remove these braces now that all branches of the condition are one-liners, as this appears to be the style used in this file.
https://codereview.chromium.org/2677563006/diff/1/chrome/app/chrome_main_dele... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2677563006/diff/1/chrome/app/chrome_main_dele... chrome/app/chrome_main_delegate.cc:891: if (process_type.empty()) { On 2017/02/08 09:32:28, grt (UTC plus 1) wrote: > nit: remove these braces now that all branches of the condition are one-liners, > as this appears to be the style used in this file. Done.
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, mariakhomenko@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2677563006/#ps20001 (title: "remove braces")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486563270233990,
"parent_rev": "f2df51214d4918a13bf59909064997facbdefff3", "commit_rev":
"e76f62c07f2dfbddef18d4011c576f678fc1f54b"}
Message was sent while issue was closed.
Description was changed from ========== android: Report java exception in child Enable JavaExceptionReporter for child processes so that unhandled java exceptions generate crash reports. Note this also requires fixing InitNonBrowserCrashReporterForAndroid so that SetDumpWithoutCrashingFunction is called in child processes. Tested with chrome://gpu-java-crash added in r447908, manually enabling breakpad microdumps with local build, and adding --enable-crash-reporter-for-testing. Verified microdump is generated with the java stack information in logcat BUG=680775 ========== to ========== android: Report java exception in child Enable JavaExceptionReporter for child processes so that unhandled java exceptions generate crash reports. Note this also requires fixing InitNonBrowserCrashReporterForAndroid so that SetDumpWithoutCrashingFunction is called in child processes. Tested with chrome://gpu-java-crash added in r447908, manually enabling breakpad microdumps with local build, and adding --enable-crash-reporter-for-testing. Verified microdump is generated with the java stack information in logcat BUG=680775 Review-Url: https://codereview.chromium.org/2677563006 Cr-Commit-Position: refs/heads/master@{#449000} Committed: https://chromium.googlesource.com/chromium/src/+/e76f62c07f2dfbddef18d4011c57... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e76f62c07f2dfbddef18d4011c57...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2684893003/ by boliu@chromium.org. The reason for reverting is: tobiasjs@ pointed out: DumpWithoutCrash doesn't really work on android in child processes since android child processes sends the dump through a FD, and browser only picks up the FD after it detects child has crashed. So if child calls DumpWithoutCrash and *doesn't* crash, then a subsequent real crash report won't be picked up. Unhandled java exception is actually not the problem here. The problem is all the other places in child processes that calls DumpWithoutCrash. Risk of breaking normal crash reports seems high to me, so reverting to be safe.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
