Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2169063002:
Add retry logic for FindThreadIDWithSyscall() (Closed)
DescriptionAdd retry logic for FindThreadIDWithSyscall()
Crashing thread might be in "running" state, i.e. after sys_sendmsg()
and before sys_read().
Wait for a short period in browser process, allow the crashing thread to
catch up and retry.
BUG=internal b/30003601
TEST=Manually crash the main thread in utility process and make sure
thread ID is translated.
Committed: https://crrev.com/d713d28a57220f3410cc1c9c7c5b6c8bb523b3fa
Cr-Commit-Position: refs/heads/master@{#407662}
Patch Set 1 #Patch Set 2 : Add retry logic for FindThreadIDWithSyscall() #
Total comments: 7
Patch Set 3 : Updated to PostDelayedTask(). #
Total comments: 2
Patch Set 4 : Minor change. #Patch Set 5 : Fixed ASAN #
Messages
Total messages: 24 (8 generated)
Description was changed from
==========
Add retry logic for FindThreadIDWithSyscall()
Crashing thread might be in "running" state, i.e. after sys_sendmsg()
and before sys_read().
Wait for a short period in browser process, allow the crashing thread to
catch up and retry.
BUG=internal b/30003601
TEST=Manually crash the main thread in utility process and make sure
thread ID is translated.
==========
to
==========
Add retry logic for FindThreadIDWithSyscall()
Crashing thread might be in "running" state, i.e. after sys_sendmsg()
and before sys_read().
Wait for a short period in browser process, allow the crashing thread to
catch up and retry.
BUG=internal b/30003601
TEST=Manually crash the main thread in utility process and make sure
thread ID is translated.
==========
wzhong@chromium.org changed reviewers: + bcf@chromium.org, brucedawson@chromium.org, thestig@chromium.org
I've noticed that the breakpad crash code was componentized not too long ago to this location. Who is the proper reviewer and owner?
On 2016/07/22 00:25:28, wzhong wrote: > I've noticed that the breakpad crash code was componentized not too long ago to > this location. > > Who is the proper reviewer and owner? Me, though I haven't touched it in a while so I need to jog my memory. Will get back to you on this CL tomorrow.
OK. Thanks. On Jul 21, 2016 5:50 PM, <thestig@chromium.org> wrote: > On 2016/07/22 00:25:28, wzhong wrote: > > I've noticed that the breakpad crash code was componentized not too long > ago > to > > this location. > > > > Who is the proper reviewer and owner? > > Me, though I haven't touched it in a while so I need to jog my memory. > Will get > back to you on this CL tomorrow. > > https://codereview.chromium.org/2169063002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2169063002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_handler_host_linux.cc (right): https://codereview.chromium.org/2169063002/diff/20001/components/crash/conten... components/crash/content/browser/crash_handler_host_linux.cc:295: &syscall_supported); If |syscall_supported| comes back false, this should not continue looping, because it will continue to fail. Though I imagine it should return true on most modern Linux machines. https://codereview.chromium.org/2169063002/diff/20001/components/crash/conten... components/crash/content/browser/crash_handler_host_linux.cc:299: base::PlatformThread::Sleep( It's not good to sleep on the IO (AKA IPC) thread because the browser can't process any other IPCs while waiting here. Is it possible to refactor the code to capture the current function state and retry via PostDelayedTask() to the current thread?
https://codereview.chromium.org/2169063002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_handler_host_linux.cc (right): https://codereview.chromium.org/2169063002/diff/20001/components/crash/conten... components/crash/content/browser/crash_handler_host_linux.cc:295: &syscall_supported); On 2016/07/22 23:37:36, Lei Zhang wrote: > If |syscall_supported| comes back false, this should not continue looping, > because it will continue to fail. Though I imagine it should return true on most > modern Linux machines. Acknowledged. https://codereview.chromium.org/2169063002/diff/20001/components/crash/conten... components/crash/content/browser/crash_handler_host_linux.cc:299: base::PlatformThread::Sleep( On 2016/07/22 23:37:36, Lei Zhang wrote: > It's not good to sleep on the IO (AKA IPC) thread because the browser can't > process any other IPCs while waiting here. Is it possible to refactor the code > to capture the current function state and retry via PostDelayedTask() to the > current thread? Good point. It should be doable although context needs to be passed around. How about PlatformThread::YieldCurrentThread()? Seems a little bit of yield or forced scheduling already allows the crashing thread to catch up.
https://codereview.chromium.org/2169063002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_handler_host_linux.cc (right): https://codereview.chromium.org/2169063002/diff/20001/components/crash/conten... components/crash/content/browser/crash_handler_host_linux.cc:299: base::PlatformThread::Sleep( On 2016/07/23 00:25:11, wzhong wrote: > On 2016/07/22 23:37:36, Lei Zhang wrote: > > It's not good to sleep on the IO (AKA IPC) thread because the browser can't > > process any other IPCs while waiting here. Is it possible to refactor the code > > to capture the current function state and retry via PostDelayedTask() to the > > current thread? > > Good point. It should be doable although context needs to be passed around. > > How about PlatformThread::YieldCurrentThread()? Seems a little bit of yield or > forced scheduling already allows the crashing thread to catch up. Actually I'd still prefer PostDelayedTask().
https://codereview.chromium.org/2169063002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_handler_host_linux.cc (right): https://codereview.chromium.org/2169063002/diff/20001/components/crash/conten... components/crash/content/browser/crash_handler_host_linux.cc:295: &syscall_supported); On 2016/07/23 00:25:11, wzhong wrote: > On 2016/07/22 23:37:36, Lei Zhang wrote: > > If |syscall_supported| comes back false, this should not continue looping, > > because it will continue to fail. Though I imagine it should return true on > most > > modern Linux machines. > > Acknowledged. Done. https://codereview.chromium.org/2169063002/diff/20001/components/crash/conten... components/crash/content/browser/crash_handler_host_linux.cc:299: base::PlatformThread::Sleep( On 2016/07/23 00:30:11, wzhong wrote: > On 2016/07/23 00:25:11, wzhong wrote: > > On 2016/07/22 23:37:36, Lei Zhang wrote: > > > It's not good to sleep on the IO (AKA IPC) thread because the browser can't > > > process any other IPCs while waiting here. Is it possible to refactor the > code > > > to capture the current function state and retry via PostDelayedTask() to the > > > current thread? > > > > Good point. It should be doable although context needs to be passed around. > > > > How about PlatformThread::YieldCurrentThread()? Seems a little bit of yield or > > forced scheduling already allows the crashing thread to catch up. > > Actually I'd still prefer PostDelayedTask(). Done.
lgtm https://codereview.chromium.org/2169063002/diff/40001/components/crash/conten... File components/crash/content/browser/crash_handler_host_linux.cc (right): https://codereview.chromium.org/2169063002/diff/40001/components/crash/conten... components/crash/content/browser/crash_handler_host_linux.cc:312: crashing_tid = base::FindThreadIDWithSyscall(crashing_pid, Combine with previous line?
https://codereview.chromium.org/2169063002/diff/40001/components/crash/conten... File components/crash/content/browser/crash_handler_host_linux.cc (right): https://codereview.chromium.org/2169063002/diff/40001/components/crash/conten... components/crash/content/browser/crash_handler_host_linux.cc:312: crashing_tid = base::FindThreadIDWithSyscall(crashing_pid, On 2016/07/25 18:46:57, Lei Zhang wrote: > Combine with previous line? Done.
++lgtm, if you are waiting for that.
The CQ bit was checked by wzhong@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Looks like you need to handle the ASAN cases as well. See the #if defined(ADDRESS_SANITIZER) bits.
The CQ bit was checked by wzhong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2169063002/#ps80001 (title: "Fixed ASAN")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Add retry logic for FindThreadIDWithSyscall()
Crashing thread might be in "running" state, i.e. after sys_sendmsg()
and before sys_read().
Wait for a short period in browser process, allow the crashing thread to
catch up and retry.
BUG=internal b/30003601
TEST=Manually crash the main thread in utility process and make sure
thread ID is translated.
==========
to
==========
Add retry logic for FindThreadIDWithSyscall()
Crashing thread might be in "running" state, i.e. after sys_sendmsg()
and before sys_read().
Wait for a short period in browser process, allow the crashing thread to
catch up and retry.
BUG=internal b/30003601
TEST=Manually crash the main thread in utility process and make sure
thread ID is translated.
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
Add retry logic for FindThreadIDWithSyscall()
Crashing thread might be in "running" state, i.e. after sys_sendmsg()
and before sys_read().
Wait for a short period in browser process, allow the crashing thread to
catch up and retry.
BUG=internal b/30003601
TEST=Manually crash the main thread in utility process and make sure
thread ID is translated.
==========
to
==========
Add retry logic for FindThreadIDWithSyscall()
Crashing thread might be in "running" state, i.e. after sys_sendmsg()
and before sys_read().
Wait for a short period in browser process, allow the crashing thread to
catch up and retry.
BUG=internal b/30003601
TEST=Manually crash the main thread in utility process and make sure
thread ID is translated.
Committed: https://crrev.com/d713d28a57220f3410cc1c9c7c5b6c8bb523b3fa
Cr-Commit-Position: refs/heads/master@{#407662}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d713d28a57220f3410cc1c9c7c5b6c8bb523b3fa Cr-Commit-Position: refs/heads/master@{#407662} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
