Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2652133004:
Sanitize mini- and microdumps for webview (Closed)
|
Created:
3 years, 11 months ago by Tobias Sargeant Modified:
3 years, 10 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, android-webview-reviews_chromium.org, sadrul, kalyank, Primiano Tucci (use gerrit), boliu Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSanitize mini- and microdumps for webview
Enable the breakpad logic for sanitizing dumps in webview. Also skip
dumping any thread stacks that do not reference WebView's text
section.
To determine the webview text section address range, we provide the
address of a function that we know is part of the webview shared
library (PrincipalMappingMarker). Breakpad looks up the mapping that
contains that address, causing it to not generate a microdump if the
IP or the stack of the crashing thread does not contain a pointer
into this region.
BUG=664460, 682278
Review-Url: https://codereview.chromium.org/2652133004
Cr-Commit-Position: refs/heads/master@{#449620}
Committed: https://chromium.googlesource.com/chromium/src/+/238463db7875fa84f4a0f9b227cbde2c6d7ef011
Patch Set 1 #Patch Set 2 : finalize implementation #
Total comments: 8
Patch Set 3 : Address review comments. Also apply settings when g_breakpad is constructed. #
Total comments: 2
Patch Set 4 : Robert's comments. #
Messages
Total messages: 27 (12 generated)
Description was changed from ========== Do not generate a microdump if there are no webview pointers on the stack. To determine the webview text section address range, we provide the address of a function that we know is part of the webview shared library. Breakpad looks up the mapping that contains that address, causing it to not generate a microdump if the IP or the stack of the crashing thread does not contain a pointer into this region. BUG=664460 ========== to ========== Sanitize mini- and microdumps for webview Enable the breakpad logic for sanitizing dumps in webview. Also skip dumping any thread stacks that do not reference WebView's text section. To determine the webview text section address range, we provide the address of a function that we know is part of the webview shared library (PrincipalMappingMarker). Breakpad looks up the mapping that contains that address, causing it to not generate a microdump if the IP or the stack of the crashing thread does not contain a pointer into this region. BUG=664460 ==========
tobiasjs@chromium.org changed reviewers: + rsesek@chromium.org, torne@chromium.org
Adding Bo, because of potentially relevant changes in breakpad_linux.cc
primiano@chromium.org changed reviewers: + primiano@chromium.org
just a drive by comment, plz don't wait for my reply once you get LG from reviewers. https://codereview.chromium.org/2652133004/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2652133004/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:925: void MicrodumpInfo::SetPrincipalMappingAddress(uintptr_t addr) { I think this should have some stronger name to explain that doing this will automatically exclude any dump that doesn't contain the module. How about SetSkipDumpIfPrincipalModuleNotReferenced(uintptr_t addr_in_principal_module)
https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:233: reinterpret_cast<uintptr_t>(&PrincipalMappingMarker)); Any reason to not just use something like the ctor of this class instead? https://codereview.chromium.org/2652133004/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2652133004/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:151: bool skip_dump_if_principal_mapping_not_referenced_; Are these stored instead of just set directly to allow the methods to be called before initializing breakpad?
Generally seems reasonable, but agree with robert's questions :) https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:233: reinterpret_cast<uintptr_t>(&PrincipalMappingMarker)); On 2017/02/08 22:36:20, Robert Sesek wrote: > Any reason to not just use something like the ctor of this class instead? Yeah, I'd probably use the function in which this code appears :)
https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:233: reinterpret_cast<uintptr_t>(&PrincipalMappingMarker)); On 2017/02/09 13:55:57, Torne wrote: > On 2017/02/08 22:36:20, Robert Sesek wrote: > > Any reason to not just use something like the ctor of this class instead? > > Yeah, I'd probably use the function in which this code appears :) AFAICT it's not possible to take the address of a non static method. I don't know if that applies to the ctor as well.
On 2017/02/09 13:59:26, Tobias Sargeant wrote: > https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... > File android_webview/lib/main/aw_main_delegate.cc (right): > > https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... > android_webview/lib/main/aw_main_delegate.cc:233: > reinterpret_cast<uintptr_t>(&PrincipalMappingMarker)); > On 2017/02/09 13:55:57, Torne wrote: > > On 2017/02/08 22:36:20, Robert Sesek wrote: > > > Any reason to not just use something like the ctor of this class instead? > > > > Yeah, I'd probably use the function in which this code appears :) > > AFAICT it's not possible to take the address of a non static method. I don't > know if that applies to the ctor as well. Oops, yeah. You need a static/non-member function here. (you can't take the address of a constructor either).
https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:233: reinterpret_cast<uintptr_t>(&PrincipalMappingMarker)); On 2017/02/09 13:59:25, Tobias Sargeant wrote: > On 2017/02/09 13:55:57, Torne wrote: > > On 2017/02/08 22:36:20, Robert Sesek wrote: > > > Any reason to not just use something like the ctor of this class instead? > > > > Yeah, I'd probably use the function in which this code appears :) > > AFAICT it's not possible to take the address of a non static method. I don't > know if that applies to the ctor as well. Uh? AFAIK that's only true for member functions of local classes (classes defined within a block). % that weird case, it should be always possible to take the address of a member function, as they have a defined linkage. otherwise it would make little sense to be able to declare pointers to member functions. The problem here is that constructors are *not* member functions. so you can take the address of anything (even BasicStatupComplete itself) but not a ctor. ----- class Foo { public: Foo() { printf("ctor\n"); } void Method() { printf("method\n"); } }; int main() { printf("%x\n", &Foo::Foo); return 0; } -----
On 2017/02/09 14:11:32, Primiano Tucci wrote: > https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... > File android_webview/lib/main/aw_main_delegate.cc (right): > > https://codereview.chromium.org/2652133004/diff/20001/android_webview/lib/mai... > android_webview/lib/main/aw_main_delegate.cc:233: > reinterpret_cast<uintptr_t>(&PrincipalMappingMarker)); > On 2017/02/09 13:59:25, Tobias Sargeant wrote: > > On 2017/02/09 13:55:57, Torne wrote: > > > On 2017/02/08 22:36:20, Robert Sesek wrote: > > > > Any reason to not just use something like the ctor of this class instead? > > > > > > Yeah, I'd probably use the function in which this code appears :) > > > > AFAICT it's not possible to take the address of a non static method. I don't > > know if that applies to the ctor as well. > > Uh? AFAIK that's only true for member functions of local classes (classes > defined within a block). > % that weird case, it should be always possible to take the address of a member > function, as they have a defined linkage. otherwise it would make little sense > to be able to declare pointers to member functions. > The problem here is that constructors are *not* member functions. so you can > take the address of anything (even BasicStatupComplete itself) but not a ctor. > > ----- > class Foo { > public: > Foo() { printf("ctor\n"); } > void Method() { printf("method\n"); } > }; > > > int main() { > printf("%x\n", &Foo::Foo); > return 0; > } > ----- No, pointer-to-member is not even the same size as pointer-to-function. It's a completely different type that's magic (and usually something like 3*sizeof(intptr_t)).
The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run
https://codereview.chromium.org/2652133004/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2652133004/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:151: bool skip_dump_if_principal_mapping_not_referenced_; On 2017/02/08 22:36:21, Robert Sesek wrote: > Are these stored instead of just set directly to allow the methods to be called > before initializing breakpad? Yes, and also because they should be applied in multiple places (to g_breakpad and g_microdump). I realised also that these needed to be applied where g_breakpad is constructed. https://codereview.chromium.org/2652133004/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:925: void MicrodumpInfo::SetPrincipalMappingAddress(uintptr_t addr) { On 2017/02/08 16:25:03, Primiano Tucci wrote: > I think this should have some stronger name to explain that doing this will > automatically exclude any dump that doesn't contain the module. > How about SetSkipDumpIfPrincipalModuleNotReferenced(uintptr_t > addr_in_principal_module) Done.
Dry run: 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
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Sanitize mini- and microdumps for webview Enable the breakpad logic for sanitizing dumps in webview. Also skip dumping any thread stacks that do not reference WebView's text section. To determine the webview text section address range, we provide the address of a function that we know is part of the webview shared library (PrincipalMappingMarker). Breakpad looks up the mapping that contains that address, causing it to not generate a microdump if the IP or the stack of the crashing thread does not contain a pointer into this region. BUG=664460 ========== to ========== Sanitize mini- and microdumps for webview Enable the breakpad logic for sanitizing dumps in webview. Also skip dumping any thread stacks that do not reference WebView's text section. To determine the webview text section address range, we provide the address of a function that we know is part of the webview shared library (PrincipalMappingMarker). Breakpad looks up the mapping that contains that address, causing it to not generate a microdump if the IP or the stack of the crashing thread does not contain a pointer into this region. BUG=664460,682278 ==========
lgtm https://codereview.chromium.org/2652133004/diff/40001/android_webview/common/... File android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/2652133004/diff/40001/android_webview/common/... android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc:159: reinterpret_cast<uintptr_t>(&PrincipalMappingMarker)); I'd still probably just use EnableCrashReporter.
https://codereview.chromium.org/2652133004/diff/40001/android_webview/common/... File android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/2652133004/diff/40001/android_webview/common/... android_webview/common/crash_reporter/aw_microdump_crash_reporter.cc:159: reinterpret_cast<uintptr_t>(&PrincipalMappingMarker)); On 2017/02/09 22:21:54, Robert Sesek wrote: > I'd still probably just use EnableCrashReporter. Done. Didn't also realise when moving the call to this file that there were more non-method options now. Thanks for pointing it out.
LGTM also :)
The CQ bit was checked by tobiasjs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2652133004/#ps60001 (title: "Robert's comments.")
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": 60001, "attempt_start_ts": 1486735738503560,
"parent_rev": "4f443d0a5d9fb3dfd0f8bbbd91b9511fef7c54f8", "commit_rev":
"238463db7875fa84f4a0f9b227cbde2c6d7ef011"}
Message was sent while issue was closed.
Description was changed from ========== Sanitize mini- and microdumps for webview Enable the breakpad logic for sanitizing dumps in webview. Also skip dumping any thread stacks that do not reference WebView's text section. To determine the webview text section address range, we provide the address of a function that we know is part of the webview shared library (PrincipalMappingMarker). Breakpad looks up the mapping that contains that address, causing it to not generate a microdump if the IP or the stack of the crashing thread does not contain a pointer into this region. BUG=664460,682278 ========== to ========== Sanitize mini- and microdumps for webview Enable the breakpad logic for sanitizing dumps in webview. Also skip dumping any thread stacks that do not reference WebView's text section. To determine the webview text section address range, we provide the address of a function that we know is part of the webview shared library (PrincipalMappingMarker). Breakpad looks up the mapping that contains that address, causing it to not generate a microdump if the IP or the stack of the crashing thread does not contain a pointer into this region. BUG=664460,682278 Review-Url: https://codereview.chromium.org/2652133004 Cr-Commit-Position: refs/heads/master@{#449620} Committed: https://chromium.googlesource.com/chromium/src/+/238463db7875fa84f4a0f9b227cb... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/238463db7875fa84f4a0f9b227cb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
