Extensions: Pull duplicated functionality into ExtensionsTest fixture.

extensions/browser/extensions_test.cc

Index: extensions/browser/extensions_test.cc
diff --git a/extensions/browser/extensions_test.cc b/extensions/browser/extensions_test.cc
index f9f88a107396febf551987a681d23aa9ca11ae91..957bc946714e7ff6652376254a14bea659bff23e 100644
--- a/extensions/browser/extensions_test.cc
+++ b/extensions/browser/extensions_test.cc
@@ -5,9 +5,15 @@
 #include "extensions/browser/extensions_test.h"
 
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/pref_service_factory.h"
+#include "components/prefs/testing_pref_store.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/common/content_client.h"
 #include "content/public/test/test_browser_context.h"
+#include "extensions/browser/extension_pref_value_map.h"
+#include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_prefs_factory.h"
 #include "extensions/browser/test_extensions_browser_client.h"
 #include "extensions/test/test_content_browser_client.h"
 #include "extensions/test/test_content_utility_client.h"
@@ -26,6 +32,7 @@ ExtensionsTest::ExtensionsTest()
     : content_browser_client_(new TestContentBrowserClient),
       content_utility_client_(new TestContentUtilityClient),
       browser_context_(new content::TestBrowserContext),
+      incognito_context_(new content::TestBrowserContextIncognito),

[Devlin, 2017/04/04 14:18:11]

I wonder if we should create this as-needed instead?

[karandeepb, 2017/04/04 19:00:37]

I don't see any harm in creating one by default. There are no performance
concerns here and we should be trying to make it easier to write tests.

[Devlin, 2017/04/04 19:04:15]

My concern is whether the instantiation has side effects elsewhere.  e.g., if
this were a profile, it would register the profile as an OTR profile, etc.  If
it were a RenderProcessHost, it would be returned from
RenderProcessHost::AllHostsIterator.  I'm not sure whether or not there's
anything like that for BrowserContexts.  But, given lack of evidence one or way
the other, I'm fine keeping it until it becomes a problem (though it'd probably
be good to move it into SetUp(), as well).

[karandeepb, 2017/04/04 19:16:42]

One side effect(desired) is that
ExtensionsBrowserClient::GetOffTheRecordContext(browser_context()) would return
this incogito_context_ and
ExtensionsBrowserClient::HasOffTheRecordContext(browser_context()) would return
true. I am not sure whether there are other unintended side effects.

       extensions_browser_client_(
           new TestExtensionsBrowserClient(browser_context_.get())) {
   content::SetBrowserClientForTesting(content_browser_client_.get());
@@ -33,6 +40,27 @@ ExtensionsTest::ExtensionsTest()
   ExtensionsBrowserClient::Set(extensions_browser_client_.get());
   extensions_browser_client_->set_extension_system_factory(
       &extension_system_factory_);
+  extensions_browser_client_->SetIncognitoContext(incognito_context_.get());
+
+  // Set up all the dependencies of ExtensionPrefs.
+  extension_pref_value_map_.reset(new ExtensionPrefValueMap());
+  PrefServiceFactory factory;
+  factory.set_user_prefs(new TestingPrefStore());
+  factory.set_extension_prefs(new TestingPrefStore());
+  user_prefs::PrefRegistrySyncable* pref_registry =
+      new user_prefs::PrefRegistrySyncable();
+  // Prefs should be registered before the PrefService is created.
+  ExtensionPrefs::RegisterProfilePrefs(pref_registry);
+  pref_service_ = factory.Create(pref_registry);
+
+  std::unique_ptr<ExtensionPrefs> extension_prefs(ExtensionPrefs::Create(
+      browser_context(), pref_service_.get(),
+      browser_context()->GetPath().AppendASCII("Extensions"),
+      extension_pref_value_map_.get(), false /* extensions_disabled */,
+      std::vector<ExtensionPrefsObserver*>()));
+
+  ExtensionPrefsFactory::GetInstance()->SetInstanceForTesting(
+      browser_context(), std::move(extension_prefs));
 }
 
 ExtensionsTest::~ExtensionsTest() {
@@ -55,6 +83,7 @@ void ExtensionsTest::TearDown() {
       browser_context_.get());
   extensions_browser_client_.reset();

[Devlin, 2017/04/04 14:18:11]

We initialize this in the ctor, but destroy it in tear down; that can cause
problems.  Also, it's not unset as the singleton until the dtor, so the
singleton instance can easily lead to UAF.

[karandeepb, 2017/04/04 19:16:42]

Added a TODO.

   browser_context_.reset();
+  incognito_context_.reset();
 }
 
 }  // namespace extensions