Fix a crash in downloads.acceptDanger()

On mac, when the ConstrainedWindow (DownloadDangerPrompt) stole key-ness from
the extension popup, the popup was automatically closing, which would
automatically close the DownloadDangerPrompt before the user could interact with
it.

Now when ExtensionPopupController receives a windowDidResignKey notification, it
drops the notification if there are any modal dialogs showing for its WebContents.
There's also an extra windowDidResignKey notification right after the modal
dialog closes, so ExtensionPopupController::windowDidResignKey() sets a flag to
ignore it.

TabModalConfirmDialogMac::OnConstrainedWindowClosed() could be called twice
causing double-deletes. Passing the delegate_ to a stack variable allows the
second call to return before double-deleting.

Add DownloadsAcceptDangerFunction::OnPromptCreatedForTesting(callback) so that
the browser test can fake the user clicking in the DownloadDangerPrompt.

Override ChromeWebModalDialogManagerDelegate::GetWebContentsModalDialogHost() in
ExtensionHost to return a valid WebContentsModalDialogHost (ExtensionDialogHost)
instead of NULL. This object is necessary in
CreateWebContentsModalDialogViews(). ExtensionHost outlives ExtensionDialogHost
which outlives the dialog.

BUG=273114

Review URL: https://chromiumcodereview.appspot.com/23064010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@219455 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 152 insertions, 5 deletions

diff --git a/chrome/browser/download/download_danger_prompt.h b/chrome/browser/download/download_danger_prompt.h
index 8c2407f..e1b5c61 100644
--- a/chrome/browser/download/download_danger_prompt.h
+++ b/chrome/browser/download/download_danger_prompt.h
@@ -43,9 +43,6 @@ class DownloadDangerPrompt {
       bool show_context,
       const OnDone& done);
 
- protected:
-  friend class DownloadDangerPromptTest;
-
   // Only to be used by tests. Subclasses must override to manually call the
   // respective button click handler.
   virtual void InvokeActionForTesting(Action action) = 0;
diff --git a/chrome/browser/extensions/api/downloads/downloads_api.cc b/chrome/browser/extensions/api/downloads/downloads_api.cc
index aeeac02..36cb9b6 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api.cc
@@ -1245,6 +1245,9 @@ DownloadsAcceptDangerFunction::DownloadsAcceptDangerFunction() {}
 
 DownloadsAcceptDangerFunction::~DownloadsAcceptDangerFunction() {}
 
