[wip] Introduce SiteInstanceKeepAlive.

content/browser/frame_host/navigation_entry_screenshot_manager.cc

Index: content/browser/frame_host/navigation_entry_screenshot_manager.cc
diff --git a/content/browser/frame_host/navigation_entry_screenshot_manager.cc b/content/browser/frame_host/navigation_entry_screenshot_manager.cc
index b2bf429dc0a6c66a34266a5267ed62267a8aacb6..9d9301a1df59dca7f46fb472837b54aa5b04b7ac 100644
--- a/content/browser/frame_host/navigation_entry_screenshot_manager.cc
+++ b/content/browser/frame_host/navigation_entry_screenshot_manager.cc
@@ -11,6 +11,7 @@
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/render_widget_host_view.h"
+#include "content/public/browser/site_instance.h"
 #include "content/public/common/content_switches.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkPaint.h"
@@ -75,6 +76,28 @@ class ScreenshotData : public base::RefCountedThreadSafe<ScreenshotData> {
   DISALLOW_COPY_AND_ASSIGN(ScreenshotData);
 };
 
+// A temporary solution to keep a RenderViewHost and SiteInstanceImpl live

[Charlie Reis, 2014/02/20 19:01:53]

Please add a TODO saying when this can be removed (when RenderFrameProxy
replaces swappedout://).

[sadrul, 2014/02/21 03:51:16]

Done (is there a crbug I can reference from the TODO?)

+// (after cross-process/cross-site navigation) for long enough to get the
+// screenshot.
+class SiteInstanceKeepAlive {
+ public:
+  explicit SiteInstanceKeepAlive(scoped_refptr<SiteInstanceImpl> instance)
+      : instance_(instance) {
+    instance_->increment_active_view_count();
+    instance_->GetProcess()->AddPendingView();
+  }
+
+  ~SiteInstanceKeepAlive() {
+    instance_->GetProcess()->RemovePendingView();
+    instance_->decrement_active_view_count();

[sadrul, 2014/02/20 05:13:31]

Do we need to do anything here to make sure the SiteInstance/RenderProcessHost
gets destroyed if a previous attempt to destroy them was avoided because of this
keep-alive object?

[Charlie Reis, 2014/02/20 19:01:53]

Those methods don't do anything on their own, so yes.  I would recommend adding
a test for this, since this is very subtle behavior.

[sadrul, 2014/02/21 03:51:16]

I have exposed RenderFrameHostManager::ShutdownRenderFrameHostsInSiteInstance()
as a public static function, and called it from here if
SiteInstanceImpl::active_view_count() becomes zero after dcrementing it here. I
believe this makes sure the SiteInstanceImpl is destroyed if it needs to be.

Now, I am also depending on this to clean up the RenderProcessHosts as needed. I
added some code to a browser-test
(WebContentsViewAuraTest.ScreenshotForSwappedOutRenderViews) that depends on
this, and it does seem to work OK. But is that always expected to cleanup
RenderProcessHosts like this? If not, should we do something more explicit (like
call into RenderProcessHostImpl::OnShutdownRequest(), or something less ugly)?

+  }
+
+ private:
+  scoped_refptr<SiteInstanceImpl> instance_;
+
+  DISALLOW_COPY_AND_ASSIGN(SiteInstanceKeepAlive);
+};
+
 NavigationEntryScreenshotManager::NavigationEntryScreenshotManager(
     NavigationControllerImpl* owner)
     : owner_(owner),
@@ -134,11 +157,16 @@ void NavigationEntryScreenshotManager::TakeScreenshotImpl(
     NavigationEntryImpl* entry) {
   DCHECK(host && host->GetView());
   DCHECK(entry);
+  scoped_refptr<SiteInstanceImpl> site_instance(
+      static_cast<SiteInstanceImpl*>(host->GetSiteInstance()));
+  scoped_ptr<SiteInstanceKeepAlive> keep_alive(
+      new SiteInstanceKeepAlive(site_instance));
   host->CopyFromBackingStore(gfx::Rect(),
       host->GetView()->GetViewBounds().size(),
       base::Bind(&NavigationEntryScreenshotManager::OnScreenshotTaken,

[Charlie Reis, 2014/02/20 19:01:53]

What happens if we never make it to OnScreenshotTaken?  Is that possible?  Will
the process ever go away, or is it stuck as a zombie in that case?

[sadrul, 2014/02/21 03:51:16]

CopyFromBackingStore callback is guaranteed to be called back. The possible
exception is during browser shutdown. So there's no risk of having zombies.

                  screenshot_factory_.GetWeakPtr(),
-                 entry->GetUniqueID()));
+                 entry->GetUniqueID(),
+                 base::Passed(&keep_alive)));

[Charlie Reis, 2014/02/20 19:01:53]

Albert, can I get your eyes on this?

[awong, 2014/02/20 22:44:31]

Seems legit, but using a raw pointer (as in the later patch set) also seems
legit.

[sadrul, 2014/02/21 03:51:16]

I have changed the way the keep-alive object is sent to the callback.

 }
 
 void NavigationEntryScreenshotManager::SetMinScreenshotIntervalMS(
@@ -147,9 +175,11 @@ void NavigationEntryScreenshotManager::SetMinScreenshotIntervalMS(
   min_screenshot_interval_ms_ = interval_ms;
 }
 
-void NavigationEntryScreenshotManager::OnScreenshotTaken(int unique_id,
-                                                     bool success,
-                                                     const SkBitmap& bitmap) {
+void NavigationEntryScreenshotManager::OnScreenshotTaken(
+    int unique_id,
+    scoped_ptr<SiteInstanceKeepAlive> keep_alive,
+    bool success,
+    const SkBitmap& bitmap) {
   NavigationEntryImpl* entry = NULL;
   int entry_count = owner_->GetEntryCount();
   for (int i = 0; i < entry_count; ++i) {
@@ -180,6 +210,14 @@ void NavigationEntryScreenshotManager::OnScreenshotTaken(int unique_id,
                  screenshot));
 }
 
+void NavigationEntryScreenshotManager::OnScreenshotTakenInTest(
+    int unique_id,
+    bool success,
+    const SkBitmap& bitmap) {
+  OnScreenshotTaken(unique_id, scoped_ptr<SiteInstanceKeepAlive>(), success,
+                    bitmap);
+}
+
 int NavigationEntryScreenshotManager::GetScreenshotCount() const {
   int screenshot_count = 0;
   int entry_count = owner_->GetEntryCount();