Make V4GetHashProtocolManager::RegisterFactory safe.

components/safe_browsing_db/database_manager_unittest.cc

Index: components/safe_browsing_db/database_manager_unittest.cc
diff --git a/components/safe_browsing_db/database_manager_unittest.cc b/components/safe_browsing_db/database_manager_unittest.cc
index 8abd111fb9ab8a3a02b488be19a57e7d13339f09..db8fd9cf20db3505ef686b1051cfead35ceb0831 100644
--- a/components/safe_browsing_db/database_manager_unittest.cc
+++ b/components/safe_browsing_db/database_manager_unittest.cc
@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
@@ -117,8 +118,8 @@ class TestClient : public SafeBrowsingDatabaseManager::Client {
 class SafeBrowsingDatabaseManagerTest : public testing::Test {
  protected:
   void SetUp() override {
-    TestV4GetHashProtocolManagerFactory get_hash_pm_factory;
-    V4GetHashProtocolManager::RegisterFactory(&get_hash_pm_factory);
+    V4GetHashProtocolManager::RegisterFactory(
+        base::WrapUnique(new TestV4GetHashProtocolManagerFactory()));

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

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.

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

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.

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

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]

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

 
     db_manager_ = new TestSafeBrowsingDatabaseManager();
     db_manager_->StartOnIOThread(NULL, V4ProtocolConfig());
@@ -127,6 +128,7 @@ class SafeBrowsingDatabaseManagerTest : public testing::Test {
   void TearDown() override {
     base::RunLoop().RunUntilIdle();
     db_manager_->StopOnIOThread(false);
+    V4GetHashProtocolManager::RegisterFactory(nullptr);
   }
 
   scoped_refptr<SafeBrowsingDatabaseManager> db_manager_;