Add KeepaliveImpulse to extension process manager.

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 a1ee794ace65ef55a82aa923bf09acabab0e1e1a..844257df7d2e0a3c3309744c9ab5d07aa24b863c 100644
--- a/extensions/browser/process_manager.cc
+++ b/extensions/browser/process_manager.cc
@@ -133,6 +133,10 @@ 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;
@@ -147,7 +151,11 @@ struct ProcessManager::BackgroundPageData {
   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) {}
 };
 
 //
@@ -197,22 +205,25 @@ ProcessManager::ProcessManager(BrowserContext* 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() {
@@ -360,20 +371,23 @@ int ProcessManager::IncrementLazyKeepaliveCount(const Extension* extension) {
 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_);
   }
 
@@ -394,6 +408,51 @@ void ProcessManager::IncrementLazyKeepaliveCountForView(
   }
 }
 
+// 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);