Now closes existing background contents if app opens a new one.

chrome/browser/tab_contents/render_view_host_delegate_helper.cc

Index: chrome/browser/tab_contents/render_view_host_delegate_helper.cc
diff --git a/chrome/browser/tab_contents/render_view_host_delegate_helper.cc b/chrome/browser/tab_contents/render_view_host_delegate_helper.cc
index 4f1afa9f3cabec79ac868870aca66ae923cef978..beb7ce96e510cef333f3d25eaab22c3f9e79a4dc 100644
--- a/chrome/browser/tab_contents/render_view_host_delegate_helper.cc
+++ b/chrome/browser/tab_contents/render_view_host_delegate_helper.cc
@@ -107,11 +107,10 @@ RenderViewHostDelegateViewHelper::MaybeCreateBackgroundContents(
   if (extension->background_url().is_valid())
     return NULL;
 
-  // Only allow a single background contents per app.
+  // No BackgroundContents allowed if BackgroundContentsService doesn't exist.
   BackgroundContentsService* service =
       BackgroundContentsServiceFactory::GetForProfile(profile);
-  if (!service || service->GetAppBackgroundContents(
-          ASCIIToUTF16(extension->id())))
+  if (!service)
     return NULL;
 
   // Ensure that we're trying to open this from the extension's process.
@@ -121,6 +120,16 @@ RenderViewHostDelegateViewHelper::MaybeCreateBackgroundContents(
       site->GetProcess() != process_manager->GetExtensionProcess(opener_url))
     return NULL;
 
+  // Only allow a single background contents per app. If one already exists,
+  // close it.
+  BackgroundContents* existing =
+      service->GetAppBackgroundContents(ASCIIToUTF16(extension->id()));
+  if (existing) {
+    DLOG(WARNING) << "Closing existing BackgroundContents for " << opener_url;

[brettw, 2011/10/04 22:03:47]

I don't quite get this code. Is this an illegal condition?

I'm also surprised that the right way to close a background contents is to reach
inside its RVH and tell it to close before deleting it. Is that step necessary.
Can it be made less fragile?

[Andrew T Wilson (Slow), 2011/10/05 19:45:58]

The warning is because "reopening an existing BackgroundContents" is a fairly
exceptional condition (which is why it took us several months before we figured
out a repro case).
Good point. I've removed the call to ClosePage() and verified that deleting the
BackgroundContents by itself properly closes the page.

[brettw, 2011/10/05 20:03:34]

Let's think about this in terms of somebody random that sees this message in
their output. What should they do? It's a warning so it must be bad. Should they
fix a bug, report this condition to someone? Unless normal people need to do
something, I'd suggest no warning. If it could be useful to people, it should be
rephrased or made into an INFO.

[Andrew T Wilson (Slow), 2011/10/05 20:49:13]

Done.

+    existing->render_view_host()->ClosePage();
+    delete existing;
+  }
+
   // Passed all the checks, so this should be created as a BackgroundContents.
   return service->CreateBackgroundContents(site, route_id, profile, frame_name,
                                            ASCIIToUTF16(extension->id()));