Make V4GetHashProtocolManager::RegisterFactory safe.

Make V4GetHashProtocolManager::RegisterFactory safe.

This patch makes V4GetHashProtocolManager::RegisterFactory safe by
passing ownership of the testing factory and making sure it is cleared
before the next test is run.

BUG=608053
Committed: https://crrev.com/3ea614851dc01884f58559b520c5a013a9beb2e2
Cr-Commit-Position: refs/heads/master@{#391339}

[Reilly Grant (use Gerrit), 2016-05-02 21:43:05]

reillyg@chromium.org changed reviewers:
+ mattm@chromium.org

[Reilly Grant (use Gerrit), 2016-05-02 21:43:06]

Matt, please take a look.

[mattm, 2016-05-02 22:36:52]

mattm@chromium.org changed reviewers:
+ kcarattini@chromium.org, nparker@chromium.org

[mattm, 2016-05-02 22:36:53]

Adding Nathan and Kendra from the original CL.

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/database_manager_unittest.cc (right):

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager_unittest.cc:122:
base::WrapUnique(new TestV4GetHashProtocolManagerFactory()));
The RegisterFactory idiom is already used in a bunch of other places in
safe_browsing that just take a raw pointer, so changing only one of them to a
different style does make things a bit inconsistent.

Also this avoids memory errors but if you forget to unregister the factory it
could still cause other confusing errors if tests use a factory they weren't
expecting, so I'm not sure it's better than just making the
TestV4GetHashProtocolManagerFactory a member of the Test class.

An alternate approach would be some sort of "scoped factory registration" that
would automatically unregister when the object is destroyed (eg by the Test
object getting deleted), but that does make unregistration order / thread / etc
less obvious.

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right):

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:130: factory_ =
factory.release();
If you do switch to passing in a unique_ptr, why not make factory_ a unique_ptr?

[Reilly Grant (use Gerrit), 2016-05-02 22:55:27]

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/database_manager_unittest.cc (right):

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager_unittest.cc:122:
base::WrapUnique(new TestV4GetHashProtocolManagerFactory()));
On 2016/05/02 at 22:36:53, mattm wrote:
> The RegisterFactory idiom is already used in a bunch of other places in
safe_browsing that just take a raw pointer, so changing only one of them to a
different style does make things a bit inconsistent.
> 
> Also this avoids memory errors but if you forget to unregister the factory it
could still cause other confusing errors if tests use a factory they weren't
expecting, so I'm not sure it's better than just making the
TestV4GetHashProtocolManagerFactory a member of the Test class.
> 
> An alternate approach would be some sort of "scoped factory registration" that
would automatically unregister when the object is destroyed (eg by the Test
object getting deleted), but that does make unregistration order / thread / etc
less obvious.

I agree in principle but this is the only place in which this function is called
so the extra infrastructure seems unnecessary. If this pattern is used in other
places I suggest the owners take a look at whether it it causing any problems
that have not yet been noticed. This issue can be reproduced by building
components_unittests with MSan and then running with --single-process-tests to
catch interactions between tests run in the same process.

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right):

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:130: factory_ =
factory.release();
On 2016/05/02 at 22:36:53, mattm wrote:
> If you do switch to passing in a unique_ptr, why not make factory_ a
unique_ptr?

factory_ is a global variable. It must be a primitive type.

[kcarattini, 2016-05-03 01:16:32]

LGTM

Thank you for finding and fixing this!

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/database_manager_unittest.cc (right):

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager_unittest.cc:122:
base::WrapUnique(new TestV4GetHashProtocolManagerFactory()));
On 2016/05/02 22:55:27, Reilly Grant wrote:
> On 2016/05/02 at 22:36:53, mattm wrote:
> > The RegisterFactory idiom is already used in a bunch of other places in
> safe_browsing that just take a raw pointer, so changing only one of them to a
> different style does make things a bit inconsistent.
> > 
> > Also this avoids memory errors but if you forget to unregister the factory
it
> could still cause other confusing errors if tests use a factory they weren't
> expecting, so I'm not sure it's better than just making the
> TestV4GetHashProtocolManagerFactory a member of the Test class.
> > 
> > An alternate approach would be some sort of "scoped factory registration"
that
> would automatically unregister when the object is destroyed (eg by the Test
> object getting deleted), but that does make unregistration order / thread /
etc
> less obvious.
> 
> I agree in principle but this is the only place in which this function is
called
> so the extra infrastructure seems unnecessary. If this pattern is used in
other
> places I suggest the owners take a look at whether it it causing any problems
> that have not yet been noticed. This issue can be reproduced by building
> components_unittests with MSan and then running with --single-process-tests to
> catch interactions between tests run in the same process.

