Add KeepaliveImpulse to extension process manager.

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 115 matching lines @@
 
   ProcessManager* process_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(RenderViewHostDestructionObserver);
 };
 
 struct ProcessManager::BackgroundPageData {
   // The count of things keeping the lazy background page alive.
   int lazy_keepalive_count;
 
+  // Tracks if an impulse event has occured since the last polling check.
+  bool keepalive_impulse;
+  bool previous_keepalive_impulse;
+
   // This is used with the ShouldSuspend message, to ensure that the extension
   // remained idle between sending the message and receiving the ack.
   int close_sequence_id;
 
   // True if the page responded to the ShouldSuspend message and is currently
   // dispatching the suspend event. During this time any events that arrive will
   // cancel the suspend process and an onSuspendCanceled event will be
   // dispatched to the page.
   bool is_closing;
 
   // Keeps track of when this page was last suspended. Used for perf metrics.
   linked_ptr<base::ElapsedTimer> since_suspended;
 
   BackgroundPageData()
-      : lazy_keepalive_count(0), close_sequence_id(0), is_closing(false) {}
+      : lazy_keepalive_count(0),
+        keepalive_impulse(false),
+        previous_keepalive_impulse(false),
+        close_sequence_id(0),
+        is_closing(false) {}
 };
 
 //
 // ProcessManager
 //
 
 // static
 ProcessManager* ProcessManager::Create(BrowserContext* context) {
   if (context->IsOffTheRecord()) {
     BrowserContext* original_context =
@@ skipping 29 matching lines @@
   registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
                  content::Source<BrowserContext>(original_context));
   registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
                  content::Source<BrowserContext>(context));
   if (context->IsOffTheRecord()) {
     registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
                    content::Source<BrowserContext>(original_context));
   }
 
   event_page_idle_time_ = base::TimeDelta::FromSeconds(10);
-  unsigned idle_time_sec = 0;
+  unsigned idle_time_msec = 0;
   if (base::StringToUint(CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-          extensions::switches::kEventPageIdleTime), &idle_time_sec)) {
-    event_page_idle_time_ = base::TimeDelta::FromSeconds(idle_time_sec);
+          extensions::switches::kEventPageIdleTime), &idle_time_msec)) {
+    CHECK(idle_time_msec > 0);  // OnKeepaliveImpulseCheck requires non zero.

[tapted, 2013/11/28 03:03:37]

optional nit: maybe CHECK_NE(0u, idle_time_msec)? (can't really argue that
outputting the value is handy though, since it would always be zero).

[scheib, 2013/12/02 21:22:38]

I think this check output is pretty clear as is with
":FATAL:process_manager.cc(212)] Check failed: idle_time_msec > 0"

+    event_page_idle_time_ = base::TimeDelta::FromMilliseconds(idle_time_msec);
   }
   event_page_suspending_time_ = base::TimeDelta::FromSeconds(5);
-  unsigned suspending_time_sec = 0;
+  unsigned suspending_time_msec = 0;
   if (base::StringToUint(CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
                              extensions::switches::kEventPageSuspendingTime),
-                         &suspending_time_sec)) {
+                         &suspending_time_msec)) {
     event_page_suspending_time_ =
-        base::TimeDelta::FromSeconds(suspending_time_sec);
+        base::TimeDelta::FromMilliseconds(suspending_time_msec);
   }
 
   content::DevToolsManager::GetInstance()->AddAgentStateCallback(
       devtools_callback_);
+
+  OnKeepaliveImpulseCheck();
 }
 
 ProcessManager::~ProcessManager() {
   CloseBackgroundHosts();
   DCHECK(background_hosts_.empty());
   content::DevToolsManager::GetInstance()->RemoveAgentStateCallback(
       devtools_callback_);
 }
 
 const ProcessManager::ViewSet ProcessManager::GetAllViews() const {
@@ skipping 127 matching lines @@
   int& count = background_page_data_[extension->id()].lazy_keepalive_count;
   if (++count == 1)
     OnLazyBackgroundPageActive(extension->id());
 
   return count;
 }
 
 int ProcessManager::DecrementLazyKeepaliveCount(const Extension* extension) {
   if (!BackgroundInfo::HasLazyBackgroundPage(extension))
     return 0;
+  return DecrementLazyKeepaliveCount(extension->id());
+}
 
