Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1512463003:
win: Get Kasko crash keys from Crashpad instead of Breakpad (Closed)
Descriptionwin: Get Kasko crash keys from Crashpad instead of Breakpad
TBR=siggi
BUG=567781, 546288, 564329
Committed: https://crrev.com/c29d37e88ecef8d0b9d9fc26c84b79ca6e1f841b
Cr-Commit-Position: refs/heads/master@{#365352}
Patch Set 1 #
Total comments: 1
Patch Set 2 : remove unnecessary initialization #
Total comments: 7
Patch Set 3 : . #Patch Set 4 : unnecessary includes #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 4
Messages
Total messages: 44 (15 generated)
Description was changed from ========== win: Get Kasko crash keys from Crashpad instead of Breakpad BUG=567827, 564288 ========== to ========== win: Get Kasko crash keys from Crashpad instead of Breakpad BUG=567781, 546288 ==========
Description was changed from ========== win: Get Kasko crash keys from Crashpad instead of Breakpad BUG=567781, 546288 ========== to ========== win: Get Kasko crash keys from Crashpad instead of Breakpad BUG=567781, 546288 ==========
scottmg@chromium.org changed reviewers: + chrisha@chromium.org, mark@chromium.org, siggi@chromium.org
I built a Release syzyasan with GYP_DEFINES=syzyasan=1 win_z7=0 chromium_win_pch=0. Can you suggest a way to test that this doing what we might hope? (manually/locally/whatever) https://codereview.chromium.org/1512463003/diff/1/components/crash/content/ap... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/1512463003/diff/1/components/crash/content/ap... components/crash/content/app/crashpad_win.cc:121: void __declspec(dllexport) __cdecl SetCrashKeyValueImpl( From breakpad_win.cc.
Description was changed from ========== win: Get Kasko crash keys from Crashpad instead of Breakpad BUG=567781, 546288 ========== to ========== win: Get Kasko crash keys from Crashpad instead of Breakpad BUG=567781, 546288, 564329 ==========
I'd just focus on a unittest of SetCrashKeyValueImpl and GetKaskoCrashKeys. Assuming those functions work as advertised then everything else is fine. We have separate end to end testing of SyzyASAN/Kasko integration, soon to be on a waterfall near you. (It's on our own waterfall, but not yet on Chrome's.) https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... File components/crash/content/app/crashpad.h (right): https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... components/crash/content/app/crashpad.h:75: std::vector<std::pair<std::string, std::string>> GetCrashKeys(); If we're poking a hole here, we could always just write directly into the final format, and avoid the multiple conversions and allocations? Define a compatible type here (lifted from crash_keys.h), ad make this simply write directly into a provided std::vector? https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... components/crash/content/app/crashpad_win.cc:121: void __declspec(dllexport) __cdecl SetCrashKeyValueImpl( Won't this be doubly defined now because of breakpad_win.cc as well? Or is that file removed from the build when Crashpad is enabled?
On 2015/12/08 21:07:57, chrisha wrote: > I'd just focus on a unittest of SetCrashKeyValueImpl and GetKaskoCrashKeys. > Assuming those functions work as advertised then everything else is fine. We > have separate end to end testing of SyzyASAN/Kasko integration, soon to be on a > waterfall near you. (It's on our own waterfall, but not yet on Chrome's.) Oh! Is there a broken bot somewhere then? It should be broken now as there's no SetCrashKeyValueImpl() in chrome.exe at the moment. I'm really more worried about the integration-level test here since we're flying blind. Is there a way for me to force a crash locally? > > https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... > File components/crash/content/app/crashpad.h (right): > > https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... > components/crash/content/app/crashpad.h:75: std::vector<std::pair<std::string, > std::string>> GetCrashKeys(); > If we're poking a hole here, we could always just write directly into the final > format, and avoid the multiple conversions and allocations? > > Define a compatible type here (lifted from crash_keys.h), > ad make this simply write directly into a provided std::vector? > > https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... > File components/crash/content/app/crashpad_win.cc (right): > > https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... > components/crash/content/app/crashpad_win.cc:121: void __declspec(dllexport) > __cdecl SetCrashKeyValueImpl( > Won't this be doubly defined now because of breakpad_win.cc as well? Or is that > file removed from the build when Crashpad is enabled?
https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... File components/crash/content/app/crashpad.h (right): https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... components/crash/content/app/crashpad.h:75: std::vector<std::pair<std::string, std::string>> GetCrashKeys(); On 2015/12/08 21:07:57, chrisha wrote: > If we're poking a hole here, we could always just write directly into the final > format, and avoid the multiple conversions and allocations? > > Define a compatible type here (lifted from crash_keys.h), > ad make this simply write directly into a provided std::vector? I didn't really like the ~20 lines of static_assert that required, and I don't want to include Kasko headers into Crashpad. So I thought it was better to do the truncation to wchar[64] in the Kasko code. But you're right that it'd be less allocations. Mark, any preference either for the components/crash/... code? https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... File components/crash/content/app/crashpad_win.cc (right): https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... components/crash/content/app/crashpad_win.cc:121: void __declspec(dllexport) __cdecl SetCrashKeyValueImpl( On 2015/12/08 21:07:57, chrisha wrote: > Won't this be doubly defined now because of breakpad_win.cc as well? Or is that > file removed from the build when Crashpad is enabled? Right, breakpad_win.cc isn't compiled into Chrome any more.
On 2015/12/08 22:16:28, scottmg wrote: > Oh! Is there a broken bot somewhere then? It should be broken now as there's no > SetCrashKeyValueImpl() in chrome.exe at the moment. I'm really more worried > about the integration-level test here since we're flying blind. We have a test on the Syzygy bot, but it only runs when we kick it off manually. We also have the win-asan continuous builder, but it does no tests. I'm adding our integration test to the win-asan continuous builder as a final sanity check. > Is there a way for me to force a crash locally? Kasko has a debug URL which will force it to take a crash. If you go to chrome://kasko/send-report it should generate and upload a crash report.
On 2015/12/10 13:57:31, chrisha wrote: > On 2015/12/08 22:16:28, scottmg wrote: > > Oh! Is there a broken bot somewhere then? It should be broken now as there's > no > > SetCrashKeyValueImpl() in chrome.exe at the moment. I'm really more worried > > about the integration-level test here since we're flying blind. > > We have a test on the Syzygy bot, but it only runs when we kick it off manually. > We also have the win-asan continuous builder, but it does no tests. I'm adding > our integration test to the win-asan continuous builder as a final sanity check. > > > Is there a way for me to force a crash locally? > > Kasko has a debug URL which will force it to take a crash. If you go to > chrome://kasko/send-report it should generate and upload a crash report. Forgot to mention, you need to also build with branding=Chrome for Kasko support to fully work!
https://codereview.chromium.org/1512463003/diff/20001/chrome/app/kasko_client.cc File chrome/app/kasko_client.cc (right): https://codereview.chromium.org/1512463003/diff/20001/chrome/app/kasko_client... chrome/app/kasko_client.cc:38: wcscpy_s(kv.name, arraysize(kv.name), OK to clear instead of truncate if arraysize(kv.name) is too small? Maybe it won’t ever be too small, I didn’t look for its declaration. https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... File components/crash/content/app/crashpad.h (right): https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... components/crash/content/app/crashpad.h:75: std::vector<std::pair<std::string, std::string>> GetCrashKeys(); Conceptually it’s more of a map<string, string>, although if you had a reason to not do that, this is fine too. https://codereview.chromium.org/1512463003/diff/20001/components/crash/conten... components/crash/content/app/crashpad.h:75: std::vector<std::pair<std::string, std::string>> GetCrashKeys(); On 2015/12/08 22:16:44, scottmg wrote: > On 2015/12/08 21:07:57, chrisha wrote: > > If we're poking a hole here, we could always just write directly into the > final > > format, and avoid the multiple conversions and allocations? > > > > Define a compatible type here (lifted from crash_keys.h), > > ad make this simply write directly into a provided std::vector? > > I didn't really like the ~20 lines of static_assert that required, and I don't > want to include Kasko headers into Crashpad. So I thought it was better to do > the truncation to wchar[64] in the Kasko code. But you're right that it'd be > less allocations. > > Mark, any preference either for the components/crash/... code? Well if you’re saying that this should be doing truncation, I’m afraid… https://msdn.microsoft.com/en-us/library/td1esda9 (the last row in the Error Conditions table under Return Value)
Patchset #3 (id:40001) has been deleted
Sorry for the delay (caught the Kindergarten Children Winter Plague last week). I switched to a Kasko-specific function since nothing else is going to need to do this, and we can remove it all at once in the future. I tried chrome://kasko/send-report, but then I couldn't find what the report id is. :/
On 2015/12/14 21:18:30, scottmg (out) wrote:
> Sorry for the delay (caught the Kindergarten Children Winter Plague last
week).
>
> I switched to a Kasko-specific function since nothing else is going to need to
> do this, and we can remove it all at once in the future.
>
> I tried chrome://kasko/send-report, but then I couldn't find what the report
id
> is. :/
Ah, I just saw your integration test CL in crrev.com/1529583002, thanks for
writing that! I get:
d:\src\cr3\src>python chrome\test\kasko\kasko_integration_test.py --chrome-dir
out\release
INFO:kasko_integration_test.py:Waiting for the watcher process to launch.
INFO:kasko_integration_test.py:Waiting for Kasko report
INFO:kasko_integration_test.py:Shutting down process with PID=8640.
INFO:kasko_integration_test.py:Waiting for process with PID=8640 to exit.
INFO:kasko_integration_test.py:Process exited with status 1.
INFO:kasko_integration_test.py:Cleaning up temp directory
[c:\users\scott\appdata\local\temp\kasko_integration_jh4f9v]
Traceback (most recent call last):
File "chrome\test\kasko\kasko_integration_test.py", line 520, in <module>
sys.exit(Main())
File "chrome\test\kasko\kasko_integration_test.py", line 504, in Main
raise Exception('No Kasko report received.')
Exception: No Kasko report received.
so let me dig into why that's happening.
Just to double check: you're using a Chrome branded, non-component, release build? On Mon, 14 Dec 2015 at 16:56 <scottmg@chromium.org> wrote: > On 2015/12/14 21:18:30, scottmg (out) wrote: > > Sorry for the delay (caught the Kindergarten Children Winter Plague last > week). > > > I switched to a Kasko-specific function since nothing else is going to > > need to > > do this, and we can remove it all at once in the future. > > > I tried chrome://kasko/send-report, but then I couldn't find what the > > report > id > > is. :/ > > Ah, I just saw your integration test CL in crrev.com/1529583002, thanks > for > writing that! I get: > > d:\src\cr3\src>python chrome\test\kasko\kasko_integration_test.py > --chrome-dir > out\release > INFO:kasko_integration_test.py:Waiting for the watcher process to launch. > INFO:kasko_integration_test.py:Waiting for Kasko report > INFO:kasko_integration_test.py:Shutting down process with PID=8640. > INFO:kasko_integration_test.py:Waiting for process with PID=8640 to exit. > INFO:kasko_integration_test.py:Process exited with status 1. > INFO:kasko_integration_test.py:Cleaning up temp directory > [c:\users\scott\appdata\local\temp\kasko_integration_jh4f9v] > Traceback (most recent call last): > File "chrome\test\kasko\kasko_integration_test.py", line 520, in > <module> > sys.exit(Main()) > File "chrome\test\kasko\kasko_integration_test.py", line 504, in Main > raise Exception('No Kasko report received.') > Exception: No Kasko report received. > > so let me dig into why that's happening. > > https://codereview.chromium.org/1512463003/ > -- 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.
And you also need to rebase to grab these 2 CLs too: https://crrev.com/6c1ee213efd0a1cb2268bf8b922cf78bb2fe93e0 https://crrev.com/84dbacb55d840c4e595fa32872347b2022433b0d On Mon, 14 Dec 2015 at 17:21 Chris Hamilton <chrisha@chromium.org> wrote: > Just to double check: you're using a Chrome branded, non-component, > release build? > > On Mon, 14 Dec 2015 at 16:56 <scottmg@chromium.org> wrote: > >> On 2015/12/14 21:18:30, scottmg (out) wrote: >> > Sorry for the delay (caught the Kindergarten Children Winter Plague last >> week). >> >> > I switched to a Kasko-specific function since nothing else is going to >> > need to >> > do this, and we can remove it all at once in the future. >> >> > I tried chrome://kasko/send-report, but then I couldn't find what the >> > report >> id >> > is. :/ >> >> Ah, I just saw your integration test CL in crrev.com/1529583002, thanks >> for >> writing that! I get: >> >> d:\src\cr3\src>python chrome\test\kasko\kasko_integration_test.py >> --chrome-dir >> out\release >> INFO:kasko_integration_test.py:Waiting for the watcher process to launch. >> INFO:kasko_integration_test.py:Waiting for Kasko report >> INFO:kasko_integration_test.py:Shutting down process with PID=8640. >> INFO:kasko_integration_test.py:Waiting for process with PID=8640 to exit. >> INFO:kasko_integration_test.py:Process exited with status 1. >> INFO:kasko_integration_test.py:Cleaning up temp directory >> [c:\users\scott\appdata\local\temp\kasko_integration_jh4f9v] >> Traceback (most recent call last): >> File "chrome\test\kasko\kasko_integration_test.py", line 520, in >> <module> >> sys.exit(Main()) >> File "chrome\test\kasko\kasko_integration_test.py", line 504, in Main >> raise Exception('No Kasko report received.') >> Exception: No Kasko report received. >> >> so let me dig into why that's happening. >> >> https://codereview.chromium.org/1512463003/ >> > -- 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.
On 2015/12/14 22:21:27, chrisha wrote:
> Just to double check: you're using a Chrome branded, non-component, release
> build?
Yes, yes, and yes. I can tell because it takes 20+ minutes to link, argh.
>
> On Mon, 14 Dec 2015 at 16:56 <mailto:scottmg@chromium.org> wrote:
>
> > On 2015/12/14 21:18:30, scottmg (out) wrote:
> > > Sorry for the delay (caught the Kindergarten Children Winter Plague last
> > week).
> >
> > > I switched to a Kasko-specific function since nothing else is going to
> > > need to
> > > do this, and we can remove it all at once in the future.
> >
> > > I tried chrome://kasko/send-report, but then I couldn't find what the
> > > report
> > id
> > > is. :/
> >
> > Ah, I just saw your integration test CL in crrev.com/1529583002, thanks
> > for
> > writing that! I get:
> >
> > d:\src\cr3\src>python chrome\test\kasko\kasko_integration_test.py
> > --chrome-dir
> > out\release
> > INFO:kasko_integration_test.py:Waiting for the watcher process to launch.
> > INFO:kasko_integration_test.py:Waiting for Kasko report
> > INFO:kasko_integration_test.py:Shutting down process with PID=8640.
> > INFO:kasko_integration_test.py:Waiting for process with PID=8640 to exit.
> > INFO:kasko_integration_test.py:Process exited with status 1.
> > INFO:kasko_integration_test.py:Cleaning up temp directory
> > [c:\users\scott\appdata\local\temp\kasko_integration_jh4f9v]
> > Traceback (most recent call last):
> > File "chrome\test\kasko\kasko_integration_test.py", line 520, in
> > <module>
> > sys.exit(Main())
> > File "chrome\test\kasko\kasko_integration_test.py", line 504, in Main
> > raise Exception('No Kasko report received.')
> > Exception: No Kasko report received.
> >
> > so let me dig into why that's happening.
> >
> > https://codereview.chromium.org/1512463003/
> >
>
> --
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
On 2015/12/14 21:56:20, scottmg (out) wrote:
> On 2015/12/14 21:18:30, scottmg (out) wrote:
> > Sorry for the delay (caught the Kindergarten Children Winter Plague last
> week).
> >
> > I switched to a Kasko-specific function since nothing else is going to need
to
> > do this, and we can remove it all at once in the future.
> >
> > I tried chrome://kasko/send-report, but then I couldn't find what the report
> id
> > is. :/
>
> Ah, I just saw your integration test CL in crrev.com/1529583002, thanks for
> writing that! I get:
>
> d:\src\cr3\src>python chrome\test\kasko\kasko_integration_test.py --chrome-dir
> out\release
> INFO:kasko_integration_test.py:Waiting for the watcher process to launch.
> INFO:kasko_integration_test.py:Waiting for Kasko report
> INFO:kasko_integration_test.py:Shutting down process with PID=8640.
> INFO:kasko_integration_test.py:Waiting for process with PID=8640 to exit.
> INFO:kasko_integration_test.py:Process exited with status 1.
> INFO:kasko_integration_test.py:Cleaning up temp directory
> [c:\users\scott\appdata\local\temp\kasko_integration_jh4f9v]
> Traceback (most recent call last):
> File "chrome\test\kasko\kasko_integration_test.py", line 520, in <module>
> sys.exit(Main())
> File "chrome\test\kasko\kasko_integration_test.py", line 504, in Main
> raise Exception('No Kasko report received.')
> Exception: No Kasko report received.
>
> so let me dig into why that's happening.
Kasko is crashing in kasko::api::SendReport(), presumably it doesn't like the
keys I'm giving it.
crash_key_count 18 unsigned int
- crash_keys,18 0x0dbcbaf8 {{name=0x0dbcbaf8 L"ptype" value=0x0dbcbb78
L"browser" }, {name=0x0dbcbbf8 L"pid" value=0x0dbcbc78 L"740" }, ...} const
kasko::api::CrashKey[18]
+ [0] {name=0x0dbcbaf8 L"ptype" value=0x0dbcbb78 L"browser" } const
kasko::api::CrashKey
+ [1] {name=0x0dbcbbf8 L"pid" value=0x0dbcbc78 L"740" } const
kasko::api::CrashKey
+ [2] {name=0x0dbcbcf8 L"ptype" value=0x0dbcbd78 L"browser" } const
kasko::api::CrashKey
+ [3] {name=0x0dbcbdf8 L"pid" value=0x0dbcbe78 L"740" } const
kasko::api::CrashKey
+ [4] {name=0x0dbcbef8 L"metrics_client_id" value=0x0dbcbf78
L"68C92049376D4860BFBEAD19393FB922" } const kasko::api::CrashKey
+ [5] {name=0x0dbcbff8 L"num-switches" value=0x0dbcc078 L"7" } const
kasko::api::CrashKey
+ [6] {name=0x0dbcc0f8 L"switch-1" value=0x0dbcc178 L"--disable-extensions"
} const kasko::api::CrashKey
+ [7] {name=0x0dbcc1f8 L"switch-2" value=0x0dbcc278 L"--disable-javascript"
} const kasko::api::CrashKey
+ [8] {name=0x0dbcc2f8 L"switch-3" value=0x0dbcc378 L"--disable-plugins"
} const kasko::api::CrashKey
+ [9] {name=0x0dbcc3f8 L"switch-4" value=0x0dbcc478
L"--user-data-dir=c:\\users\\scott\\appdata\\local\\temp\\kasko_integra" } const
kasko::api::CrashKey
+ [10] {name=0x0dbcc4f8 L"switch-5" value=0x0dbcc578 L"about:blank" } const
kasko::api::CrashKey
+ [11] {name=0x0dbcc5f8 L"num-experiments" value=0x0dbcc678 L"0" } const
kasko::api::CrashKey
+ [12] {name=0x0dbcc6f8 L"gpu-venid" value=0x0dbcc778 L"0x1002" } const
kasko::api::CrashKey
+ [13] {name=0x0dbcc7f8 L"gpu-devid" value=0x0dbcc878 L"0x6811" } const
kasko::api::CrashKey
+ [14] {name=0x0dbcc8f8 L"gpu-driver" value=0x0dbcc978 L"15.201.1151.1008"
} const kasko::api::CrashKey
+ [15] {name=0x0dbcc9f8 L"gpu-psver" value=0x0dbcca78 L"5.0" } const
kasko::api::CrashKey
+ [16] {name=0x0dbccaf8 L"gpu-vsver" value=0x0dbccb78 L"5.0" } const
kasko::api::CrashKey
+ [17] {name=0x0dbccbf8 L"num-extensions" value=0x0dbccc78 L"0" } const
kasko::api::CrashKey
I guess maybe the duplication ptype/pid?
> I guess maybe the duplication ptype/pid?
After fixing the duplication (oops, ps#6), I still get the same crash when
running the integration test:
ntdll.dll!_memcpy () Unknown
rpcrt4.dll!NdrpConformantArrayMarshall(struct _MIDL_STUB_MESSAGE *,unsigned
char *,unsigned char const *) Unknown
rpcrt4.dll!_NdrConformantArrayMarshall@12 () Unknown
rpcrt4.dll!NDR_MRSHL_POINTER_QUEUE_ELEMENT::Dispatch() Unknown
rpcrt4.dll!NDR_POINTER_QUEUE::Dispatch() Unknown
rpcrt4.dll!NdrComplexStructMarshall() Unknown
rpcrt4.dll!@NdrpClientMarshal@4 () Unknown
rpcrt4.dll!_NdrClientCall2 () Unknown
kasko.dll!KaskoClient_SendDiagnosticReport(void * IDL_handle,
__MIDL_Kasko_0005 request) Line 108 C
kasko.dll!common::rpc::InvokeRpc<unsigned char __cdecl(void
*,__MIDL_Kasko_0005),void *,__MIDL_Kasko_0005>(unsigned char(*)(void *,
__MIDL_Kasko_0005) p1, void * const & p2, const __MIDL_Kasko_0005 &) Line 77 C++
kasko.dll!kasko::Client::SendReport(const kasko::MinidumpRequest &) Line
98 C++
> kasko.dll!kasko::api::SendReport(const _EXCEPTION_POINTERS *
exception_pointers, kasko::api::MinidumpType minidump_type, const char *
protobuf, unsigned int protobuf_length, const kasko::api::CrashKey * crash_keys,
unsigned int crash_key_count, const kasko::api::MemoryRange *
user_selected_memory_ranges, unsigned int user_selected_memory_range_count) Line
122 C++
chrome.exe!ReportCrashWithProtobufAndMemoryRanges(_EXCEPTION_POINTERS * info,
const char * protobuf, unsigned int protobuf_length, const void * const *
base_addresses, const unsigned int * lengths) Line 92 C++
chrome.exe!ReportCrashWithProtobuf(_EXCEPTION_POINTERS * info, const char *
protobuf, unsigned int protobuf_length) Line 110 C++
chrome.dll!content::HandleDebugURL(const GURL & url, ui::PageTransition
transition) Line 175 C++
...
which I'm not sure about.
Keys look reasonable to me:
crash_key_count 16 unsigned int
- crash_keys,16 0x0dcb9660 {{name=0x0dcb9660 L"ptype" value=0x0dcb96e0
L"browser" }, {name=0x0dcb9760 L"pid" value=0x0dcb97e0 L"6536" }, ...} const
kasko::api::CrashKey[16]
+ [0] {name=0x0dcb9660 L"ptype" value=0x0dcb96e0 L"browser" } const
kasko::api::CrashKey
+ [1] {name=0x0dcb9760 L"pid" value=0x0dcb97e0 L"6536" } const
kasko::api::CrashKey
+ [2] {name=0x0dcb9860 L"metrics_client_id" value=0x0dcb98e0
L"68C92049376D4860BFBEAD19393FB922" } const kasko::api::CrashKey
+ [3] {name=0x0dcb9960 L"num-switches" value=0x0dcb99e0 L"7" } const
kasko::api::CrashKey
+ [4] {name=0x0dcb9a60 L"switch-1" value=0x0dcb9ae0 L"--disable-extensions"
} const kasko::api::CrashKey
+ [5] {name=0x0dcb9b60 L"switch-2" value=0x0dcb9be0 L"--disable-javascript"
} const kasko::api::CrashKey
+ [6] {name=0x0dcb9c60 L"switch-3" value=0x0dcb9ce0 L"--disable-plugins"
} const kasko::api::CrashKey
+ [7] {name=0x0dcb9d60 L"switch-4" value=0x0dcb9de0
L"--user-data-dir=c:\\users\\scott\\appdata\\local\\temp\\kasko_integra" } const
kasko::api::CrashKey
+ [8] {name=0x0dcb9e60 L"switch-5" value=0x0dcb9ee0 L"about:blank" } const
kasko::api::CrashKey
+ [9] {name=0x0dcb9f60 L"num-experiments" value=0x0dcb9fe0 L"0" } const
kasko::api::CrashKey
+ [10] {name=0x0dcba060 L"gpu-venid" value=0x0dcba0e0 L"0x1002" } const
kasko::api::CrashKey
+ [11] {name=0x0dcba160 L"gpu-devid" value=0x0dcba1e0 L"0x6811" } const
kasko::api::CrashKey
+ [12] {name=0x0dcba260 L"gpu-driver" value=0x0dcba2e0 L"15.201.1151.1008"
} const kasko::api::CrashKey
+ [13] {name=0x0dcba360 L"gpu-psver" value=0x0dcba3e0 L"5.0" } const
kasko::api::CrashKey
+ [14] {name=0x0dcba460 L"gpu-vsver" value=0x0dcba4e0 L"5.0" } const
kasko::api::CrashKey
+ [15] {name=0x0dcba560 L"num-extensions" value=0x0dcba5e0 L"0" } const
kasko::api::CrashKey
After rebasing, this works as expected with Chris's integration test: d:\src\cr3\src>python chrome\test\kasko\kasko_integration_test.py --chrome-dir out\release INFO:kasko_integration_test.py:Waiting for the watcher process to launch. INFO:kasko_integration_test.py:Waiting for Kasko report 127.0.0.1 - - [15/Dec/2015 08:49:11] "POST /crash HTTP/1.1" 200 - INFO:kasko_integration_test.py:Shutting down process with PID=2024. INFO:kasko_integration_test.py:Waiting for process with PID=2024 to exit. INFO:kasko_integration_test.py:Process exited with status -2147483645. INFO:kasko_integration_test.py:Shutting down process with PID=17892. INFO:kasko_integration_test.py:Waiting for process with PID=17892 to exit. INFO:kasko_integration_test.py:Process exited with status 1. INFO:kasko_integration_test.py:Cleaning up temp directory [c:\users\scott\appdata\local\temp\kasko_integration_mw6ftf] So, ready for another look.
LGTM from my perspective. https://codereview.chromium.org/1512463003/diff/120001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/1512463003/diff/120001/components/crash/conte... components/crash/content/app/crashpad.cc:250: for (;;) { I like while ((entry = iter.Next()) != nullptr) but I feel like we may have been down this road before and you had reasons for writing it this way, which is fine too.
https://codereview.chromium.org/1512463003/diff/120001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/1512463003/diff/120001/components/crash/conte... components/crash/content/app/crashpad.cc:250: for (;;) { On 2015/12/15 18:19:34, Mark Mentovai wrote: > I like > > while ((entry = iter.Next()) != nullptr) > > but I feel like we may have been down this road before and you had reasons for > writing it this way, which is fine too. My main reason is that I feel like there'll be a compiler or config or linter or sanitizer somewhere, eventually, that will complain about assignment in the conditional, so I just don't bother. But I don't feel too strongly about either being easier to read.
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512463003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512463003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
scottmg@chromium.org changed reviewers: + grt@chromium.org
Oops, +grt for chrome/app owners.
rubberstamp lgtm with an optional suggestion that you're welcome to disregard. https://codereview.chromium.org/1512463003/diff/120001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/1512463003/diff/120001/components/crash/conte... components/crash/content/app/crashpad.cc:250: for (;;) { On 2015/12/15 18:37:12, scottmg wrote: > On 2015/12/15 18:19:34, Mark Mentovai wrote: > > I like > > > > while ((entry = iter.Next()) != nullptr) > > > > but I feel like we may have been down this road before and you had reasons for > > writing it this way, which is fine too. > > My main reason is that I feel like there'll be a compiler or config or linter or > sanitizer somewhere, eventually, that will complain about assignment in the > conditional, so I just don't bother. > > But I don't feel too strongly about either being easier to read. ? for (const auto* entry = iter.Next(); entry; entry = iter.Next()) {
https://codereview.chromium.org/1512463003/diff/120001/components/crash/conte... File components/crash/content/app/crashpad.cc (right): https://codereview.chromium.org/1512463003/diff/120001/components/crash/conte... components/crash/content/app/crashpad.cc:250: for (;;) { On 2015/12/15 20:12:36, grt wrote: > On 2015/12/15 18:37:12, scottmg wrote: > > On 2015/12/15 18:19:34, Mark Mentovai wrote: > > > I like > > > > > > while ((entry = iter.Next()) != nullptr) > > > > > > but I feel like we may have been down this road before and you had reasons > for > > > writing it this way, which is fine too. > > > > My main reason is that I feel like there'll be a compiler or config or linter > or > > sanitizer somewhere, eventually, that will complain about assignment in the > > conditional, so I just don't bother. > > > > But I don't feel too strongly about either being easier to read. > > ? > for (const auto* entry = iter.Next(); entry; entry = iter.Next()) { But then there's unnecessary duplication of "iter.Next()". /shrug
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512463003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512463003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== win: Get Kasko crash keys from Crashpad instead of Breakpad BUG=567781, 546288, 564329 ========== to ========== win: Get Kasko crash keys from Crashpad instead of Breakpad TBR=siggi BUG=567781, 546288, 564329 ==========
Meh, I dunno who it wants. tbr siggi for DEPS modification I guess.
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512463003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512463003/120001
Message was sent while issue was closed.
Description was changed from ========== win: Get Kasko crash keys from Crashpad instead of Breakpad TBR=siggi BUG=567781, 546288, 564329 ========== to ========== win: Get Kasko crash keys from Crashpad instead of Breakpad TBR=siggi BUG=567781, 546288, 564329 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== win: Get Kasko crash keys from Crashpad instead of Breakpad TBR=siggi BUG=567781, 546288, 564329 ========== to ========== win: Get Kasko crash keys from Crashpad instead of Breakpad TBR=siggi BUG=567781, 546288, 564329 Committed: https://crrev.com/c29d37e88ecef8d0b9d9fc26c84b79ca6e1f841b Cr-Commit-Position: refs/heads/master@{#365352} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c29d37e88ecef8d0b9d9fc26c84b79ca6e1f841b Cr-Commit-Position: refs/heads/master@{#365352}
Message was sent while issue was closed.
siggi@chromium.org changed reviewers: + pmonette@chromium.org
Message was sent while issue was closed.
Hey guys, this is not lgtm - as this makes a copy of the crash keys at a point in time. This is now blocking Patrick from shipping a build to grab hang dumps. Can we please do ask I asked and keep a dupe copy of the keys for now? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