+DownloadsAcceptDangerFunction::OnPromptCreatedCallback*
+    DownloadsAcceptDangerFunction::on_prompt_created_ = NULL;
+
 bool DownloadsAcceptDangerFunction::RunImpl() {
   scoped_ptr<downloads::AcceptDanger::Params> params(
       downloads::AcceptDanger::Params::Create(*args_));
@@ -1262,13 +1265,15 @@ bool DownloadsAcceptDangerFunction::RunImpl() {
   RecordApiFunctions(DOWNLOADS_FUNCTION_ACCEPT_DANGER);
   // DownloadDangerPrompt displays a modal dialog using native widgets that the
   // user must either accept or cancel. It cannot be scripted.
-  DownloadDangerPrompt::Create(
+  DownloadDangerPrompt* prompt = DownloadDangerPrompt::Create(
       download_item,
       web_contents,
       true,
       base::Bind(&DownloadsAcceptDangerFunction::DangerPromptCallback,
                  this, params->download_id));
   // DownloadDangerPrompt deletes itself
+  if (on_prompt_created_ && !on_prompt_created_->is_null())
+    on_prompt_created_->Run(prompt);
   return true;
 }
 
diff --git a/chrome/browser/extensions/api/downloads/downloads_api.h b/chrome/browser/extensions/api/downloads/downloads_api.h
index e7db6d0..fed2a8b 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.h
+++ b/chrome/browser/extensions/api/downloads/downloads_api.h
@@ -191,6 +191,12 @@ class DownloadsRemoveFileFunction : public AsyncExtensionFunction,
 
 class DownloadsAcceptDangerFunction : public AsyncExtensionFunction {
  public:
+  typedef base::Callback<void(DownloadDangerPrompt*)> OnPromptCreatedCallback;
+  static void OnPromptCreatedForTesting(
+      OnPromptCreatedCallback* callback) {
+    on_prompt_created_ = callback;
+  }
+
   DECLARE_EXTENSION_FUNCTION("downloads.acceptDanger", DOWNLOADS_ACCEPTDANGER)
   DownloadsAcceptDangerFunction();
   virtual bool RunImpl() OVERRIDE;
@@ -201,6 +207,7 @@ class DownloadsAcceptDangerFunction : public AsyncExtensionFunction {
                             DownloadDangerPrompt::Action action);
 
  private:
+  static OnPromptCreatedCallback* on_prompt_created_;
   DISALLOW_COPY_AND_ASSIGN(DownloadsAcceptDangerFunction);
 };
 
diff --git a/chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc b/chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
index 98c5f48..2c81f39 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc
@@ -18,6 +18,7 @@
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/download/download_test_file_activity_observer.h"
 #include "chrome/browser/extensions/api/downloads/downloads_api.h"
+#include "chrome/browser/extensions/browser_action_test_util.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -3469,6 +3470,45 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
 // TODO(benjhayden) Test that the shelf is shown for download() both with and
 // without a WebContents.
 
+void OnDangerPromptCreated(DownloadDangerPrompt* prompt) {
+  prompt->InvokeActionForTesting(DownloadDangerPrompt::ACCEPT);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_AcceptDanger) {
+  // Download a file that will be marked dangerous; click the browser action
+  // button; the browser action poup will call acceptDanger(); when the
+  // DownloadDangerPrompt is created, pretend that the user clicks the Accept
+  // button; wait until the download completes.
+  LoadExtension("downloads_split");
+  scoped_ptr<base::Value> result(RunFunctionAndReturnResult(
+      new DownloadsDownloadFunction(),
+      "[{\"url\": \"data:,\", \"filename\": \"dangerous.swf\"}]"));
+  ASSERT_TRUE(result.get());
+  int result_id = -1;
+  ASSERT_TRUE(result->GetAsInteger(&result_id));
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
+  ASSERT_TRUE(item);
+  ASSERT_TRUE(WaitFor(api::OnChanged::kEventName, base::StringPrintf(
+      "[{\"id\": %d, "
+      "  \"danger\": {"
+      "    \"previous\": \"safe\","
+      "    \"current\": \"file\"}}]",
+      result_id)));
+  ASSERT_TRUE(item->IsDangerous());
+  ScopedCancellingItem canceller(item);
+  scoped_ptr<content::DownloadTestObserver> observer(
+      new content::DownloadTestObserverTerminal(
+          GetCurrentManager(), 1,
+          content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_IGNORE));
+  DownloadsAcceptDangerFunction::OnPromptCreatedCallback callback =
+      base::Bind(&OnDangerPromptCreated);
+  DownloadsAcceptDangerFunction::OnPromptCreatedForTesting(
+      &callback);
+  BrowserActionTestUtil(browser()).Press(0);
+  observer->WaitForFinished();
+}
+
 class DownloadsApiTest : public ExtensionApiTest {
  public:
   DownloadsApiTest() {}
diff --git a/chrome/browser/extensions/extension_host.cc b/chrome/browser/extensions/extension_host.cc
index 7388727..d7cd528 100644
--- a/chrome/browser/extensions/extension_host.cc
+++ b/chrome/browser/extensions/extension_host.cc
@@ -44,6 +44,7 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
+#include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_view.h"
@@ -287,6 +288,34 @@ void ExtensionHost::Close() {
       content::Details<ExtensionHost>(this));
 }
 
+#if !defined(OS_ANDROID)
+web_modal::WebContentsModalDialogHost*
+ExtensionHost::GetWebContentsModalDialogHost() {
+  return this;
+}
+
+gfx::NativeView ExtensionHost::GetHostView() const {
+  return view_ ? view_->native_view() : NULL;
+}
+
+gfx::Point ExtensionHost::GetDialogPosition(const gfx::Size& size) {
+  if (!GetVisibleWebContents())
+    return gfx::Point();
+  gfx::Rect bounds = GetVisibleWebContents()->GetView()->GetViewBounds();
+  return gfx::Point(
+      std::max(0, (bounds.width() - size.width()) / 2),
+      std::max(0, (bounds.height() - size.height()) / 2));
+}
+
+void ExtensionHost::AddObserver(
+    web_modal::WebContentsModalDialogHostObserver* observer) {
+}
+
+void ExtensionHost::RemoveObserver(
+    web_modal::WebContentsModalDialogHostObserver* observer) {
+}
+#endif
+
 void ExtensionHost::Observe(int type,
                             const content::NotificationSource& source,
                             const content::NotificationDetails& details) {
diff --git a/chrome/browser/extensions/extension_host.h b/chrome/browser/extensions/extension_host.h
index 3b084e4..fe97ef2 100644
--- a/chrome/browser/extensions/extension_host.h
+++ b/chrome/browser/extensions/extension_host.h
@@ -31,6 +31,7 @@
 
 #if !defined(OS_ANDROID)
 #include "chrome/browser/ui/chrome_web_modal_dialog_manager_delegate.h"
+#include "components/web_modal/web_contents_modal_dialog_host.h"
 #endif
 
 class Browser;
@@ -53,6 +54,7 @@ class WindowController;
 class ExtensionHost : public content::WebContentsDelegate,
 #if !defined(OS_ANDROID)
                       public ChromeWebModalDialogManagerDelegate,
+                      public web_modal::WebContentsModalDialogHost,
 #endif
                       public content::WebContentsObserver,
                       public ExtensionFunctionDispatcher::Delegate,
@@ -197,6 +199,20 @@ class ExtensionHost : public content::WebContentsDelegate,
   // Closes this host (results in deletion).
   void Close();
 
+#if !defined(OS_ANDROID)
+  // ChromeWebModalDialogManagerDelegate
+  virtual web_modal::WebContentsModalDialogHost*
+      GetWebContentsModalDialogHost() OVERRIDE;
+
+  // web_modal::WebContentsModalDialogHost
+  virtual gfx::NativeView GetHostView() const OVERRIDE;
+  virtual gfx::Point GetDialogPosition(const gfx::Size& size) OVERRIDE;
+  virtual void AddObserver(
+      web_modal::WebContentsModalDialogHostObserver* observer) OVERRIDE;
+  virtual void RemoveObserver(
+      web_modal::WebContentsModalDialogHostObserver* observer) OVERRIDE;
+#endif
+
   // ExtensionFunctionDispatcher::Delegate
   virtual WindowController* GetExtensionWindowController() const OVERRIDE;
 
diff --git a/chrome/browser/ui/cocoa/extensions/extension_popup_controller.h b/chrome/browser/ui/cocoa/extensions/extension_popup_controller.h
index 121ca64..84bf9c2 100644
--- a/chrome/browser/ui/cocoa/extensions/extension_popup_controller.h
+++ b/chrome/browser/ui/cocoa/extensions/extension_popup_controller.h
@@ -51,6 +51,10 @@ class ExtensionHost;
   // Whether the popup has a devtools window attached to it.
   BOOL beingInspected_;
 
+  // There's an extra windowDidResignKey: notification right after a
+  // ConstrainedWindow closes that should be ignored.
+  BOOL ignoreWindowDidResignKey_;
+
   // The size once the ExtensionView has loaded.
   NSSize pendingSize_;
 }
diff --git a/chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm b/chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm
index 8cdb921..e3efa41 100644
--- a/chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm
+++ b/chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm
@@ -17,6 +17,7 @@
 #import "chrome/browser/ui/cocoa/browser_window_cocoa.h"
 #import "chrome/browser/ui/cocoa/extensions/extension_view_mac.h"
 #import "chrome/browser/ui/cocoa/info_bubble_window.h"
+#include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/devtools_manager.h"
 #include "content/public/browser/notification_details.h"
@@ -163,6 +164,7 @@ class DevtoolsNotificationBridge : public content::NotificationObserver {
                          anchoredAt:anchoredAt])) {
     host_.reset(host);
     beingInspected_ = devMode;
+    ignoreWindowDidResignKey_ = NO;
 
     InfoBubbleView* view = self.bubble;
     [view setArrowLocation:arrowLocation];
@@ -201,6 +203,16 @@ class DevtoolsNotificationBridge : public content::NotificationObserver {
   DevToolsWindow::OpenDevToolsWindow(host_->render_view_host());
 }
 
+- (void)close {
+  web_modal::WebContentsModalDialogManager* modalDialogManager =
+      web_modal::WebContentsModalDialogManager::FromWebContents(
+          host_->host_contents());
+  if (!modalDialogManager ||
+      !modalDialogManager->IsShowingDialog()) {
+    [super close];
+  }
+}
+
 - (void)windowWillClose:(NSNotification *)notification {
   [super windowWillClose:notification];
   gPopup = nil;
@@ -210,6 +222,22 @@ class DevtoolsNotificationBridge : public content::NotificationObserver {
 }
 
 - (void)windowDidResignKey:(NSNotification*)notification {
+  // When a modal dialog is opened on top of the popup and when it's closed, it
+  // steals key-ness from the popup. Don't close the popup when this happens.
+  // There's an extra windowDidResignKey: notification after the modal dialog
+  // closes that should also be ignored.
+  web_modal::WebContentsModalDialogManager* modalDialogManager =
+      web_modal::WebContentsModalDialogManager::FromWebContents(
+          host_->host_contents());
+  if (modalDialogManager &&
+      modalDialogManager->IsShowingDialog()) {
+    ignoreWindowDidResignKey_ = YES;
+    return;
+  }
+  if (ignoreWindowDidResignKey_) {
+    ignoreWindowDidResignKey_ = NO;
+    return;
+  }
   if (!beingInspected_)
     [super windowDidResignKey:notification];
 }
diff --git a/chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm b/chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm
index 40a949a..7301374 100644
--- a/chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm
+++ b/chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm
@@ -112,8 +112,13 @@ void TabModalConfirmDialogMac::CloseDialog() {
 
 void TabModalConfirmDialogMac::OnConstrainedWindowClosed(
     ConstrainedWindowMac* window) {
+  // If this method should mistakenly be called during Delegate::Close(),
+  // prevent a double-delete by moving delegate_ to a stack variable.
+  if (!delegate_)
+    return;
+  scoped_ptr<TabModalConfirmDialogDelegate> delegate(delegate_.Pass());
   // Provide a disposition in case the dialog was closed without accepting or
   // cancelling.
-  delegate_->Close();
+  delegate->Close();
   delete this;
 }
diff --git a/chrome/test/data/extensions/api_test/downloads_split/accept_danger.html b/chrome/test/data/extensions/api_test/downloads_split/accept_danger.html
new file mode 100644
index 0000000..eddfeae
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/downloads_split/accept_danger.html
@@ -0,0 +1 @@
+<script src="accept_danger.js"></script>
diff --git a/chrome/test/data/extensions/api_test/downloads_split/accept_danger.js b/chrome/test/data/extensions/api_test/downloads_split/accept_danger.js
new file mode 100644
index 0000000..604aad1
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/downloads_split/accept_danger.js
@@ -0,0 +1,14 @@
+// Copyright 2013 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.
+
+chrome.downloads.search({}, function(items) {
+  for (var i = 0; i < items.length; ++i) {
+    if (items[i].state == 'in_progress' &&
+        items[i].danger != 'safe' &&
+        items[i].danger != 'accepted') {
+      console.log(items[i].id);
+      chrome.downloads.acceptDanger(items[i].id);
+    }
+  }
+});
diff --git a/chrome/test/data/extensions/api_test/downloads_split/manifest.json b/chrome/test/data/extensions/api_test/downloads_split/manifest.json
index 1eaa6e1..aea946e 100644
--- a/chrome/test/data/extensions/api_test/downloads_split/manifest.json
+++ b/chrome/test/data/extensions/api_test/downloads_split/manifest.json
@@ -5,6 +5,7 @@
   "description": "downloads incognito split apitest",
   "incognito": "split",
   "web_accessible_resources": ["empty.html"],
+  "browser_action": {"default_popup": "accept_danger.html"},
   "permissions": [
     "downloads",
     "downloads.open",