Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8508060:
Add functionality to the GCAPI to detect when Chrome was last run. (Closed)
|
Created:
9 years, 1 month ago by robertshield Modified:
6 years, 3 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd functionality to the GCAPI to detect when Chrome was last run.
Also fix a buffer overrun while I'm here.
Also, fix up the export definitions to use the more-sane .def file approach.
BUG=103909, 88622
TEST=Invoke the GoogleChromeDaysSinceLastRun() export on GCAPI.dll, observe that it reports the number of days since Chrome was last run.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109919
Patch Set 1 #
Total comments: 27
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 6
Patch Set 4 : '' #
Total comments: 8
Patch Set 5 : '' #
Total comments: 2
Messages
Total messages: 15 (1 generated)
Go nuts, but please keep in mind that while there are several unkosher things about the existing implementation, I have a specific non-goal of rewriting the whole thing :-)
lgtm
Nice catch on the buffer overrun! LG overall. http://codereview.chromium.org/8508060/diff/1/chrome/chrome_installer.gypi File chrome/chrome_installer.gypi (right): http://codereview.chromium.org/8508060/diff/1/chrome/chrome_installer.gypi#ne... chrome/chrome_installer.gypi:11: ['OS=="win"', { now that this relies on base, gcapi_dll can only be shipped when component=="static_library". i can see that it may be useful to be able to work on gcapi in a component build, but we shouldn't ever make a shipping binary that way. how about adding something like 'component=="static_library"' to the conditions? http://codereview.chromium.org/8508060/diff/1/chrome/chrome_installer.gypi#ne... chrome/chrome_installer.gypi:33: 'installer_util', why doesn't this also depend on base like gcapi_dll does? http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc File chrome/installer/gcapi/gcapi.cc (right): http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... chrome/installer/gcapi/gcapi.cc:5: // NOTE: This code is a legacy utility API for partners to checking whether checking -> check http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... chrome/installer/gcapi/gcapi.cc:472: #pragma comment(linker, "/EXPORT:LaunchGoogleChromeWithDimensions=_LaunchGoogleChromeWithDimensions@24,PRIVATE") LaunchGoogleChromeWithDimensions -> GoogleChromeDaysSinceLastRun http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... chrome/installer/gcapi/gcapi.cc:480: HKEY hives[] = {HKEY_LOCAL_MACHINE, HKEY_CURRENT_USER}; Chrome doesn't even try to write to HKLM since it doesn't run elevated; see http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/installer/util.... http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... chrome/installer/gcapi/gcapi.cc:493: int days_elapsed = difference.InDays(); include <algorithm> and replace these four lines with: days_since_last_run = std::min(difference.InDays(), days_since_last_run); ? http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... File chrome/installer/gcapi/gcapi_last_run_test.cc (right): http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:30: public: the pattern i've seen most often is for all of this to be protected rather than public. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:35: override_manager_.OverrideRegistry(HKEY_LOCAL_MACHINE, hklm_override); seems simpler to me to move these two calls to OverrideRegistry into SetUp() and then replace all of TearDown() with a call to RemoveAllOverrides(). this also guarantees that any test funcs added in the future don't accidentally leave extra state behind. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:44: override_manager_.RemoveAllOverrides(); remove this (well, replace it with a delete) if you follow my advice about new/delete below. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:71: return (client_state.Valid() && "client_state.Valid() &&" serves no purpose, remove it http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:77: static registry_util::RegistryOverrideManager override_manager_; make this a pointer and new/delete in SUTC and TDTC so that there are no static ctors/dtors of non-POD types. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:114: } add tests for bad data in the registry: - the value isn't a number ("hi, mom!") - the value is out of range of a signed int in both directions
Thanks for your comments, please take another look. Also I can't take credit for spotting the overrun, that goes to Lisa from recurity-labs, the helpful reporter of 88622. Given how this is used, it's not something that can really impact users, but still very much worth fixing. http://codereview.chromium.org/8508060/diff/1/chrome/chrome_installer.gypi File chrome/chrome_installer.gypi (right): http://codereview.chromium.org/8508060/diff/1/chrome/chrome_installer.gypi#ne... chrome/chrome_installer.gypi:11: ['OS=="win"', { On 2011/11/11 17:01:01, grt wrote: > now that this relies on base, gcapi_dll can only be shipped when > component=="static_library". i can see that it may be useful to be able to work > on gcapi in a component build, but we shouldn't ever make a shipping binary that > way. how about adding something like 'component=="static_library"' to the > conditions? Meaning that gcapi_dll wouldn't be present in the component build? We don't do that for any of the other shipping components do we? Ideally I'd like a way to enforce that gcapi_dll.dll always statically linked base et al, but I don't know of a way of doing that beyond tracking all the things component=='shared_library' modifies and adding back their defaults here, which isn't maintainable. http://codereview.chromium.org/8508060/diff/1/chrome/chrome_installer.gypi#ne... chrome/chrome_installer.gypi:33: 'installer_util', On 2011/11/11 17:01:01, grt wrote: > why doesn't this also depend on base like gcapi_dll does? It does, I just forgot to add it. It likely builds because it picks it up from installer_util (at a guess). Added the dependency explicitly. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc File chrome/installer/gcapi/gcapi.cc (right): http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... chrome/installer/gcapi/gcapi.cc:5: // NOTE: This code is a legacy utility API for partners to checking whether On 2011/11/11 17:01:01, grt wrote: > checking -> check Done. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... chrome/installer/gcapi/gcapi.cc:472: #pragma comment(linker, "/EXPORT:LaunchGoogleChromeWithDimensions=_LaunchGoogleChromeWithDimensions@24,PRIVATE") On 2011/11/11 17:01:01, grt wrote: > LaunchGoogleChromeWithDimensions -> GoogleChromeDaysSinceLastRun heh, done http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... chrome/installer/gcapi/gcapi.cc:480: HKEY hives[] = {HKEY_LOCAL_MACHINE, HKEY_CURRENT_USER}; On 2011/11/11 17:01:01, grt wrote: > Chrome doesn't even try to write to HKLM since it doesn't run elevated; see > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/installer/util.... so, as discussed modified this to assume that one day Chrome will write to HKLM under ClientStateMedium, Chrome side change coming separately. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... chrome/installer/gcapi/gcapi.cc:493: int days_elapsed = difference.InDays(); On 2011/11/11 17:01:01, grt wrote: > include <algorithm> and replace these four lines with: > days_since_last_run = std::min(difference.InDays(), days_since_last_run); > ? Done. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... File chrome/installer/gcapi/gcapi_last_run_test.cc (right): http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:30: public: On 2011/11/11 17:01:01, grt wrote: > the pattern i've seen most often is for all of this to be protected rather than > public. Done. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:35: override_manager_.OverrideRegistry(HKEY_LOCAL_MACHINE, hklm_override); On 2011/11/11 17:01:01, grt wrote: > seems simpler to me to move these two calls to OverrideRegistry into SetUp() and > then replace all of TearDown() with a call to RemoveAllOverrides(). this also > guarantees that any test funcs added in the future don't accidentally leave > extra state behind. Done. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:44: override_manager_.RemoveAllOverrides(); On 2011/11/11 17:01:01, grt wrote: > remove this (well, replace it with a delete) if you follow my advice about > new/delete below. removed. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:71: return (client_state.Valid() && On 2011/11/11 17:01:01, grt wrote: > "client_state.Valid() &&" serves no purpose, remove it it avoided a teeny bit of extra work, but ok. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:77: static registry_util::RegistryOverrideManager override_manager_; On 2011/11/11 17:01:01, grt wrote: > make this a pointer and new/delete in SUTC and TDTC so that there are no static > ctors/dtors of non-POD types. Made it non-static instead. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:114: } On 2011/11/11 17:01:01, grt wrote: > add tests for bad data in the registry: > - the value isn't a number ("hi, mom!") Done. > - the value is out of range of a signed int in both directions I use TimeDelta which truncates from int64 -> int. When out of range it will return a totally bogus ToDays value that is hard to guard against, so I don't. I do (now) avoid returning negative numbers other than -1 though and test for that. I feel this is good enough as a contract for the method being tested, what say you?
Oh, and I also cleaned up the DLL export definitions a bit, since they were overly verbose and in one case just wrong (assigning an ordinal value of 0). On 2011/11/12 05:25:38, robertshield wrote: > Thanks for your comments, please take another look. > > Also I can't take credit for spotting the overrun, that goes to Lisa from > recurity-labs, the helpful reporter of 88622. Given how this is used, it's not > something that can really impact users, but still very much worth fixing. > > http://codereview.chromium.org/8508060/diff/1/chrome/chrome_installer.gypi > File chrome/chrome_installer.gypi (right): > > http://codereview.chromium.org/8508060/diff/1/chrome/chrome_installer.gypi#ne... > chrome/chrome_installer.gypi:11: ['OS=="win"', { > On 2011/11/11 17:01:01, grt wrote: > > now that this relies on base, gcapi_dll can only be shipped when > > component=="static_library". i can see that it may be useful to be able to > work > > on gcapi in a component build, but we shouldn't ever make a shipping binary > that > > way. how about adding something like 'component=="static_library"' to the > > conditions? > > Meaning that gcapi_dll wouldn't be present in the component build? We don't do > that for any of the other shipping components do we? > > Ideally I'd like a way to enforce that gcapi_dll.dll always statically linked > base et al, but I don't know of a way of doing that beyond tracking all the > things component=='shared_library' modifies and adding back their defaults here, > which isn't maintainable. > > http://codereview.chromium.org/8508060/diff/1/chrome/chrome_installer.gypi#ne... > chrome/chrome_installer.gypi:33: 'installer_util', > On 2011/11/11 17:01:01, grt wrote: > > why doesn't this also depend on base like gcapi_dll does? > > It does, I just forgot to add it. It likely builds because it picks it up from > installer_util (at a guess). Added the dependency explicitly. > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc > File chrome/installer/gcapi/gcapi.cc (right): > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... > chrome/installer/gcapi/gcapi.cc:5: // NOTE: This code is a legacy utility API > for partners to checking whether > On 2011/11/11 17:01:01, grt wrote: > > checking -> check > > Done. > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... > chrome/installer/gcapi/gcapi.cc:472: #pragma comment(linker, > "/EXPORT:LaunchGoogleChromeWithDimensions=_LaunchGoogleChromeWithDimensions@24,PRIVATE") > On 2011/11/11 17:01:01, grt wrote: > > LaunchGoogleChromeWithDimensions -> GoogleChromeDaysSinceLastRun > > heh, done > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... > chrome/installer/gcapi/gcapi.cc:480: HKEY hives[] = {HKEY_LOCAL_MACHINE, > HKEY_CURRENT_USER}; > On 2011/11/11 17:01:01, grt wrote: > > Chrome doesn't even try to write to HKLM since it doesn't run elevated; see > > > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/installer/util.... > > so, as discussed modified this to assume that one day Chrome will write to HKLM > under ClientStateMedium, Chrome side change coming separately. > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi.cc... > chrome/installer/gcapi/gcapi.cc:493: int days_elapsed = difference.InDays(); > On 2011/11/11 17:01:01, grt wrote: > > include <algorithm> and replace these four lines with: > > days_since_last_run = std::min(difference.InDays(), days_since_last_run); > > ? > > Done. > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... > File chrome/installer/gcapi/gcapi_last_run_test.cc (right): > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... > chrome/installer/gcapi/gcapi_last_run_test.cc:30: public: > On 2011/11/11 17:01:01, grt wrote: > > the pattern i've seen most often is for all of this to be protected rather > than > > public. > > Done. > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... > chrome/installer/gcapi/gcapi_last_run_test.cc:35: > override_manager_.OverrideRegistry(HKEY_LOCAL_MACHINE, hklm_override); > On 2011/11/11 17:01:01, grt wrote: > > seems simpler to me to move these two calls to OverrideRegistry into SetUp() > and > > then replace all of TearDown() with a call to RemoveAllOverrides(). this also > > guarantees that any test funcs added in the future don't accidentally leave > > extra state behind. > > Done. > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... > chrome/installer/gcapi/gcapi_last_run_test.cc:44: > override_manager_.RemoveAllOverrides(); > On 2011/11/11 17:01:01, grt wrote: > > remove this (well, replace it with a delete) if you follow my advice about > > new/delete below. > > removed. > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... > chrome/installer/gcapi/gcapi_last_run_test.cc:71: return (client_state.Valid() > && > On 2011/11/11 17:01:01, grt wrote: > > "client_state.Valid() &&" serves no purpose, remove it > > it avoided a teeny bit of extra work, but ok. > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... > chrome/installer/gcapi/gcapi_last_run_test.cc:77: static > registry_util::RegistryOverrideManager override_manager_; > On 2011/11/11 17:01:01, grt wrote: > > make this a pointer and new/delete in SUTC and TDTC so that there are no > static > > ctors/dtors of non-POD types. > > Made it non-static instead. > > http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... > chrome/installer/gcapi/gcapi_last_run_test.cc:114: } > On 2011/11/11 17:01:01, grt wrote: > > add tests for bad data in the registry: > > - the value isn't a number ("hi, mom!") > Done. > > - the value is out of range of a signed int in both directions > > I use TimeDelta which truncates from int64 -> int. When out of range it will > return a totally bogus ToDays value that is hard to guard against, so I don't. I > do (now) avoid returning negative numbers other than -1 though and test for > that. I feel this is good enough as a contract for the method being tested, what > say you?
Apologies in advance for the yak hair. http://codereview.chromium.org/8508060/diff/1/chrome/chrome_installer.gypi File chrome/chrome_installer.gypi (right): http://codereview.chromium.org/8508060/diff/1/chrome/chrome_installer.gypi#ne... chrome/chrome_installer.gypi:11: ['OS=="win"', { On 2011/11/12 05:25:38, robertshield wrote: > On 2011/11/11 17:01:01, grt wrote: > > now that this relies on base, gcapi_dll can only be shipped when > > component=="static_library". i can see that it may be useful to be able to > work > > on gcapi in a component build, but we shouldn't ever make a shipping binary > that > > way. how about adding something like 'component=="static_library"' to the > > conditions? > > Meaning that gcapi_dll wouldn't be present in the component build? We don't do > that for any of the other shipping components do we? > > Ideally I'd like a way to enforce that gcapi_dll.dll always statically linked > base et al, but I don't know of a way of doing that beyond tracking all the > things component=='shared_library' modifies and adding back their defaults here, > which isn't maintainable. Nod. I agree that it would be strange to special-case this (since other shipping things aren't disabled in component builds). I was a bit concerned about this being "shipped" from someone's dev checkout, but hopefully this won't happen. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... File chrome/installer/gcapi/gcapi_last_run_test.cc (right): http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:71: return (client_state.Valid() && On 2011/11/12 05:25:38, robertshield wrote: > On 2011/11/11 17:01:01, grt wrote: > > "client_state.Valid() &&" serves no purpose, remove it > > it avoided a teeny bit of extra work, but ok. it added a teeny bit of extra work for the common case (where the key is valid) in exchange for avoiding a teeny bit of extra work in the uncommon case. http://codereview.chromium.org/8508060/diff/1/chrome/installer/gcapi/gcapi_la... chrome/installer/gcapi/gcapi_last_run_test.cc:114: } On 2011/11/12 05:25:38, robertshield wrote: > On 2011/11/11 17:01:01, grt wrote: > > add tests for bad data in the registry: > > - the value isn't a number ("hi, mom!") > Done. > > - the value is out of range of a signed int in both directions > > I use TimeDelta which truncates from int64 -> int. When out of range it will > return a totally bogus ToDays value that is hard to guard against, so I don't. I > do (now) avoid returning negative numbers other than -1 though and test for > that. I feel this is good enough as a contract for the method being tested, what > say you? looks good. http://codereview.chromium.org/8508060/diff/10001/chrome/installer/gcapi/gcap... File chrome/installer/gcapi/gcapi.cc (right): http://codereview.chromium.org/8508060/diff/10001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:489: RegKey client_state(reg_data[i].hive, reg_data[i].path, KEY_QUERY_VALUE); i think this is prone to flakiness since it doesn't check for installation of the product (install system-level chrome, run it once, uninstall it, install user-level chrome. this will give you the no-longer-present system-level chrome's lastrun until Omaha cleans it up). to be correct, it should look in the Clients key for a "pv" value. only if that is present should it look in ClientStateMedium/ClientState. http://codereview.chromium.org/8508060/diff/10001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:502: if (new_days_since_last_run >= 0) { heh. now that you need this check anyway, i'd get rid of std::min and <algorithm> and do: if (new_days_since_last_run >= 0 && new_days_since_last_run < days_since_last_run) days_since_last_run = new_days_since_last_run; http://codereview.chromium.org/8508060/diff/10001/chrome/installer/gcapi/gcap... File chrome/installer/gcapi/gcapi_last_run_test.cc (right): http://codereview.chromium.org/8508060/diff/10001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi_last_run_test.cc:60: void TearDown() { remove this since override_manager_'s dtor will do everything you want.
Thanks, PTAL. http://codereview.chromium.org/8508060/diff/10001/chrome/installer/gcapi/gcap... File chrome/installer/gcapi/gcapi.cc (right): http://codereview.chromium.org/8508060/diff/10001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:489: RegKey client_state(reg_data[i].hive, reg_data[i].path, KEY_QUERY_VALUE); On 2011/11/14 14:24:09, grt wrote: > i think this is prone to flakiness since it doesn't check for installation of > the product (install system-level chrome, run it once, uninstall it, install > user-level chrome. this will give you the no-longer-present system-level > chrome's lastrun until Omaha cleans it up). to be correct, it should look in > the Clients key for a "pv" value. only if that is present should it look in > ClientStateMedium/ClientState. Done. http://codereview.chromium.org/8508060/diff/10001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:502: if (new_days_since_last_run >= 0) { On 2011/11/14 14:24:09, grt wrote: > heh. now that you need this check anyway, i'd get rid of std::min and > <algorithm> and do: > if (new_days_since_last_run >= 0 && new_days_since_last_run < > days_since_last_run) > days_since_last_run = new_days_since_last_run; Done. http://codereview.chromium.org/8508060/diff/10001/chrome/installer/gcapi/gcap... File chrome/installer/gcapi/gcapi_last_run_test.cc (right): http://codereview.chromium.org/8508060/diff/10001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi_last_run_test.cc:60: void TearDown() { On 2011/11/14 14:24:09, grt wrote: > remove this since override_manager_'s dtor will do everything you want. Done.
i'm sorry, i couldn't resist. let me know if you want me to write the code. mini_installer.cc uses RegQueryValueEx correctly to get a terminated string. http://codereview.chromium.org/8508060/diff/16001/chrome/installer/gcapi/gcap... File chrome/installer/gcapi/gcapi.cc (right): http://codereview.chromium.org/8508060/diff/16001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:21: #include <algorithm> remove this http://codereview.chromium.org/8508060/diff/16001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:173: reinterpret_cast<LPDWORD>(size)) == ERROR_SUCCESS)) { casting a size_t* to a dword*? what could go wrong? http://codereview.chromium.org/8508060/diff/16001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:174: ::RegCloseKey(key); sweet! this only closes the key if the value existed and was read. would you mind fixing this? it's not nice to let friends load us into their process and then leak registry handles. http://codereview.chromium.org/8508060/diff/16001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:175: return true; why bother making sure the value was a REG_SZ and that it was terminated? it's not like we'd send the string to CreateProcess or something. WAIT, THAT'S EXACTLY WHAT WE DO!!!1!1!!!
LGTM with the header removal from my previous comment.
Thanks. http://codereview.chromium.org/8508060/diff/16001/chrome/installer/gcapi/gcap... File chrome/installer/gcapi/gcapi.cc (right): http://codereview.chromium.org/8508060/diff/16001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:21: #include <algorithm> On 2011/11/14 16:24:47, grt wrote: > remove this Done. http://codereview.chromium.org/8508060/diff/16001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:173: reinterpret_cast<LPDWORD>(size)) == ERROR_SUCCESS)) { On 2011/11/14 16:24:47, grt wrote: > casting a size_t* to a dword*? what could go wrong? Acknowledged, but I have a non-goal of re-writing legacy code without thorough tests. I've probably made too many changes already :S http://codereview.chromium.org/8508060/diff/16001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:174: ::RegCloseKey(key); On 2011/11/14 16:24:47, grt wrote: > sweet! this only closes the key if the value existed and was read. would you > mind fixing this? it's not nice to let friends load us into their process and > then leak registry handles. idem. http://codereview.chromium.org/8508060/diff/16001/chrome/installer/gcapi/gcap... chrome/installer/gcapi/gcapi.cc:175: return true; On 2011/11/14 16:24:47, grt wrote: > why bother making sure the value was a REG_SZ and that it was terminated? it's > not like we'd send the string to CreateProcess or something. WAIT, THAT'S > EXACTLY WHAT WE DO!!!1!1!!! ditto.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robertshield@chromium.org/8508060/15002
Presubmit check for 8508060-15002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Warnings **
New code should not use wstrings. If you are calling an API that accepts a
wstring, fix the API.
chrome/installer/gcapi/gcapi.cc:491
Presubmit checks took 3.3s to calculate.
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/8508060/diff/15002/chrome/installer/gcapi/gca... File chrome/installer/gcapi/gcapi_last_run_test.cc (right): https://codereview.chromium.org/8508060/diff/15002/chrome/installer/gcapi/gca... chrome/installer/gcapi/gcapi_last_run_test.cc:33: L"hkcu_override\\%ls", ASCIIToWide(guid::GenerateGUID())); ASCIIToWide returns a wstring, you want to call c_str() on it. As-is, this won'd do anything useful: ..\..\chrome\installer\gcapi\gcapi_last_run_test.cc(30,32) : error(clang): cannot pass object of non-trivial type 'std::wstring' (aka 'basic_string<wchar_t, char_traits<wchar_t>, allocator<wchar_t> >') through variadic function; call will abort at runtime [-Wnon-pod-varargs] L"hkcu_override\\%ls", base::ASCIIToWide(base::GenerateGUID())); ^ 1 error generated. I suppose the test doesn't really care which value this is set to, and so it still passes?
Message was sent while issue was closed.
https://codereview.chromium.org/8508060/diff/15002/chrome/installer/gcapi/gca... File chrome/installer/gcapi/gcapi_last_run_test.cc (right): https://codereview.chromium.org/8508060/diff/15002/chrome/installer/gcapi/gca... chrome/installer/gcapi/gcapi_last_run_test.cc:33: L"hkcu_override\\%ls", ASCIIToWide(guid::GenerateGUID())); On 2014/08/29 22:08:46, Nico (hiding) wrote: > ASCIIToWide returns a wstring, you want to call c_str() on it. As-is, this won'd > do anything useful: > > > ..\..\chrome\installer\gcapi\gcapi_last_run_test.cc(30,32) : error(clang): > cannot pass object of non-trivial type 'std::wstring' (aka > 'basic_string<wchar_t, char_traits<wchar_t>, allocator<wchar_t> >') through > variadic function; call will abort at runtime [-Wnon-pod-varargs] > L"hkcu_override\\%ls", base::ASCIIToWide(base::GenerateGUID())); > ^ > 1 error generated. > > > I suppose the test doesn't really care which value this is set to, and so it > still passes? Huh yeah, that was silly and no the test doesn't really care. This would only really matter if the same test (or another test using the same scheme) was being run twice on the same machine at the same time. Thanks for finding this. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
