Merge 203436 "Unregister for notifications before calling observ..."

> Unregister for notifications before calling observers in BC destructor.
> 
> The BackgroundContents code notifies listeners in its destructor so they know the object is going away. It previously also would free itself in response to certain notifications. In rare circumstances, sending out its going-away notification to its observers would trigger a notification that would cause it to try to free itself again. To avoid this, the code now stops listening for notifications before calling out to its observers.
> 
> BUG=237781
> 
> Review URL: https://chromiumcodereview.appspot.com/16173007

TBR=atwilson@chromium.org

Review URL: https://codereview.chromium.org/16263002

git-svn-id: svn://svn.chromium.org/chrome/branches/1500/src@203449 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 81 insertions, 5 deletions

diff --git a/chrome/browser/extensions/app_background_page_apitest.cc b/chrome/browser/extensions/app_background_page_apitest.cc
index 4f1cbbc..ba770cf 100644
--- a/chrome/browser/extensions/app_background_page_apitest.cc
+++ b/chrome/browser/extensions/app_background_page_apitest.cc
@@ -12,6 +12,7 @@
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension.h"
@@ -20,6 +21,10 @@
 #include "content/public/test/test_utils.h"
 #include "net/dns/mock_host_resolver.h"
 
+#if defined(OS_MACOSX)
+#include "base/mac/scoped_nsautorelease_pool.h"
+#endif
+
 using extensions::Extension;
 
 class AppBackgroundPageApiTest : public ExtensionApiTest {
@@ -76,6 +81,26 @@ class AppBackgroundPageApiTest : public ExtensionApiTest {
 #endif
   }
 
+  void CloseBrowser(Browser* browser) {
+    content::WindowedNotificationObserver observer(
+        chrome::NOTIFICATION_BROWSER_CLOSED,
+        content::NotificationService::AllSources());
+    browser->window()->Close();
+#if defined(OS_MACOSX)
+    // BrowserWindowController depends on the auto release pool being recycled
+    // in the message loop to delete itself, which frees the Browser object
+    // which fires this event.
+    AutoreleasePool()->Recycle();
+#endif
+    observer.Wait();
+  }
+
+  void UnloadExtensionViaTask(const std::string& id) {
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&AppBackgroundPageApiTest::UnloadExtension, this, id));
+  }
+
  private:
   base::ScopedTempDir app_dir_;
 };
@@ -419,3 +444,53 @@ IN_PROC_BROWSER_TEST_F(AppBackgroundPageApiTest, DISABLED_OpenThenClose) {
       BackgroundContentsServiceFactory::GetForProfile(browser()->profile())->
           GetAppBackgroundContents(ASCIIToUTF16(extension->id())));
 }
+
+IN_PROC_BROWSER_TEST_F(AppBackgroundPageApiTest, UnloadExtensionWhileHidden) {
+  host_resolver()->AddRule("a.com", "127.0.0.1");
+  ASSERT_TRUE(StartTestServer());
+
+  std::string app_manifest = base::StringPrintf(
+      "{"
+      "  \"name\": \"App\","
+      "  \"version\": \"0.1\","
+      "  \"manifest_version\": 2,"
+      "  \"app\": {"
+      "    \"urls\": ["
+      "      \"http://a.com/\""
+      "    ],"
+      "    \"launch\": {"
+      "      \"web_url\": \"http://a.com:%d/\""
+      "    }"
+      "  },"
+      "  \"permissions\": [\"background\"],"
+      "  \"background\": {"
+      "    \"page\": \"http://a.com:%d/test.html\""
+      "  }"
+      "}",
+      test_server()->host_port_pair().port(),
+      test_server()->host_port_pair().port());
+
+  base::FilePath app_dir;
+  ASSERT_TRUE(CreateApp(app_manifest, &app_dir));
+  // Background mode should not be active now because no background app was
+  // loaded.
+  ASSERT_TRUE(LoadExtension(app_dir));
+  // Background mode be active now because a background page was created when
+  // the app was loaded.
+  ASSERT_TRUE(WaitForBackgroundMode(true));
+
+  const Extension* extension = GetSingleLoadedExtension();
+  ASSERT_TRUE(
+      BackgroundContentsServiceFactory::GetForProfile(browser()->profile())->
+          GetAppBackgroundContents(ASCIIToUTF16(extension->id())));
+
+  // Close all browsers - app should continue running.
+  SetExitWhenLastBrowserCloses(false);
+  CloseBrowser(browser());
+
+  // Post a task to unload the extension - this should cause Chrome to exit
+  // cleanly (not crash).
+  UnloadExtensionViaTask(extension->id());
+  content::RunAllPendingInMessageLoop();
+  ASSERT_TRUE(WaitForBackgroundMode(false));
+}
diff --git a/chrome/browser/tab_contents/background_contents.cc b/chrome/browser/tab_contents/background_contents.cc
index 3f8b589..106351c 100644
--- a/chrome/browser/tab_contents/background_contents.cc
+++ b/chrome/browser/tab_contents/background_contents.cc
@@ -57,15 +57,16 @@ BackgroundContents::BackgroundContents()
 BackgroundContents::~BackgroundContents() {
   if (!web_contents_.get())   // Will be null for unit tests.
     return;
+
+  // Unregister for any notifications before notifying observers that we are
+  // going away - this prevents any re-entrancy due to chained notifications
+  // (http://crbug.com/237781).
+  registrar_.RemoveAll();
+
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_BACKGROUND_CONTENTS_DELETED,
       content::Source<Profile>(profile_),
       content::Details<BackgroundContents>(this));
-  // Manually clear web_contents_ to try to track down http://crbug.com/164617.
-  // Freeing the WebContents can cause the RenderViewHost to be destroyed, so
-  // we need to do this after sending out BACKGROUND_CONTENTS_DELETED to give
-  // things like the TaskManager a chance to clean up their references first.
-  web_contents_.reset();
 }
 
 const GURL& BackgroundContents::GetURL() const {