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/path_util.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() {
+  content::WebContentsObserver::Observe(web_ui()->GetWebContents());

[Finnur, 2014/07/14 10:25:58]

Do we need to call Observe(NULL) when we are done?

[gpdavis, 2014/07/14 18:58:19]

Done with what, exactly?  Done with observing for the notification that the UI
is ready to receive failures?

Observe(NULL) would remove this as an observer, right?  This object exists
through page refreshes.  It's constructed when the page is loaded and destructed
when the tab is closed.  So this needs to continue observing web contents,
because this object needs to continue being notified of page refreshes.

[Finnur, 2014/07/15 10:38:45]

Sounds good. Maybe that warrants a comment in the code?

[gpdavis, 2014/07/15 18:33:14]

Will do!

   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(
+      "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_);
+  const base::FilePath file_path = failed_paths_.back();
+  failed_paths_.pop_back();
+  LoadUnpackedExtensionImpl(file_path);

[Finnur, 2014/07/14 10:25:58]

This got me thinking... If I have two failures (extensions A, B -- in that order
on the failed_paths_ list), what happens if I fix A and retry? Will it only try
reloading B?

[gpdavis, 2014/07/14 18:58:19]

Yes, that is the current functionality.  The "Retry" and "Give up" buttons are
for the failure that is currently being displayed.  This is why there's a
separation with "additional failures".  It seemed to me like the user would
likely attempt to fix the failure whose manifest is being displayed.

Suggestions are more than welcome :)

[Finnur, 2014/07/15 10:38:45]

I see. That's good.

+}
+
+void ExtensionLoaderHandler::HandleIgnoreFailure(const base::ListValue* args) {
+  DCHECK(args->empty());
+  failed_paths_.pop_back();

[Finnur, 2014/07/14 10:25:58]

Similar argument here. Two failures: Should Ignore dismiss and remove one or
both?

[gpdavis, 2014/07/14 18:58:19]

I guess that's more of a design question for you guys.  I would make the
argument that "Give up" should only dismiss the failure that's currently being
displayed.  That way it can be used like a stack, and the failures can be dealt
with one by one.

[Finnur, 2014/07/15 10:38:45]

Yeah, that's fine.

[Devlin, 2014/07/15 17:51:44]

Just a reminder: There are only multiple failures in the case where:
a) The user has multiple unpacked extensions
b) Multiple unpacked extensions have load errors
c) The user hits refresh on the chrome:extensions page (we can never see
multiple errors from a reload or load unpacked command).

Since this situation is pretty rare (and since our current mechanism is to pop
up multiple modal dialogs [yuck!]), I think we can have something a bit rougher
here, as long as it makes sense.

(No action necessary - just documenting reasoning for future generations of
CLs).

+}
+
+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;
   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::DidStartNavigationToPendingEntry(
+    const GURL& url,
+    content::NavigationController::ReloadType reload_type) {
+  if (reload_type != content::NavigationController::NO_RELOAD)
+    display_ready_ = false;
+}
+
+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));
+  failed_paths_.push_back(file_path);
+  base::FilePath prettified_path = path_util::PrettifyPath(file_path);
 
-  base::DictionaryValue manifest_value;
+  scoped_ptr<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.get());
 
+  scoped_ptr<base::DictionaryValue> failure(new base::DictionaryValue());
+  failure->Set("path",
+               new base::StringValue(prettified_path.LossyDisplayName()));
+  failure->Set("error", new base::StringValue(base::UTF8ToUTF16(error)));
+  failure->Set("manifest", manifest_value.release());
+  failures_.Append(failure.release());
+
+  // Only notify the frontend if the display is ready.
+  if (display_ready_)
+    NotifyFrontendOfFailure();
+}
+
+void ExtensionLoaderHandler::NotifyFrontendOfFailure() {
   web_ui()->CallJavascriptFunction(
       "extensions.ExtensionLoader.notifyLoadFailed",
-      file_value,
-      error_value,
-      manifest_value);
+      failures_);
+  failures_.Clear();
 }
 
 }  // namespace extensions