Base: Make RegistryOverrideManager support sharded/parallel tests.

Base: Make RegistryOverrideManager support sharded/parallel tests.

BUG=314800
TBR=ajwong@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234367

[grt (UTC plus 2), 2013-11-05 03:26:58]

nice. it'll be good to have this fixed once and for all.

https://codereview.chromium.org/57423008/diff/1/base/test/test_reg_util_win_u...
File base/test/test_reg_util_win_unittest.cc (right):

https://codereview.chromium.org/57423008/diff/1/base/test/test_reg_util_win_u...
base/test/test_reg_util_win_unittest.cc:28:
key.DeleteKey(RegistryOverrideManager::kTempTestKeyPath);
does this mean that this unit test can't run in parallel with any other test
that uses RegistryOverrideManager? is that not a problem (i have no idea if
tests can be parallelized that way, but maybe it's safer to make it so they
can)?

[tommycli, 2013-11-05 03:31:48]

https://codereview.chromium.org/57423008/diff/1/base/test/test_reg_util_win_u...
File base/test/test_reg_util_win_unittest.cc (right):

https://codereview.chromium.org/57423008/diff/1/base/test/test_reg_util_win_u...
base/test/test_reg_util_win_unittest.cc:28:
key.DeleteKey(RegistryOverrideManager::kTempTestKeyPath);
On 2013/11/05 03:26:58, grt wrote:
> does this mean that this unit test can't run in parallel with any other test
> that uses RegistryOverrideManager? is that not a problem (i have no idea if
> tests can be parallelized that way, but maybe it's safer to make it so they
> can)?

Nice catch. Yes. This whole test is problematic and may need to be scratched.

This test will break any other process that's using RegistryOverrideManager on
the whole system. Basically the test for RegistryOverrideManager needs a
RegistryOverrideManager...

[grt (UTC plus 2), 2013-11-05 03:34:25]

On 2013/11/05 03:31:48, tommycli wrote:
>
https://codereview.chromium.org/57423008/diff/1/base/test/test_reg_util_win_u...
> File base/test/test_reg_util_win_unittest.cc (right):
> 
>
https://codereview.chromium.org/57423008/diff/1/base/test/test_reg_util_win_u...
> base/test/test_reg_util_win_unittest.cc:28:
> key.DeleteKey(RegistryOverrideManager::kTempTestKeyPath);
> On 2013/11/05 03:26:58, grt wrote:
> > does this mean that this unit test can't run in parallel with any other test
> > that uses RegistryOverrideManager? is that not a problem (i have no idea if
> > tests can be parallelized that way, but maybe it's safer to make it so they
> > can)?
> 
> Nice catch. Yes. This whole test is problematic and may need to be scratched.
> 
> This test will break any other process that's using RegistryOverrideManager on
> the whole system. Basically the test for RegistryOverrideManager needs a
> RegistryOverrideManager...

Can this be done by allowing the test to pass in a test-only value that's used
in place of RegistryOverrideManager::kTempTestKeyPath in the implementation?

[tommycli, 2013-11-05 19:03:51]

Hi. I used the scheme you suggested to update the test. It should not interfere
with other RegistryOverrideManagers now.

I was able to run two instances of this test in parallel on my machine. Good
idea!

[tommycli, 2013-11-05 19:27:30]

+phajdan.jr, OWNER review for RegistryOverrideManager

[Paweł Hajdan Jr., 2013-11-06 20:33:58]

Please get an l-g-t-m from a Windows expert before my final stamp.

https://codereview.chromium.org/57423008/diff/100002/base/test/test_reg_util_...
File base/test/test_reg_util_win.cc (right):

https://codereview.chromium.org/57423008/diff/100002/base/test/test_reg_util_...
base/test/test_reg_util_win.cc:22: return;
Since this can fail, let's return bool and add WARN_UNUSED_RESULT.

https://codereview.chromium.org/57423008/diff/100002/base/test/test_reg_util_...
base/test/test_reg_util_win.cc:38: if (age > base::TimeDelta::FromHours(1))
Let's make this bigger: 24 hours.

https://codereview.chromium.org/57423008/diff/100002/base/test/test_reg_util_...
base/test/test_reg_util_win.cc:43: }
nit:  // namespace

