Show alert failure for reloading unpacked extensions with bad manifest

chrome/browser/ui/webui/extensions/extension_loader_handler.cc

Index: chrome/browser/ui/webui/extensions/extension_loader_handler.cc
diff --git a/chrome/browser/ui/webui/extensions/extension_loader_handler.cc b/chrome/browser/ui/webui/extensions/extension_loader_handler.cc
index 6e43177a4a3abd3fe57b684c2c362b5012a36f9b..2201f9187fc629b031dca6946d2737d62146582e 100644
--- a/chrome/browser/ui/webui/extensions/extension_loader_handler.cc
+++ b/chrome/browser/ui/webui/extensions/extension_loader_handler.cc
@@ -12,7 +12,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/values.h"
 #include "chrome/browser/extensions/unpacked_installer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
@@ -131,8 +130,11 @@ void ExtensionLoaderHandler::FileHelper::MultiFilesSelected(
 ExtensionLoaderHandler::ExtensionLoaderHandler(Profile* profile)
     : profile_(profile),
       file_helper_(new FileHelper(this)),
+      extension_error_reporter_observer_(this),
+      display_ready_(false),
       weak_ptr_factory_(this) {
   DCHECK(profile_);
+  extension_error_reporter_observer_.Add(ExtensionErrorReporter::GetInstance());
 }
 
 ExtensionLoaderHandler::~ExtensionLoaderHandler() {
@@ -155,6 +157,9 @@ void ExtensionLoaderHandler::GetLocalizedValues(
   source->AddString(
       "extensionLoadCouldNotLoadManifest",
       l10n_util::GetStringUTF16(IDS_EXTENSIONS_LOAD_COULD_NOT_LOAD_MANIFEST));
+  source->AddString(
+      "extensionLoadAdditionalFailures",
+      l10n_util::GetStringUTF16(IDS_EXTENSIONS_LOAD_ADDITIONAL_FAILURES));
 }
 
 void ExtensionLoaderHandler::RegisterMessages() {
@@ -166,6 +171,18 @@ void ExtensionLoaderHandler::RegisterMessages() {
       "extensionLoaderRetry",
       base::Bind(&ExtensionLoaderHandler::HandleRetry,
                  weak_ptr_factory_.GetWeakPtr()));
+  web_ui()->RegisterMessageCallback(
+      "extensionLoaderIgnoreFailure",
+      base::Bind(&ExtensionLoaderHandler::HandleIgnoreFailure,
+                 weak_ptr_factory_.GetWeakPtr()));
+  web_ui()->RegisterMessageCallback(
+      "extensionLoaderSetDisplayLoading",
+      base::Bind(&ExtensionLoaderHandler::HandleSetDisplayLoading,
+                 weak_ptr_factory_.GetWeakPtr()));
+  web_ui()->RegisterMessageCallback(
+      "extensionLoaderDisplayFailures",
+      base::Bind(&ExtensionLoaderHandler::HandleDisplayFailures,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ExtensionLoaderHandler::HandleLoadUnpacked(const base::ListValue* args) {
@@ -175,16 +192,34 @@ void ExtensionLoaderHandler::HandleLoadUnpacked(const base::ListValue* args) {
 
 void ExtensionLoaderHandler::HandleRetry(const base::ListValue* args) {
   DCHECK(args->empty());
-  LoadUnpackedExtensionImpl(failed_path_);
+  LoadUnpackedExtensionImpl(failed_paths_.back());

[Devlin, 2014/07/07 20:44:22]

We should probably pop, then run.

[gpdavis, 2014/07/09 01:35:57]

But pop_back deletes the element and doesn't return it.  I decided to pop_back
afterward because there are two possible cases: the reload is successful, in
which the path is popped off, or the reload fails, and the path is pushed on and
then popped back off.  Either way, the last element will be correct.

But I understand why we might want to change it anyway.  I can just create a
filepath from the last element in the vector, then pop_back, and pass the
filepath onward.

[Devlin, 2014/07/09 19:53:17]

Right, so because pop_back doesn't return it (one of the worst choices in the
STL, I swear...), you need to get a copy of it before.

base::FilePath failed_path = failed_paths_.back();
failed_paths_.pop_back();
LoadUnpackedExtensionImpl(failed_path);

[gpdavis, 2014/07/09 21:16:25]

Done.

+  failed_paths_.pop_back();
+}
+
+void ExtensionLoaderHandler::HandleIgnoreFailure(const base::ListValue* args) {
+  DCHECK(args->empty());
+  failed_paths_.pop_back();
+}
+
+void ExtensionLoaderHandler::HandleSetDisplayLoading(
+    const base::ListValue* args) {
+  DCHECK(args->empty());
+  display_ready_ = false;
+}
+
+void ExtensionLoaderHandler::HandleDisplayFailures(
+    const base::ListValue* args) {
+  DCHECK(args->empty());
+  display_ready_ = true;
+
+  if (!failures_.empty())
+    NotifyFrontendOfFailure();
 }
 
 void ExtensionLoaderHandler::LoadUnpackedExtensionImpl(
     const base::FilePath& file_path) {
   scoped_refptr<UnpackedInstaller> installer = UnpackedInstaller::Create(
       ExtensionSystem::Get(profile_)->extension_service());
-  installer->set_on_failure_callback(
-      base::Bind(&ExtensionLoaderHandler::OnLoadFailure,
-                 weak_ptr_factory_.GetWeakPtr()));
 
   // We do our own error handling, so we don't want a load failure to trigger
   // a dialog.
@@ -195,7 +230,7 @@ void ExtensionLoaderHandler::LoadUnpackedExtensionImpl(
 
 void ExtensionLoaderHandler::OnLoadFailure(const base::FilePath& file_path,
                                            const std::string& error) {
-  failed_path_ = file_path;
+  failed_paths_.push_back(file_path);
   size_t line = 0u;
   size_t column = 0u;
   std::string regex =
@@ -215,32 +250,46 @@ void ExtensionLoaderHandler::OnLoadFailure(const base::FilePath& file_path,
       content::BrowserThread::GetBlockingPool(),
       FROM_HERE,
       base::Bind(&ReadFileToString, file_path.Append(kManifestFilename)),
-      base::Bind(&ExtensionLoaderHandler::NotifyFrontendOfFailure,
+      base::Bind(&ExtensionLoaderHandler::AddFailure,
                  weak_ptr_factory_.GetWeakPtr(),
                  file_path,
                  error,
                  line));
 }
 
-void ExtensionLoaderHandler::NotifyFrontendOfFailure(
+void ExtensionLoaderHandler::AddFailure(
     const base::FilePath& file_path,
     const std::string& error,
     size_t line_number,
     const std::string& manifest) {
-  base::StringValue file_value(file_path.LossyDisplayName());
-  base::StringValue error_value(base::UTF8ToUTF16(error));
+  base::StringValue* file_value =

[Devlin, 2014/07/07 20:44:21]

inline these.

[gpdavis, 2014/07/09 01:35:57]

Done.

+      new base::StringValue(file_path.LossyDisplayName());
+  base::StringValue* error_value =
+      new base::StringValue(base::UTF8ToUTF16(error));
 
-  base::DictionaryValue manifest_value;
+  base::DictionaryValue* manifest_value = new base::DictionaryValue();
   SourceHighlighter highlighter(manifest, line_number);
   // If the line number is 0, this highlights no regions, but still adds the
   // full manifest.
-  highlighter.SetHighlightedRegions(&manifest_value);
+  highlighter.SetHighlightedRegions(manifest_value);
+
+  base::DictionaryValue* failure = new base::DictionaryValue();

[Devlin, 2014/07/07 20:44:21]

For better or worse, we're overly paranoid about memory leaks.  Please put this
in a scoped ptr.

[gpdavis, 2014/07/09 01:35:57]

You can never be overly paranoid about memory leaks ;) I used a regular pointer
because pop_back should destruct the element.  I tried running it with a scoped
pointer, and it crashed.

base::ListValue::Clear() actually deletes all the items in the list, so there
shouldn't be a memory issue here.  Correct me if I'm misunderstanding, though.

https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&rcl...

[Devlin, 2014/07/09 19:53:17]

Right, but we like extremely clear ownership models.  That is, there should
never (if we can help it) have an unowned object, and, at line 276 (in this
patch), it is unowned.  Instead, it should be:

scoped_ptr<base::DictionaryValue> failure(new base::DictionaryValue());
failure->Set(...);
failures_.Append(failure.release());

Then the object is always owned, and will always be deleted, even if we were to
say insert an early return somewhere.

A completely contrived example:
We realize that we are sometimes showing failures with non-existent paths.  We
fix it by changing this to:

base::DictionaryValue* failure = new base::DictionaryValue();
failure->Set("error", error);
failure->Set("manifest", manifest);
// We obviously don't want to show a path that doesn't exist.
if (!base::PathExists(base::FilePath(file_path)))
  return;
failure->SetPath("path", file_path);
failures_.Append(failure);

But wait!  |failure| leaks!!

Would this ever happen?  Perhaps not.  But, like, I said, we are overly
paranoid. :)

[gpdavis, 2014/07/09 21:16:25]

Ah, gotcha.  I must have been doing something wrong before because it crashed
with a rather cryptic stacktrace the last time I did it.  I think I screwed up
the initialization of the scoped pointer, and it snuck past the compiler
somehow.

Anyhow, taken care of.  I also scoped manifest_value since it has the same
vulnerability as it stands in this patch set.

+  failure->Set("path", file_value);
+  failure->Set("error", error_value);
+  failure->Set("manifest", manifest_value);
+  failures_.Append(failure);
+
+  // Only notify the frontend if the display is ready.
+  if (display_ready_) {

[Devlin, 2014/07/07 20:44:21]

nit: no brackets around single-line ifs.

[gpdavis, 2014/07/09 01:35:57]

Done.

+    NotifyFrontendOfFailure();
+  }
+}
 
+void ExtensionLoaderHandler::NotifyFrontendOfFailure() {
   web_ui()->CallJavascriptFunction(
       "extensions.ExtensionLoader.notifyLoadFailed",
-      file_value,
-      error_value,
-      manifest_value);
+      failures_);
+  failures_.Clear();
 }
 
 }  // namespace extensions