Show alert failure for reloading unpacked extensions with bad manifest

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

 // Copyright 2014 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.
 
 #include "chrome/browser/ui/webui/extensions/extension_loader_handler.h"
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string16.h"
 #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"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_data_source.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/file_highlighter.h"
@@ skipping 98 matching lines @@
 }
 
 void ExtensionLoaderHandler::FileHelper::MultiFilesSelected(
       const std::vector<base::FilePath>& files, void* params) {
   NOTREACHED();
 }
 
 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() {
 }
 
 void ExtensionLoaderHandler::GetLocalizedValues(
     content::WebUIDataSource* source) {
   source->AddString(
       "extensionLoadErrorHeading",
       l10n_util::GetStringUTF16(IDS_EXTENSIONS_LOAD_ERROR_HEADING));
   source->AddString(
       "extensionLoadErrorMessage",
       l10n_util::GetStringUTF16(IDS_EXTENSIONS_LOAD_ERROR_MESSAGE));
   source->AddString(
       "extensionLoadErrorRetry",
       l10n_util::GetStringUTF16(IDS_EXTENSIONS_LOAD_ERROR_RETRY));
   source->AddString(
       "extensionLoadErrorGiveUp",
       l10n_util::GetStringUTF16(IDS_EXTENSIONS_LOAD_ERROR_GIVE_UP));
   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() {
   web_ui()->RegisterMessageCallback(
       "extensionLoaderLoadUnpacked",
       base::Bind(&ExtensionLoaderHandler::HandleLoadUnpacked,
                  weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback(
       "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) {
   DCHECK(args->empty());
   file_helper_->ChooseFile();
 }
 
 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.
   installer->set_be_noisy_on_failure(false);
 
   installer->Load(file_path);
 }
 
 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 =
       base::StringPrintf("%s  Line: (\\d+), column: (\\d+), Syntax error.",
                          manifest_errors::kManifestParseError);
   // If this was a JSON parse error, we can highlight the exact line with the
   // error. Otherwise, we should still display the manifest (for consistency,
   // reference, and so that if we ever make this really fancy and add an editor,
   // it's ready).
   //
   // This regex call can fail, but if it does, we just don't highlight anything.
   re2::RE2::FullMatch(error, regex, &line, &column);
 
   // This will read the manifest and call NotifyFrontendOfFailure with the read
   // manifest contents.
   base::PostTaskAndReplyWithResult(
       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