[tommycli, 2013-11-06 21:44:22]

phajdan-jr: Addresed your comments. I'll look for a windows expert.

https://codereview.chromium.org/57423008/diff/100002/base/test/test_reg_util_...
File base/test/test_reg_util_win.cc (right):

https://codereview.chromium.org/57423008/diff/100002/base/test/test_reg_util_...
base/test/test_reg_util_win.cc:22: return;
On 2013/11/06 20:33:58, Paweł Hajdan Jr. wrote:
> Since this can fail, let's return bool and add WARN_UNUSED_RESULT.

This occurs on first-run, and is harmless. This should be ignored.

https://codereview.chromium.org/57423008/diff/100002/base/test/test_reg_util_...
base/test/test_reg_util_win.cc:38: if (age > base::TimeDelta::FromHours(1))
On 2013/11/06 20:33:58, Paweł Hajdan Jr. wrote:
> Let's make this bigger: 24 hours.

Done.

https://codereview.chromium.org/57423008/diff/100002/base/test/test_reg_util_...
base/test/test_reg_util_win.cc:43: }
On 2013/11/06 20:33:58, Paweł Hajdan Jr. wrote:
> nit:  // namespace

Done.

[tommycli, 2013-11-06 22:10:39]

+rogerta for Windows review (original author)

+cc tommi

[grt (UTC plus 2), 2013-11-07 02:39:33]