-  int& count = background_page_data_[extension->id()].lazy_keepalive_count;
+int ProcessManager::DecrementLazyKeepaliveCount(std::string extension_id) {
+  int& count = background_page_data_[extension_id].lazy_keepalive_count;
   DCHECK_GT(count, 0);
 
   // If we reach a zero keepalive count when the lazy background page is about
   // to be closed, incrementing close_sequence_id will cancel the close
   // sequence and cause the background page to linger. So check is_closing
   // before initiating another close sequence.
-  if (--count == 0 && !background_page_data_[extension->id()].is_closing) {
+  if (--count == 0 && !background_page_data_[extension_id].is_closing) {
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&ProcessManager::OnLazyBackgroundPageIdle,
-                   weak_ptr_factory_.GetWeakPtr(), extension->id(),
-                   ++background_page_data_[extension->id()].close_sequence_id),
+                   weak_ptr_factory_.GetWeakPtr(), extension_id,
+                   ++background_page_data_[extension_id].close_sequence_id),
         event_page_idle_time_);
   }
 
   return count;
 }
 
 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) {
+    bd.keepalive_impulse = true;
+    if (!bd.previous_keepalive_impulse) {
+      IncrementLazyKeepaliveCount(extension);
+    }
+  }
+}
+
+// DecrementLazyKeepaliveCount is called when no calls to KeepaliveImpulse
+// have been made for at least event_page_idle_time_. In the best case an
+// impulse was made just before being cleared, and the decrement will occur
+// event_page_idle_time_ later, causing a 2 * event_page_idle_time_ total time
+// for extension to be shut down based on impulses. Worst case is an impulse
+// just after a clear, adding one check cycle and resulting in 3x total time.
+void ProcessManager::OnKeepaliveImpulseCheck() {
+  for (BackgroundPageDataMap::iterator i = background_page_data_.begin();
+       i != background_page_data_.end();
+       ++i) {
+    bool this_impulse = i->second.keepalive_impulse;

[tapted, 2013/11/28 03:03:37]

(optional) This might be easier to read without the temporaries (I don't think
the order matters). i.e.

if (i->second.previous_keepalive_impulse && !i->second.keepalive_impulse)
  DecrementLazyKeepaliveCount(i->first);

i->second.previous_keepalive_impulse = i->second.keepalive_impulse;
i->second.keepalive_impulse = false;


.. it still feels tricky though. I think I understood it better after realising
the sequence doesn't really matter. e.g. You could have

bool BackgroundPageData::HasKeepalive() const {
  return keepalive_impulse || previous_keepalive_impulse;
}

Then here you could go

for (..) {
  if (!i->second.HasKeepalive())
    continue;

  i->second.previous_keepalive_impulse = i->second.keepalive_impulse;
  i->second.keepalive_impulse = false;
  if (!i->second.HasKeepalive())
    DecrementLazyKeepaliveCount(i->first);
}

and in KeepaliveImpulse() do

if (!bd.HasKeepalive())
  IncrementLazyKeepaliveCount(extension);
bd.keepalive_impulse = true;


Maybe HasKeepalive -> IsImpulseIdle to get rid of the inversions.

[scheib, 2013/12/02 21:22:38]

Done; your first suggestion. The latter didn't reuse much code and I think the
balance is too far into indirection.

+    bool previous_impulse = i->second.previous_keepalive_impulse;
+    i->second.keepalive_impulse = false;
+    i->second.previous_keepalive_impulse = this_impulse;
+
+    if (previous_impulse && !this_impulse)
+      DecrementLazyKeepaliveCount(i->first);

[tapted, 2013/11/28 03:03:37]

I thought it weird that the return value wasn't checked... but it looks like
nothing uses the return value - maybe the functions should just return void?

[scheib, 2013/12/02 21:22:38]

Done.

+  }
+
+  if (base::MessageLoop::current()) {

[tapted, 2013/11/28 03:03:37]

Maybe a comment here to explain this check (e.g. Should
DecrementLazyKeepaliveCount() be checking too?)

[scheib, 2013/12/02 21:22:38]

Done.

+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&ProcessManager::OnKeepaliveImpulseCheck,
+                   weak_ptr_factory_.GetWeakPtr()),
+        event_page_idle_time_);
+  }
+}
+
 void ProcessManager::OnLazyBackgroundPageIdle(const std::string& extension_id,
                                               int sequence_id) {
   ExtensionHost* host = GetBackgroundHostForExtension(extension_id);
   if (host && !background_page_data_[extension_id].is_closing &&
       sequence_id == background_page_data_[extension_id].close_sequence_id) {
     // Tell the renderer we are about to close. This is a simple ping that the
     // renderer will respond to. The purpose is to control sequencing: if the
     // extension remains idle until the renderer responds with an ACK, then we
     // know that the extension process is ready to shut down. If our
     // close_sequence_id has already changed, then we would ignore the
@@ skipping 399 matching lines @@
 }
 
 bool IncognitoProcessManager::IsIncognitoEnabled(const Extension* extension) {
   // Keep in sync with duplicate in extension_info_map.cc.
   ExtensionService* service = ExtensionSystem::GetForBrowserContext(
       GetBrowserContext())->extension_service();
   return extension_util::IsIncognitoEnabled(extension->id(), service);
 }
 
 }  // namespace extensions