Add a way to kill apps without killing windows

extensions/browser/process_manager.cc

 // 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.
 
 #include "extensions/browser/process_manager.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
@@ skipping 174 matching lines @@
   linked_ptr<base::ElapsedTimer> since_suspended;
 
   BackgroundPageData()
       : lazy_keepalive_count(0),
         keepalive_impulse(false),
         previous_keepalive_impulse(false),
         is_closing(false),
         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
 //
 
 // static
 ProcessManager* ProcessManager::Get(BrowserContext* context) {
   return ProcessManagerFactory::GetForBrowserContext(context);
 }
 
 // static
@@ skipping 156 matching lines @@
 
 const Extension* ProcessManager::GetExtensionForRenderViewHost(
     RenderViewHost* render_view_host) {
   if (!render_view_host->GetSiteInstance())
     return NULL;
 
   return extension_registry_->enabled_extensions().GetByID(
       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 =
       all_extension_views_.find(render_view_host);
   if (view == all_extension_views_.end())
     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) {
   const Extension* extension = GetExtensionForRenderViewHost(
       render_view_host);
   if (!extension)
     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;
 }
 
 SiteInstance* ProcessManager::GetSiteInstanceForURL(const GURL& url) {
   return site_instance_->GetRelatedSiteInstance(url);
 }
 
 bool ProcessManager::IsBackgroundHostClosing(const std::string& extension_id) {
   ExtensionHost* host = GetBackgroundHostForExtension(extension_id);
   return (host && background_page_data_[extension_id].is_closing);
@@ skipping 37 matching lines @@
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&ProcessManager::OnLazyBackgroundPageIdle,
                    weak_ptr_factory_.GetWeakPtr(),
                    extension_id,
                    last_background_close_sequence_id_),
         base::TimeDelta::FromMilliseconds(g_event_page_idle_time_msec));
   }
 }
 
-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.
 void ProcessManager::KeepaliveImpulse(const Extension* extension) {
   if (!BackgroundInfo::HasLazyBackgroundPage(extension))
     return;
 
   BackgroundPageData& bd = background_page_data_[extension->id()];
 
   if (!bd.keepalive_impulse) {
@@ skipping 113 matching lines @@
                  extension_id,
                  sequence_id),
       base::TimeDelta::FromMilliseconds(g_event_page_suspending_time_msec));
 }
 
 void ProcessManager::CloseLazyBackgroundPageNow(const std::string& extension_id,
                                                 uint64 sequence_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);
   }
 }
 
 void ProcessManager::OnNetworkRequestStarted(
     content::RenderFrameHost* render_frame_host) {
   ExtensionHost* host = GetBackgroundHostForExtension(
       GetExtensionIDFromFrame(render_frame_host));
@@ skipping 282 matching lines @@
 }
 
 void ProcessManager::ClearBackgroundPageData(const std::string& extension_id) {
   background_page_data_.erase(extension_id);
 
   // Re-register all RenderViews for this extension. We do this to restore
   // the lazy_keepalive_count (if any) to properly reflect the number of open
   // 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);
   }
 }
 
 //
 // IncognitoProcessManager
 //
 
 IncognitoProcessManager::IncognitoProcessManager(
     BrowserContext* incognito_context,
     BrowserContext* original_context,
@@ skipping 29 matching lines @@
   if (extension && !IncognitoInfo::IsSplitMode(extension)) {
     BrowserContext* original_context =
         ExtensionsBrowserClient::Get()->GetOriginalContext(GetBrowserContext());
     return ProcessManager::Get(original_context)->GetSiteInstanceForURL(url);
   }
 
   return ProcessManager::GetSiteInstanceForURL(url);
 }
 
 }  // namespace extensions