Resume a backgrounded renderer that was purged and suspended

chrome/browser/memory/tab_manager.cc

Index: chrome/browser/memory/tab_manager.cc
diff --git a/chrome/browser/memory/tab_manager.cc b/chrome/browser/memory/tab_manager.cc
index e761e81a7c43a1d31ef1ebd15c33c984e6faa98b..76adc41e147cd427faac4ff4227e22cd3c603292 100644
--- a/chrome/browser/memory/tab_manager.cc
+++ b/chrome/browser/memory/tab_manager.cc
@@ -84,6 +84,16 @@ const int kRecentTabDiscardIntervalSeconds = 60;
 // machine was suspended and correct the timing statistics.
 const int kSuspendThresholdSeconds = kAdjustmentIntervalSeconds * 4;
 
+// If a backgrounded renderer is kept backgrounded for this time after
+// being suspended, resume the renderer to avoid breaking web.
+constexpr base::TimeDelta kSuspendResumedRendererTime =
+    base::TimeDelta::FromSeconds(10);
+
+// After resuming and running a backgrounded renderer for this period,
+// the renderer is purged and suspended again.
+constexpr base::TimeDelta kResumeSuspendedRendererTime =
+    base::TimeDelta::FromSeconds(120);

[Avi (use Gerrit), 2016/10/18 06:15:37]

These comments seem to be opposite what you say in the CL description.

The CL description says
- Renderer suspended for 120s
- Renderer resumed for 10s
repeat

whereas these comments read to me that
"if a [...] renderer is kept backgrounded for [...10s...] resume it" and
"after [...] running a backgrounded renderer for [...120s...] then suspend it
again"

which seems the opposite, and also seems less sensical. (And seems in
disagreement with their use in code below.)

Also, these names are so abbreviated and similar to be confusable, and make no
sense out of context. Can you find them new names?
kMaxTimeRendererAllowedToBeSuspendedBeforeResume and
kSuspendedRendererLengthOfResumption as examples of what come to mind.

[tasak, 2016/10/18 10:38:42]

Yeah, my comment was completely wrong. Thank you.
Done.

+
 // The time during which a tab is protected from discarding after it stops being
 // audible.
 const int kAudioProtectionTimeSeconds = 60;
@@ -700,8 +710,9 @@ void TabManager::PurgeAndSuspendBackgroundedTabs() {
   }
   if (purge_and_suspend_time <= 0)
     return;
+  base::TimeTicks current_time = NowTicks();
   auto purge_and_suspend_time_threshold =
-      NowTicks() - base::TimeDelta::FromSeconds(purge_and_suspend_time);
+      current_time - base::TimeDelta::FromSeconds(purge_and_suspend_time);
   auto tab_stats = GetUnsortedTabStats();
   for (auto& tab : tab_stats) {
     if (!tab.render_process_host->IsProcessBackgrounded())
@@ -714,7 +725,25 @@ void TabManager::PurgeAndSuspendBackgroundedTabs() {
       continue;
     if (!CanSuspendBackgroundedRenderer(tab.child_process_host_id))
       continue;
-    tab.render_process_host->PurgeAndSuspend();
+
+    auto last_purged_and_suspended_time =
+        tab.render_process_host->GetLastPurgedAndSuspendedTime();
+    auto last_resumed_time =
+        tab.render_process_host->GetLastResumedInBackgroundTime();
+
+    if (last_purged_and_suspended_time < tab.last_active ||
+        (last_resumed_time > last_purged_and_suspended_time &&
+         last_purged_and_suspended_time > tab.last_active &&
+         ((current_time - last_resumed_time) > kSuspendResumedRendererTime))) {

[Avi (use Gerrit), 2016/10/18 06:15:36]

Holy moly. I just spent twenty minutes pondering this if() and I'm still not
sure I understand it or that it's correct.

My first thought is that this is

if (A || (B && C && D))

A and C are opposites. If A is true, we short-circuit (B && C && D). If A is
false, C is true, so we shouldn't need it.

Otherwise...
ugh...

We have three times here:
1. last_purged_and_suspended_time
2. last_resumed_time
3. tab.last_active

plus a boolean of whether it's been 10s since last_resumed_time.

That's six orderings of time ×2 for the boolean = 12 possibilities. Which ones
are the ones that trigger this? I can't tell. Can you lay out the possibilities
in a comment, so that as a code reader I can convince myself that it's correct?

[tasak, 2016/10/18 10:38:42]

Yeah... the code was dirty...

So I add "purge-and-suspend state" and "last modified time" to
TabManager::WebContentsData.

+      tab.render_process_host->PurgeAndSuspend();
+      if (!task_runner_.get())
+        task_runner_ = base::ThreadTaskRunnerHandle::Get();
+    } else {
+      auto delta = current_time - last_purged_and_suspended_time;
+      if (last_resumed_time < last_purged_and_suspended_time &&
+          delta > kResumeSuspendedRendererTime)
+        tab.render_process_host->Resume();
+    }
   }
 }