Extensions: Pull duplicated functionality into ExtensionsTest fixture.

extensions/browser/extensions_test.cc

 // Copyright 2014 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 "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"
 
 namespace extensions {
 
 // This class does work in the constructor instead of SetUp() to give subclasses

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

This isn't related to this patch set at all, but this code is broken.

It claims it does this to allow subclasses do work in their ctors, but that's
kind of a broken pattern (see also below).  Instead, it subclasses should also
do work in their own SetUp methods.

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

Filed a bug and added a TODO.

 // a valid BrowserContext to use while initializing their members. For example:
 //
 // class MyExtensionsTest : public ExtensionsTest {
 //   MyExtensionsTest()
 //     : my_object_(browser_context())) {
 //   }
 // };
 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());
   content::SetUtilityClientForTesting(content_utility_client_.get());
   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() {
   ExtensionsBrowserClient::Set(nullptr);
   content::SetBrowserClientForTesting(nullptr);
   content::SetUtilityClientForTesting(nullptr);
 }
 
 void ExtensionsTest::SetUp() {
   // Crashing here? Don't use this class in Chrome's unit_tests. See header.
   BrowserContextDependencyManager::GetInstance()
       ->CreateBrowserContextServicesForTest(browser_context_.get());
 }
 
 void ExtensionsTest::TearDown() {
   // Allows individual tests to have BrowserContextKeyedServiceFactory objects

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

This seems to be in direct violation of a comment in the header (though this is
probably more correct to do)

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

Removed the comment.

   // as member variables instead of singletons. The individual services will be
   // cleaned up before the factories are destroyed.
   BrowserContextDependencyManager::GetInstance()->DestroyBrowserContextServices(
       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