Add a way to kill apps without killing windows

extensions/browser/process_manager.cc

Index: extensions/browser/process_manager.cc
diff --git a/extensions/browser/process_manager.cc b/extensions/browser/process_manager.cc
index 60af62c4c83d9b9ed6bfd434573bc243867e620e..d9c47ea6b880727f404b62fccdd97e639046efcd 100644
--- a/extensions/browser/process_manager.cc
+++ b/extensions/browser/process_manager.cc
@@ -192,6 +192,23 @@ struct ProcessManager::BackgroundPageData {
         close_sequence_id(0) {}
 };
 
+struct ProcessManager::ExtensionRenderViewData {

[scheib, 2014/10/31 19:49:05]

Probably comment the struct. All classes have summary comments by style guide,
and it's seems good practice for structs as well.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Comments

[hashimoto, 2014/11/03 16:02:26]

Done.

+  // The type of the view.
+  extensions::ViewType view_type;
+
+  // Whether the view is keeping the lazy background page alive or not.
+  bool has_keepalive;
+
+  ExtensionRenderViewData()
+      : view_type(VIEW_TYPE_INVALID), has_keepalive(false) {}
+
+  // Returns whether the view can keep the lazy background page alive or not.
+  bool CanKeepalive() const {
+    return view_type != VIEW_TYPE_INVALID &&

[scheib, 2014/10/31 19:49:05]

Consider a switch(view_type) so that there will be a compiler error if the any
new ViteType enumerations are created.

[hashimoto, 2014/11/03 16:02:26]

Done.

+           view_type != VIEW_TYPE_EXTENSION_BACKGROUND_PAGE;
+  }
+};
+
 //
 // ProcessManager
 //
@@ -368,6 +385,40 @@ const Extension* ProcessManager::GetExtensionForRenderViewHost(
       GetExtensionID(render_view_host));
 }
 
