Add DCHECKs to StatefulExternalExtensionProvider

Also try to make the handling of pref_ more robust by making it private and always initializing it in the constructor of subclasses.

BUG=65925
TEST=ExtensionManagementTest.{ExternalUrlUpdate,ExternalPolicyRefresh}

Review URL: http://codereview.chromium.org/5784004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69532 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 70 insertions, 14 deletions

diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 16b66c4..afb7e04 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -302,9 +302,8 @@ ExtensionServiceBackend::ExtensionServiceBackend(
   // variable so that UpdateExternalPolicyExtensionProvider can access it and
   // update its extension list later.
   external_policy_extension_provider_.reset(
-      new ExternalPolicyExtensionProvider());
-  external_policy_extension_provider_->SetPreferences(
-      prefs->GetList(prefs::kExtensionInstallForceList));
+      new ExternalPolicyExtensionProvider(
+          prefs->GetList(prefs::kExtensionInstallForceList)));
   external_extension_providers_.push_back(external_policy_extension_provider_);
 }
 
diff --git a/chrome/browser/extensions/external_policy_extension_provider.cc b/chrome/browser/extensions/external_policy_extension_provider.cc
index 5b0e489..5e87379 100644
--- a/chrome/browser/extensions/external_policy_extension_provider.cc
+++ b/chrome/browser/extensions/external_policy_extension_provider.cc
@@ -7,6 +7,7 @@
 #include "base/logging.h"
 #include "base/values.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/browser/browser_thread.h"
 #include "chrome/browser/extensions/stateful_external_extension_provider.h"
 #include "chrome/browser/prefs/pref_service.h"
 
@@ -30,16 +31,27 @@ bool CheckExtension(std::string id, std::string update_url) {
 
 }
 
-ExternalPolicyExtensionProvider::ExternalPolicyExtensionProvider()
-  : StatefulExternalExtensionProvider(Extension::INVALID,
-                                      Extension::EXTERNAL_POLICY_DOWNLOAD) {
+ExternalPolicyExtensionProvider::ExternalPolicyExtensionProvider(
+    const ListValue* forcelist)
+        : StatefulExternalExtensionProvider(
+            Extension::INVALID,
+            Extension::EXTERNAL_POLICY_DOWNLOAD) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  ProcessPreferences(forcelist);
 }
 
 ExternalPolicyExtensionProvider::~ExternalPolicyExtensionProvider() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 void ExternalPolicyExtensionProvider::SetPreferences(
     const ListValue* forcelist) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  ProcessPreferences(forcelist);
+}
+
+void ExternalPolicyExtensionProvider::ProcessPreferences(
+    const ListValue* forcelist) {
   DictionaryValue* result = new DictionaryValue();
   if (forcelist != NULL) {
     std::string extension_desc;
@@ -60,5 +72,5 @@ void ExternalPolicyExtensionProvider::SetPreferences(
       }
     }
   }
-  prefs_.reset(result);
+  set_prefs(result);
 }
diff --git a/chrome/browser/extensions/external_policy_extension_provider.h b/chrome/browser/extensions/external_policy_extension_provider.h
index bb4cc7d..9c36960 100644
--- a/chrome/browser/extensions/external_policy_extension_provider.h
+++ b/chrome/browser/extensions/external_policy_extension_provider.h
@@ -14,11 +14,14 @@ class PrefService;
 
 // A specialization of the ExternalExtensionProvider that uses
 // prefs::kExtensionInstallForceList to look up which external extensions are
