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"
@@ skipping 13 matching lines @@
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/file_highlighter.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest_constants.h"
 #include "grit/generated_resources.h"
 #include "third_party/re2/re2/re2.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/shell_dialogs/select_file_dialog.h"
 
+class ExtensionErrorReporter;

[Devlin, 2014/06/24 22:21:12]

Don't need to forward declare classes either forward declared in this .h or
whose headers have been included.

+class ExtensionErrorReporterObserver;
+
 namespace extensions {
 
 namespace {
 
 // Read a file to a string and return.
 std::string ReadFileToString(const base::FilePath& path) {
   std::string data;
   // This call can fail, but it doesn't matter for our purposes. If it fails,
   // we simply return an empty string for the manifest, and ignore it.
   base::ReadFileToString(path, &data);
@@ skipping 80 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)),
-      weak_ptr_factory_(this) {
+      weak_ptr_factory_(this),
+      extension_error_reporter_observer_(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));
@@ skipping 29 matching lines @@
 
 void ExtensionLoaderHandler::HandleRetry(const base::ListValue* args) {
   DCHECK(args->empty());
   LoadUnpackedExtensionImpl(failed_path_);
 }
 
 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) {
@@ skipping 31 matching lines @@
     const std::string& manifest) {
   base::StringValue file_value(file_path.LossyDisplayName());
   base::StringValue error_value(base::UTF8ToUTF16(error));
 
   base::DictionaryValue manifest_value;
   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);
 
   web_ui()->CallJavascriptFunction(

[gpdavis, 2014/06/21 01:22:07]

There is an issue with synchronicity here.  On a page reload, if an unpacked
extension triggers an OnLoadError, this CallJavascriptFunction call will trigger
a javascript function call before 'extensions' exists on the page (before the
DOM is fully loaded), which results in an error:

"Uncaught ReferenceError: extensions is not defined"

If there is some way to delay this call until the page is fully loaded, this
will work for a page refresh.  As of right now, this code simply fixes the issue
of old warnings when an unpacked extension is manually reloaded and triggers an
OnLoadFailure.  The new UI now shows up in this case.

[Devlin, 2014/06/24 22:21:12]

What I would envision would be keeping track of all extensions which failed to
load, and showing the UI for it/them once the page fully loads.

(We do also need to figure out what we're going to do for multiple load failures
- is the current behavior that it pops up multiple dialogs?)

[gpdavis, 2014/06/25 00:21:01]

The current behavior is that only the last one is shown, because the template
for this only has the architecture to support displaying one error.  This is
definitely a big decision that needs to be made.

I do like the idea of keeping track of all the extensions that failed to load. 
So maybe the best idea would be to do that, and then send a message once the
page loads to display information about any extensions that failed to load while
the page was rendering?  Doesn't seem like an incredibly difficult thing to
implement; I was just planning on getting it to work for one extension before we
move on to determining what should be done if multiple extensions fail.  Maybe
we can show the UI we have now for one failure, but if multiple fail, we can
display an additional section that indicates that more extensions failed to load
but that we aren't showing exactly where in the manifest there's a problem.  So
then one problem can be displayed at a time.

[Devlin, 2014/06/25 19:49:36]

Sorry, I was unclear.  By "current behavior", I meant "behavior in released
chrome", vs "behavior in this patch".  That is, if we currently break three
extensions, do we show a modal dialog for each?

And yeah, keeping track of extensions and then sending a message once the page
is ready sounds good.

[Devlin, 2014/06/27 22:31:08]

Still waiting on an answer for current behavior?

       "extensions.ExtensionLoader.notifyLoadFailed",
       file_value,
       error_value,
       manifest_value);
 }
 
 }  // namespace extensions