+void ProcessManager::AcquireLazyKeepaliveCountForView(
+    content::RenderViewHost* render_view_host) {
+  auto it = all_extension_views_.find(render_view_host);
+  if (it == all_extension_views_.end())
+    return;
+
+  ExtensionRenderViewData* data = &it->second;
+  if (data->CanKeepalive() && !data->has_keepalive) {
+    const Extension* extension =
+        GetExtensionForRenderViewHost(render_view_host);
+    if (extension) {
+      IncrementLazyKeepaliveCount(extension);
+      data->has_keepalive = true;
+    }
+  }
+}
+
+void ProcessManager::ReleaseLazyKeepaliveCountForView(
+    content::RenderViewHost* render_view_host) {
+  auto it = all_extension_views_.find(render_view_host);
+  if (it == all_extension_views_.end())
+    return;
+
+  ExtensionRenderViewData* data = &it->second;
+  if (data->CanKeepalive() && data->has_keepalive) {
+    const Extension* extension =
+        GetExtensionForRenderViewHost(render_view_host);
+    if (extension) {
+      DecrementLazyKeepaliveCount(extension);
+      data->has_keepalive = false;
+    }
+  }
+}
+
 void ProcessManager::UnregisterRenderViewHost(
     RenderViewHost* render_view_host) {
   ExtensionRenderViews::iterator view =
@@ -376,17 +427,10 @@ void ProcessManager::UnregisterRenderViewHost(
     return;
 
   OnRenderViewHostUnregistered(GetBrowserContext(), render_view_host);
-  ViewType view_type = view->second;
-  all_extension_views_.erase(view);
 
   // Keepalive count, balanced in RegisterRenderViewHost.
-  if (view_type != VIEW_TYPE_INVALID &&
-      view_type != VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
-    const Extension* extension = GetExtensionForRenderViewHost(
-        render_view_host);
-    if (extension)
-      DecrementLazyKeepaliveCount(extension);
-  }
+  ReleaseLazyKeepaliveCountForView(render_view_host);
+  all_extension_views_.erase(view);
 }
 
 bool ProcessManager::RegisterRenderViewHost(RenderViewHost* render_view_host) {
@@ -396,12 +440,13 @@ bool ProcessManager::RegisterRenderViewHost(RenderViewHost* render_view_host) {
     return false;
 
   WebContents* web_contents = WebContents::FromRenderViewHost(render_view_host);
-  all_extension_views_[render_view_host] = GetViewType(web_contents);
+  ExtensionRenderViewData* data = &all_extension_views_[render_view_host];
+  data->view_type = GetViewType(web_contents);
 
   // Keep the lazy background page alive as long as any non-background-page
   // extension views are visible. Keepalive count balanced in
   // UnregisterRenderViewHost.
-  IncrementLazyKeepaliveCountForView(render_view_host);
+  AcquireLazyKeepaliveCountForView(render_view_host);
   return true;
 }
 
@@ -459,20 +504,6 @@ void ProcessManager::DecrementLazyKeepaliveCount(
   }
 }
 
-void ProcessManager::IncrementLazyKeepaliveCountForView(
-    RenderViewHost* render_view_host) {
-  WebContents* web_contents =
-      WebContents::FromRenderViewHost(render_view_host);
-  ViewType view_type = GetViewType(web_contents);
-  if (view_type != VIEW_TYPE_INVALID &&
-      view_type != VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
-    const Extension* extension = GetExtensionForRenderViewHost(
-        render_view_host);
-    if (extension)
-      IncrementLazyKeepaliveCount(extension);
-  }
-}
-
 // This implementation layers on top of the keepalive count. An impulse sets
 // a per extension flag. On a regular interval that flag is checked. Changes
 // from the flag not being set to set cause an IncrementLazyKeepaliveCount.
@@ -606,6 +637,24 @@ void ProcessManager::CloseLazyBackgroundPageNow(const std::string& extension_id,
   ExtensionHost* host = GetBackgroundHostForExtension(extension_id);
   if (host &&
       sequence_id == background_page_data_[extension_id].close_sequence_id) {
+    // Close remaining views.
+    std::vector<RenderViewHost*> views_to_close;
+    for (const auto& view : all_extension_views_) {
+      if (view.second.CanKeepalive() &&
+          GetExtensionID(view.first) == extension_id) {
+        DCHECK(!view.second.has_keepalive);
+        views_to_close.push_back(view.first);
+      }
+    }
+    for (auto view : views_to_close) {
+      view->ClosePage();
+      // RenderViewHost::ClosePage() may result in calling
+      // UnregisterRenderViewHost() asynchronously and may cause race conditions
+      // when the background page is reloaded.
+      // To avoid this, unregister the view now.
+      UnregisterRenderViewHost(view);
+    }
+
     ExtensionHost* host = GetBackgroundHostForExtension(extension_id);
     if (host)
       CloseBackgroundHost(host);
@@ -908,8 +957,8 @@ void ProcessManager::ClearBackgroundPageData(const std::string& extension_id) {
   // views.
   for (ExtensionRenderViews::const_iterator it = all_extension_views_.begin();
        it != all_extension_views_.end(); ++it) {
-    if (GetExtensionID(it->first) == extension_id)
-      IncrementLazyKeepaliveCountForView(it->first);
+    if (GetExtensionID(it->first) == extension_id && it->second.has_keepalive)

[scheib, 2014/10/31 19:49:05]

If this is only run when has_keepalive is true,
then inside AcquireLazyKeepaliveCountForView the
logic "if (data->CanKeepalive() && !data->has_keepalive)"
will always be false, and the keep alive count will fail to
be incremented!

Under what circumstance will we have a view that doesn't have a keepalive?
Previously we incremented for all views, I suppose I'm not seeing why we don't
continue to do so. If there's a reason, please explain it in comments. 

[hashimoto, 2014/11/03 16:02:26]

Thanks!
You're right.
I was about to break this logic completely.
Fixed to do the thing apparently needed by the comment.

BTW, this logic was added in https://codereview.chromium.org/10310005 against a
race bug, in a day when CloseLazyBackgroundPageNow() was not checking the close
sequence ID.
However, since we are checking the close sequence ID now, this logic seems
unneeded to me.
Maybe it's better to just remove this function at all?
As written in the reply to your comment in the header, Athena needs to decrement
the keepalive count when its AppWindow becomes inactive (and needs to increment
the count when being active).

[scheib, 2014/11/03 17:48:28]

It is hard to be certain that this method is not needed. I suspect the
background_page_data_.erase() is needed to reset values to be prepared for when
a background page is started again.

This logic with views reference count is too indirect. I suggest you land this
patch as is, and if you're motivated to refactor then we create an issue, add
DCHECKs, run tests, and commit to see if any assertions are revealed on
developer machines. If it all runs through without requiring the
IncrementLazyKeepaliveCount for an extension that just had
ClearBackgroundPageData called then we should remove this code block.

[hashimoto, 2014/11/04 05:47:33]

Sounds good.
Filed crbug.com/429959.

+      AcquireLazyKeepaliveCountForView(it->first);
   }
 }