Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694083004:
Fix minidump-generation for debug builds. (Closed)
Description(almost) Fix minidump-generation for debug builds.
Some refactoring performed in https://codereview.chromium.org/2652133004
caused some extra microdump-info to be added to minidumps.
Remove this extra info since we hit an assert because of it (in debug
builds), and since the extra info isn't used in the minidumps anyway.
WebView also hits a DCHECK when trying to add unregistered crash keys,
this will be fixed in a follow-up.
BUG=691560
Review-Url: https://codereview.chromium.org/2694083004
Cr-Commit-Position: refs/heads/master@{#452820}
Committed: https://chromium.googlesource.com/chromium/src/+/8167aa05471108079a7f8aefae5135b037c92f45
Patch Set 1 #Patch Set 2 : Remove some old setters. #
Total comments: 9
Patch Set 3 : Only declare + use SanitizationInfo on Android. #
Total comments: 4
Patch Set 4 : Add OWNERS file for Robert to change crash keys. Also C++ variable naming. #Patch Set 5 : Add comment about adding new crash_keys to android_webview/ as well. Also C++ variable naming again… #Patch Set 6 : Remove unused fields from MinidumpInfo. #
Total comments: 8
Patch Set 7 : Fix Primiano's comments (some inline comments + #if-block refactoring). #
Total comments: 15
Patch Set 8 : Remove extra crash keys - this will be fixed in a follow-up. Fix Toby's comments. #Patch Set 9 : Fill in Microdump GPU info correctly if SetGpuFingerprint called before Initialize. #Patch Set 10 : Remove globals only used at init. Remove UpdateMicrodump[ExceptionHandler|Descriptor] methods. #
Total comments: 2
Patch Set 11 : Add back leak-annotations. #
Messages
Total messages: 39 (8 generated)
gsennton@chromium.org changed reviewers: + primiano@chromium.org, rsesek@chromium.org, tobiasjs@chromium.org, torne@chromium.org
Hey y'all, does this look remotely OK? I tried running the minidump generation code on a debug build and I hit some different asserts/DCHECKs which are addressed in this CL. The first problem is hit when setting the should_sanitize/principal_mapping_address values since at that point we would copy-construct our minidump in breakpad_linux to overwrite the changed values - this hits an assert in the minidump_descriptor copy-constructor that asserts that the path of the descriptor being copied hasn't been set. I solve this by only ever copy-constructing the descriptor for the MICROdump - not the minidump - when we get the gpu-fingerprint information (because the minidump doesn't use that gpu-fingerprint info), and passing the sanitization related flags at initialization instead of through setters. The second problem is crash keys that are not registered for WebView (but added in e.g. content/) - I'm just adding the keys we are missing. I realize that debug-builds are not our primary concern here since we will probably just enable minidump uploading for WebView for release-builds, but it seems it would be nice to be able to use the enabled-for-testing flag on a debug build to ensure we aren't hitting any asserts (especially in the minidump-uploading code). WDYT? Torne for android_webview/ Robert for components/crash/ Toby and Primiano as FYI
Generally seems reasonable, but some comments: https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... File android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc:161: sanitizationInfo.skip_dump_if_principal_mapping_not_referenced = false; Can you make this line part of an #if/#else instead of setting it to one value and then overwriting it? https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... File android_webview/common/crash_reporter/crash_keys.cc (right): https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... android_webview/common/crash_reporter/crash_keys.cc:25: {kActiveURL, kLargeSize}, If these keys are added by the content layer it seems sucky to repeat them here. Can we refactor things so that the content layer is also responsible for registering the ones it uses, and chrome/webview only register ones that are specific to them? https://codereview.chromium.org/2694083004/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.h (right): https://codereview.chromium.org/2694083004/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.h:19: struct SanitizationInfo { Can this go inside the OS_ANDROID block, or is it used somewhere non-OS-specific?
Do we have any kinds of tests for breakpad_linux.*? https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... File android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc:161: sanitizationInfo.skip_dump_if_principal_mapping_not_referenced = false; On 2017/02/15 11:45:13, Torne wrote: > Can you make this line part of an #if/#else instead of setting it to one value > and then overwriting it? Done. https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... File android_webview/common/crash_reporter/crash_keys.cc (right): https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... android_webview/common/crash_reporter/crash_keys.cc:25: {kActiveURL, kLargeSize}, On 2017/02/15 11:45:13, Torne wrote: > If these keys are added by the content layer it seems sucky to repeat them here. > Can we refactor things so that the content layer is also responsible for > registering the ones it uses, and chrome/webview only register ones that are > specific to them? According to Robert there is a plan to fix this (and remove the pre-registration). I'll leave the explanation of why a component-based approach won't work to him ;) https://codereview.chromium.org/2694083004/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.h (right): https://codereview.chromium.org/2694083004/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.h:19: struct SanitizationInfo { On 2017/02/15 11:45:13, Torne wrote: > Can this go inside the OS_ANDROID block, or is it used somewhere > non-OS-specific? Done.
lgtm
https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... File android_webview/common/crash_reporter/crash_keys.cc (right): https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... android_webview/common/crash_reporter/crash_keys.cc:24: base::debug::CrashKey fixed_keys[] = { Can you also update the comment here, so that people register them in this list as well? https://cs.chromium.org/chromium/src/chrome/common/crash_keys.cc?q=crash_keys... Also, please do per-file OWNERS on this file so I can approve without needing an android_webview OWNERS to stamp. https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... android_webview/common/crash_reporter/crash_keys.cc:25: {kActiveURL, kLargeSize}, On 2017/02/15 17:26:42, gsennton wrote: > On 2017/02/15 11:45:13, Torne wrote: > > If these keys are added by the content layer it seems sucky to repeat them > here. > > Can we refactor things so that the content layer is also responsible for > > registering the ones it uses, and chrome/webview only register ones that are > > specific to them? > > According to Robert there is a plan to fix this (and remove the > pre-registration). I'll leave the explanation of why a component-based approach > won't work to him ;) The short answer is no, that's not possible right now. Chrome ELF can't depend on components, and crash keys need to be registered at ELF-time, so that's not really workable. We are planning on removing the pre-registration requirement, but that's an intermediate-term project. We should use this approach for the time being.
https://codereview.chromium.org/2694083004/diff/40001/android_webview/common/... File android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/2694083004/diff/40001/android_webview/common/... android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc:159: breakpad::SanitizationInfo sanitizationInfo; naming: sanitization_info https://codereview.chromium.org/2694083004/diff/40001/components/crash/conten... File components/crash/content/app/breakpad_linux.h (right): https://codereview.chromium.org/2694083004/diff/40001/components/crash/conten... components/crash/content/app/breakpad_linux.h:31: const SanitizationInfo& sanitizationInfo); naming: sanitizationInfo (use under_scores, and throughout this file)
https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... File android_webview/common/crash_reporter/crash_keys.cc (right): https://codereview.chromium.org/2694083004/diff/20001/android_webview/common/... android_webview/common/crash_reporter/crash_keys.cc:24: base::debug::CrashKey fixed_keys[] = { On 2017/02/15 17:37:26, Robert Sesek wrote: > Can you also update the comment here, so that people register them in this list > as well? > https://cs.chromium.org/chromium/src/chrome/common/crash_keys.cc?q=crash_keys... > > Also, please do per-file OWNERS on this file so I can approve without needing an > android_webview OWNERS to stamp. Done. https://codereview.chromium.org/2694083004/diff/40001/android_webview/common/... File android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/2694083004/diff/40001/android_webview/common/... android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc:159: breakpad::SanitizationInfo sanitizationInfo; On 2017/02/15 17:38:43, Robert Sesek wrote: > naming: sanitization_info Done. https://codereview.chromium.org/2694083004/diff/40001/components/crash/conten... File components/crash/content/app/breakpad_linux.h (right): https://codereview.chromium.org/2694083004/diff/40001/components/crash/conten... components/crash/content/app/breakpad_linux.h:31: const SanitizationInfo& sanitizationInfo); On 2017/02/15 17:38:43, Robert Sesek wrote: > naming: sanitizationInfo (use under_scores, and throughout this file) Done.
Ooops - PS5 didn't remove the fields that are no longer in use. PS6 should be fine! :)
LGTM with minor comments. No idea about the crash keys part, but I see you have the right set of people (aka rsesek) here :) https://codereview.chromium.org/2694083004/diff/100001/android_webview/common... File android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/2694083004/diff/100001/android_webview/common... android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc:162: sanitization_info.skip_dump_if_principal_mapping_not_referenced = false; there shouldn't be any need for this as false is the default value, right? can you just #if !defined(COMPONENT ? https://codereview.chromium.org/2694083004/diff/100001/components/crash/conte... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2694083004/diff/100001/components/crash/conte... components/crash/content/app/breakpad_linux.cc:1920: // In Android we define two InitCrashReporter functions, and define this comment is probably redundant, everything you say can be inferred by reading the code around: this comment. If you want to add a comment here I'd instead explain why, something like // On Android WebView microdumps are conditionally emitted (depending on the cause of the crash) and can be be sanitized to prevent exposing unncessary data from the hosting application. or something like that https://codereview.chromium.org/2694083004/diff/100001/components/crash/conte... File components/crash/content/app/breakpad_linux.h (right): https://codereview.chromium.org/2694083004/diff/100001/components/crash/conte... components/crash/content/app/breakpad_linux.h:18: bool should_sanitize_dumps; FYI these days you can use Non-Static Class Member Initializers, so can writ this just as: bool should_sanitize_dumps = false; ... (not mandatory though, so both styles are fine) https://codereview.chromium.org/2694083004/diff/100001/components/crash/conte... components/crash/content/app/breakpad_linux.h:33: extern void InitCrashReporter(const std::string& process_type); looks like you can factor this out of the #if block by moving one line below and avoiding repeating it twice
Thanks Primiano! https://codereview.chromium.org/2694083004/diff/100001/android_webview/common... File android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/2694083004/diff/100001/android_webview/common... android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc:162: sanitization_info.skip_dump_if_principal_mapping_not_referenced = false; On 2017/02/15 18:39:07, Primiano Tucci wrote: > there shouldn't be any need for this as false is the default value, right? can > you just #if !defined(COMPONENT ? Done. https://codereview.chromium.org/2694083004/diff/100001/components/crash/conte... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2694083004/diff/100001/components/crash/conte... components/crash/content/app/breakpad_linux.cc:1920: // In Android we define two InitCrashReporter functions, and define On 2017/02/15 18:39:07, Primiano Tucci wrote: > this comment is probably redundant, everything you say can be inferred by > reading the code around: this comment. If you want to add a comment here I'd > instead explain why, something like > > // On Android WebView microdumps are conditionally emitted (depending on the > cause of the crash) and can be be sanitized to prevent exposing unncessary data > from the hosting application. > or something like that Neat, thanks! https://codereview.chromium.org/2694083004/diff/100001/components/crash/conte... File components/crash/content/app/breakpad_linux.h (right): https://codereview.chromium.org/2694083004/diff/100001/components/crash/conte... components/crash/content/app/breakpad_linux.h:18: bool should_sanitize_dumps; On 2017/02/15 18:39:08, Primiano Tucci wrote: > FYI these days you can use Non-Static Class Member Initializers, so can writ > this just as: > > bool should_sanitize_dumps = false; > ... > > (not mandatory though, so both styles are fine) Done. https://codereview.chromium.org/2694083004/diff/100001/components/crash/conte... components/crash/content/app/breakpad_linux.h:33: extern void InitCrashReporter(const std::string& process_type); On 2017/02/15 18:39:08, Primiano Tucci wrote: > looks like you can factor this out of the #if block by moving one line below and > avoiding repeating it twice Moved this above SanitizationInfo instead and merged the two OS_ANDROID blocks.
Description was changed from ========== Fix minidump-generation for debug builds. 1. Some refactoring performed in https://codereview.chromium.org/2652133004 caused some extra microdump-info to be added to minidumps. Remove this extra info since we hit an assert because of it (in debug builds), and since the extra info isn't used in the minidumps anyway. 2. Add crash keys used by different chrome components to ensure WebView debug builds won't crash because of non-registered keys. BUG=691560 ========== to ========== Fix minidump-generation for debug builds. 1. Some refactoring performed in https://codereview.chromium.org/2652133004 caused some extra microdump-info to be added to minidumps. Remove this extra info since we hit an assert because of it (in debug builds), and since the extra info isn't used in the minidumps anyway. 2. Add crash keys used by different chrome components to ensure WebView debug builds won't crash because of non-registered keys. BUG=691560 BUG=655614 ==========
LGTM
On 2017/02/16 15:06:44, Robert Sesek wrote: > LGTM not lgtm - I just realized that I have a couple of TODO comments that should remove from android_webview/common/crash_reporter/crash_keys.cc, but also - more importantly - I'm not entirely sure it is ok to upload all these crash keys from WebView (will have to investigate).
On 2017/02/16 15:35:35, gsennton wrote: > On 2017/02/16 15:06:44, Robert Sesek wrote: > > LGTM > > not lgtm - I just realized that I have a couple of TODO comments that should > remove from android_webview/common/crash_reporter/crash_keys.cc, but also - > more importantly - I'm not entirely sure it is ok to upload all these crash keys > from WebView (will have to investigate). Oh I thought I could block the CL somehow with a 'not lgtm', didn't now it just withdraws your own lgtm ;)
gsennton@chromium.org changed reviewers: + gsennton@chromium.org
On 2017/02/16 15:37:20, gsennton wrote: > On 2017/02/16 15:35:35, gsennton wrote: > > On 2017/02/16 15:06:44, Robert Sesek wrote: > > > LGTM > > > > not lgtm - I just realized that I have a couple of TODO comments that should > > remove from android_webview/common/crash_reporter/crash_keys.cc, but also - > > more importantly - I'm not entirely sure it is ok to upload all these crash > keys > > from WebView (will have to investigate). > > Oh I thought I could block the CL somehow with a 'not lgtm', didn't now it just > withdraws your own lgtm ;) let's see if adding myself as a reviewer before writing not lgtm works.
Thanks for addressing the DCHECK, and sorry I didn't pick it up. I don't think we need to change the crash keys (see rationales in comments). https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... File android_webview/common/crash_reporter/crash_keys.cc (right): https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... android_webview/common/crash_reporter/crash_keys.cc:25: {kActiveURL, kLargeSize}, We shouldn't log this; it's PII. https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... android_webview/common/crash_reporter/crash_keys.cc:39: {"ppapi_path", kMediumSize}, Pepper plugin isn't used in webview. https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... android_webview/common/crash_reporter/crash_keys.cc:40: {"subresource_url", kLargeSize}, Shouldn't log URLs. https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... android_webview/common/crash_reporter/crash_keys.cc:48: {"seccomp-sigsys", kMediumSize}, Correct me if I'm wrong, Robert, but won't this be useless on android? https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... android_webview/common/crash_reporter/crash_keys.cc:50: // Temporary for http://crbug.com/575245. We've only seen 8 reports for this bug. I don't think it's worth adding. https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... android_webview/common/crash_reporter/crash_keys.cc:71: // Temporary for https://crbug.com/591478. 0 reports for this one. https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... android_webview/common/crash_reporter/crash_keys.cc:81: // Temporary for https://crbug.com/626802. Also 0. https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... android_webview/common/crash_reporter/crash_keys.cc:98: // Temporary for https://crbug.com/612711. No extensions for webview. https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... android_webview/common/crash_reporter/crash_keys.cc:101: // Temporary for https://crbug.com/616149. Ditto. https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... android_webview/common/crash_reporter/crash_keys.cc:104: // Temporary for https://crbug.com/668633. Service workers are effectively unused in webview, and won't exist in their own process? https://codereview.chromium.org/2694083004/diff/120001/chrome/common/crash_ke... File chrome/common/crash_keys.cc (right): https://codereview.chromium.org/2694083004/diff/120001/chrome/common/crash_ke... chrome/common/crash_keys.cc:106: // RegisterWebViewCrashKeys(). It should probably be something more along the lines of "consider adding to webview" https://codereview.chromium.org/2694083004/diff/120001/components/crash/conte... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2694083004/diff/120001/components/crash/conte... components/crash/content/app/breakpad_linux.cc:1927: InitCrashReporter(process_type, sanitization_info); It seems redundant to have initializers in the definition, and to also set them to the same thing here.
Right, I agree that the crash keys seem unnecessary - the issue is that we have a DCHECK here: https://cs.chromium.org/chromium/src/base/debug/crash_logging.cc?&l=59 that ensures that crash keys are registered before being set (and the keys - at least some of them - that I added to WebView in this CL are all set at run-time in some lower layer like content/). Thus we hit the DCHECK at the point of setting those keys, and crash. Robert: could we add a flag or similar to ignore the DCHECK - for use in WebView?
If you're not interested in crash keys, you can not initialize the reporting functions, and then setting crash keys is a no-op: https://cs.chromium.org/chromium/src/base/debug/crash_logging.h?type=cs&sq=pa...
On 2017/02/21 17:02:37, Robert Sesek wrote: > If you're not interested in crash keys, you can not initialize the reporting > functions, and then setting crash keys is a no-op: > > https://cs.chromium.org/chromium/src/base/debug/crash_logging.h?type=cs&sq=pa... We do want crash keys, however, so not initializing isn't an option.
Given that the logic added to breakpad_linux.cc in this CL is non-trivial I'm splitting this CL in two: the current one, and one where I'll add a flag to base/debug/crash_logging to ignore unregistered keys (so that WebView can register a sub-set of the keys, and ignore the rest). https://codereview.chromium.org/2694083004/diff/120001/chrome/common/crash_ke... File chrome/common/crash_keys.cc (right): https://codereview.chromium.org/2694083004/diff/120001/chrome/common/crash_ke... chrome/common/crash_keys.cc:106: // RegisterWebViewCrashKeys(). On 2017/02/20 11:47:18, Tobias Sargeant wrote: > It should probably be something more along the lines of "consider adding to > webview" Done. https://codereview.chromium.org/2694083004/diff/120001/components/crash/conte... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2694083004/diff/120001/components/crash/conte... components/crash/content/app/breakpad_linux.cc:1927: InitCrashReporter(process_type, sanitization_info); On 2017/02/20 11:47:18, Tobias Sargeant wrote: > It seems redundant to have initializers in the definition, and to also set them > to the same thing here. Done.
Description was changed from ========== Fix minidump-generation for debug builds. 1. Some refactoring performed in https://codereview.chromium.org/2652133004 caused some extra microdump-info to be added to minidumps. Remove this extra info since we hit an assert because of it (in debug builds), and since the extra info isn't used in the minidumps anyway. 2. Add crash keys used by different chrome components to ensure WebView debug builds won't crash because of non-registered keys. BUG=691560 BUG=655614 ========== to ========== (almost) Fix minidump-generation for debug builds. Some refactoring performed in https://codereview.chromium.org/2652133004 caused some extra microdump-info to be added to minidumps. Remove this extra info since we hit an assert because of it (in debug builds), and since the extra info isn't used in the minidumps anyway. WebView also hits a DCHECK when trying to add unregistered crash keys, this will be fixed in a follow-up. BUG=691560 ==========
I'm not convinced the "add a flag to base" is the right approach. For starters, it may cover up issues that the pre-registration system is there to prevent. But it also is not going to be compatible with the future system in which pre-registration is no longer required. It seems like doing adding this flag will implicitly take advantage of ignoring unregistered keys to filter out ones WebView is not interested in collecting (due to privacy or other reasons). But as soon as the pre-registration is gone, that is not going to be possible anymore. So I don't know how WebView is going to select which crash keys to use in that situation. How large is/which keys are the subset of keys that WebView is interested in collecting? https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... File android_webview/common/crash_reporter/crash_keys.cc (right): https://codereview.chromium.org/2694083004/diff/120001/android_webview/common... android_webview/common/crash_reporter/crash_keys.cc:48: {"seccomp-sigsys", kMediumSize}, On 2017/02/20 11:47:18, Tobias Sargeant wrote: > Correct me if I'm wrong, Robert, but won't this be useless on android? Not at all. This was added for Android.
On 2017/02/22 16:41:36, Robert Sesek wrote: > I'm not convinced the "add a flag to base" is the right approach. For starters, > it may cover up issues that the pre-registration system is there to prevent. But > it also is not going to be compatible with the future system in which > pre-registration is no longer required. It seems like doing adding this flag > will implicitly take advantage of ignoring unregistered keys to filter out ones > WebView is not interested in collecting (due to privacy or other reasons). But > as soon as the pre-registration is gone, that is not going to be possible > anymore. So I don't know how WebView is going to select which crash keys to use > in that situation. How large is/which keys are the subset of keys that WebView > is interested in collecting? What about adding a CrashReporterClient method that takes a key and returns whether to include it in the dump? Then WebView registers all the keys, but filters them at dump time? Alternatively webview could pass a list of whitelisted keys to the crash component, which would get around the problem that the above method would execute in a compromised context.
Is the idea that any client (e.g. WebView/Clank) that doesn't want a specific key should make the relevant calls to SetCrashKeyValue conditional on whether that specific client is used? Otherwise, it seems wrong not to be able to just use a sub-set of the keys for which we call SetCrashKeyValue - at least given WebView special PII constraints. What are the different constraints/requirements on this crash-key-value system? Changing any of the calls to SetCrashKeyValue doesn't seem like the correct way to solve this since those calls live in lower-level layers (so e.g. an android_webview/ OWNER wouldn't have any control over them). Toby's suggestions above should work but it feels like they are workarounds (and adding a second layer of key-registration - like adding a key-filtering method in CrashReporterClient - seems error-prone). Also, given that this CL is orthogonal to the crash-keys stuff, can we just land this CL, and continue this discussion in https://codereview.chromium.org/2708883006/ ? :) (or over VC or w/e).
Yeah, I think the basic problem is that WebView has different PII handling restrictions and so we don't just want all "normal" crash keys used in chrome to automatically be reported for WebView as well when people add new ones without considering webview. Some kind of whitelisting logic is the conservative approach, but there's lots of different ways we could whitelist this.
On 2017/02/23 12:12:12, gsennton wrote: > Also, given that this CL is orthogonal to the crash-keys stuff, can we just land > this CL, and continue this discussion in > https://codereview.chromium.org/2708883006/ ? :) (or over VC or w/e). Yes, LGTM
Ooopsie! I almost messed up the case where Initialize is called after SetGpuFingerprint. We sooo need tests for this code :/ Is it very difficult to add tests for this file? (if it is feasible I would volunteer to do so).
Alright, some more cleaning, should be enough with another look through from Toby :)
https://codereview.chromium.org/2694083004/diff/180001/components/crash/conte... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2694083004/diff/180001/components/crash/conte... components/crash/content/app/breakpad_linux.cc:957: microdump_extra_info->product_info = It looks like you would need ANNOTATE_LEAKING_OBJECT_PTR still for all of these.
On 2017/02/24 11:36:24, Tobias Sargeant wrote: > https://codereview.chromium.org/2694083004/diff/180001/components/crash/conte... > File components/crash/content/app/breakpad_linux.cc (right): > > https://codereview.chromium.org/2694083004/diff/180001/components/crash/conte... > components/crash/content/app/breakpad_linux.cc:957: > microdump_extra_info->product_info = > It looks like you would need ANNOTATE_LEAKING_OBJECT_PTR still for all of these. Other than that, PS10 cleanup LGTM.
COMMITTING :D https://codereview.chromium.org/2694083004/diff/180001/components/crash/conte... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2694083004/diff/180001/components/crash/conte... components/crash/content/app/breakpad_linux.cc:957: microdump_extra_info->product_info = On 2017/02/24 11:36:24, Tobias Sargeant wrote: > It looks like you would need ANNOTATE_LEAKING_OBJECT_PTR still for all of these. Done.
The CQ bit was checked by gsennton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org, primiano@chromium.org, rsesek@chromium.org, tobiasjs@chromium.org Link to the patchset: https://codereview.chromium.org/2694083004/#ps200001 (title: "Add back leak-annotations.")
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": 200001, "attempt_start_ts": 1487943877280820,
"parent_rev": "ae67baea2fe52aaaad5e9705d00340f6ef5dee2e", "commit_rev":
"8167aa05471108079a7f8aefae5135b037c92f45"}
Message was sent while issue was closed.
Description was changed from ========== (almost) Fix minidump-generation for debug builds. Some refactoring performed in https://codereview.chromium.org/2652133004 caused some extra microdump-info to be added to minidumps. Remove this extra info since we hit an assert because of it (in debug builds), and since the extra info isn't used in the minidumps anyway. WebView also hits a DCHECK when trying to add unregistered crash keys, this will be fixed in a follow-up. BUG=691560 ========== to ========== (almost) Fix minidump-generation for debug builds. Some refactoring performed in https://codereview.chromium.org/2652133004 caused some extra microdump-info to be added to minidumps. Remove this extra info since we hit an assert because of it (in debug builds), and since the extra info isn't used in the minidumps anyway. WebView also hits a DCHECK when trying to add unregistered crash keys, this will be fixed in a follow-up. BUG=691560 Review-Url: https://codereview.chromium.org/2694083004 Cr-Commit-Position: refs/heads/master@{#452820} Committed: https://chromium.googlesource.com/chromium/src/+/8167aa05471108079a7f8aefae51... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/8167aa05471108079a7f8aefae51... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
