[Extensions] Fix IsBeingUpgraded runtime data bug and make RuntimeData take ids
Fix a bug where IsBeingUpgraded would be reset during the reload process (even
though it's also *set* during the reload process...), and make RuntimeData take
extension ids instead of extensions.
BUG=435336
[email protected] (micro change in ui/toolbar/)
Review URL: https://codereview.chromium.org/743373002
Cr-Commit-Position: refs/heads/master@{#305134}
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 283f42a1..c96dc5a 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -608,7 +608,7 @@
path = transient_current_extension->path();
// BeingUpgraded is set back to false when the extension is added.
- system_->runtime_data()->SetBeingUpgraded(transient_current_extension,
+ system_->runtime_data()->SetBeingUpgraded(transient_current_extension->id(),
true);
DisableExtension(extension_id, Extension::DISABLE_RELOAD);
reloading_extensions_.insert(extension_id);
@@ -1342,7 +1342,12 @@
if (!Manifest::IsUnpackedLocation(extension->location()))
CHECK_GE(version_compare_result, 0);
}
- system_->runtime_data()->SetBeingUpgraded(extension, is_extension_upgrade);
+ // If the extension was disabled for a reload, then enable it.
+ bool reloading = reloading_extensions_.erase(extension->id()) > 0;
+
+ // Set the upgraded bit; we consider reloads upgrades.
+ system_->runtime_data()->SetBeingUpgraded(extension->id(),
+ is_extension_upgrade || reloading);
// The extension is now loaded, remove its data from unloaded extension map.
unloaded_extension_paths_.erase(extension->id());
@@ -1350,9 +1355,6 @@
// If a terminated extension is loaded, remove it from the terminated list.
UntrackTerminatedExtension(extension->id());
- // If the extension was disabled for a reload, then enable it.
- bool reloading = reloading_extensions_.erase(extension->id()) > 0;
-
// Check if the extension's privileges have changed and mark the
// extension disabled if necessary.
CheckPermissionsIncrease(extension, is_extension_installed);
@@ -1414,7 +1416,7 @@
extension_sync_service_->SyncExtensionChangeIfNeeded(*extension);
NotifyExtensionLoaded(extension);
}
- system_->runtime_data()->SetBeingUpgraded(extension, false);
+ system_->runtime_data()->SetBeingUpgraded(extension->id(), false);
}
void ExtensionService::AddComponentExtension(const Extension* extension) {
diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
index 7dec674..5f57805 100644
--- a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
+++ b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc
@@ -314,7 +314,7 @@
// If this is just an upgrade, then don't worry about resizing.
if (extensions::ExtensionSystem::Get(browser_->profile())->runtime_data()->
- IsBeingUpgraded(extension)) {
+ IsBeingUpgraded(extension->id())) {
delegate_->Redraw(false);
} else {
// We may need to resize (e.g. to show the new icon, or the chevron).
@@ -340,7 +340,7 @@
// If the extension is being upgraded we don't want the bar to shrink
// because the icon is just going to get re-added to the same location.
if (!extensions::ExtensionSystem::Get(browser_->profile())->runtime_data()->
- IsBeingUpgraded(extension)) {
+ IsBeingUpgraded(extension->id())) {
if (toolbar_actions_.size() > model_->visible_icon_count()) {
// If we have more icons than we can show, then we must not be changing
// the container size (since we either removed an icon from the main
diff --git a/extensions/browser/api/declarative_webrequest/webrequest_rules_registry.cc b/extensions/browser/api/declarative_webrequest/webrequest_rules_registry.cc
index e95deef..21f7a0a0 100644
--- a/extensions/browser/api/declarative_webrequest/webrequest_rules_registry.cc
+++ b/extensions/browser/api/declarative_webrequest/webrequest_rules_registry.cc
@@ -233,7 +233,7 @@
content::BrowserThread::PostTask(
content::BrowserThread::UI, FROM_HERE,
base::Bind(&extension_web_request_api_helpers::NotifyWebRequestAPIUsed,
- browser_context_, make_scoped_refptr(extension)));
+ browser_context_, extension->id()));
}
return std::string();
diff --git a/extensions/browser/api/web_request/web_request_api.cc b/extensions/browser/api/web_request/web_request_api.cc
index c1e8f9d3..509dd98 100644
--- a/extensions/browser/api/web_request/web_request_api.cc
+++ b/extensions/browser/api/web_request/web_request_api.cc
@@ -2256,7 +2256,7 @@
FROM_HERE,
base::Bind(&helpers::NotifyWebRequestAPIUsed,
profile_id(),
- make_scoped_refptr(extension)));
+ extension->id()));
return true;
}
diff --git a/extensions/browser/api/web_request/web_request_api_helpers.cc b/extensions/browser/api/web_request/web_request_api_helpers.cc
index 825b0bd..7dd2739f 100644
--- a/extensions/browser/api/web_request/web_request_api_helpers.cc
+++ b/extensions/browser/api/web_request/web_request_api_helpers.cc
@@ -1194,9 +1194,8 @@
}
}
-void NotifyWebRequestAPIUsed(
- void* browser_context_id,
- scoped_refptr<const extensions::Extension> extension) {
+void NotifyWebRequestAPIUsed(void* browser_context_id,
+ const std::string& extension_id) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
content::BrowserContext* browser_context =
reinterpret_cast<content::BrowserContext*>(browser_context_id);
@@ -1206,11 +1205,9 @@
extensions::RuntimeData* runtime_data =
extensions::ExtensionSystem::Get(browser_context)->runtime_data();
- if (extension.get()) {
- if (runtime_data->HasUsedWebRequest(extension.get()))
- return;
- runtime_data->SetHasUsedWebRequest(extension.get(), true);
- }
+ if (runtime_data->HasUsedWebRequest(extension_id))
+ return;
+ runtime_data->SetHasUsedWebRequest(extension_id, true);
for (content::RenderProcessHost::iterator it =
content::RenderProcessHost::AllHostsIterator();
@@ -1234,7 +1231,7 @@
for (extensions::ExtensionSet::const_iterator it = extensions.begin();
!webrequest_used && it != extensions.end();
++it) {
- webrequest_used |= runtime_data->HasUsedWebRequest(it->get());
+ webrequest_used |= runtime_data->HasUsedWebRequest((*it)->id());
}
host->Send(new ExtensionMsg_UsingWebRequestAPI(webrequest_used));
diff --git a/extensions/browser/api/web_request/web_request_api_helpers.h b/extensions/browser/api/web_request/web_request_api_helpers.h
index a7ae5dd6..d5ea20a 100644
--- a/extensions/browser/api/web_request/web_request_api_helpers.h
+++ b/extensions/browser/api/web_request/web_request_api_helpers.h
@@ -310,12 +310,11 @@
void ClearCacheOnNavigation();
// Tells renderer processes that the web request or declarative web request
-// API has been used by |extension| in browser_context |browser_context_id| to
-// collect UMA statistics on Page Load Times. Needs to be called on the UI
-// thread.
-void NotifyWebRequestAPIUsed(
- void* browser_context_id,
- scoped_refptr<const extensions::Extension> extension);
+// API has been used by the extension with the given |extension_id| in the
+// given |browser_context_id| to collect UMA statistics on Page Load Times.
+// Needs to be called on the UI thread.
+void NotifyWebRequestAPIUsed(void* browser_context_id,
+ const std::string& extension_id);
// Send updates to |host| with information about what webRequest-related
// extensions are installed.
diff --git a/extensions/browser/extension_host.cc b/extensions/browser/extension_host.cc
index 4f2941b..9aa14c3 100644
--- a/extensions/browser/extension_host.cc
+++ b/extensions/browser/extension_host.cc
@@ -313,7 +313,7 @@
DCHECK(extension_host_type_ == VIEW_TYPE_EXTENSION_BACKGROUND_PAGE);
ExtensionSystem::Get(browser_context_)
->runtime_data()
- ->SetBackgroundPageReady(extension_, true);
+ ->SetBackgroundPageReady(extension_->id(), true);
content::NotificationService::current()->Notify(
extensions::NOTIFICATION_EXTENSION_BACKGROUND_PAGE_READY,
content::Source<const Extension>(extension_),
diff --git a/extensions/browser/runtime_data.cc b/extensions/browser/runtime_data.cc
index 8f359bc..3cee846 100644
--- a/extensions/browser/runtime_data.cc
+++ b/extensions/browser/runtime_data.cc
@@ -21,32 +21,35 @@
bool RuntimeData::IsBackgroundPageReady(const Extension* extension) const {
if (!BackgroundInfo::HasPersistentBackgroundPage(extension))
return true;
- return HasFlag(extension, BACKGROUND_PAGE_READY);
+ return HasFlag(extension->id(), BACKGROUND_PAGE_READY);
}
-void RuntimeData::SetBackgroundPageReady(const Extension* extension,
+void RuntimeData::SetBackgroundPageReady(const std::string& extension_id,
bool value) {
- SetFlag(extension, BACKGROUND_PAGE_READY, value);
+ SetFlag(extension_id, BACKGROUND_PAGE_READY, value);
}
-bool RuntimeData::IsBeingUpgraded(const Extension* extension) const {
- return HasFlag(extension, BEING_UPGRADED);
+bool RuntimeData::IsBeingUpgraded(const std::string& extension_id) const {
+ return HasFlag(extension_id, BEING_UPGRADED);
}
-void RuntimeData::SetBeingUpgraded(const Extension* extension, bool value) {
- SetFlag(extension, BEING_UPGRADED, value);
+void RuntimeData::SetBeingUpgraded(const std::string& extension_id,
+ bool value) {
+ SetFlag(extension_id, BEING_UPGRADED, value);
}
-bool RuntimeData::HasUsedWebRequest(const Extension* extension) const {
- return HasFlag(extension, HAS_USED_WEBREQUEST);
+bool RuntimeData::HasUsedWebRequest(const std::string& extension_id) const {
+ return HasFlag(extension_id, HAS_USED_WEBREQUEST);
}
-void RuntimeData::SetHasUsedWebRequest(const Extension* extension, bool value) {
- SetFlag(extension, HAS_USED_WEBREQUEST, value);
+void RuntimeData::SetHasUsedWebRequest(const std::string& extension_id,
+ bool value) {
+ SetFlag(extension_id, HAS_USED_WEBREQUEST, value);
}
-bool RuntimeData::HasExtensionForTesting(const Extension* extension) const {
- return extension_flags_.find(extension->id()) != extension_flags_.end();
+bool RuntimeData::HasExtensionForTesting(
+ const std::string& extension_id) const {
+ return extension_flags_.find(extension_id) != extension_flags_.end();
}
void RuntimeData::ClearAll() {
@@ -56,23 +59,26 @@
void RuntimeData::OnExtensionUnloaded(content::BrowserContext* browser_context,
const Extension* extension,
UnloadedExtensionInfo::Reason reason) {
- extension_flags_.erase(extension->id());
+ ExtensionFlagsMap::iterator iter = extension_flags_.find(extension->id());
+ if (iter != extension_flags_.end())
+ iter->second = iter->second & kPersistAcrossUnloadMask;
}
-bool RuntimeData::HasFlag(const Extension* extension, RuntimeFlag flag) const {
- ExtensionFlagsMap::const_iterator it = extension_flags_.find(extension->id());
+bool RuntimeData::HasFlag(const std::string& extension_id,
+ RuntimeFlag flag) const {
+ ExtensionFlagsMap::const_iterator it = extension_flags_.find(extension_id);
if (it == extension_flags_.end())
return false;
return !!(it->second & flag);
}
-void RuntimeData::SetFlag(const Extension* extension,
+void RuntimeData::SetFlag(const std::string& extension_id,
RuntimeFlag flag,
bool value) {
if (value)
- extension_flags_[extension->id()] |= flag;
+ extension_flags_[extension_id] |= flag;
else
- extension_flags_[extension->id()] &= ~flag;
+ extension_flags_[extension_id] &= ~flag;
}
} // namespace extensions
diff --git a/extensions/browser/runtime_data.h b/extensions/browser/runtime_data.h
index 9d74041..c937a99f7 100644
--- a/extensions/browser/runtime_data.h
+++ b/extensions/browser/runtime_data.h
@@ -13,13 +13,15 @@
namespace extensions {
-class Extension;
class ExtensionRegistry;
// Contains per-extension data that can change during the life of the process,
// but does not persist across restarts. Shared between incognito and regular
// browser contexts. Lives on the UI thread. Must be destroyed before
// ExtensionRegistry.
+// If we start putting to much into this, we should expose the generic
+// "[G|S]etRuntimeProperty(const std::string& extension_id, RuntimeFlag flag)"
+// instead of all these.
class RuntimeData : public ExtensionRegistryObserver {
public:
// Observes |registry| to clean itself up when extensions change state.
@@ -31,20 +33,21 @@
// other components until then. If there is no background page, or if it is
// non-persistent (lazy), we consider it to be ready.
bool IsBackgroundPageReady(const Extension* extension) const;
- void SetBackgroundPageReady(const Extension* extension, bool value);
+ void SetBackgroundPageReady(const std::string& extension_id, bool value);
// Getter and setter for the flag that specifies whether the extension is
// being upgraded.
- bool IsBeingUpgraded(const Extension* extension) const;
- void SetBeingUpgraded(const Extension* extension, bool value);
+ // For these purposes, a reload counts as an upgrade.
+ bool IsBeingUpgraded(const std::string& extension_id) const;
+ void SetBeingUpgraded(const std::string& extension_id, bool value);
// Getter and setter for the flag that specifies if the extension has used
// the webrequest API.
- bool HasUsedWebRequest(const Extension* extension) const;
- void SetHasUsedWebRequest(const Extension* extension, bool value);
+ bool HasUsedWebRequest(const std::string& extension_id) const;
+ void SetHasUsedWebRequest(const std::string& extension_id, bool value);
// Returns true if the extension is being tracked. Used only for testing.
- bool HasExtensionForTesting(const Extension* extension) const;
+ bool HasExtensionForTesting(const std::string& extension_id) const;
// Erase runtime data for all extensions. Used only for testing. Cannot be
// named ClearAllForTesting due to false-positive presubmit errors.
@@ -66,12 +69,16 @@
HAS_USED_WEBREQUEST = 1 << 2,
};
+ // The mask of any data that should persist across the extension being
+ // unloaded.
+ static const int kPersistAcrossUnloadMask = BEING_UPGRADED;
+
// Returns the setting for the flag or false if the extension isn't found.
- bool HasFlag(const Extension* extension, RuntimeFlag flag) const;
+ bool HasFlag(const std::string& extension_id, RuntimeFlag flag) const;
// Sets |flag| for |extension| to |value|. Adds |extension| to the list of
// extensions if it isn't present.
- void SetFlag(const Extension* extension, RuntimeFlag flag, bool value);
+ void SetFlag(const std::string& extension_id, RuntimeFlag flag, bool value);
// Map from extension ID to the RuntimeFlags bits.
typedef std::map<std::string, int> ExtensionFlagsMap;
diff --git a/extensions/browser/runtime_data_unittest.cc b/extensions/browser/runtime_data_unittest.cc
index 451308c..bec1e130 100644
--- a/extensions/browser/runtime_data_unittest.cc
+++ b/extensions/browser/runtime_data_unittest.cc
@@ -53,9 +53,9 @@
EXPECT_FALSE(runtime_data_.IsBackgroundPageReady(with_background.get()));
// The flag can be toggled.
- runtime_data_.SetBackgroundPageReady(with_background.get(), true);
+ runtime_data_.SetBackgroundPageReady(with_background->id(), true);
EXPECT_TRUE(runtime_data_.IsBackgroundPageReady(with_background.get()));
- runtime_data_.SetBackgroundPageReady(with_background.get(), false);
+ runtime_data_.SetBackgroundPageReady(with_background->id(), false);
EXPECT_FALSE(runtime_data_.IsBackgroundPageReady(with_background.get()));
}
@@ -63,37 +63,41 @@
scoped_refptr<Extension> extension = test_util::CreateEmptyExtension();
// An extension is not being upgraded until the flag is set.
- EXPECT_FALSE(runtime_data_.IsBeingUpgraded(extension.get()));
+ EXPECT_FALSE(runtime_data_.IsBeingUpgraded(extension->id()));
// The flag can be toggled.
- runtime_data_.SetBeingUpgraded(extension.get(), true);
- EXPECT_TRUE(runtime_data_.IsBeingUpgraded(extension.get()));
- runtime_data_.SetBeingUpgraded(extension.get(), false);
- EXPECT_FALSE(runtime_data_.IsBeingUpgraded(extension.get()));
+ runtime_data_.SetBeingUpgraded(extension->id(), true);
+ EXPECT_TRUE(runtime_data_.IsBeingUpgraded(extension->id()));
+ runtime_data_.SetBeingUpgraded(extension->id(), false);
+ EXPECT_FALSE(runtime_data_.IsBeingUpgraded(extension->id()));
}
TEST_F(RuntimeDataTest, HasUsedWebRequest) {
scoped_refptr<Extension> extension = test_util::CreateEmptyExtension();
// An extension has not used web request until the flag is set.
- EXPECT_FALSE(runtime_data_.HasUsedWebRequest(extension.get()));
+ EXPECT_FALSE(runtime_data_.HasUsedWebRequest(extension->id()));
// The flag can be toggled.
- runtime_data_.SetHasUsedWebRequest(extension.get(), true);
- EXPECT_TRUE(runtime_data_.HasUsedWebRequest(extension.get()));
- runtime_data_.SetHasUsedWebRequest(extension.get(), false);
- EXPECT_FALSE(runtime_data_.HasUsedWebRequest(extension.get()));
+ runtime_data_.SetHasUsedWebRequest(extension->id(), true);
+ EXPECT_TRUE(runtime_data_.HasUsedWebRequest(extension->id()));
+ runtime_data_.SetHasUsedWebRequest(extension->id(), false);
+ EXPECT_FALSE(runtime_data_.HasUsedWebRequest(extension->id()));
}
-// Unloading an extension stops tracking it.
+// Unloading an extension erases any data that shouldn't explicitly be kept
+// across loads.
TEST_F(RuntimeDataTest, OnExtensionUnloaded) {
scoped_refptr<Extension> extension = CreateExtensionWithBackgroundPage();
- runtime_data_.SetBackgroundPageReady(extension.get(), true);
- ASSERT_TRUE(runtime_data_.HasExtensionForTesting(extension.get()));
+ runtime_data_.SetBackgroundPageReady(extension->id(), true);
+ ASSERT_TRUE(runtime_data_.HasExtensionForTesting(extension->id()));
+ runtime_data_.SetBeingUpgraded(extension->id(), true);
runtime_data_.OnExtensionUnloaded(
NULL, extension.get(), UnloadedExtensionInfo::REASON_DISABLE);
- EXPECT_FALSE(runtime_data_.HasExtensionForTesting(extension.get()));
+ EXPECT_TRUE(runtime_data_.HasExtensionForTesting(extension->id()));
+ EXPECT_FALSE(runtime_data_.IsBackgroundPageReady(extension.get()));
+ EXPECT_TRUE(runtime_data_.IsBeingUpgraded(extension->id()));
}
} // namespace