-// registered.
+// registered. The value of this preference is received via the constructor and
+// via |SetPreferences| in case of run-time updates.
+// Instances of this class are expected to be created and destroyed on the UI
+// thread and they are expecting public method calls from the FILE thread.
 class ExternalPolicyExtensionProvider
     : public StatefulExternalExtensionProvider {
  public:
-  explicit ExternalPolicyExtensionProvider();
+  explicit ExternalPolicyExtensionProvider(const ListValue* forcelist);
   virtual ~ExternalPolicyExtensionProvider();
 
   // Set the internal list of extensions based on |forcelist|.
@@ -28,6 +31,9 @@ class ExternalPolicyExtensionProvider
  private:
   friend class MockExternalPolicyExtensionProviderVisitor;
 
+  // Set the internal list of extensions based on |forcelist|.
+  // Does not take ownership of |forcelist|.
+  void ProcessPreferences(const ListValue* forcelist);
 };
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTERNAL_POLICY_EXTENSION_PROVIDER_H_
diff --git a/chrome/browser/extensions/external_policy_extension_provider_unittest.cc b/chrome/browser/extensions/external_policy_extension_provider_unittest.cc
index 9c3056a..cd5a3df 100644
--- a/chrome/browser/extensions/external_policy_extension_provider_unittest.cc
+++ b/chrome/browser/extensions/external_policy_extension_provider_unittest.cc
@@ -7,11 +7,26 @@
 #include "base/logging.h"
 #include "base/values.h"
 #include "base/version.h"
+#include "chrome/browser/browser_thread.h"
 #include "chrome/browser/extensions/external_policy_extension_provider.h"
 #include "chrome/common/extensions/extension.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 class ExternalPolicyExtensionProviderTest : public testing::Test {
+ public:
+  ExternalPolicyExtensionProviderTest()
+      : loop_(MessageLoop::TYPE_IO),
+        ui_thread_(BrowserThread::UI, &loop_),
+        file_thread_(BrowserThread::FILE, &loop_) {
+  }
+
+  virtual ~ExternalPolicyExtensionProviderTest() {
+  }
+
+ private:
+  MessageLoop loop_;
+  BrowserThread ui_thread_;
+  BrowserThread file_thread_;
 };
 
 class MockExternalPolicyExtensionProviderVisitor
@@ -25,9 +40,7 @@ class MockExternalPolicyExtensionProviderVisitor
   void Visit(ListValue* policy_forcelist,
              ListValue* policy_validlist,
              const std::set<std::string>& ignore_list) {
-    provider_.reset(new ExternalPolicyExtensionProvider());
-    // Give the list extensions to the provider.
-    provider_->SetPreferences(policy_forcelist);
+    provider_.reset(new ExternalPolicyExtensionProvider(policy_forcelist));
 
     // Extensions will be removed from this list as they visited,
     // so it should be emptied by the end.
diff --git a/chrome/browser/extensions/external_pref_extension_provider.cc b/chrome/browser/extensions/external_pref_extension_provider.cc
index 7580388..7ea5f4a 100644
--- a/chrome/browser/extensions/external_pref_extension_provider.cc
+++ b/chrome/browser/extensions/external_pref_extension_provider.cc
@@ -9,12 +9,14 @@
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/path_service.h"
+#include "chrome/browser/browser_thread.h"
 #include "chrome/browser/extensions/stateful_external_extension_provider.h"
 #include "chrome/common/json_value_serializer.h"
 
 ExternalPrefExtensionProvider::ExternalPrefExtensionProvider()
   : StatefulExternalExtensionProvider(Extension::EXTERNAL_PREF,
                                       Extension::EXTERNAL_PREF_DOWNLOAD) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   FilePath json_file;
   PathService::Get(app::DIR_EXTERNAL_EXTENSIONS, &json_file);
   json_file = json_file.Append(FILE_PATH_LITERAL("external_extensions.json"));
@@ -23,11 +25,12 @@ ExternalPrefExtensionProvider::ExternalPrefExtensionProvider()
     JSONFileValueSerializer serializer(json_file);
     SetPreferences(&serializer);
   } else {
-    prefs_.reset(new DictionaryValue());
+    set_prefs(new DictionaryValue());
   }
 }
 
 ExternalPrefExtensionProvider::~ExternalPrefExtensionProvider() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 void ExternalPrefExtensionProvider::SetPreferencesForTesting(
@@ -50,5 +53,5 @@ void ExternalPrefExtensionProvider::SetPreferences(
       dictionary.reset(static_cast<DictionaryValue*>(extensions));
     }
   }
-  prefs_.reset(dictionary.release());
+  set_prefs(dictionary.release());
 }
