Fix bug where AppInfoDialog was outliving ExtensionSettingsHandler

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

 // Copyright (c) 2012 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_settings_handler.h"
 
 #include "apps/app_load_service.h"
 #include "apps/saved_files_service.h"
 #include "base/auto_reset.h"
 #include "base/base64.h"
@@ skipping 121 matching lines @@
                              int render_view_id,
                              bool incognito,
                              bool generated_background_page)
     : url(url),
       render_process_id(render_process_id),
       render_view_id(render_view_id),
       incognito(incognito),
       generated_background_page(generated_background_page) {
 }
 
-// On Mac, the install prompt is not modal. This means that the user can
-// navigate while the dialog is up, causing the dialog handler to outlive the
-// ExtensionSettingsHandler. That's a problem because the dialog framework will
-// try to contact us back once the dialog is closed, which causes a crash.
-// This class is designed to broker the message between the two objects, while
-// managing its own lifetime so that it can outlive the ExtensionSettingsHandler
-// and (when doing so) gracefully ignore the message from the dialog.
+// On Mac (and Linux with Unity), the install prompt is not modal. This means

[not at google - send to devlin, 2014/11/03 17:37:49]

How about just "The install prompt is not necessarily modal (e.g. Mac, Linux
Unity)".

Cover your bases for the future.

[sashab, 2014/11/04 01:32:20]

Nice suggestion; done.

+// that the user can navigate while the dialog is up, causing the dialog handler
+// to outlive the ExtensionSettingsHandler. That's a problem because the dialog
+// framework will try to contact us back once the dialog is closed, which causes
+// a crash. This class is designed to broker the message between the two
+// objects, while managing its own lifetime so that it can outlive the
+// ExtensionSettingsHandler and (when doing so) gracefully ignore the message
+// from the dialog.
 class BrokerDelegate : public ExtensionInstallPrompt::Delegate {
  public:
   explicit BrokerDelegate(
       const base::WeakPtr<ExtensionSettingsHandler>& delegate)
       : delegate_(delegate) {}
 
   // ExtensionInstallPrompt::Delegate implementation.
   void InstallUIProceed() override {
     if (delegate_)
       delegate_->InstallUIProceed();
     delete this;
   };
 
   void InstallUIAbort(bool user_initiated) override {
     if (delegate_)
       delegate_->InstallUIAbort(user_initiated);
     delete this;
   };
 
+  void AppInfoDialogClosed() {
+    if (delegate_)
+      delegate_->AppInfoDialogClosed();
+    delete this;
+  }
+
  private:
   base::WeakPtr<ExtensionSettingsHandler> delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(BrokerDelegate);
 };
 
 ///////////////////////////////////////////////////////////////////////////////
 //
 // ExtensionSettingsHandler
 //
@@ skipping 1023 matching lines @@
   std::string extension_id(base::UTF16ToUTF8(ExtractStringValue(args)));
   CHECK(!extension_id.empty());
   const Extension* extension =
       ExtensionRegistry::Get(Profile::FromWebUI(web_ui()))
           ->GetExtensionById(extension_id, ExtensionRegistry::EVERYTHING);
   if (!extension)
     return;
 
   if (!extension_id_prompting_.empty())
     return;  // Only one prompt at a time.
+  extension_id_prompting_ = extension->id();
 
-  extension_id_prompting_ = extension->id();
+  // The BrokerDelegate manages its own lifetime.
+  BrokerDelegate* broker_delegate = new BrokerDelegate(AsWeakPtr());
 
   // Show the new-style extensions dialog when the flag is set. The flag cannot
   // be set on Mac platforms.
   if (ShouldDisplayExtensionInfoDialog()) {
     UMA_HISTOGRAM_ENUMERATION("Apps.AppInfoDialog.Launches",
                               AppInfoLaunchSource::FROM_EXTENSIONS_PAGE,
                               AppInfoLaunchSource::NUM_LAUNCH_SOURCES);
 
     // Display the dialog at a size similar to the app list.
     const int kAppInfoDialogWidth = 380;
     const int kAppInfoDialogHeight = 490;
+
     ShowAppInfoInNativeDialog(
         web_contents()->GetTopLevelNativeWindow(),
         gfx::Size(kAppInfoDialogWidth, kAppInfoDialogHeight),
-        Profile::FromWebUI(web_ui()),
-        extension,
-        base::Bind(&ExtensionSettingsHandler::AppInfoDialogClosed,
-                   base::Unretained(this)));
+        Profile::FromWebUI(web_ui()), extension,
+        base::Bind(&BrokerDelegate::AppInfoDialogClosed,
+                   base::Unretained(broker_delegate)));
   } else {
     prompt_.reset(new ExtensionInstallPrompt(web_contents()));
     std::vector<base::FilePath> retained_file_paths;
     if (extension->permissions_data()->HasAPIPermission(
             APIPermission::kFileSystem)) {
       std::vector<apps::SavedFileEntry> retained_file_entries =
           apps::SavedFilesService::Get(Profile::FromWebUI(web_ui()))
               ->GetAllFileEntries(extension_id_prompting_);
       for (size_t i = 0; i < retained_file_entries.size(); ++i) {
         retained_file_paths.push_back(retained_file_entries[i].path);
       }
     }
     std::vector<base::string16> retained_device_messages;
     if (extension->permissions_data()->HasAPIPermission(APIPermission::kUsb)) {
       retained_device_messages =
           extensions::DevicePermissionsManager::Get(
               Profile::FromWebUI(web_ui()))
               ->GetPermissionMessageStrings(extension_id_prompting_);
     }
 
-    // The BrokerDelegate manages its own lifetime.
-    prompt_->ReviewPermissions(new BrokerDelegate(AsWeakPtr()),
-                               extension,
-                               retained_file_paths,
+    prompt_->ReviewPermissions(broker_delegate, extension, retained_file_paths,
                                retained_device_messages);
   }
 }
 
 void ExtensionSettingsHandler::HandleShowButtonMessage(
     const base::ListValue* args) {
   const Extension* extension = GetActiveExtension(args);
   if (!extension)
     return;
   ExtensionActionAPI::SetBrowserActionVisibility(
@@ skipping 242 matching lines @@
     extension_service_->EnableExtension(extension_id);
   } else {
     ExtensionErrorReporter::GetInstance()->ReportError(
         base::UTF8ToUTF16(JoinString(requirement_errors, ' ')),
         true);  // Be noisy.
   }
   requirements_checker_.reset();
 }
 
 }  // namespace extensions