Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567643003:
Send crash dumps for odd ERR_CERT_DATE_INVALID cert errors (Closed)
DescriptionSend crash dumps for odd ERR_CERT_DATE_INVALID cert errors
Safe Browsing Extended Reporting reports show a number of certificate
errors from Windows users where the certificate error is
ERR_CERT_DATE_INVALID but the certificate chain appears to have
perfectly valid dates.
To get some diagnostic data from Canary, this CL calls
DumpWithoutCrashing() when we observe such a chain. This CL is intended
to be reverted after obtaining a few days of data from Canary.
BUG=672906
Committed: https://crrev.com/822fd5f5b3ab71603d34a376928bf05323abfd0b
Cr-Commit-Position: refs/heads/master@{#437726}
Patch Set 1 #Patch Set 2 : build fixes #Patch Set 3 : More fixes #
Total comments: 9
Patch Set 4 : rsleevi, wfh comments #Patch Set 5 : Use two separate ScopedCrashKeys #Messages
Total messages: 28 (19 generated)
The CQ bit was checked by estark@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...
The CQ bit was checked by estark@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...
The CQ bit was checked by estark@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
estark@chromium.org changed reviewers: + rsleevi@chromium.org
rsleevi, PTAL? Thanks (cc wfh who confirmed that this was a reasonable thing to do on canary for a few days)
wfh@chromium.org changed reviewers: + wfh@chromium.org
https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:914: base::debug::Alias(&temp_error_status); base::debug::Alias requires you to open up the crash dump in windbg you could consider using a base::debug::ScopedCrashKey to set a crash key then it would display in the crash UI directly. https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:934: base::debug::DumpWithoutCrashing(); can you make this canary only?
LGTM % will's comments :) https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:914: base::debug::Alias(&temp_error_status); Seems like 913-914 should be moved to 932, since that's where it's first used. https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:1219: chain_context->TrustStatus.dwErrorStatus); I'd recommend you grab both dwErrorStatus and dwInfoStatus
The CQ bit was checked by estark@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...
Thanks, both. wfh: please see question inline about limiting to canary https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:914: base::debug::Alias(&temp_error_status); On 2016/12/09 21:35:53, Ryan Sleevi wrote: > Seems like 913-914 should be moved to 932, since that's where it's first used. Done. https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:914: base::debug::Alias(&temp_error_status); On 2016/12/09 21:33:05, Will Harris wrote: > base::debug::Alias requires you to open up the crash dump in windbg > > you could consider using a base::debug::ScopedCrashKey to set a crash key then > it would display in the crash UI directly. Done. https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:934: base::debug::DumpWithoutCrashing(); On 2016/12/09 21:33:05, Will Harris wrote: > can you make this canary only? Oh, sorry, I forgot to mention that -- I couldn't figure out a way to do that outside of //chrome (i.e. without using chrome::GetChannel). Am I missing something obvious? https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:1219: chain_context->TrustStatus.dwErrorStatus); On 2016/12/09 21:35:53, Ryan Sleevi wrote: > I'd recommend you grab both dwErrorStatus and dwInfoStatus Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_win.cc (right): https://codereview.chromium.org/2567643003/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_win.cc:934: base::debug::DumpWithoutCrashing(); ah yes that's right you can't depend on chrome/ from net/ in which case, just try and revert this as soon as you have the data.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2567643003/#ps80001 (title: "Use two separate ScopedCrashKeys")
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": 1481330158411380,
"parent_rev": "eda0df929f49b55c6cf63f7dd09c3bdcd871d5d9", "commit_rev":
"d6cb7ed29a17f3541a2b57a6785da64838f0da5d"}
Message was sent while issue was closed.
Description was changed from ========== Send crash dumps for odd ERR_CERT_DATE_INVALID cert errors Safe Browsing Extended Reporting reports show a number of certificate errors from Windows users where the certificate error is ERR_CERT_DATE_INVALID but the certificate chain appears to have perfectly valid dates. To get some diagnostic data from Canary, this CL calls DumpWithoutCrashing() when we observe such a chain. This CL is intended to be reverted after obtaining a few days of data from Canary. BUG=672906 ========== to ========== Send crash dumps for odd ERR_CERT_DATE_INVALID cert errors Safe Browsing Extended Reporting reports show a number of certificate errors from Windows users where the certificate error is ERR_CERT_DATE_INVALID but the certificate chain appears to have perfectly valid dates. To get some diagnostic data from Canary, this CL calls DumpWithoutCrashing() when we observe such a chain. This CL is intended to be reverted after obtaining a few days of data from Canary. BUG=672906 Review-Url: https://codereview.chromium.org/2567643003 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Send crash dumps for odd ERR_CERT_DATE_INVALID cert errors Safe Browsing Extended Reporting reports show a number of certificate errors from Windows users where the certificate error is ERR_CERT_DATE_INVALID but the certificate chain appears to have perfectly valid dates. To get some diagnostic data from Canary, this CL calls DumpWithoutCrashing() when we observe such a chain. This CL is intended to be reverted after obtaining a few days of data from Canary. BUG=672906 Review-Url: https://codereview.chromium.org/2567643003 ========== to ========== Send crash dumps for odd ERR_CERT_DATE_INVALID cert errors Safe Browsing Extended Reporting reports show a number of certificate errors from Windows users where the certificate error is ERR_CERT_DATE_INVALID but the certificate chain appears to have perfectly valid dates. To get some diagnostic data from Canary, this CL calls DumpWithoutCrashing() when we observe such a chain. This CL is intended to be reverted after obtaining a few days of data from Canary. BUG=672906 Committed: https://crrev.com/822fd5f5b3ab71603d34a376928bf05323abfd0b Cr-Commit-Position: refs/heads/master@{#437726} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/822fd5f5b3ab71603d34a376928bf05323abfd0b Cr-Commit-Position: refs/heads/master@{#437726}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2572553002/ by estark@chromium.org. The reason for reverting is: Turns out this issue is trivial to reproduce locally (I probably should have tried that first...) so diagnostic data from crash dumps isn't needed.. |
