Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
311893005:
Can now adjust the number of retries before the blacklist is disabled. (Closed)
|
Created:
6 years, 6 months ago by krstnmnlsn Modified:
6 years, 6 months ago CC:
chromium-reviews, jar+watch_chromium.org, asvitkine+watch_chromium.org, caitkp+watch_chromium.org, robertshield Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCan now adjust the number of retries before the blacklist is disabled.
Also added UMA recording of the blacklist disabled and retry succeeded events.
Deprecating the blacklist start up events 'BLACKLIST_THUNK_SETUP_FAILED' and
'BLACKLIST_INTERCEPTION_FAILED' as we are switching over to a more coarse
grained recording of the set up status (this information can be gathered from
crash reporting now anyways).
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277614
Patch Set 1 : Adding setup failure counter, UMA recording and unit tests #
Total comments: 7
Patch Set 2 : Corrected code so that it actually works as intended. Updated unit tests. #
Total comments: 26
Patch Set 3 : Made csharp's changes. #
Total comments: 25
Patch Set 4 : Mostly clarifying comments. #
Total comments: 8
Patch Set 5 : nits done. #Patch Set 6 : Merging in previous changes so that trybots can actually run hopefully. #
Total comments: 6
Patch Set 7 : Jesse's changes #
Total comments: 2
Patch Set 8 : RetryWorked->RetryAttempts.Success #
Total comments: 8
Patch Set 9 : Ilya's changes #
Total comments: 2
Patch Set 10 : UMA_HISTOGRAM_COUNTS->UMA_HISTOGRAM_COUNTS_100 #
Total comments: 12
Patch Set 11 : Robert's changes #Patch Set 12 : <windows.h> is back #
Messages
Total messages: 29 (0 generated)
This code won't increase the number of consecutive retries before disabling the blacklist, you'll need to modify the code in blacklist.cc to ensure we retry consecutively. https://codereview.chromium.org/311893005/diff/160001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.h (right): https://codereview.chromium.org/311893005/diff/160001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.h:8: #include "windows.h" Looking at code search, I think this should be "#include <windows.h>", but Robert might know better https://codereview.chromium.org/311893005/diff/160001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/311893005/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1761: + <owner>krstnmnlsn@chromium.org</owner> Add me as an owner to the two new histograms as well. https://codereview.chromium.org/311893005/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1779: + After a number of failing tries to set up the blacklist during start up we This seems a bit wordy, maybe just say "Records the number of attempts needed before the blacklist is properly setup, or disabled due to too many failures"
https://codereview.chromium.org/311893005/diff/160001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.h (right): https://codereview.chromium.org/311893005/diff/160001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.h:8: #include "windows.h" On 2014/06/05 14:55:01, csharp wrote: > Looking at code search, I think this should be "#include <windows.h>", but > Robert might know better The one example I can find in the style guide uses <windows.h> so you seem right. https://codereview.chromium.org/311893005/diff/160001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/311893005/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1761: + <owner>krstnmnlsn@chromium.org</owner> On 2014/06/05 14:55:01, csharp wrote: > Add me as an owner to the two new histograms as well. Done. https://codereview.chromium.org/311893005/diff/160001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1779: + After a number of failing tries to set up the blacklist during start up we On 2014/06/05 14:55:01, csharp wrote: > This seems a bit wordy, maybe just say "Records the number of attempts needed > before the blacklist is properly setup, or disabled due to too many failures" Done.
https://codereview.chromium.org/311893005/diff/160001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.h (right): https://codereview.chromium.org/311893005/diff/160001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.h:8: #include "windows.h" On 2014/06/09 20:20:47, krstnmnlsn wrote: > On 2014/06/05 14:55:01, csharp wrote: > > Looking at code search, I think this should be "#include <windows.h>", but > > Robert might know better > The one example I can find in the style guide uses <windows.h> so you seem > right. Yes, please. All system headers use <>
https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_unittest_win.cc (right): https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_unittest_win.cc:135: // setup failures, this should be reset to zero. nit: this -> which https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:48: // The blacklist was disabled for this run. .. because it has failed too many time (explain why the blacklist was disabled) https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:131: RecordBlacklistSetupEvent(BLACKLIST_SETUP_FAILED); Probably worth adding a comment here saying he don't need to check the attempt count because blacklist.cc has already handled that, so we know the setup has failed too often by this point https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.h (right): https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.h:18: // Helper function to grab from registry the number of times the startup has This function doesn't seem to exist anymore https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.cc (right): https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:49: // Helper to set DWORD registry values. Closes the key on failure. Closing the key only value seems an odd thing to do, I would remove it (The code that use this also seem to generally already handle closing the key on failure, but it also isn't always checking for failure). And if we remove that part, I'm not sure this code is worth still being a function https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:106: // Some part of the blacklist setup failed last time. If this has occured Could you move the code in this if statement into a helper function https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:144: if (blacklist_state == BLACKLIST_ENABLED) This should probably be join via else with the above block since only 1 can happen at a time https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:183: if (blacklist_state == BLACKLIST_SETUP_RUNNING) Please add a comment explaining why we only reset the value in this case https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:285: // failure) or disabled and abort if so. nit: disabled -> disabled, https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/te... File chrome_elf/blacklist/test/blacklist_test.cc (right): https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:219: EXPECT_TRUE(blacklist::ResetBeacon()); Nit: Please add a blank line above https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:229: base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER, A lot of tests seem to be declaring this, is it worth making it a class variable and just declaring it once in the constructor? https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:249: // Ensure that when the number of failed tries reaches the maximum allowed the nit: allowed -> allowed, https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:309: } // namespace Please add a newline to the end of the file (lint is complaining about it)
https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_unittest_win.cc (right): https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_unittest_win.cc:135: // setup failures, this should be reset to zero. On 2014/06/10 14:07:02, csharp wrote: > nit: this -> which Done. https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:48: // The blacklist was disabled for this run. On 2014/06/10 14:07:02, csharp wrote: > .. because it has failed too many time (explain why the blacklist was disabled) Done. https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:131: RecordBlacklistSetupEvent(BLACKLIST_SETUP_FAILED); On 2014/06/10 14:07:02, csharp wrote: > Probably worth adding a comment here saying he don't need to check the attempt > count because blacklist.cc has already handled that, so we know the setup has > failed too often by this point Done. https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.h (right): https://codereview.chromium.org/311893005/diff/370001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.h:18: // Helper function to grab from registry the number of times the startup has On 2014/06/10 14:07:02, csharp wrote: > This function doesn't seem to exist anymore Done. https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.cc (right): https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:49: // Helper to set DWORD registry values. Closes the key on failure. On 2014/06/10 14:07:02, csharp wrote: > Closing the key only value seems an odd thing to do, I would remove it (The code > that use this also seem to generally already handle closing the key on failure, > but it also isn't always checking for failure). > > And if we remove that part, I'm not sure this code is worth still being a > function Half done :) https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:106: // Some part of the blacklist setup failed last time. If this has occured On 2014/06/10 14:07:02, csharp wrote: > Could you move the code in this if statement into a helper function Done. https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:144: if (blacklist_state == BLACKLIST_ENABLED) On 2014/06/10 14:07:02, csharp wrote: > This should probably be join via else with the above block since only 1 can > happen at a time Done. https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:183: if (blacklist_state == BLACKLIST_SETUP_RUNNING) On 2014/06/10 14:07:02, csharp wrote: > Please add a comment explaining why we only reset the value in this case Done. https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:285: // failure) or disabled and abort if so. On 2014/06/10 14:07:02, csharp wrote: > nit: disabled -> disabled, Done. https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/te... File chrome_elf/blacklist/test/blacklist_test.cc (right): https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:219: EXPECT_TRUE(blacklist::ResetBeacon()); On 2014/06/10 14:07:02, csharp wrote: > Nit: Please add a blank line above Done. https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:229: base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER, On 2014/06/10 14:07:02, csharp wrote: > A lot of tests seem to be declaring this, is it worth making it a class variable > and just declaring it once in the constructor? Done. https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:249: // Ensure that when the number of failed tries reaches the maximum allowed the On 2014/06/10 14:07:02, csharp wrote: > nit: allowed -> allowed, Done. https://codereview.chromium.org/311893005/diff/370001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:309: } // namespace On 2014/06/10 14:07:02, csharp wrote: > Please add a newline to the end of the file (lint is complaining about it) Checked my previous commit (which also complained about this) and it seems to be just the CL.
looking pretty good https://codereview.chromium.org/311893005/diff/410001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/410001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:131: // Can set the state to disabled without checking that the maximum number of Nit: can -> We can https://codereview.chromium.org/311893005/diff/410001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:132: // attempts was exceeded because the failed state indicates this was already nit: because... -> because blacklist.cc would have already changed the start if it wasn't https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.cc (right): https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:70: bool HandlePreviousBeacon(HKEY* key, DWORD blacklist_state) { This function shouldn't be indented and can be moved into the unnamed namespace https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:86: if (result == ERROR_FILE_NOT_FOUND) { I'm not sure we need lines 88->95. It just sets the value of kBeaconAttempt, but then the next bit of code increase the value, so we don't really need to write it to the registry. It might be cleaner if we just have "attempt_count=0" for when we hit this error. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:93: ::RegCloseKey(*key); It probably is cleaner to move all the ::RegCloseKey calls to line 155, so you only need to have it once (instead of needing to remember if for all the return false statements here) https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:97: attempt_count = attempt_count + 1; ++attempt_count instead? https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:99: if (result != ERROR_SUCCESS) { I wonder if we shouldn't try to abort here. If we fail to change the key here, then we don't try to disable the blacklist on line 106, which might be required, so we might end up running too many times. If we do make that change, the only downside is we might end up under reporting how many time we try before failing, right? What are your thoughts? https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:154: if (!HandlePreviousBeacon(&key, blacklist_state)) This name seems a bit vague, I don't really have a better idea right now, but I'm not a bit fan of this. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.h (right): https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.h:26: bool HandlePreviousBeacon(); This probably doesn't need to be declared here since it is just local helper function. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.h:31: // beacon is to act as an extra failure mode protection whereby if Chrome for nit: remove "for some reason" https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/te... File chrome_elf/blacklist/test/blacklist_test.cc (right): https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:47: public: protected is generally used here instead of public. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:48: base::win::RegKey* blacklist_registry_key; Why not just have them as non-pointer members? Also, member variables should have their names end with _
https://codereview.chromium.org/311893005/diff/410001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/410001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:131: // Can set the state to disabled without checking that the maximum number of On 2014/06/11 13:34:43, csharp wrote: > Nit: can -> We can Done. https://codereview.chromium.org/311893005/diff/410001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:132: // attempts was exceeded because the failed state indicates this was already On 2014/06/11 13:34:43, csharp wrote: > nit: because... -> because blacklist.cc would have already changed the start if > it wasn't Done. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.cc (right): https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:70: bool HandlePreviousBeacon(HKEY* key, DWORD blacklist_state) { On 2014/06/11 13:34:43, csharp wrote: > This function shouldn't be indented and can be moved into the unnamed namespace Done. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:86: if (result == ERROR_FILE_NOT_FOUND) { On 2014/06/11 13:34:43, csharp wrote: > I'm not sure we need lines 88->95. It just sets the value of kBeaconAttempt, but > then the next bit of code increase the value, so we don't really need to write > it to the registry. > > It might be cleaner if we just have "attempt_count=0" for when we hit this > error. Done. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:93: ::RegCloseKey(*key); On 2014/06/11 13:34:43, csharp wrote: > It probably is cleaner to move all the ::RegCloseKey calls to line 155, so you > only need to have it once (instead of needing to remember if for all the return > false statements here) Yea I realized after that might be better. Done. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:97: attempt_count = attempt_count + 1; On 2014/06/11 13:34:43, csharp wrote: > ++attempt_count instead? Done. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:99: if (result != ERROR_SUCCESS) { On 2014/06/11 13:34:43, csharp wrote: > I wonder if we shouldn't try to abort here. If we fail to change the key here, > then we don't try to disable the blacklist on line 106, which might be required, > so we might end up running too many times. > > If we do make that change, the only downside is we might end up under reporting > how many time we try before failing, right? What are your thoughts? Yeah, I don't think taking it out even introduces any bad cases that weren't already there. On the other hand, right now I guess the worst case is that we return false, and so skip setup but claim it is running .. and end up resetting the count to zero. So I'll take it out! https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:154: if (!HandlePreviousBeacon(&key, blacklist_state)) On 2014/06/11 13:34:43, csharp wrote: > This name seems a bit vague, I don't really have a better idea right now, but > I'm not a bit fan of this. Yea I couldn't think of a reasonably short but also descriptive and not misleading name. I guess I could call it something like UpdateAttemptCount? though it does slightly more than that. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.h (right): https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.h:26: bool HandlePreviousBeacon(); On 2014/06/11 13:34:43, csharp wrote: > This probably doesn't need to be declared here since it is just local helper > function. Done. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.h:31: // beacon is to act as an extra failure mode protection whereby if Chrome for On 2014/06/11 13:34:43, csharp wrote: > nit: remove "for some reason" Done. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/te... File chrome_elf/blacklist/test/blacklist_test.cc (right): https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:47: public: On 2014/06/11 13:34:43, csharp wrote: > protected is generally used here instead of public. Done. https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:48: base::win::RegKey* blacklist_registry_key; On 2014/06/11 13:34:43, csharp wrote: > Why not just have them as non-pointer members? > > Also, member variables should have their names end with _ I think the best I can do is making override_manager a non-pointer. If blacklist_registry_key is a member either: (a) It gets default constructed, and then we don't seem to get a public assignment operator, so I can't fix it? (b) I write a constructor with an initialization list, but OverrideRegistry(HKEY_CURRENT_USER, L"beacon_test") needs to run before blacklist_registry_key's constructor and I don't think that's possible? Is it worth writing a constructor to make override_manager not a pointer?
lgtm with nits https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/te... File chrome_elf/blacklist/test/blacklist_test.cc (right): https://codereview.chromium.org/311893005/diff/410001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:48: base::win::RegKey* blacklist_registry_key; On 2014/06/11 16:35:27, krstnmnlsn wrote: > On 2014/06/11 13:34:43, csharp wrote: > > Why not just have them as non-pointer members? > > > > Also, member variables should have their names end with _ > > I think the best I can do is making override_manager a non-pointer. > If blacklist_registry_key is a member either: > (a) It gets default constructed, and then we don't seem to get a public > assignment operator, so I can't fix it? > (b) I write a constructor with an initialization list, but > OverrideRegistry(HKEY_CURRENT_USER, L"beacon_test") needs to run before > blacklist_registry_key's constructor and I don't think that's possible? > > Is it worth writing a constructor to make override_manager not a pointer? I wouldn't worry about not being able to make blacklist_registry_key a pointer. I would say adding the constructor is worth it for override_manager_ no longer being a pointer https://codereview.chromium.org/311893005/diff/450001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.cc (right): https://codereview.chromium.org/311893005/diff/450001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:59: bool HandlePreviousBeacon(HKEY* key, DWORD blacklist_state) { GenerateStateFromBeaconAndAttemptCount? https://codereview.chromium.org/311893005/diff/450001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:63: // some minimum number of times in a row we switch the state to failed and "some minimum number of "-> "blacklist::kBeaconMaxAttempts" https://codereview.chromium.org/311893005/diff/450001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:70: 0, Please use NULL instead of 0 https://codereview.chromium.org/311893005/diff/450001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:71: &attempt_count_type, Since we don't examine this value anymore can we get away with passing NULL here instead?
https://codereview.chromium.org/311893005/diff/450001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.cc (right): https://codereview.chromium.org/311893005/diff/450001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:59: bool HandlePreviousBeacon(HKEY* key, DWORD blacklist_state) { On 2014/06/11 17:07:13, csharp wrote: > GenerateStateFromBeaconAndAttemptCount? Done. https://codereview.chromium.org/311893005/diff/450001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:63: // some minimum number of times in a row we switch the state to failed and On 2014/06/11 17:07:13, csharp wrote: > "some minimum number of "-> "blacklist::kBeaconMaxAttempts" Done. https://codereview.chromium.org/311893005/diff/450001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:70: 0, On 2014/06/11 17:07:13, csharp wrote: > Please use NULL instead of 0 Leaving this until we decide to change all of them. https://codereview.chromium.org/311893005/diff/450001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:71: &attempt_count_type, On 2014/06/11 17:07:13, csharp wrote: > Since we don't examine this value anymore can we get away with passing NULL here > instead? Done.
Adding Jesse and Ilya, could one of you guys look at histograms.xml and maybe where I use UMA reporting in chrome_elf_init_win.cc
https://codereview.chromium.org/311893005/diff/540001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/311893005/diff/540001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1768: +<histogram name="Blacklist.RetryWorked"> This name doesn't seem quite right. What about something like Blacklist.RetryAttempts.Success? Or something that captures that this is counting the number of attempts. https://codereview.chromium.org/311893005/diff/540001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1782: + startup (since the blacklist runs before crash reporting is set up). This nit: remove double space ". This".
https://codereview.chromium.org/311893005/diff/540001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/311893005/diff/540001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1773: + setup. Please mention when this is logged, eg: right after a successful setup.
https://codereview.chromium.org/311893005/diff/540001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/311893005/diff/540001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1768: +<histogram name="Blacklist.RetryWorked"> On 2014/06/12 17:58:40, Jesse Doherty wrote: > This name doesn't seem quite right. What about something like > Blacklist.RetryAttempts.Success? Or something that captures that this is > counting the number of attempts. Done. https://codereview.chromium.org/311893005/diff/540001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1773: + setup. On 2014/06/12 18:18:30, Jesse Doherty wrote: > Please mention when this is logged, eg: right after a successful setup. Done. https://codereview.chromium.org/311893005/diff/540001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1782: + startup (since the blacklist runs before crash reporting is set up). This On 2014/06/12 17:58:40, Jesse Doherty wrote: > nit: remove double space ". This". Done.
LGTM with one comment for histograms. https://codereview.chromium.org/311893005/diff/550001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/550001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:152: UMA_HISTOGRAM_COUNTS("Blacklist.RetryWorked", attempt_count); Update histogram name.
https://codereview.chromium.org/311893005/diff/550001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/550001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:152: UMA_HISTOGRAM_COUNTS("Blacklist.RetryWorked", attempt_count); On 2014/06/13 17:36:50, Jesse Doherty wrote: > Update histogram name. Oh I thought I had, Thanks!
https://codereview.chromium.org/311893005/diff/570001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/570001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:46: // Depreciated. The blacklist interception code failed to execute. nit: "Depreciated" -> "Deprecated" (obsolete vs. diminished in value) https://codereview.chromium.org/311893005/diff/570001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:50: BLACKLIST_SETUP_DISABLED, Did you mean to add this to the enum in histograms.xml? https://codereview.chromium.org/311893005/diff/570001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:152: UMA_HISTOGRAM_COUNTS("Blacklist.RetryAttempts.Success", attempt_count); The upper bound for UMA_HISTOGRAM_COUNTS is 1000000. Do you think such a large upper bound is useful for this histogram? I'd guess that UMA_HISTOGRAM_COUNTS_100 might serve you better, as it'd give you a finer resolution in the mid-range. https://codereview.chromium.org/311893005/diff/570001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/311893005/diff/570001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1773: + setup. This is logged immediately after a successful setup. nit: "properly setup" -> "properly set up". (The second instance of "setup" is fine then -- "set up" is a verb, whereas "setup" is a noun.)
https://codereview.chromium.org/311893005/diff/570001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/570001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:46: // Depreciated. The blacklist interception code failed to execute. On 2014/06/13 19:42:57, Ilya Sherman wrote: > nit: "Depreciated" -> "Deprecated" (obsolete vs. diminished in value) Done. https://codereview.chromium.org/311893005/diff/570001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:50: BLACKLIST_SETUP_DISABLED, On 2014/06/13 19:42:57, Ilya Sherman wrote: > Did you mean to add this to the enum in histograms.xml? Done. https://codereview.chromium.org/311893005/diff/570001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:152: UMA_HISTOGRAM_COUNTS("Blacklist.RetryAttempts.Success", attempt_count); On 2014/06/13 19:42:57, Ilya Sherman wrote: > The upper bound for UMA_HISTOGRAM_COUNTS is 1000000. Do you think such a large > upper bound is useful for this histogram? I'd guess that > UMA_HISTOGRAM_COUNTS_100 might serve you better, as it'd give you a finer > resolution in the mid-range. I was wondering if I should do that, this number should always be very small so I'll switch it. https://codereview.chromium.org/311893005/diff/570001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/311893005/diff/570001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1773: + setup. This is logged immediately after a successful setup. On 2014/06/13 19:42:57, Ilya Sherman wrote: > nit: "properly setup" -> "properly set up". (The second instance of "setup" is > fine then -- "set up" is a verb, whereas "setup" is a noun.) Done.
LGTM % comment below. Thanks, Kristina. https://codereview.chromium.org/311893005/diff/590001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/590001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:152: UMA_HISTOGRAM_COUNTS("Blacklist.RetryAttempts.Success", attempt_count); Hmm, did you mean to update this? Your reply suggests that you do, but held off on the change -- if so, rather than the change simply got lost, could you clarify why?
https://codereview.chromium.org/311893005/diff/590001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/590001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:152: UMA_HISTOGRAM_COUNTS("Blacklist.RetryAttempts.Success", attempt_count); On 2014/06/13 20:59:44, Ilya Sherman wrote: > Hmm, did you mean to update this? Your reply suggests that you do, but held off > on the change -- if so, rather than the change simply got lost, could you > clarify why? Thanks for the catch, definitely meant to change it.
https://codereview.chromium.org/311893005/diff/610001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/610001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:43: // Deprecated. The blacklist thunk setup code failed to execute. Mention in the CL description that these are being deprecated and why it is ok to do so. https://codereview.chromium.org/311893005/diff/610001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.cc (right): https://codereview.chromium.org/311893005/diff/610001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:67: LONG result; = 0 https://codereview.chromium.org/311893005/diff/610001/chrome_elf/blacklist/te... File chrome_elf/blacklist/test/blacklist_test.cc (right): https://codereview.chromium.org/311893005/diff/610001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:53: base::win::RegKey* blacklist_registry_key_; While this case is quite simple, still prefer using scoped_ptr over a raw pointer here. It's a good habit to have, the moral equivalent of always wearing a helmet on a construction site. https://codereview.chromium.org/311893005/diff/610001/chrome_elf/chrome_elf_c... File chrome_elf/chrome_elf_constants.h (right): https://codereview.chromium.org/311893005/diff/610001/chrome_elf/chrome_elf_c... chrome_elf/chrome_elf_constants.h:34: extern const DWORD kBeaconMaxAttempts; can this be a uint32 or something so we can avoid the #include windows.h above? https://codereview.chromium.org/311893005/diff/610001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/311893005/diff/610001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1773: + up. This is logged immediately after a successful setup. nit: 1-space after a period, or keep it at 2 if you are of that type. Either or be consistent with the change in BlacklistSetup just below.
https://codereview.chromium.org/311893005/diff/610001/chrome/browser/chrome_e... File chrome/browser/chrome_elf_init_win.cc (right): https://codereview.chromium.org/311893005/diff/610001/chrome/browser/chrome_e... chrome/browser/chrome_elf_init_win.cc:43: // Deprecated. The blacklist thunk setup code failed to execute. On 2014/06/16 13:56:54, robertshield wrote: > Mention in the CL description that these are being deprecated and why it is ok > to do so. Done. https://codereview.chromium.org/311893005/diff/610001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.cc (right): https://codereview.chromium.org/311893005/diff/610001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:67: LONG result; On 2014/06/16 13:56:54, robertshield wrote: > = 0 Done. https://codereview.chromium.org/311893005/diff/610001/chrome_elf/blacklist/te... File chrome_elf/blacklist/test/blacklist_test.cc (right): https://codereview.chromium.org/311893005/diff/610001/chrome_elf/blacklist/te... chrome_elf/blacklist/test/blacklist_test.cc:53: base::win::RegKey* blacklist_registry_key_; On 2014/06/16 13:56:54, robertshield wrote: > While this case is quite simple, still prefer using scoped_ptr over a raw > pointer here. It's a good habit to have, the moral equivalent of always wearing > a helmet on a construction site. Done. I will try to remember this! https://codereview.chromium.org/311893005/diff/610001/chrome_elf/chrome_elf_c... File chrome_elf/chrome_elf_constants.h (right): https://codereview.chromium.org/311893005/diff/610001/chrome_elf/chrome_elf_c... chrome_elf/chrome_elf_constants.h:34: extern const DWORD kBeaconMaxAttempts; On 2014/06/16 13:56:54, robertshield wrote: > can this be a uint32 or something so we can avoid the #include windows.h above? I could use uint32_t but then I need to include <stdint.h>, is that preferable? https://codereview.chromium.org/311893005/diff/610001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/311893005/diff/610001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1773: + up. This is logged immediately after a successful setup. On 2014/06/16 13:56:54, robertshield wrote: > nit: 1-space after a period, or keep it at 2 if you are of that type. Either or > be consistent with the change in BlacklistSetup just below. Done.
lgtm https://codereview.chromium.org/311893005/diff/610001/chrome_elf/chrome_elf_c... File chrome_elf/chrome_elf_constants.h (right): https://codereview.chromium.org/311893005/diff/610001/chrome_elf/chrome_elf_c... chrome_elf/chrome_elf_constants.h:34: extern const DWORD kBeaconMaxAttempts; On 2014/06/16 17:37:43, krstnmnlsn wrote: > On 2014/06/16 13:56:54, robertshield wrote: > > can this be a uint32 or something so we can avoid the #include windows.h > above? > > I could use uint32_t but then I need to include <stdint.h>, is that preferable? Hrm, so in general, yes for portability and such. This said, this is code that will only ever be compiled on Windows, so you can probably leave it as is since everything #includes <windows.h> eventually anyway. I rescind my earlier suggestion.
The CQ bit was checked by krstnmnlsn@chromium.org
The CQ bit was checked by krstnmnlsn@chromium.org
https://codereview.chromium.org/311893005/diff/610001/chrome_elf/chrome_elf_c... File chrome_elf/chrome_elf_constants.h (right): https://codereview.chromium.org/311893005/diff/610001/chrome_elf/chrome_elf_c... chrome_elf/chrome_elf_constants.h:34: extern const DWORD kBeaconMaxAttempts; On 2014/06/16 18:30:38, robertshield wrote: > On 2014/06/16 17:37:43, krstnmnlsn wrote: > > On 2014/06/16 13:56:54, robertshield wrote: > > > can this be a uint32 or something so we can avoid the #include windows.h > > above? > > > > I could use uint32_t but then I need to include <stdint.h>, is that > preferable? > > Hrm, so in general, yes for portability and such. This said, this is code that > will only ever be compiled on Windows, so you can probably leave it as is since > everything #includes <windows.h> eventually anyway. > > I rescind my earlier suggestion. Alright, sounds good :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/krstnmnlsn@chromium.org/311893005/650001
Message was sent while issue was closed.
Change committed as 277614 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