I'm not sure I follow why passing a unique_ptr is necessary when the
RegisterFactory method now deletes the old pointer?

[Reilly Grant (use Gerrit), 2016-05-03 01:18:21]

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/database_manager_unittest.cc (right):

https://codereview.chromium.org/1944433002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager_unittest.cc:122:
base::WrapUnique(new TestV4GetHashProtocolManagerFactory()));
On 2016/05/03 at 01:16:32, kcarattini wrote:
> On 2016/05/02 22:55:27, Reilly Grant wrote:
> > On 2016/05/02 at 22:36:53, mattm wrote:
> > > The RegisterFactory idiom is already used in a bunch of other places in
> > safe_browsing that just take a raw pointer, so changing only one of them to
a
> > different style does make things a bit inconsistent.
> > > 
> > > Also this avoids memory errors but if you forget to unregister the factory
it
> > could still cause other confusing errors if tests use a factory they weren't
> > expecting, so I'm not sure it's better than just making the
> > TestV4GetHashProtocolManagerFactory a member of the Test class.
> > > 
> > > An alternate approach would be some sort of "scoped factory registration"
that
> > would automatically unregister when the object is destroyed (eg by the Test
> > object getting deleted), but that does make unregistration order / thread /
etc
> > less obvious.
> > 
> > I agree in principle but this is the only place in which this function is
called
> > so the extra infrastructure seems unnecessary. If this pattern is used in
other
> > places I suggest the owners take a look at whether it it causing any
problems
> > that have not yet been noticed. This issue can be reproduced by building
> > components_unittests with MSan and then running with --single-process-tests
to
> > catch interactions between tests run in the same process.
> 
> I'm not sure I follow why passing a unique_ptr is necessary when the
RegisterFactory method now deletes the old pointer?

It prevents you from passing a pointer to a local variable.

[Reilly Grant (use Gerrit), 2016-05-03 01:18:30]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2016-05-03 01:18:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1944433002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1944433002/20001

[Reilly Grant (use Gerrit), 2016-05-03 01:18:47]

The CQ bit was unchecked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-05-03 17:48:15]

mattm/nparker, I can't check this in without a review from one of you.

[Nathan Parker, 2016-05-03 19:06:10]

lgtm

[Reilly Grant (use Gerrit), 2016-05-03 19:13:07]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2016-05-03 19:13:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1944433002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1944433002/20001

[commit-bot: I haz the power, 2016-05-03 20:17:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-03 20:18:49]

Description was changed from

==========
Make V4GetHashProtocolManager::RegisterFactory safe.

This patch makes V4GetHashProtocolManager::RegisterFactory safe by
passing ownership of the testing factory and making sure it is cleared
before the next test is run.

BUG=608053
==========

to

==========
Make V4GetHashProtocolManager::RegisterFactory safe.

This patch makes V4GetHashProtocolManager::RegisterFactory safe by
passing ownership of the testing factory and making sure it is cleared
before the next test is run.

BUG=608053

Committed: https://crrev.com/3ea614851dc01884f58559b520c5a013a9beb2e2
Cr-Commit-Position: refs/heads/master@{#391339}
==========

[commit-bot: I haz the power, 2016-05-03 20:18:50]

Patchset 2 (id:??) landed as
https://crrev.com/3ea614851dc01884f58559b520c5a013a9beb2e2
Cr-Commit-Position: refs/heads/master@{#391339}