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
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 @@
// 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 5b0e48903..5e873793 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 @@
}
-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 @@
}
}
}
- 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 @@
// 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 @@
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 @@
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..7ea5f4a2 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 @@
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 @@
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..b74be396 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 d922db83..fd1a42af 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 @@
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 @@
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 @@
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..2fd481cb 100644
--- a/chrome/browser/extensions/stateful_external_extension_provider.h
+++ b/chrome/browser/extensions/stateful_external_extension_provider.h
@@ -20,6 +20,8 @@
// 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 @@
// 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_;
};
