Make V4GetHashProtocolManager::RegisterFactory safe.

components/safe_browsing_db/database_manager_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/safe_browsing_db/database_manager.h"
 
 #include <stddef.h>
 
 #include <string>
 #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"
 #include "components/safe_browsing_db/test_database_manager.h"
 #include "components/safe_browsing_db/v4_get_hash_protocol_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ skipping 87 matching lines @@
   std::vector<std::string> blocked_permissions_;
   bool callback_invoked_;
   DISALLOW_COPY_AND_ASSIGN(TestClient);
 };
 
 }  // namespace
 
 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());
   }
 
   void TearDown() override {
     base::RunLoop().RunUntilIdle();
     db_manager_->StopOnIOThread(false);
+    V4GetHashProtocolManager::RegisterFactory(nullptr);
   }
 
   scoped_refptr<SafeBrowsingDatabaseManager> db_manager_;
 
  private:
   content::TestBrowserThreadBundle test_browser_thread_bundle_;
 };
 
 TEST_F(SafeBrowsingDatabaseManagerTest, CheckApiBlacklistUrlWrongScheme) {
   TestClient client;
@@ skipping 97 matching lines @@
   EXPECT_FALSE(db_manager_->CheckApiBlacklistUrl(url, &client));
   EXPECT_TRUE(db_manager_->CancelApiCheck(&client));
   base::RunLoop().RunUntilIdle();
 
   const std::vector<std::string>& permissions = client.GetBlockedPermissions();
   EXPECT_EQ(0ul, permissions.size());
   EXPECT_FALSE(client.callback_invoked());
 }
 
 }  // namespace safe_browsing