Fix issue 174250, crash when reloading packaged app.

When a shell window was closed, it wasn't removed from the registry immediately
(it was removed in a callback from OnNativeClose, which happens a bit later.)
This meant that when the app was loaded again, the window was still in the
registry with a stale pointer to the old extension. ExtensionSettingsHandler
was tickling this by iterating shell windows on extension load.

BUG=174250


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

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

chrome/browser/extensions/platform_app_browsertest.cc

@@ -28,6 +28,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/web_contents_modal_dialog_manager.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/devtools_agent_host.h"
@@ -686,7 +687,7 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, ReloadRelaunches) {
   ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
   ASSERT_TRUE(GetFirstShellWindow());
 
-  // Now tell the app to reload itself
+  // Now tell the app to reload itself.
   ExtensionTestMessageListener launched_listener2("Launched", false);
   launched_listener.Reply("reload");
   ASSERT_TRUE(launched_listener2.WaitUntilSatisfied());
@@ -851,4 +852,22 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, MAYBE_WebContentsHasFocus) {
   CloseShellWindow(window);
 }
 
+IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, DontCrashWhenReloading) {
+  // Regression test for http://crbug.com/174250
+  browser()->profile()->GetPrefs()->SetBoolean(
+      prefs::kExtensionsUIDeveloperMode, true);
+  ui_test_utils::NavigateToURL(browser(), GURL("chrome://extensions"));
+  ExtensionTestMessageListener launched_listener("Launched", true);
+  const Extension* extension = LoadAndLaunchPlatformApp("reload");
+  ASSERT_TRUE(extension);
+  ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
+  ASSERT_TRUE(GetFirstShellWindow());
+
+  // Now tell the app to reload itself.
+  ExtensionTestMessageListener launched_listener2("Launched", false);
+  launched_listener.Reply("reload");
+  ASSERT_TRUE(launched_listener2.WaitUntilSatisfied());
+  ASSERT_TRUE(GetFirstShellWindow());
+}
+
 }  // namespace extensions

chrome/browser/extensions/shell_window_registry.cc

@@ -67,8 +67,9 @@ void ShellWindowRegistry::ShellWindowIconChanged(ShellWindow* shell_window) {
 }
 
 void ShellWindowRegistry::RemoveShellWindow(ShellWindow* shell_window) {
-  shell_windows_.erase(shell_window);
-  FOR_EACH_OBSERVER(Observer, observers_, OnShellWindowRemoved(shell_window));
+  size_t num_erased = shell_windows_.erase(shell_window);
+  if (num_erased > 0)
+    FOR_EACH_OBSERVER(Observer, observers_, OnShellWindowRemoved(shell_window));
 }
 
 void ShellWindowRegistry::AddObserver(Observer* observer) {

chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.mm

@@ -4,7 +4,9 @@
 
 #include "chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.h"
 
+#include "base/bind.h"
 #include "base/mac/mac_util.h"
+#include "base/message_loop.h"
 #include "base/sys_string_conversions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/cocoa/browser_window_utils.h"
@@ -679,7 +681,13 @@ gfx::Insets NativeAppWindowCocoa::GetFrameInsets() const {
 void NativeAppWindowCocoa::WindowWillClose() {
   [window_controller_ setAppWindow:NULL];
   shell_window_->OnNativeWindowChanged();
-  shell_window_->OnNativeClose();
+  // On other platforms, the native window doesn't get destroyed synchronously.
+  // We simulate that here so that ShellWindow can assume that it doesn't get
+  // deleted immediately upon calling Close().
+  MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&ShellWindow::OnNativeClose,
+                 base::Unretained(shell_window_)));
 }
 
 void NativeAppWindowCocoa::WindowDidBecomeKey() {

chrome/browser/ui/extensions/shell_window.cc

@@ -244,6 +244,11 @@ ShellWindow::~ShellWindow() {
   chrome::EndKeepAlive();
 }
 
+void ShellWindow::Close() {
+  extensions::ShellWindowRegistry::Get(profile_)->RemoveShellWindow(this);
+  native_app_window_->Close();
+}
+
 void ShellWindow::RequestMediaAccessPermission(
     content::WebContents* web_contents,
     const content::MediaStreamRequest& request,
@@ -343,9 +348,16 @@ void ShellWindow::RequestToLockMouse(WebContents* web_contents,
 }
 
 void ShellWindow::OnNativeClose() {
+  // This method is shared between the path for the user clicking the close
+  // button and the path where a close is triggered by code (e.g. by the
+  // extension being unloaded). In the latter case, this RemoveShellWindow is
+  // superfluous, since it will already have been removed, but the call is
+  // idempotent so it's harmless the second time.
   extensions::ShellWindowRegistry::Get(profile_)->RemoveShellWindow(this);
-  content::RenderViewHost* rvh = web_contents_->GetRenderViewHost();
-  rvh->Send(new ExtensionMsg_AppWindowClosed(rvh->GetRoutingID()));
+  if (extension_) {
+    content::RenderViewHost* rvh = web_contents_->GetRenderViewHost();
+    rvh->Send(new ExtensionMsg_AppWindowClosed(rvh->GetRoutingID()));
+  }
   delete this;
 }
 
@@ -487,7 +499,7 @@ void ShellWindow::UpdateExtensionAppIcon() {
 
 void ShellWindow::CloseContents(WebContents* contents) {
   DCHECK(contents == web_contents_);
-  native_app_window_->Close();
+  Close();
 }
 
 bool ShellWindow::ShouldSuppressDialogs() {
@@ -561,12 +573,16 @@ void ShellWindow::Observe(int type,
       const extensions::Extension* unloaded_extension =
           content::Details<extensions::UnloadedExtensionInfo>(
               details)->extension;
-      if (extension_ == unloaded_extension)
-        native_app_window_->Close();
+      if (extension_ == unloaded_extension) {
+        Close();
+        // After this notification finishes processing, the Extension will be
+        // deleted, so we null out our reference to avoid bad access.
+        extension_ = NULL;
+      }
       break;
     }
     case chrome::NOTIFICATION_APP_TERMINATING:
-      native_app_window_->Close();
+      Close();
       break;
     default:
       NOTREACHED() << "Received unexpected notification";

chrome/browser/ui/extensions/shell_window.h

@@ -133,6 +133,11 @@ class ShellWindow : public content::NotificationObserver,
   // per platform). Public users of ShellWindow should use ShellWindow::Create.
   void Init(const GURL& url, const CreateParams& params);
 
+  // Remove the window from the ShellWindowRegistry and tell the native
+  // window to close. The native window won't be actually closed until
+  // OnNativeClose().
+  void Close();
+
   // content::WebContentsObserver implementation.
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
 

chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/gtk/extensions/native_app_window_gtk.h"
 
+#include "base/message_loop.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.h"
@@ -184,12 +185,15 @@ void NativeAppWindowGtk::Close() {
   // To help catch bugs in any event handlers that might get fired during the
   // destruction, set window_ to NULL before any handlers will run.
   window_ = NULL;
-
-  // OnNativeClose does a delete this so no other members should
-  // be accessed after. gtk_widget_destroy is safe (and must
-  // be last).
-  shell_window_->OnNativeClose();
   gtk_widget_destroy(window);
+
+  // On other platforms, the native window doesn't get destroyed synchronously.
+  // We simulate that here so that ShellWindow can assume that it doesn't get
+  // deleted immediately upon calling Close().
+  MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&ShellWindow::OnNativeClose,
+                 base::Unretained(shell_window_)));
 }
 
 void NativeAppWindowGtk::Activate() {