https://codereview.chromium.org/57423008/diff/310001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/57423008/diff/310001/base/base.gyp#newcode824
base/base.gyp:824: 'test/test_reg_util_win_unittest.cc',
the rules in src/build/filename_rules.gypi should handle this for you. please
remove this and the other two _win_unittest.cc files here and send a tryjob to a
non-win bot to confirm.

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
File base/test/test_reg_util_win.cc (right):

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win.cc:19: if (ERROR_SUCCESS !=
the following is more in-line with Chromium style:
  if (test_root_key.Open(HKEY_CURRENT_USER, test_key_root.c_str(),
			 KEY_ALL_ACCESS) != ERROR_SUCCESS) {

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
File base/test/test_reg_util_win.h (right):

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win.h:36: static const wchar_t kTempTestKeyPath[];
It feels to me like this should be an implementation detail. I see this value
used in two places: 1) product_state_unittest.cc (see my comments there) and 2)
src/chrome/browser/rlz/rlz_unittest.cc. Both are suspect. Please move the
declaration/definition of this into the .cc file in the unnamed namespace
(without "static").

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
File base/test/test_reg_util_win_unittest.cc (right):

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win_unittest.cc:22: public:
instead of public ctor and dtor, do this work in overrides of SetUp() and
TearDown(). also, make those overrides protected.

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win_unittest.cc:48: ASSERT_EQ(ERROR_SUCCESS,
as per http://www.chromium.org/developers/coding-style#Code_Formatting:
    ASSERT_EQ(ERROR_SUCCESS,
	      key.Open(HKEY_CURRENT_USER, key_path.c_str(), KEY_READ))
      << key_path << " does not exist.";
here and elsewhere

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win_unittest.cc:86: create_key.Create(HKEY_CURRENT_USER,
kTestKeyPath, KEY_ALL_ACCESS));
align with arg on previous line as per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
here and elsewhere

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/gcapi/g...
File chrome/installer/gcapi/gcapi_reactivation_test.cc (right):

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi_reactivation_test.cc:27: void SetUp() {
while you're here:
  virtual void SetUp() OVERRIDE {

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util_unittest.cc (right):

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/setup/s...
chrome/installer/setup/setup_util_unittest.cc:409:
registry_override_manager_.OverrideRegistry(kRootKey,
i think it's desirable to do the cleanup in TearDown rather than in the
fixture's dtor. please either make registry_override_manager_ a scoped_ptr, new
it up here, and .reset() it in TearDown; or retain something like
RemoveAllOverrides. i left a similar comment in another file.

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/util/pr...
File chrome/installer/util/product_state_unittest.cc (right):

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/util/pr...
chrome/installer/util/product_state_unittest.cc:73: // Override for test
purposes.  We don't use ScopedRegistryKeyOverride
Can RegistryOverrideManager replace this? As it is, this test looks like it
can't be parallelized.

https://codereview.chromium.org/57423008/diff/310001/chrome_frame/test/util_u...
File chrome_frame/test/util_unittests.cc (left):

https://codereview.chromium.org/57423008/diff/310001/chrome_frame/test/util_u...
chrome_frame/test/util_unittests.cc:38:
registry_virtualization_.RemoveAllOverrides();
see other comment related to cleanup

[tommycli, 2013-11-07 18:02:06]

grt: addressed your comments

https://codereview.chromium.org/57423008/diff/310001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/57423008/diff/310001/base/base.gyp#newcode824
base/base.gyp:824: 'test/test_reg_util_win_unittest.cc',
On 2013/11/07 02:39:33, grt wrote:
> the rules in src/build/filename_rules.gypi should handle this for you. please
> remove this and the other two _win_unittest.cc files here and send a tryjob to
a
> non-win bot to confirm.

Done.

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
File base/test/test_reg_util_win.cc (right):

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win.cc:19: if (ERROR_SUCCESS !=
On 2013/11/07 02:39:33, grt wrote:
> the following is more in-line with Chromium style:
>   if (test_root_key.Open(HKEY_CURRENT_USER, test_key_root.c_str(),
> 			 KEY_ALL_ACCESS) != ERROR_SUCCESS) {

Done.

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
File base/test/test_reg_util_win.h (right):

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win.h:36: static const wchar_t kTempTestKeyPath[];
On 2013/11/07 02:39:33, grt wrote:
> It feels to me like this should be an implementation detail. I see this value
> used in two places: 1) product_state_unittest.cc (see my comments there) and
2)
> src/chrome/browser/rlz/rlz_unittest.cc. Both are suspect. Please move the
> declaration/definition of this into the .cc file in the unnamed namespace
> (without "static").

Unit test needs this to function properly. I moved it to a separate function at
the end of the file:

string16 GenerateTempKeyPath();

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
File base/test/test_reg_util_win_unittest.cc (right):

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win_unittest.cc:22: public:
On 2013/11/07 02:39:33, grt wrote:
> instead of public ctor and dtor, do this work in overrides of SetUp() and
> TearDown(). also, make those overrides protected.

Hmm... I was following 
https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de...

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win_unittest.cc:48: ASSERT_EQ(ERROR_SUCCESS,
On 2013/11/07 02:39:33, grt wrote:
> as per http://www.chromium.org/developers/coding-style#Code_Formatting:
>     ASSERT_EQ(ERROR_SUCCESS,
> 	      key.Open(HKEY_CURRENT_USER, key_path.c_str(), KEY_READ))
>       << key_path << " does not exist.";
> here and elsewhere

Done.

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win_unittest.cc:86: create_key.Create(HKEY_CURRENT_USER,
kTestKeyPath, KEY_ALL_ACCESS));
On 2013/11/07 02:39:33, grt wrote:
> align with arg on previous line as per
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
> here and elsewhere

Done.

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/gcapi/g...
File chrome/installer/gcapi/gcapi_reactivation_test.cc (right):

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/gcapi/g...
chrome/installer/gcapi/gcapi_reactivation_test.cc:27: void SetUp() {
On 2013/11/07 02:39:33, grt wrote:
> while you're here:
>   virtual void SetUp() OVERRIDE {

Done.

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util_unittest.cc (right):

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/setup/s...
chrome/installer/setup/setup_util_unittest.cc:409:
registry_override_manager_.OverrideRegistry(kRootKey,
On 2013/11/07 02:39:33, grt wrote:
> i think it's desirable to do the cleanup in TearDown rather than in the
> fixture's dtor. please either make registry_override_manager_ a scoped_ptr,
new
> it up here, and .reset() it in TearDown; or retain something like
> RemoveAllOverrides. i left a similar comment in another file.

I think adding a RemoveAllOverrides() method would just confuse people, since
the manager is designed to operate in a scoped-manner.

I see what you mean about the lack-of-symmetry between SetUp and TearDown
though. I was really hoping for a manager with 'scoped' semantics. It is kind of
lame that there are setup functions that need to be called for each overridden
hive.

Maybe there's a better 'third' solution.

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/util/pr...
File chrome/installer/util/product_state_unittest.cc (right):

https://codereview.chromium.org/57423008/diff/310001/chrome/installer/util/pr...
chrome/installer/util/product_state_unittest.cc:73: // Override for test
purposes.  We don't use ScopedRegistryKeyOverride
On 2013/11/07 02:39:33, grt wrote:
> Can RegistryOverrideManager replace this? As it is, this test looks like it
> can't be parallelized.

Done.

https://codereview.chromium.org/57423008/diff/310001/chrome_frame/test/util_u...
File chrome_frame/test/util_unittests.cc (left):

https://codereview.chromium.org/57423008/diff/310001/chrome_frame/test/util_u...
chrome_frame/test/util_unittests.cc:38:
registry_virtualization_.RemoveAllOverrides();
On 2013/11/07 02:39:33, grt wrote:
> see other comment related to cleanup

Replied to other comment.

[grt (UTC plus 2), 2013-11-08 04:33:05]

lgtm w/ final nits

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
File base/test/test_reg_util_win_unittest.cc (right):

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win_unittest.cc:22: public:
On 2013/11/07 18:02:07, tommycli wrote:
> On 2013/11/07 02:39:33, grt wrote:
> > instead of public ctor and dtor, do this work in overrides of SetUp() and
> > TearDown(). also, make those overrides protected.
> 
> Hmm... I was following 
>
https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de...

Oh, cool. I hadn't seen that. Thanks for the pointer. In that case, I think the
ctor and dtor could be made protected. They're only to be called by subclasses
created by gtest, IIUC.

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win_unittest.cc:86: create_key.Create(HKEY_CURRENT_USER,
kTestKeyPath, KEY_ALL_ACCESS));
On 2013/11/07 18:02:07, tommycli wrote:
> On 2013/11/07 02:39:33, grt wrote:
> > align with arg on previous line as per
> >
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
> > here and elsewhere
> 
> Done.

really? i don't see a change here. for example:
  EXPECT_EQ(ERROR_SUCCESS,
            create_key.Create(...));

[tommycli, 2013-11-08 16:24:00]

grt: Thanks! I addressed your last comments.

phajdan-jr: Got my Windows expert review. Do you think I should just TBR the
other owners, or do I need an explicit review for the other (fairly trivial)
changes?

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
File base/test/test_reg_util_win_unittest.cc (right):

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win_unittest.cc:22: public:
On 2013/11/08 04:33:05, grt wrote:
> On 2013/11/07 18:02:07, tommycli wrote:
> > On 2013/11/07 02:39:33, grt wrote:
> > > instead of public ctor and dtor, do this work in overrides of SetUp() and
> > > TearDown(). also, make those overrides protected.
> > 
> > Hmm... I was following 
> >
>
https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de...
> 
> Oh, cool. I hadn't seen that. Thanks for the pointer. In that case, I think
the
> ctor and dtor could be made protected. They're only to be called by subclasses
> created by gtest, IIUC.

Done.

https://codereview.chromium.org/57423008/diff/310001/base/test/test_reg_util_...
base/test/test_reg_util_win_unittest.cc:86: create_key.Create(HKEY_CURRENT_USER,
kTestKeyPath, KEY_ALL_ACCESS));
On 2013/11/08 04:33:05, grt wrote:
> On 2013/11/07 18:02:07, tommycli wrote:
> > On 2013/11/07 02:39:33, grt wrote:
> > > align with arg on previous line as per
> > >
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
> > > here and elsewhere
> > 
> > Done.
> 
> really? i don't see a change here. for example:
>   EXPECT_EQ(ERROR_SUCCESS,
>             create_key.Create(...));

Sorry I must have missed it. Patchsets 6 and 7 should address this.

[tommi (sloooow) - chröme, 2013-11-11 13:09:48]

fwiw if Greg has signed off on the cl that's good enough for me.  I'm the
original author of the code before Roger moved it, so you have my rubberstamp
lgtm at least.

[tommycli, 2013-11-11 18:13:57]

phajdan-jr: ping

tommi: thx!

[Roger Tawa OOO till Jul 10th, 2013-11-11 20:03:41]

lgtm

[Paweł Hajdan Jr., 2013-11-11 20:18:29]

LGTM

[commit-bot: I haz the power, 2013-11-11 20:22:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/57423008/690001

[commit-bot: I haz the power, 2013-11-12 00:14:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/57423008/690001

[commit-bot: I haz the power, 2013-11-12 01:47:04]

Change committed as 234367

[gab, 2013-11-22 03:55:12]

(Apparently replying doesn't work when issue is closed....)

It appears this CL broke compilation rlz_unittest.cc for the unit_tests target
(is RLZ not enabled on the waterfall?).

d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(212) : error C2039:
'kTempTestKeyPath' : is not a member of 'registry_util::RegistryOverrideManager'
        d:\src\chrome\src\base\test\test_reg_util_win.h(32) : see declaration of
'registry_util::RegistryOverrideManager'
d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(212) : error C2065:
'kTempTestKeyPath' : undeclared identifier
d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(222) : error C2039:
'kTempTestKeyPath' : is not a member of 'registry_util::RegistryOverrideManager'
        d:\src\chrome\src\base\test\test_reg_util_win.h(32) : see declaration of
'registry_util::RegistryOverrideManager'
d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(222) : error C2065:
'kTempTestKeyPath' : undeclared identifier

Please considering reverting this CL or fixing this ASAP.

Thanks,
Gab

[grt (UTC plus 2), 2013-11-22 14:54:21]

On 2013/11/22 03:55:12, gab wrote:
> (Apparently replying doesn't work when issue is closed....)
> 
> It appears this CL broke compilation rlz_unittest.cc for the unit_tests target
> (is RLZ not enabled on the waterfall?).
> 
> d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(212) : error C2039:
> 'kTempTestKeyPath' : is not a member of
'registry_util::RegistryOverrideManager'
>         d:\src\chrome\src\base\test\test_reg_util_win.h(32) : see declaration
of
> 'registry_util::RegistryOverrideManager'
> d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(212) : error C2065:
> 'kTempTestKeyPath' : undeclared identifier
> d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(222) : error C2039:
> 'kTempTestKeyPath' : is not a member of
'registry_util::RegistryOverrideManager'
>         d:\src\chrome\src\base\test\test_reg_util_win.h(32) : see declaration
of
> 'registry_util::RegistryOverrideManager'
> d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(222) : error C2065:
> 'kTempTestKeyPath' : undeclared identifier
> 
> Please considering reverting this CL or fixing this ASAP.

This was reverted in r234627 and reapplied in r235284 following the RLZ fixes in
r235090. Is it still failing somewhere?

[gab, 2013-11-22 18:03:34]

On 2013/11/22 14:54:21, grt wrote:
> On 2013/11/22 03:55:12, gab wrote:
> > (Apparently replying doesn't work when issue is closed....)
> > 
> > It appears this CL broke compilation rlz_unittest.cc for the unit_tests
target
> > (is RLZ not enabled on the waterfall?).
> > 
> > d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(212) : error C2039:
> > 'kTempTestKeyPath' : is not a member of
> 'registry_util::RegistryOverrideManager'
> >         d:\src\chrome\src\base\test\test_reg_util_win.h(32) : see
declaration
> of
> > 'registry_util::RegistryOverrideManager'
> > d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(212) : error C2065:
> > 'kTempTestKeyPath' : undeclared identifier
> > d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(222) : error C2039:
> > 'kTempTestKeyPath' : is not a member of
> 'registry_util::RegistryOverrideManager'
> >         d:\src\chrome\src\base\test\test_reg_util_win.h(32) : see
declaration
> of
> > 'registry_util::RegistryOverrideManager'
> > d:\src\chrome\src\chrome\browser\rlz\rlz_unittest.cc(222) : error C2065:
> > 'kTempTestKeyPath' : undeclared identifier
> > 
> > Please considering reverting this CL or fixing this ASAP.
> 
> This was reverted in r234627 and reapplied in r235284 following the RLZ fixes
in
> r235090. Is it still failing somewhere?

Ah ok my bad, looks like I haven't synced in a while, I'm on r234510, that would
explain it, thanks!