Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552753002:
Do not generate a microdump if there are no webview pointers on the stack. (Closed)
|
Created:
4 years ago by Tobias Sargeant Modified:
4 years ago CC:
chromium-reviews, jam, darin-cc_chromium.org, android-webview-reviews_chromium.org, sadrul, kalyank Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not generate a microdump if there are no webview pointers on the stack.
To determine the webview text section address range, we use the linker
defines symbols __executable_start and __etext. These are passed to
breakpad in the microdump extra info struct, causing it to not generate
a microdump if the stack of the crashing thread does not contain a
pointer into this region.
BUG=664460
Committed: https://crrev.com/19f0a50c6eab29917f459557b10cafc70428a4a2
Committed: https://crrev.com/aca68a011a9fd116972e329871f67c73ca8ca389
Cr-Original-Commit-Position: refs/heads/master@{#437003}
Cr-Commit-Position: refs/heads/master@{#437234}
Patch Set 1 #Patch Set 2 : set suppression flag #
Total comments: 12
Patch Set 3 : Address review comments #
Total comments: 2
Patch Set 4 : Address #11 comments #
Total comments: 2
Patch Set 5 : oops #Patch Set 6 : fix mipsel linker error #
Messages
Total messages: 32 (15 generated)
The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tobiasjs@chromium.org changed reviewers: + primiano@chromium.org, rsesek@chromium.org, torne@chromium.org
https://codereview.chromium.org/2552753002/diff/20001/DEPS File DEPS (right): https://codereview.chromium.org/2552753002/diff/20001/DEPS#newcode119 DEPS:119: Var('chromium_git') + '/breakpad/breakpad/src.git' + '@' + '4530732f83f9a9c61a1419184e24416766d5bbc6', If you're rolling DEPS I think normally we do that in a separate commit (unless the changes are required to keep the roll compiling), and mention the git log summary of the commits being rolled in? Probably only matters much in the case of a revert being needed though.. https://codereview.chromium.org/2552753002/diff/20001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/2552753002/diff/20001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:49: extern char __executable_start; Stick a comment on these explaining that they're defined by the linker and only their addresses are meaningful. https://codereview.chromium.org/2552753002/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.h (right): https://codereview.chromium.org/2552753002/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.h:34: extern void SetWebViewTextAddrRange(uintptr_t start, uintptr_t end); Maybe call this SetNativeCodeTextAddrRange or something? Not sure it needs to be webview specific at this layer, even if non-webview cases don't use it (yet?).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2552753002/diff/20001/DEPS File DEPS (right): https://codereview.chromium.org/2552753002/diff/20001/DEPS#newcode119 DEPS:119: Var('chromium_git') + '/breakpad/breakpad/src.git' + '@' + '4530732f83f9a9c61a1419184e24416766d5bbc6', On 2016/12/05 15:19:31, Torne wrote: > If you're rolling DEPS I think normally we do that in a separate commit (unless > the changes are required to keep the roll compiling), and mention the git log > summary of the commits being rolled in? Probably only matters much in the case > of a revert being needed though.. +1 please roll DEPS separately (unless you need to do it atomically). Also use roll_dep.py in depot_tools to produce a nicely formatted CL message. https://codereview.chromium.org/2552753002/diff/20001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/2552753002/diff/20001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:49: extern char __executable_start; Are these CXA mangled or should they be extern C? https://codereview.chromium.org/2552753002/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2552753002/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:967: } Incorrect indent. https://codereview.chromium.org/2552753002/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.h (right): https://codereview.chromium.org/2552753002/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.h:34: extern void SetWebViewTextAddrRange(uintptr_t start, uintptr_t end); Please comment this as well, so that it's clear why this is necessary.
https://codereview.chromium.org/2552753002/diff/20001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/2552753002/diff/20001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:49: extern char __executable_start; On 2016/12/05 15:19:31, Torne wrote: > Stick a comment on these explaining that they're defined by the linker and only > their addresses are meaningful. Done. https://codereview.chromium.org/2552753002/diff/20001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:49: extern char __executable_start; On 2016/12/05 16:19:18, Robert Sesek wrote: > Are these CXA mangled or should they be extern C? They should be extern "C". (Done.) https://codereview.chromium.org/2552753002/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2552753002/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:967: } On 2016/12/05 16:19:18, Robert Sesek wrote: > Incorrect indent. Done. https://codereview.chromium.org/2552753002/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.h (right): https://codereview.chromium.org/2552753002/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.h:34: extern void SetWebViewTextAddrRange(uintptr_t start, uintptr_t end); On 2016/12/05 15:19:32, Torne wrote: > Maybe call this SetNativeCodeTextAddrRange or something? Not sure it needs to be > webview specific at this layer, even if non-webview cases don't use it (yet?). Done. https://codereview.chromium.org/2552753002/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.h:34: extern void SetWebViewTextAddrRange(uintptr_t start, uintptr_t end); On 2016/12/05 16:19:18, Robert Sesek wrote: > Please comment this as well, so that it's clear why this is necessary. Edit.
LGTM
LGTM with a micro comment. Also I think you want to #if defined(COMPONENT_BUILD) to make local debugging less random. All this won't work reliably in a component build I think . Out of curiosity I did an internal code search to see in how many places we use __executable_start (go/reszv) today. The only instances I found are: 1. The gold linker (well this is expected) 2. elfutils (ditto) 3. emacs* I am sure I don't need to add anything more at this point. May be the force be with you my brave soldier. *https://github.com/Wilfred/remacs/blob/f0eb70d8935be90f7c03e187c12d9b60e7214cc6/src/emacs.c#L1616 https://codereview.chromium.org/2552753002/diff/40001/android_webview/lib/mai... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/2552753002/diff/40001/android_webview/lib/mai... android_webview/lib/main/aw_main_delegate.cc:222: crash_reporter::SetWebViewTextAddrRange((uintptr_t)&__executable_start, nit: reinterpret_cast<uintptr_t>(&foo) https://codereview.chromium.org/2552753002/diff/40001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/2552753002/diff/40001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:114: microdump_process_type_(nullptr), ahhh haaa! (good catch)
On 2016/12/06 18:18:13, Primiano Tucci wrote: > LGTM with a micro comment. > Also I think you want to #if defined(COMPONENT_BUILD) to make local debugging > less random. All this won't work reliably in a component build I think . As you pointed out offline, you added that check and was precisely around the other comment I added. awkward. Ignore my comment, it's all good.
LGTM except for one comment that I think means it doesn't compile ;) https://codereview.chromium.org/2552753002/diff/60001/android_webview/crash_r... File android_webview/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/2552753002/diff/60001/android_webview/crash_r... android_webview/crash_reporter/aw_microdump_crash_reporter.cc:158: breakpad::SetWebViewTextAddrRange(start, end); I think you missed updating this function call?
https://codereview.chromium.org/2552753002/diff/60001/android_webview/crash_r... File android_webview/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/2552753002/diff/60001/android_webview/crash_r... android_webview/crash_reporter/aw_microdump_crash_reporter.cc:158: breakpad::SetWebViewTextAddrRange(start, end); On 2016/12/07 15:24:54, Torne wrote: > I think you missed updating this function call? Done.
The CQ bit was checked by tobiasjs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, rsesek@chromium.org, torne@chromium.org Link to the patchset: https://codereview.chromium.org/2552753002/#ps80001 (title: "oops")
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": 80001, "attempt_start_ts": 1481129126474810,
"parent_rev": "4eefd32db39e90abf64efc97f1850e89a7ec2332", "commit_rev":
"99061434f9d41be8566157d1a82503c035a817e5"}
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
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 use the linker defines symbols __executable_start and __etext. These are passed to breakpad in the microdump extra info struct, causing it to not generate a microdump if the stack of the crashing thread does not contain a pointer into this region. BUG=664460 ========== to ========== Do not generate a microdump if there are no webview pointers on the stack. To determine the webview text section address range, we use the linker defines symbols __executable_start and __etext. These are passed to breakpad in the microdump extra info struct, causing it to not generate a microdump if the stack of the crashing thread does not contain a pointer into this region. BUG=664460 Committed: https://crrev.com/19f0a50c6eab29917f459557b10cafc70428a4a2 Cr-Commit-Position: refs/heads/master@{#437003} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/19f0a50c6eab29917f459557b10cafc70428a4a2 Cr-Commit-Position: refs/heads/master@{#437003}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2557143002/ by yolandyan@chromium.org. The reason for reverting is: Failed to compile on Android MIPS Builder https://build.chromium.org/p/chromium.android/builders/Android%20MIPS%20Build....
Message was sent while issue was closed.
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 use the linker defines symbols __executable_start and __etext. These are passed to breakpad in the microdump extra info struct, causing it to not generate a microdump if the stack of the crashing thread does not contain a pointer into this region. BUG=664460 Committed: https://crrev.com/19f0a50c6eab29917f459557b10cafc70428a4a2 Cr-Commit-Position: refs/heads/master@{#437003} ========== to ========== Do not generate a microdump if there are no webview pointers on the stack. To determine the webview text section address range, we use the linker defines symbols __executable_start and __etext. These are passed to breakpad in the microdump extra info struct, causing it to not generate a microdump if the stack of the crashing thread does not contain a pointer into this region. BUG=664460 Committed: https://crrev.com/19f0a50c6eab29917f459557b10cafc70428a4a2 Cr-Commit-Position: refs/heads/master@{#437003} ==========
The CQ bit was checked by tobiasjs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, rsesek@chromium.org, torne@chromium.org Link to the patchset: https://codereview.chromium.org/2552753002/#ps100001 (title: "fix mipsel linker error")
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": 100001, "attempt_start_ts": 1481199743905120,
"parent_rev": "7cc75e67dec999be368c4fab476d0f3909136264", "commit_rev":
"1b8cd35eca98b13d2554778505d4d56a96a2dcd1"}
Message was sent while issue was closed.
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 use the linker defines symbols __executable_start and __etext. These are passed to breakpad in the microdump extra info struct, causing it to not generate a microdump if the stack of the crashing thread does not contain a pointer into this region. BUG=664460 Committed: https://crrev.com/19f0a50c6eab29917f459557b10cafc70428a4a2 Cr-Commit-Position: refs/heads/master@{#437003} ========== to ========== Do not generate a microdump if there are no webview pointers on the stack. To determine the webview text section address range, we use the linker defines symbols __executable_start and __etext. These are passed to breakpad in the microdump extra info struct, causing it to not generate a microdump if the stack of the crashing thread does not contain a pointer into this region. BUG=664460 Committed: https://crrev.com/19f0a50c6eab29917f459557b10cafc70428a4a2 Cr-Commit-Position: refs/heads/master@{#437003} ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
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 use the linker defines symbols __executable_start and __etext. These are passed to breakpad in the microdump extra info struct, causing it to not generate a microdump if the stack of the crashing thread does not contain a pointer into this region. BUG=664460 Committed: https://crrev.com/19f0a50c6eab29917f459557b10cafc70428a4a2 Cr-Commit-Position: refs/heads/master@{#437003} ========== to ========== Do not generate a microdump if there are no webview pointers on the stack. To determine the webview text section address range, we use the linker defines symbols __executable_start and __etext. These are passed to breakpad in the microdump extra info struct, causing it to not generate a microdump if the stack of the crashing thread does not contain a pointer into this region. BUG=664460 Committed: https://crrev.com/19f0a50c6eab29917f459557b10cafc70428a4a2 Committed: https://crrev.com/aca68a011a9fd116972e329871f67c73ca8ca389 Cr-Original-Commit-Position: refs/heads/master@{#437003} Cr-Commit-Position: refs/heads/master@{#437234} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/aca68a011a9fd116972e329871f67c73ca8ca389 Cr-Commit-Position: refs/heads/master@{#437234}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2565353004/ by tobiasjs@chromium.org. The reason for reverting is: Revert in preparation for modifying breakpad to infer the interest region from mappings rather than being explicitly informed.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
