[wip] Introduce SiteInstanceKeepAlive.

content/browser/frame_host/navigation_entry_screenshot_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 "content/browser/frame_host/navigation_entry_screenshot_manager.h"
 
 #include "base/command_line.h"
 #include "base/threading/worker_pool.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #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"
 #include "third_party/skia/include/effects/SkLumaColorFilter.h"
 #include "ui/gfx/codec/png_codec.h"
 
 namespace {
 
 // Minimum delay between taking screenshots.
 const int kMinScreenshotIntervalMS = 1000;
@@ skipping 44 matching lines @@
     // Encode the a8Bitmap to grayscale PNG treating alpha as color intensity
     if (gfx::PNGCodec::EncodeA8SkBitmap(a8Bitmap, &data))
       data_ = new base::RefCountedBytes(data);
   }
 
   scoped_refptr<base::RefCountedBytes> data_;
 
   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),
       screenshot_factory_(this),
       min_screenshot_interval_ms_(kMinScreenshotIntervalMS) {
 }
 
 NavigationEntryScreenshotManager::~NavigationEntryScreenshotManager() {
 }
 
@@ skipping 39 matching lines @@
         owner_->GetEntryAtIndex(i)));
   }
   DCHECK_EQ(GetScreenshotCount(), 0);
 }
 
 void NavigationEntryScreenshotManager::TakeScreenshotImpl(
     RenderViewHost* host,
     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(
     int interval_ms) {
   DCHECK_GE(interval_ms, 0);
   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) {
     NavigationEntry* iter = owner_->GetEntryAtIndex(i);
     if (iter->GetUniqueID() == unique_id) {
       entry = NavigationEntryImpl::FromNavigationEntry(iter);
       break;
     }
   }
 
   if (!entry) {
     LOG(ERROR) << "Invalid entry with unique id: " << unique_id;
     return;
   }
 
   if (!success || bitmap.empty() || bitmap.isNull()) {
     if (!ClearScreenshot(entry))
       OnScreenshotSet(entry);

[mfomitchev, 2014/02/20 19:15:48]

We should get rid of this and just have ClearScreenshot(entry) to fix the unit
test

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

Done.

     return;
   }
 
   scoped_refptr<ScreenshotData> screenshot = new ScreenshotData();
   screenshot->EncodeScreenshot(
       bitmap,
       base::Bind(&NavigationEntryScreenshotManager::OnScreenshotEncodeComplete,
                  screenshot_factory_.GetWeakPtr(),
                  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();
   for (int i = 0; i < entry_count; ++i) {
     NavigationEntryImpl* entry =
         NavigationEntryImpl::FromNavigationEntry(owner_->GetEntryAtIndex(i));
     if (entry->screenshot().get())
       screenshot_count++;
   }
   return screenshot_count;
@@ skipping 12 matching lines @@
     }
   }
   if (!entry)
     return;
   entry->SetScreenshotPNGData(screenshot->data());
   OnScreenshotSet(entry);
 }
 
 void NavigationEntryScreenshotManager::OnScreenshotSet(
     NavigationEntryImpl* entry) {
   if (entry->screenshot().get())

[mfomitchev, 2014/02/20 19:15:48]

I think this check is unnecessary if we make the fix in OnScreenshotTaken() - if
OnScreenshotSet() is called, the screenshot should be non-null

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

Done.

     PurgeScreenshotsIfNecessary();
 }
 
 bool NavigationEntryScreenshotManager::ClearScreenshot(
     NavigationEntryImpl* entry) {
   if (!entry->screenshot().get())
     return false;
 
   entry->SetScreenshotPNGData(NULL);
   return true;
@@ skipping 58 matching lines @@
         owner_->GetEntryAtIndex(forward));
     if (ClearScreenshot(entry))
       --screenshot_count;
     ++forward;
   }
   CHECK_GE(screenshot_count, 0);
   CHECK_LE(screenshot_count, kMaxScreenshots);
 }
 
 }  // namespace content