Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1834463002:
Identify the hung thread using the Wait Chain Traversal API (Closed)
|
Created:
4 years, 9 months ago by Patrick Monette Modified:
4 years, 8 months ago CC:
chromium-reviews, manzagop (departed) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIdentify the hung thread using the Wait Chain Traversal API
The exception record will be created for the culprit thread to get
better bucketing on the crash back-end.
BUG=485152, 478209
Committed: https://crrev.com/050187d61ed4be68072eb84ff864943aaf6e5d90
Cr-Commit-Position: refs/heads/master@{#387369}
Patch Set 1 : #
Total comments: 32
Patch Set 2 : Addressed comments/questions #
Total comments: 7
Patch Set 3 : Move wait chain to base and added test + fix nits #
Total comments: 21
Patch Set 4 : Siggi comments #
Total comments: 2
Patch Set 5 : Added dcheck #
Total comments: 20
Patch Set 6 : Grt comments #
Total comments: 4
Patch Set 7 : Sign mismatch + base::string16 #Patch Set 8 : Rebase #
Total comments: 3
Patch Set 9 : Fixed channel misconception #Patch Set 10 : Fixed logic error #Patch Set 11 : Make use of copy elision #
Total comments: 2
Patch Set 12 : Fixed unit test #Patch Set 13 : BASE_EXPORT #
Messages
Total messages: 56 (20 generated)
Patchset #1 (id:1) has been deleted
pmonette@chromium.org changed reviewers: + siggi@chromium.org
Hey Siggi can you take a look? This will find the hung thread generate the exception record on it for the crash report. It will also add the wait chain to the crash keys and the deadlock state found by the WCT API.
looks nice - I have some nits, plus it'd be great to have a test for the wait chain stuff. I don't know whether this is the best location to stick the code as well? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/c... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:237: void AddCrashKey(std::vector<kasko::api::CrashKey>* crash_keys, nit: crash_keys is an out param, by convention put it last? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:273: for (size_t i = 0; i < wait_chain.size(); i++) { Some running commentary here on what you expect the key/value pairs to denote and look like would be helpful for the reader. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... File chrome/chrome_watcher/wait_chain_util_win.cc (right): https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:72: default: I seem to remember there's a way to get compile-time failures if ever the enum changes (it's clearly intended to be extensible). Offhand I don't remember how that works, but one way is to compile_assert on the value of the kMax value? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:86: std::unique_ptr<HWCT, WaitChainSessionDeleter> session_handle( nit: more readable to typedef a ScopedWaitChainSessionHandle type or something above and use here? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:99: *is_deadlock = !!is_cycle; we use the !! notion for coercing to bool?
manzagop@chromium.org changed reviewers: + manzagop@chromium.org
Nice! Some comments/ questions! https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/c... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:240: DCHECK(crash_keys); DCHECK the others as well? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:275: base::StringPrintf(L"hung-process-wait-chain-%zu", i).c_str(), Since the chain can contain up to 16 things, perhaps have 0 padding so lexicographic ordering on the keys works. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... File chrome/chrome_watcher/wait_chain_util_win.cc (right): https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:86: std::unique_ptr<HWCT, WaitChainSessionDeleter> session_handle( Nit: comment to mention it's a synchronous session? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:89: return false; Should you log the error message? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:93: WAITCHAIN_NODE_INFO nodes[WCT_MAX_NODE_COUNT]; I guess you could put it directly in the vector? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:95: if (!::GetThreadWaitChain(session_handle.get(), NULL, WCTP_GETINFO_ALL_FLAGS, We should double check this is the right flag. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:97: return false; Again, log an error message? I'm guessing we shouldn't hit the ERROR_MORE_DATA? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... File chrome/chrome_watcher/wait_chain_util_win.h (right): https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. A few questions about form: - What determines whether code should go in a component or not? Eg components/browser_watcher? - when should you use the _win suffix? - should this be in a namespace? Sidenote which may affect the answer: I'd like to use this at he time of RDV. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.h:14: // contains a deadlock.Returns true on success. nit: missing whitespace https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.h:23: // Returns a string that represent the node for a wait chain. nit: represent*s*
PTAL Another look. I'll add a test to the wait chain stuff in a following CL. About this comment "I don't know whether this is the best location to stick the code as well". Are you talking about the code in DumpHungBrowserProcess or the new wait_chain_util_win files? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/c... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:237: void AddCrashKey(std::vector<kasko::api::CrashKey>* crash_keys, On 2016/03/24 19:30:18, Sigurður Ásgeirsson wrote: > nit: crash_keys is an out param, by convention put it last? Done. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:240: DCHECK(crash_keys); On 2016/03/24 19:36:36, manzagop wrote: > DCHECK the others as well? Done. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:273: for (size_t i = 0; i < wait_chain.size(); i++) { On 2016/03/24 19:30:18, Sigurður Ásgeirsson wrote: > Some running commentary here on what you expect the key/value pairs to denote > and look like would be helpful for the reader. Done. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:275: base::StringPrintf(L"hung-process-wait-chain-%zu", i).c_str(), On 2016/03/24 19:36:36, manzagop wrote: > Since the chain can contain up to 16 things, perhaps have 0 padding so > lexicographic ordering on the keys works. Done. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... File chrome/chrome_watcher/wait_chain_util_win.cc (right): https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:72: default: On 2016/03/24 19:30:18, Sigurður Ásgeirsson wrote: > I seem to remember there's a way to get compile-time failures if ever the enum > changes (it's clearly intended to be extensible). Offhand I don't remember how > that works, but one way is to compile_assert on the value of the kMax value? I'm not sure I understand your suggestion. The enum doesn't have a kMax value. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:86: std::unique_ptr<HWCT, WaitChainSessionDeleter> session_handle( On 2016/03/24 19:30:18, Sigurður Ásgeirsson wrote: > nit: more readable to typedef a ScopedWaitChainSessionHandle type or something > above and use here? I like it. Done. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:86: std::unique_ptr<HWCT, WaitChainSessionDeleter> session_handle( On 2016/03/24 19:36:36, manzagop wrote: > Nit: comment to mention it's a synchronous session? Done. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:89: return false; On 2016/03/24 19:36:36, manzagop wrote: > Should you log the error message? Done. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:93: WAITCHAIN_NODE_INFO nodes[WCT_MAX_NODE_COUNT]; On 2016/03/24 19:36:36, manzagop wrote: > I guess you could put it directly in the vector? Done. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:95: if (!::GetThreadWaitChain(session_handle.get(), NULL, WCTP_GETINFO_ALL_FLAGS, On 2016/03/24 19:36:36, manzagop wrote: > We should double check this is the right flag. I removed it. They aren't doing anything. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:97: return false; On 2016/03/24 19:36:36, manzagop wrote: > Again, log an error message? > I'm guessing we shouldn't hit the ERROR_MORE_DATA? Probably not but logging the error message is a good idea. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:99: *is_deadlock = !!is_cycle; On 2016/03/24 19:30:18, Sigurður Ásgeirsson wrote: > we use the !! notion for coercing to bool? Hem.. yeah I don't think so. What should I then? is_deadlock = is_cycle ? true : false; ? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... File chrome/chrome_watcher/wait_chain_util_win.h (right): https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/03/24 19:36:37, manzagop wrote: > A few questions about form: > - What determines whether code should go in a component or not? Eg > components/browser_watcher? > - when should you use the _win suffix? > - should this be in a namespace? > > Sidenote which may affect the answer: I'd like to use this at he time of RDV. > > Lets not mess with components! The _win suffix isn't necessary here because the whole chrome_watcher stuff is windows specific but I like to denote windows specific files with it. And I don't think there is a namespace for the chrome/ folder so that's why I didn't put one. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.h:14: // contains a deadlock.Returns true on success. On 2016/03/24 19:36:36, manzagop wrote: > nit: missing whitespace Done. https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.h:23: // Returns a string that represent the node for a wait chain. On 2016/03/24 19:36:37, manzagop wrote: > nit: represent*s* Done.
s/PTAL/Please take
Two more questions! https://codereview.chromium.org/1834463002/diff/40001/chrome/chrome_watcher/c... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/40001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:267: if (GetThreadWaitChain(main_thread_id, &wait_chain, &is_deadlock)) { IIUC this can cross process boundaries and the chain could end with a thread in a different process? https://codereview.chromium.org/1834463002/diff/40001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:269: DCHECK(wait_chain.back().ObjectType == WctThreadType); How sure are we the last element is a thread? I guess there's always a thread that holds the lock. What about cycles, or if the thread exited with the lock? Should we search from the back instead?
I think it'd be great to write that test to assess the truth to those questions? https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... File chrome/chrome_watcher/wait_chain_util_win.cc (right): https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:89: return false; On 2016/03/24 19:36:36, manzagop wrote: > Should you log the error message? Not to be too contrary, but verbose logging is generally discouraged in Chrome. As most of the time noone is listening, debug logging is more appropriate than to bloat release code with useless log statements (is the rationale). https://codereview.chromium.org/1834463002/diff/20001/chrome/chrome_watcher/w... chrome/chrome_watcher/wait_chain_util_win.cc:99: *is_deadlock = !!is_cycle; On 2016/03/24 23:21:03, Patrick Monette wrote: > On 2016/03/24 19:30:18, Sigurður Ásgeirsson wrote: > > we use the !! notion for coercing to bool? > > Hem.. yeah I don't think so. What should I then? > > is_deadlock = is_cycle ? true : false; ? I dunno. https://codereview.chromium.org/1834463002/diff/40001/chrome/chrome_watcher/c... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/40001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:267: if (GetThreadWaitChain(main_thread_id, &wait_chain, &is_deadlock)) { On 2016/03/29 13:57:48, manzagop wrote: > IIUC this can cross process boundaries and the chain could end with a thread in > a different process? oh - interesting - this is true.
https://chromiumcodereview.appspot.com/1834463002/diff/40001/chrome/chrome_wa... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://chromiumcodereview.appspot.com/1834463002/diff/40001/chrome/chrome_wa... chrome/chrome_watcher/chrome_watcher_main.cc:245: std::wcsncpy(crash_key.name, key, kasko::api::CrashKey::kNameMaxLength); Do you need a -1 to for \0?
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Take another look please. I plan to reuse PA's code to check the process id returned by the wait chain. https://codereview.chromium.org/1834463002/diff/40001/chrome/chrome_watcher/c... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/40001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:245: std::wcsncpy(crash_key.name, key, kasko::api::CrashKey::kNameMaxLength); On 2016/03/31 20:24:45, manzagop wrote: > Do you need a -1 to for \0? I'm not sure but I've gone for the safe way and added the null-termination. https://codereview.chromium.org/1834463002/diff/40001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:267: if (GetThreadWaitChain(main_thread_id, &wait_chain, &is_deadlock)) { On 2016/03/29 16:57:32, Sigurður Ásgeirsson wrote: > On 2016/03/29 13:57:48, manzagop wrote: > > IIUC this can cross process boundaries and the chain could end with a thread > in > > a different process? > > oh - interesting - this is true. Done. https://codereview.chromium.org/1834463002/diff/40001/chrome/chrome_watcher/c... chrome/chrome_watcher/chrome_watcher_main.cc:269: DCHECK(wait_chain.back().ObjectType == WctThreadType); On 2016/03/29 13:57:48, manzagop wrote: > How sure are we the last element is a thread? > I guess there's always a thread that holds the lock. What about cycles, or if > the thread exited with the lock? > Should we search from the back instead? Done. In some cases, it's possible that the last element is not a thread.
nice! https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... File base/win/wait_chain_unittest.cc (right): https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:33: std::string switch_name) { nit: StringPiece here avoids string construction and copying. https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:93: // Helper thread to cause a deadlock by acquiring 2 mutex in a given order. nit: mutexes? https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:108: // Wait until both thread are holding their mutex before trying to acquire nit: both threads https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:113: // To unblock the deadlock, one of the thread will get terminated (via nit: thread->threads https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:195: options.inherit_handles = true; nit: use the handle inheritance vector: https://code.google.com/p/chromium/codesearch#chromium/src/base/process/launc... https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:208: if (first_process != wait_chain[i].ThreadObject.ProcessId) nit: EXPECT that the node type is as expected? maybe also EXPECT that the odd nodes are of the complementary (non-thread) type? https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:216: // Creates 2 threads that acquire their designated mutex and then try to love the quick description! https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:254: // Creates a child process that acquire a mutex and then blocks. A chain of nit: acquires https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:269: ::TerminateProcess(child_process.Handle(), 0); this looks out of place? https://codereview.chromium.org/1834463002/diff/100001/chrome/chrome_watcher/... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/100001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:275: while (wait_chain_node.ObjectType != WctThreadType) { this can underrun the vector.
Patchset #4 (id:120001) has been deleted
PTAL https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... File base/win/wait_chain_unittest.cc (right): https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:33: std::string switch_name) { On 2016/04/05 20:07:01, Sigurður Ásgeirsson wrote: > nit: StringPiece here avoids string construction and copying. Unfortunatly AppendSwitchASCII takes in a std::string. Maybe someone (not me!) will change it in the future! Changed it to StringPiece to help an eventual transition. https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:93: // Helper thread to cause a deadlock by acquiring 2 mutex in a given order. On 2016/04/05 20:07:01, Sigurður Ásgeirsson wrote: > nit: mutexes? Done. https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:108: // Wait until both thread are holding their mutex before trying to acquire On 2016/04/05 20:07:01, Sigurður Ásgeirsson wrote: > nit: both threads Done. https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:113: // To unblock the deadlock, one of the thread will get terminated (via On 2016/04/05 20:07:01, Sigurður Ásgeirsson wrote: > nit: thread->threads Done. https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:195: options.inherit_handles = true; On 2016/04/05 20:07:02, Sigurður Ásgeirsson wrote: > nit: use the handle inheritance vector: > https://code.google.com/p/chromium/codesearch#chromium/src/base/process/launc... Huh, interesting. Done. https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:208: if (first_process != wait_chain[i].ThreadObject.ProcessId) On 2016/04/05 20:07:01, Sigurður Ásgeirsson wrote: > nit: EXPECT that the node type is as expected? > maybe also EXPECT that the odd nodes are of the complementary (non-thread) type? Done. https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:216: // Creates 2 threads that acquire their designated mutex and then try to On 2016/04/05 20:07:01, Sigurður Ásgeirsson wrote: > love the quick description! :) https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:254: // Creates a child process that acquire a mutex and then blocks. A chain of On 2016/04/05 20:07:01, Sigurður Ásgeirsson wrote: > nit: acquires Done. https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:269: ::TerminateProcess(child_process.Handle(), 0); On 2016/04/05 20:07:02, Sigurður Ásgeirsson wrote: > this looks out of place? Oops! It is! https://codereview.chromium.org/1834463002/diff/100001/chrome/chrome_watcher/... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/100001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:275: while (wait_chain_node.ObjectType != WctThreadType) { On 2016/04/05 20:07:02, Sigurður Ásgeirsson wrote: > this can underrun the vector. Fixed.
lgtm - sweet! https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... File base/win/wait_chain_unittest.cc (right): https://codereview.chromium.org/1834463002/diff/100001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:33: std::string switch_name) { On 2016/04/05 22:55:31, Patrick Monette wrote: > On 2016/04/05 20:07:01, Sigurður Ásgeirsson wrote: > > nit: StringPiece here avoids string construction and copying. > > Unfortunatly AppendSwitchASCII takes in a std::string. Maybe someone (not me!) > will change it in the future! Changed it to StringPiece to help an eventual > transition. Ah, that sucks.
Looking good % your donotcommit (blocked on me)! https://codereview.chromium.org/1834463002/diff/140001/chrome/chrome_watcher/... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/140001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:278: } nit: DCHECK_NE(it, wait_chain.rend())?
https://codereview.chromium.org/1834463002/diff/140001/chrome/chrome_watcher/... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/140001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:278: } On 2016/04/06 14:58:08, manzagop wrote: > nit: DCHECK_NE(it, wait_chain.rend())? Done.
pmonette@chromium.org changed reviewers: + grt@chromium.org
grt@ PTAL at base/win.
@thakis Need owners approval for base/ build files.
pmonette@chromium.org changed reviewers: + thakis@chromium.org
thakis@ Forgot to add you as reviewer. Can you please take a look at base/ build files.
très cool https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.cc File base/win/wait_chain.cc (right): https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.cc... base/win/wait_chain.cc:20: void operator()(HWCT session_handle) { const methods https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.cc... base/win/wait_chain.cc:24: using ScopedWaitChainSessionHandle = nit: blank line before this https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.cc... base/win/wait_chain.cc:49: default: can you leave off the "default" case so that there's a compiler error in the event that new enum values are added? https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.cc... base/win/wait_chain.cc:77: default: same here? https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.h File base/win/wait_chain.h (right): https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.h#... base/win/wait_chain.h:16: using WaitChain = std::vector<WAITCHAIN_NODE_INFO>; consider WaitChainNodeInfoVector or WaitChainNodeVector or something. this makes the use of the type in chrome_watcher_main.cc more obvious. when first looking at it, it wasn't clear what the reverse iterator was iterating over. by putting "Vector" in the type name, it's a little more clear. https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.h#... base/win/wait_chain.h:18: // Get the wait chain for the |thread_id|. Also specifies if the |wait_chain| nit: "Gets the wait chain for |thread_id|." https://codereview.chromium.org/1834463002/diff/160001/chrome/chrome_watcher/... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/160001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:246: base::wcslcpy(crash_key.name, key, kasko::api::CrashKey::kNameMaxLength - 1); why "- 1" here and below? based on my read of string_util.h and crash_key.h, i think you're short-changing yourself by one character. https://codereview.chromium.org/1834463002/diff/160001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:249: crash_keys->push_back(crash_key); this is doing extra copies of the strings (once into |crash_key| then once into the vector). you can do away with one of them with: crash_keys->resize(crash_keys->size() + 1); kasko::api::CrashKey& crash_key = crash_keys->back(); base::wcslcpy(crash_key.name, ...); base::wcslcpy(crash_key.value, ...); https://codereview.chromium.org/1834463002/diff/160001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:274: base::win::WaitChain::reverse_iterator it; This is a perfect use of <algorithm> and a lambda: auto it = std::find_if(wait_chain.rbegin(), wait_chain.rend(), [](WAITCHAIN_NODE_INFO& node) { return node.ObjectType == WctThreadType; }); using the algorithms is often more efficient than working with the iterators directly since the impl can check the validity of the iterators once rather than N times. https://codereview.chromium.org/1834463002/diff/160001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:298: base::StringPrintf(L"hung-process-wait-chain-%02zu", i).c_str(), #include "base/format_macros.h" L"hung-process-wait-chain-%02" PRIuS (i knew this existed, but it took me a bloody long time to find. :-) )
PTAL https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.cc File base/win/wait_chain.cc (right): https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.cc... base/win/wait_chain.cc:20: void operator()(HWCT session_handle) { On 2016/04/12 14:31:12, grt (very slow) wrote: > const methods Done. https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.cc... base/win/wait_chain.cc:24: using ScopedWaitChainSessionHandle = On 2016/04/12 14:31:12, grt (very slow) wrote: > nit: blank line before this Done. https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.cc... base/win/wait_chain.cc:49: default: On 2016/04/12 14:31:12, grt (very slow) wrote: > can you leave off the "default" case so that there's a compiler error in the > event that new enum values are added? Done. https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.cc... base/win/wait_chain.cc:77: default: On 2016/04/12 14:31:12, grt (very slow) wrote: > same here? Done. https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.h File base/win/wait_chain.h (right): https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.h#... base/win/wait_chain.h:16: using WaitChain = std::vector<WAITCHAIN_NODE_INFO>; On 2016/04/12 14:31:12, grt (very slow) wrote: > consider WaitChainNodeInfoVector or WaitChainNodeVector or something. this makes > the use of the type in chrome_watcher_main.cc more obvious. when first looking > at it, it wasn't clear what the reverse iterator was iterating over. by putting > "Vector" in the type name, it's a little more clear. Done. https://codereview.chromium.org/1834463002/diff/160001/base/win/wait_chain.h#... base/win/wait_chain.h:18: // Get the wait chain for the |thread_id|. Also specifies if the |wait_chain| On 2016/04/12 14:31:12, grt (very slow) wrote: > nit: "Gets the wait chain for |thread_id|." Done. https://codereview.chromium.org/1834463002/diff/160001/chrome/chrome_watcher/... File chrome/chrome_watcher/chrome_watcher_main.cc (right): https://codereview.chromium.org/1834463002/diff/160001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:246: base::wcslcpy(crash_key.name, key, kasko::api::CrashKey::kNameMaxLength - 1); On 2016/04/12 14:31:12, grt (very slow) wrote: > why "- 1" here and below? based on my read of string_util.h and crash_key.h, i > think you're short-changing yourself by one character. Done. https://codereview.chromium.org/1834463002/diff/160001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:249: crash_keys->push_back(crash_key); On 2016/04/12 14:31:12, grt (very slow) wrote: > this is doing extra copies of the strings (once into |crash_key| then once into > the vector). you can do away with one of them with: > crash_keys->resize(crash_keys->size() + 1); > kasko::api::CrashKey& crash_key = crash_keys->back(); > base::wcslcpy(crash_key.name, ...); > base::wcslcpy(crash_key.value, ...); Done. https://codereview.chromium.org/1834463002/diff/160001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:274: base::win::WaitChain::reverse_iterator it; On 2016/04/12 14:31:12, grt (very slow) wrote: > This is a perfect use of <algorithm> and a lambda: > auto it = std::find_if(wait_chain.rbegin(), wait_chain.rend(), > [](WAITCHAIN_NODE_INFO& node) { > return node.ObjectType == WctThreadType; > }); > using the algorithms is often more efficient than working with the iterators > directly since the impl can check the validity of the iterators once rather than > N times. Good idea. Done. https://codereview.chromium.org/1834463002/diff/160001/chrome/chrome_watcher/... chrome/chrome_watcher/chrome_watcher_main.cc:298: base::StringPrintf(L"hung-process-wait-chain-%02zu", i).c_str(), On 2016/04/12 14:31:12, grt (very slow) wrote: > #include "base/format_macros.h" > L"hung-process-wait-chain-%02" PRIuS > (i knew this existed, but it took me a bloody long time to find. :-) ) Done.
lgtm
lgtm with comments https://codereview.chromium.org/1834463002/diff/180001/base/win/wait_chain.h File base/win/wait_chain.h (right): https://codereview.chromium.org/1834463002/diff/180001/base/win/wait_chain.h#... base/win/wait_chain.h:29: std::wstring WaitChainNodeToString(const WAITCHAIN_NODE_INFO& node); prefer string16 in new code, please https://codereview.chromium.org/1834463002/diff/180001/base/win/wait_chain_un... File base/win/wait_chain_unittest.cc (right): https://codereview.chromium.org/1834463002/diff/180001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:268: EXPECT_EQ(9, wait_chain.size()); please send a win_clang try job to find sign mismatch warnings such as this one
Thanks. I've sent a try job to win_clang and will check the CQ if it passes. https://codereview.chromium.org/1834463002/diff/180001/base/win/wait_chain.h File base/win/wait_chain.h (right): https://codereview.chromium.org/1834463002/diff/180001/base/win/wait_chain.h#... base/win/wait_chain.h:29: std::wstring WaitChainNodeToString(const WAITCHAIN_NODE_INFO& node); On 2016/04/12 19:55:17, Nico wrote: > prefer string16 in new code, please Done. https://codereview.chromium.org/1834463002/diff/180001/base/win/wait_chain_un... File base/win/wait_chain_unittest.cc (right): https://codereview.chromium.org/1834463002/diff/180001/base/win/wait_chain_un... base/win/wait_chain_unittest.cc:268: EXPECT_EQ(9, wait_chain.size()); On 2016/04/12 19:55:17, Nico wrote: > please send a win_clang try job to find sign mismatch warnings such as this one Done.
Nvm I forgot I have a do not commit. Still waiting on manzagop's CL.
One more question. https://codereview.chromium.org/1834463002/diff/220001/chrome/chrome_watcher/... File chrome/chrome_watcher/kasko_util.cc (right): https://codereview.chromium.org/1834463002/diff/220001/chrome/chrome_watcher/... chrome/chrome_watcher/kasko_util.cc:153: auto current_process = base::Process::Open(it->ThreadObject.ProcessId); Is this inside the for loop?
https://codereview.chromium.org/1834463002/diff/220001/chrome/chrome_watcher/... File chrome/chrome_watcher/kasko_util.cc (right): https://codereview.chromium.org/1834463002/diff/220001/chrome/chrome_watcher/... chrome/chrome_watcher/kasko_util.cc:153: auto current_process = base::Process::Open(it->ThreadObject.ProcessId); On 2016/04/13 20:47:36, manzagop wrote: > Is this inside the for loop? Yes. The last thread node may be an external process (excel.exe) but the second-to-last can still be a valid for capture chrome.exe instance.
https://codereview.chromium.org/1834463002/diff/220001/chrome/chrome_watcher/... File chrome/chrome_watcher/kasko_util.cc (right): https://codereview.chromium.org/1834463002/diff/220001/chrome/chrome_watcher/... chrome/chrome_watcher/kasko_util.cc:153: auto current_process = base::Process::Open(it->ThreadObject.ProcessId); On 2016/04/13 20:51:11, Patrick Monette wrote: > On 2016/04/13 20:47:36, manzagop wrote: > > Is this inside the for loop? > > Yes. The last thread node may be an external process (excel.exe) but the > second-to-last can still be a valid for capture chrome.exe instance. Wooh good catch. I've fixed it.
lgtm Can you add bug 478209 to the list of related bugs?
Description was changed from ========== Identify the hung thread using the Wait Chain Traversal API The exception record will be created for the culprit thread to get better bucketing on the crash back-end. BUG=485152 ========== to ========== Identify the hung thread using the Wait Chain Traversal API The exception record will be created for the culprit thread to get better bucketing on the crash back-end. BUG=485152, 478209 ==========
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org, thakis@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1834463002/#ps260001 (title: "Fixed logic error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1834463002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834463002/260001
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org, thakis@chromium.org, siggi@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1834463002/#ps280001 (title: "Make sure of copy elision")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1834463002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834463002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/1834463002/diff/280001/chrome/chrome_watcher/... File chrome/chrome_watcher/kasko_util.cc (right): https://codereview.chromium.org/1834463002/diff/280001/chrome/chrome_watcher/... chrome/chrome_watcher/kasko_util.cc:154: *process = std::move(current_process); #include <utility> for this
https://codereview.chromium.org/1834463002/diff/280001/chrome/chrome_watcher/... File chrome/chrome_watcher/kasko_util.cc (right): https://codereview.chromium.org/1834463002/diff/280001/chrome/chrome_watcher/... chrome/chrome_watcher/kasko_util.cc:154: *process = std::move(current_process); On 2016/04/14 13:11:45, grt wrote: > #include <utility> for this Done.
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org, thakis@chromium.org, siggi@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1834463002/#ps300001 (title: "Fixed unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1834463002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834463002/300001
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org, thakis@chromium.org, siggi@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1834463002/#ps320001 (title: "BASE_EXPORT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1834463002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834463002/320001
Message was sent while issue was closed.
Description was changed from ========== Identify the hung thread using the Wait Chain Traversal API The exception record will be created for the culprit thread to get better bucketing on the crash back-end. BUG=485152, 478209 ========== to ========== Identify the hung thread using the Wait Chain Traversal API The exception record will be created for the culprit thread to get better bucketing on the crash back-end. BUG=485152, 478209 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Identify the hung thread using the Wait Chain Traversal API The exception record will be created for the culprit thread to get better bucketing on the crash back-end. BUG=485152, 478209 ========== to ========== Identify the hung thread using the Wait Chain Traversal API The exception record will be created for the culprit thread to get better bucketing on the crash back-end. BUG=485152, 478209 Committed: https://crrev.com/050187d61ed4be68072eb84ff864943aaf6e5d90 Cr-Commit-Position: refs/heads/master@{#387369} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/050187d61ed4be68072eb84ff864943aaf6e5d90 Cr-Commit-Position: refs/heads/master@{#387369} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