diff --git a/chrome/browser/extensions/external_pref_extension_provider.h b/chrome/browser/extensions/external_pref_extension_provider.h
index 8b9eb0b..b74be39 100644
--- a/chrome/browser/extensions/external_pref_extension_provider.h
+++ b/chrome/browser/extensions/external_pref_extension_provider.h
@@ -10,6 +10,8 @@
 
 // A specialization of the ExternalExtensionProvider that uses a json file to
 // look up which external extensions are registered.
+// Instances of this class are expected to be created and destroyed on the UI
+// thread and they are expecting public method calls from the FILE thread.
 class ExternalPrefExtensionProvider : public StatefulExternalExtensionProvider {
  public:
   explicit ExternalPrefExtensionProvider();
diff --git a/chrome/browser/extensions/stateful_external_extension_provider.cc b/chrome/browser/extensions/stateful_external_extension_provider.cc
index d922db8..fd1a42a 100644
--- a/chrome/browser/extensions/stateful_external_extension_provider.cc
+++ b/chrome/browser/extensions/stateful_external_extension_provider.cc
@@ -10,6 +10,7 @@
 #include "base/path_service.h"
 #include "base/values.h"
 #include "base/version.h"
+#include "chrome/browser/browser_thread.h"
 
 namespace {
 
@@ -27,13 +28,17 @@ StatefulExternalExtensionProvider::StatefulExternalExtensionProvider(
     Extension::Location download_location)
   : crx_location_(crx_location),
     download_location_(download_location) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 StatefulExternalExtensionProvider::~StatefulExternalExtensionProvider() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 void StatefulExternalExtensionProvider::VisitRegisteredExtension(
     Visitor* visitor) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(prefs_.get());
   for (DictionaryValue::key_iterator i = prefs_->begin_keys();
        i != prefs_->end_keys(); ++i) {
     const std::string& extension_id = *i;
@@ -120,12 +125,16 @@ void StatefulExternalExtensionProvider::VisitRegisteredExtension(
 
 bool StatefulExternalExtensionProvider::HasExtension(
     const std::string& id) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(prefs_.get());
   return prefs_->HasKey(id);
 }
 
 bool StatefulExternalExtensionProvider::GetExtensionDetails(
     const std::string& id, Extension::Location* location,
     scoped_ptr<Version>* version) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(prefs_.get());
   DictionaryValue* extension = NULL;
   if (!prefs_->GetDictionary(id, &extension))
     return false;
@@ -154,3 +163,7 @@ bool StatefulExternalExtensionProvider::GetExtensionDetails(
 
   return true;
 }
+
+void StatefulExternalExtensionProvider::set_prefs(DictionaryValue* prefs) {
+  prefs_.reset(prefs);
+}
diff --git a/chrome/browser/extensions/stateful_external_extension_provider.h b/chrome/browser/extensions/stateful_external_extension_provider.h
index 141f45d..2fd481c 100644
--- a/chrome/browser/extensions/stateful_external_extension_provider.h
+++ b/chrome/browser/extensions/stateful_external_extension_provider.h
@@ -20,6 +20,8 @@ class Version;
 // This provider can provide external extensions from two sources: crx files
 // and udpate URLs. The locations that the provider will report for these
 // are specified at the constructor.
+// Instances of this class are expected to be created and destroyed on the UI
+// thread and they are expecting public method calls from the FILE thread.
 class StatefulExternalExtensionProvider : public ExternalExtensionProvider {
  public:
   // Initialize the location for external extensions originating from crx
@@ -46,6 +48,12 @@ class StatefulExternalExtensionProvider : public ExternalExtensionProvider {
   // Location for external extensions that are provided by this provider from
   // update URLs.
   const Extension::Location download_location_;
+
+  // Stores the dictionary of external extensions internally. Takes ownership
+  // of |prefs|.
+  void set_prefs(DictionaryValue* prefs);
+
+ private:
   // Dictionary of the external extensions that are provided by this provider.
   scoped_ptr<DictionaryValue> prefs_;
 };