Fix Threading of ExtensionsQuotaService

The ExtensionsQuotaService uses a RepeatingTimer that is triggered on the thread it was created. With this CL, we ensure that the ExtensionsQuotaService is created, accessed and destroyed on the same thread.

BUG=118655
TEST=no


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

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

6 files changed, 42 insertions, 12 deletions

diff --git a/chrome/browser/extensions/extension_function.h b/chrome/browser/extensions/extension_function.h
index 45c7326..8c08a92 100644
--- a/chrome/browser/extensions/extension_function.h
+++ b/chrome/browser/extensions/extension_function.h
@@ -84,7 +84,9 @@ class ExtensionFunction
   // NULL-check.
   virtual void Run();
 
-  // Returns a quota limit heuristic suitable for this function.
+  // Optionally adds one or multiple QuotaLimitHeuristic instances suitable for
+  // this function to |heuristics|. The ownership of the new QuotaLimitHeuristic
+  // instances is passed to the owner of |heuristics|.
   // No quota limiting by default.
   virtual void GetQuotaLimitHeuristics(
       QuotaLimitHeuristics* heuristics) const {}
diff --git a/chrome/browser/extensions/extension_function_dispatcher.cc b/chrome/browser/extensions/extension_function_dispatcher.cc
index d1344be..0a95858 100644
--- a/chrome/browser/extensions/extension_function_dispatcher.cc
+++ b/chrome/browser/extensions/extension_function_dispatcher.cc
@@ -126,7 +126,7 @@ void ExtensionFunctionDispatcher::DispatchOnIOThread(
   function->set_include_incognito(
       extension_info_map->IsIncognitoEnabled(extension->id()));
 
-  ExtensionsQuotaService* quota = extension_info_map->quota_service();
+  ExtensionsQuotaService* quota = extension_info_map->GetQuotaService();
   if (quota->Assess(extension->id(), function, &params.arguments,
                     base::TimeTicks::Now())) {
     function->Run();
diff --git a/chrome/browser/extensions/extension_info_map.cc b/chrome/browser/extensions/extension_info_map.cc
index 929e106..82492d9 100644
--- a/chrome/browser/extensions/extension_info_map.cc
+++ b/chrome/browser/extensions/extension_info_map.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -42,6 +42,10 @@ ExtensionInfoMap::ExtensionInfoMap() {
 }
 
 ExtensionInfoMap::~ExtensionInfoMap() {
+  if (quota_service_.get()) {
+    BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE,
+                              quota_service_.release());
+  }
 }
 
 const extensions::ProcessMap& ExtensionInfoMap::process_map() const {
@@ -148,3 +152,10 @@ bool ExtensionInfoMap::SecurityOriginHasAPIPermission(
   }
   return false;
 }
+
+ExtensionsQuotaService* ExtensionInfoMap::GetQuotaService() {
+  CheckOnValidThread();
+  if (!quota_service_.get())
+    quota_service_.reset(new ExtensionsQuotaService());
+  return quota_service_.get();
+}
diff --git a/chrome/browser/extensions/extension_info_map.h b/chrome/browser/extensions/extension_info_map.h
index a4673ac..8b00664 100644
--- a/chrome/browser/extensions/extension_info_map.h
+++ b/chrome/browser/extensions/extension_info_map.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -11,6 +11,7 @@
 #include "base/basictypes.h"
 #include "base/time.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/extensions/extensions_quota_service.h"
 #include "chrome/browser/extensions/process_map.h"
 #include "chrome/common/extensions/extension_constants.h"
@@ -70,7 +71,7 @@ class ExtensionInfoMap : public base::RefCountedThreadSafe<ExtensionInfoMap> {
       const GURL& origin, int process_id,
       ExtensionAPIPermission::ID permission) const;
 
-  ExtensionsQuotaService* quota_service() { return &quota_service_; }
+  ExtensionsQuotaService* GetQuotaService();
 
  private:
   // Extra dynamic data related to an extension.
@@ -85,7 +86,9 @@ class ExtensionInfoMap : public base::RefCountedThreadSafe<ExtensionInfoMap> {
   ExtraDataMap extra_data_;
 
   // Used by dispatchers to limit API quota for individual extensions.
-  ExtensionsQuotaService quota_service_;
+  // The ExtensionQutoaService is not thread safe. We need to create and destroy
+  // it on the IO thread.
+  scoped_ptr<ExtensionsQuotaService> quota_service_;
 
   // Assignment of extensions to processes.
   extensions::ProcessMap process_map_;
diff --git a/chrome/browser/extensions/extensions_quota_service.cc b/chrome/browser/extensions/extensions_quota_service.cc
index 0f7a9a2..d311675 100644
--- a/chrome/browser/extensions/extensions_quota_service.cc
+++ b/chrome/browser/extensions/extensions_quota_service.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -25,6 +25,7 @@ ExtensionsQuotaService::ExtensionsQuotaService() {
 }
 
 ExtensionsQuotaService::~ExtensionsQuotaService() {
+  DCHECK(CalledOnValidThread());
   purge_timer_.Stop();
   Purge();
 }
@@ -32,6 +33,8 @@ ExtensionsQuotaService::~ExtensionsQuotaService() {
 bool ExtensionsQuotaService::Assess(const std::string& extension_id,
     ExtensionFunction* function, const ListValue* args,
     const base::TimeTicks& event_time) {
+  DCHECK(CalledOnValidThread());
+
   // Lookup function list for extension.
   FunctionHeuristicsMap& functions = function_heuristics_[extension_id];
 
@@ -72,6 +75,7 @@ void ExtensionsQuotaService::PurgeFunctionHeuristicsMap(
 }
 
 void ExtensionsQuotaService::Purge() {
+  DCHECK(CalledOnValidThread());
   std::map<std::string, FunctionHeuristicsMap>::iterator it =
       function_heuristics_.begin();
   for (; it != function_heuristics_.end(); function_heuristics_.erase(it++))
diff --git a/chrome/browser/extensions/extensions_quota_service.h b/chrome/browser/extensions/extensions_quota_service.h
index 2f0cab0..6b0ce5d 100644
--- a/chrome/browser/extensions/extensions_quota_service.h
+++ b/chrome/browser/extensions/extensions_quota_service.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -22,6 +22,7 @@
 #include "base/compiler_specific.h"
 #include "base/hash_tables.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/threading/non_thread_safe.h"
 #include "base/time.h"
 #include "base/timer.h"
 #include "base/values.h"
@@ -30,7 +31,13 @@ class ExtensionFunction;
 class QuotaLimitHeuristic;
 typedef std::list<QuotaLimitHeuristic*> QuotaLimitHeuristics;
 
-class ExtensionsQuotaService {
+// The ExtensionsQuotaService takes care that calls to certain extension
+// functions do not exceed predefined quotas.
+//
+// The ExtensionsQuotaService needs to live entirely on one thread, i.e.
+// be created, called and destroyed on the same thread, due to its use
+// of a RepeatingTimer.
+class ExtensionsQuotaService : public base::NonThreadSafe {
  public:
   // Some concrete heuristics (declared below) that ExtensionFunctions can
   // use to help the service make decisions about quota violations.
@@ -38,7 +45,7 @@ class ExtensionsQuotaService {
   class SustainedLimit;
 
   ExtensionsQuotaService();
-  ~ExtensionsQuotaService();
+  virtual ~ExtensionsQuotaService();
 
   // Decide whether the invocation of |function| with argument |args| by the
   // extension specified by |extension_id| results in a quota limit violation.
@@ -48,7 +55,10 @@ class ExtensionsQuotaService {
               const ListValue* args, const base::TimeTicks& event_time);
  private:
   friend class ExtensionTestQuotaResetFunction;
-  typedef std::map<std::string, QuotaLimitHeuristics> FunctionHeuristicsMap;
+  typedef std::string ExtensionId;
+  typedef std::string FunctionName;
+  // All QuotaLimitHeuristic instances in this map are owned by us.
+  typedef std::map<FunctionName, QuotaLimitHeuristics> FunctionHeuristicsMap;
 
   // Purge resets all accumulated data (except |violators_|) as if the service
   // was just created. Called periodically so we don't consume an unbounded
@@ -65,7 +75,7 @@ class ExtensionsQuotaService {
   // key for the mapping.  As an extension invokes functions, the map keeps
   // track of which functions it has invoked and the heuristics for each one.
   // Each heuristic will be evaluated and ANDed together to get a final answer.
-  std::map<std::string, FunctionHeuristicsMap> function_heuristics_;
+  std::map<ExtensionId, FunctionHeuristicsMap> function_heuristics_;
 
   // For now, as soon as an extension violates quota, we don't allow it to
   // make any more requests to quota limited functions.  This provides a quick