Resubmit 21609003: Move ExtensionError to extensions/, add error limits

Added in a fix for the memory leak.

Move ExtensionError class to extensions/browser/, since it doesn't need to be in chrome/.
Limit the number of errors stored per extension to 100.
Store errors in a map, keyed by Extension ID, since that is how we will likely be accessing them.

BUG=21734

Review URL: https://chromiumcodereview.appspot.com/22647007

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

chrome/browser/extensions/error_console/error_console.cc

@@ -4,12 +4,14 @@
 
 #include "chrome/browser/extensions/error_console/error_console.h"
 
-#include <algorithm>
+#include <list>
 
+#include "base/lazy_instance.h"
+#include "base/stl_util.h"
 #include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/extensions/error_console/extension_error.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/extension.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
@@ -17,6 +19,29 @@
 
 namespace extensions {
 
+namespace {
+
+const size_t kMaxErrorsPerExtension = 100;
+
+// Iterate through an error list and remove and delete all errors which were
+// from an incognito context.
+void DeleteIncognitoErrorsFromList(ErrorConsole::ErrorList* list) {
+  ErrorConsole::ErrorList::iterator iter = list->begin();
+  while (iter != list->end()) {
+    if ((*iter)->from_incognito()) {
+      delete *iter;
+      iter = list->erase(iter);
+    } else {
+      ++iter;
+    }
+  }
+}
+
+base::LazyInstance<ErrorConsole::ErrorList> g_empty_error_list =
+    LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace
+
 void ErrorConsole::Observer::OnErrorConsoleDestroyed() {
 }
 
@@ -24,10 +49,14 @@ ErrorConsole::ErrorConsole(Profile* profile) : profile_(profile) {
   registrar_.Add(this,
                  chrome::NOTIFICATION_PROFILE_DESTROYED,
                  content::NotificationService::AllBrowserContextsAndSources());
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_EXTENSION_UNINSTALLED,
+                 content::Source<Profile>(profile_));
 }
 
 ErrorConsole::~ErrorConsole() {
   FOR_EACH_OBSERVER(Observer, observers_, OnErrorConsoleDestroyed());
+  RemoveAllErrors();
 }
 
 // static
@@ -35,31 +64,28 @@ ErrorConsole* ErrorConsole::Get(Profile* profile) {
   return ExtensionSystem::Get(profile)->error_console();
 }
 
-void ErrorConsole::ReportError(scoped_ptr<ExtensionError> error) {
+void ErrorConsole::ReportError(scoped_ptr<const ExtensionError> scoped_error) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  errors_.push_back(error.release());
-  FOR_EACH_OBSERVER(Observer, observers_, OnErrorAdded(errors_.back()));
-}
 
-ErrorConsole::WeakErrorList ErrorConsole::GetErrorsForExtension(
-    const std::string& extension_id) const {
-  WeakErrorList result;
-  for (ErrorList::const_iterator iter = errors_.begin();
-       iter != errors_.end(); ++iter) {
-    if ((*iter)->extension_id() == extension_id)
-      result.push_back(*iter);
+  const ExtensionError* error = scoped_error.release();
+  // If there are too many errors for an extension already, limit ourselves to
+  // the most recent ones.
+  ErrorList* error_list = &errors_[error->extension_id()];
+  if (error_list->size() >= kMaxErrorsPerExtension) {
+    delete error_list->front();
+    error_list->pop_front();
   }
-  return result;
-}
+  error_list->push_back(error);
 
-void ErrorConsole::RemoveError(const ExtensionError* error) {
-  ErrorList::iterator iter = std::find(errors_.begin(), errors_.end(), error);
-  CHECK(iter != errors_.end());
-  errors_.erase(iter);
+  FOR_EACH_OBSERVER(Observer, observers_, OnErrorAdded(error));
 }
 
-void ErrorConsole::RemoveAllErrors() {
-  errors_.clear();
+const ErrorConsole::ErrorList& ErrorConsole::GetErrorsForExtension(
+    const std::string& extension_id) const {
+  ErrorMap::const_iterator iter = errors_.find(extension_id);
+  if (iter != errors_.end())
+    return iter->second;
+  return g_empty_error_list.Get();
 }
 
 void ErrorConsole::AddObserver(Observer* observer) {
@@ -73,19 +99,26 @@ void ErrorConsole::RemoveObserver(Observer* observer) {
 }
 
 void ErrorConsole::RemoveIncognitoErrors() {
-  WeakErrorList to_remove;
-  for (ErrorList::const_iterator iter = errors_.begin();
+  for (ErrorMap::iterator iter = errors_.begin();
        iter != errors_.end(); ++iter) {
-    if ((*iter)->from_incognito())
-      to_remove.push_back(*iter);
+    DeleteIncognitoErrorsFromList(&(iter->second));
   }
+}
 
-  for (WeakErrorList::const_iterator iter = to_remove.begin();
-       iter != to_remove.end(); ++iter) {
-    RemoveError(*iter);
+void ErrorConsole::RemoveErrorsForExtension(const std::string& extension_id) {
+  ErrorMap::iterator iter = errors_.find(extension_id);
+  if (iter != errors_.end()) {
+    STLDeleteContainerPointers(iter->second.begin(), iter->second.end());
+    errors_.erase(iter);
   }
 }
 
+void ErrorConsole::RemoveAllErrors() {
+  for (ErrorMap::iterator iter = errors_.begin(); iter != errors_.end(); ++iter)
+    STLDeleteContainerPointers(iter->second.begin(), iter->second.end());
+  errors_.clear();
+}
+
 void ErrorConsole::Observe(int type,
                            const content::NotificationSource& source,
                            const content::NotificationDetails& details) {
@@ -98,6 +131,12 @@ void ErrorConsole::Observe(int type,
         RemoveIncognitoErrors();
       break;
     }
+    case chrome::NOTIFICATION_EXTENSION_UNINSTALLED:
+      // No need to check the profile here, since we registered to only receive
+      // notifications from our own.
+      RemoveErrorsForExtension(
+          content::Details<Extension>(details).ptr()->id());
+      break;
     default:
       NOTREACHED();
   }

chrome/browser/extensions/error_console/error_console.h

@@ -5,16 +5,17 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_ERROR_CONSOLE_ERROR_CONSOLE_H_
 #define CHROME_BROWSER_EXTENSIONS_ERROR_CONSOLE_ERROR_CONSOLE_H_
 
-#include <vector>
+#include <deque>
+#include <map>
 
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/scoped_vector.h"
 #include "base/observer_list.h"
 #include "base/strings/string16.h"
 #include "base/threading/thread_checker.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "extensions/browser/extension_error.h"
 
 namespace content {
 class NotificationDetails;
@@ -26,17 +27,16 @@ class Profile;
 
 namespace extensions {
 class ErrorConsoleUnitTest;
-class ExtensionError;
 
 // The ErrorConsole is a central object to which all extension errors are
 // reported. This includes errors detected in extensions core, as well as
 // runtime Javascript errors.
 // This class is owned by ExtensionSystem, making it, in effect, a
 // BrowserContext-keyed service.
-class ErrorConsole : content::NotificationObserver {
+class ErrorConsole : public content::NotificationObserver {
  public:
-  typedef ScopedVector<ExtensionError> ErrorList;
-  typedef std::vector<const ExtensionError*> WeakErrorList;
+  typedef std::deque<const ExtensionError*> ErrorList;
+  typedef std::map<std::string, ErrorList> ErrorMap;
 
   class Observer {
    public:
@@ -55,24 +55,18 @@ class ErrorConsole : content::NotificationObserver {
   static ErrorConsole* Get(Profile* profile);
 
   // Report an extension error, and add it to the list.
-  void ReportError(scoped_ptr<ExtensionError> error);
+  void ReportError(scoped_ptr<const ExtensionError> error);
 
   // Get a collection of weak pointers to all errors relating to the extension
   // with the given |extension_id|.
-  WeakErrorList GetErrorsForExtension(const std::string& extension_id) const;
-
-  // Remove an error from the list of observed errors.
-  void RemoveError(const ExtensionError* error);
-
-  // Remove all errors from the list of observed errors.
-  void RemoveAllErrors();
+  const ErrorList& GetErrorsForExtension(const std::string& extension_id) const;
 
   // Add or remove observers of the ErrorConsole to be notified of any errors
   // added.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
-  const ErrorList& errors() { return errors_; }
+  const ErrorMap& errors() { return errors_; }
 
  private:
   FRIEND_TEST_ALL_PREFIXES(ErrorConsoleUnitTest, AddAndRemoveErrors);
@@ -81,6 +75,12 @@ class ErrorConsole : content::NotificationObserver {
   // the incognito profile is destroyed.
   void RemoveIncognitoErrors();
 
+  // Remove all errors relating to a particular |extension_id|.
+  void RemoveErrorsForExtension(const std::string& extension_id);
+
+  // Remove all errors for all extensions.
+  void RemoveAllErrors();
+
   // content::NotificationObserver implementation.
   virtual void Observe(int type,
                        const content::NotificationSource& source,
@@ -93,7 +93,7 @@ class ErrorConsole : content::NotificationObserver {
   ObserverList<Observer> observers_;
 
   // The errors which we have received so far.
-  ErrorList errors_;
+  ErrorMap errors_;
 
   // The profile with which the ErrorConsole is associated. Only collect errors
   // from extensions and RenderViews associated with this Profile (and it's

chrome/browser/extensions/error_console/error_console_unittest.cc

@@ -4,12 +4,16 @@
 
 #include "chrome/browser/extensions/error_console/error_console.h"
 
+#include "base/json/json_writer.h"
+#include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/browser/extensions/error_console/extension_error.h"
 #include "chrome/test/base/testing_profile.h"
-#include "extensions/common/id_util.h"
+#include "content/public/common/url_constants.h"
+#include "extensions/browser/extension_error.h"
+#include "extensions/common/constants.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::string16;
@@ -19,12 +23,32 @@ namespace extensions {
 
 namespace {
 
-scoped_ptr<ExtensionError> CreateNewManifestError(bool from_incognito) {
-  return scoped_ptr<ExtensionError>(
-      new ManifestParsingError(from_incognito,
-                               UTF8ToUTF16("source"),
-                               UTF8ToUTF16("message"),
-                               0u /* line number */ ));
+const char kExecutionContextURLKey[] = "executionContextURL";
+const char kStackTraceKey[] = "stackTrace";
+
+string16 CreateErrorDetails(const std::string& extension_id) {
+  base::DictionaryValue value;
+  value.SetString(
+      kExecutionContextURLKey,
+      std::string(kExtensionScheme) +
+          content::kStandardSchemeSeparator +
+          extension_id);
+  value.Set(kStackTraceKey, new ListValue);
+  std::string json_utf8;
+  base::JSONWriter::Write(&value, &json_utf8);
+  return UTF8ToUTF16(json_utf8);
+}
+
+scoped_ptr<const ExtensionError> CreateNewRuntimeError(
+    bool from_incognito,
+    const std::string& extension_id,
+    const string16& message) {
+  return scoped_ptr<const ExtensionError>(new JavascriptRuntimeError(
+      from_incognito,
+      UTF8ToUTF16("source"),
+      message,
+      logging::LOG_INFO,
+      CreateErrorDetails(extension_id)));
 }
 
 }  // namespace
@@ -49,26 +73,81 @@ TEST_F(ErrorConsoleUnitTest, AddAndRemoveErrors) {
 
   const size_t kNumTotalErrors = 6;
   const size_t kNumNonIncognitoErrors = 3;
+  const char kId[] = "id";
   // Populate with both incognito and non-incognito errors (evenly distributed).
-  for (size_t i = 0; i < kNumTotalErrors; ++i)
-    error_console_->ReportError(CreateNewManifestError(i % 2 == 0));
+  for (size_t i = 0; i < kNumTotalErrors; ++i) {
+    error_console_->ReportError(
+        CreateNewRuntimeError(i % 2 == 0, kId, string16()));
+  }
+
+  // There should only be one entry in the map, since errors are stored in lists
+  // keyed by extension id.
+  ASSERT_EQ(1u, error_console_->errors().size());
 
-  ASSERT_EQ(kNumTotalErrors, error_console_->errors().size());
+  ASSERT_EQ(kNumTotalErrors, error_console_->GetErrorsForExtension(kId).size());
 
   // Remove the incognito errors; three errors should remain, and all should
   // be from non-incognito contexts.
   error_console_->RemoveIncognitoErrors();
-  ASSERT_EQ(kNumNonIncognitoErrors, error_console_->errors().size());
-  for (size_t i = 0; i < error_console_->errors().size(); ++i)
-    ASSERT_FALSE(error_console_->errors()[i]->from_incognito());
-
-  // Remove an error by address.
-  error_console_->RemoveError(error_console_->errors()[1]);
-  ASSERT_EQ(kNumNonIncognitoErrors - 1, error_console_->errors().size());
-
-  // Remove all remaining errors.
+  const ErrorConsole::ErrorList& errors =
+      error_console_->GetErrorsForExtension(kId);
+  ASSERT_EQ(kNumNonIncognitoErrors, errors.size());
+  for (size_t i = 0; i < errors.size(); ++i)
+    ASSERT_FALSE(errors[i]->from_incognito());
+
+  // Add another error for a different extension id.
+  const char kSecondId[] = "id2";
+  error_console_->ReportError(
+      CreateNewRuntimeError(false, kSecondId, string16()));
+
+  // There should be two entries now, one for each id, and there should be one
+  // error for the second extension.
+  ASSERT_EQ(2u, error_console_->errors().size());
+  ASSERT_EQ(1u, error_console_->GetErrorsForExtension(kSecondId).size());
+
+  // Remove all errors for the second id.
+  error_console_->RemoveErrorsForExtension(kSecondId);
+  ASSERT_EQ(1u, error_console_->errors().size());
+  ASSERT_EQ(0u, error_console_->GetErrorsForExtension(kSecondId).size());
+  // First extension should be unaffected.
+  ASSERT_EQ(kNumNonIncognitoErrors,
+            error_console_->GetErrorsForExtension(kId).size());
+
+  // Remove remaining errors.
   error_console_->RemoveAllErrors();
   ASSERT_EQ(0u, error_console_->errors().size());
+  ASSERT_EQ(0u, error_console_->GetErrorsForExtension(kId).size());
+}
+
+// Test that if we add enough errors, only the most recent
+// kMaxErrorsPerExtension are kept.
+TEST_F(ErrorConsoleUnitTest, ExcessiveErrorsGetCropped) {
+  ASSERT_EQ(0u, error_console_->errors().size());
+
+  // This constant matches one of the same name in error_console.cc.
+  const size_t kMaxErrorsPerExtension = 100;
+  const size_t kNumExtraErrors = 5;
+  const char kId[] = "id";
+
+  // Add new errors, with each error's message set to its number.
+  for (size_t i = 0; i < kMaxErrorsPerExtension + kNumExtraErrors; ++i) {
+    error_console_->ReportError(
+        CreateNewRuntimeError(false, kId, base::UintToString16(i)));
+  }
+
+  ASSERT_EQ(1u, error_console_->errors().size());
+
+  const ErrorConsole::ErrorList& errors =
+      error_console_->GetErrorsForExtension(kId);
+  ASSERT_EQ(kMaxErrorsPerExtension, errors.size());
+
+  // We should have popped off errors in the order they arrived, so the
+  // first stored error should be the 6th reported (zero-based)...
+  ASSERT_EQ(errors.front()->message(),
+            base::UintToString16(kNumExtraErrors));
+  // ..and the last stored should be the 105th reported.
+  ASSERT_EQ(errors.back()->message(),
+            base::UintToString16(kMaxErrorsPerExtension + kNumExtraErrors - 1));
 }
 
 }  // namespace extensions

chrome/chrome_browser_extensions.gypi

@@ -54,6 +54,8 @@
         # All .cc, .h, .m, and .mm files under browser/extensions except for
         # tests and mocks.
         '../extensions/browser/extension_prefs_scope.h',
+        '../extensions/browser/extension_error.cc',
+        '../extensions/browser/extension_error.h',
         '../extensions/browser/file_reader.cc',
         '../extensions/browser/file_reader.h',
         '../extensions/browser/pref_names.cc',
@@ -573,8 +575,6 @@
         'browser/extensions/default_apps.h',
         'browser/extensions/error_console/error_console.cc',
         'browser/extensions/error_console/error_console.h',
-        'browser/extensions/error_console/extension_error.cc',
-        'browser/extensions/error_console/extension_error.h',
         'browser/extensions/event_listener_map.cc',
         'browser/extensions/event_listener_map.h',
         'browser/extensions/event_names.cc',

chrome/browser/extensions/error_console/extension_error.cc → extensions/browser/extension_error.cc

@@ -2,14 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/extensions/error_console/extension_error.h"
+#include "extensions/browser/extension_error.h"
 
 #include "base/json/json_reader.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
-#include "chrome/common/extensions/extension.h"
 #include "extensions/common/constants.h"
+#include "url/gurl.h"
 
 using base::string16;
 
@@ -28,7 +28,7 @@ const char kStackTraceKey[] = "stackTrace";
 // populates |extension_id| with the ID. On failure, returns false and leaves
 // extension_id untouched.
 bool GetExtensionIDFromGURL(const GURL& url, std::string* extension_id) {
-  if (url.SchemeIs(extensions::kExtensionScheme)) {
+  if (url.SchemeIs(kExtensionScheme)) {
     *extension_id = url.host();
     return true;
   }
@@ -38,10 +38,12 @@ bool GetExtensionIDFromGURL(const GURL& url, std::string* extension_id) {
 }  // namespace
 
 ExtensionError::ExtensionError(Type type,
+                               const std::string& extension_id,
                                bool from_incognito,
                                const string16& source,
                                const string16& message)
     : type_(type),
+      extension_id_(extension_id),
       from_incognito_(from_incognito),
       source_(source),
       message_(message) {
@@ -58,15 +60,13 @@ std::string ExtensionError::PrintForTest() const {
          "\n  ID:      " + extension_id_;
 }
 
-ManifestParsingError::ManifestParsingError(bool from_incognito,
-                                           const string16& source,
-                                           const string16& message,
-                                           size_t line_number)
+ManifestParsingError::ManifestParsingError(const std::string& extension_id,
+                                           const string16& message)
     : ExtensionError(ExtensionError::MANIFEST_PARSING_ERROR,
-                     from_incognito,
-                     source,
-                     message),
-      line_number_(line_number) {
+                     extension_id,
+                     false,  // extensions can't be installed while incognito.
+                     base::FilePath(kManifestFilename).AsUTF16Unsafe(),
+                     message) {
 }
 
 ManifestParsingError::~ManifestParsingError() {
@@ -74,8 +74,7 @@ ManifestParsingError::~ManifestParsingError() {
 
 std::string ManifestParsingError::PrintForTest() const {
   return ExtensionError::PrintForTest() +
-         "\n  Type:    ManifestParsingError" +
-         "\n  Line:    " + base::IntToString(line_number_);
+         "\n  Type:    ManifestParsingError";
 }
 
 JavascriptRuntimeError::StackFrame::StackFrame() : line_number(-1),
@@ -101,6 +100,7 @@ JavascriptRuntimeError::JavascriptRuntimeError(bool from_incognito,
                                                logging::LogSeverity level,
                                                const string16& details)
     : ExtensionError(ExtensionError::JAVASCRIPT_RUNTIME_ERROR,
+                     std::string(),  // We don't know the id yet.
                      from_incognito,
                      source,
                      message),

chrome/browser/extensions/error_console/extension_error.h → extensions/browser/extension_error.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_EXTENSIONS_ERROR_CONSOLE_EXTENSION_ERROR_H_
-#define CHROME_BROWSER_EXTENSIONS_ERROR_CONSOLE_EXTENSION_ERROR_H_
+#ifndef EXTENSIONS_BROWSER_EXTENSION_ERROR_H_
+#define EXTENSIONS_BROWSER_EXTENSION_ERROR_H_
 
 #include <string>
 #include <vector>
@@ -33,12 +33,15 @@ class ExtensionError {
 
  protected:
   ExtensionError(Type type,
+                 const std::string& extension_id,
                  bool from_incognito,
                  const base::string16& source,
                  const base::string16& message);
 
   // Which type of error this is.
   Type type_;
+  // The ID of the extension which caused the error.
+  std::string extension_id_;
   // Whether or not the error was caused while incognito.
   bool from_incognito_;
   // The source for the error; this can be a script, web page, or manifest file.
@@ -47,27 +50,18 @@ class ExtensionError {
   base::string16 source_;
   // The error message itself.
   base::string16 message_;
-  // The ID of the extension which caused the error. This may be absent, since
-  // we can't always know the id (such as when a manifest fails to parse).
-  std::string extension_id_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionError);
 };
 
 class ManifestParsingError : public ExtensionError {
  public:
-  ManifestParsingError(bool from_incognito,
-                       const base::string16& source,
-                       const base::string16& message,
-                       size_t line_number);
+  ManifestParsingError(const std::string& extension_id,
+                       const base::string16& message);
   virtual ~ManifestParsingError();
 
   virtual std::string PrintForTest() const OVERRIDE;
-
-  size_t line_number() const { return line_number_; }
  private:
-  size_t line_number_;
-
   DISALLOW_COPY_AND_ASSIGN(ManifestParsingError);
 };
 
@@ -124,4 +118,4 @@ class JavascriptRuntimeError : public ExtensionError {
 
 }  // namespace extensions
 
-#endif  // CHROME_BROWSER_EXTENSIONS_ERROR_CONSOLE_EXTENSION_ERROR_H_
+#endif  // EXTENSIONS_BROWSER_EXTENSION_ERROR_H_