Resume a backgrounded renderer that was purged and suspended

chrome/browser/memory/tab_manager.cc

 // Copyright (c) 2012 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 "chrome/browser/memory/tab_manager.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <set>
@@ skipping 66 matching lines @@
 // For each period of this length record a statistic to indicate whether or not
 // the user experienced a low memory event. If this interval is changed,
 // Tabs.Discard.DiscardInLastMinute must be replaced with a new statistic.
 const int kRecentTabDiscardIntervalSeconds = 60;
 #endif
 
 // If there has been no priority adjustment in this interval, assume the
 // 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;
 
 int FindTabStripModelById(int64_t target_web_contents_id,
                           TabStripModel** model) {
   DCHECK(model);
   for (auto* browser : *BrowserList::GetInstance()) {
     TabStripModel* local_model = browser->tab_strip_model();
     for (int idx = 0; idx < local_model->count(); idx++) {
@@ skipping 596 matching lines @@
   if (!command_line.HasSwitch(switches::kPurgeAndSuspendTime))
     return;
   int purge_and_suspend_time = 0;
   if (!base::StringToInt(
           command_line.GetSwitchValueASCII(switches::kPurgeAndSuspendTime),
           &purge_and_suspend_time)) {
     return;
   }
   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())
       continue;
     // TODO(hajimehoshi): Now calling PurgeAndSuspend is implemented without
     // timers for simplicity, so PurgeAndSuspend is called even after the
     // renderer is purged and suspended once. This should be replaced with
     // timers if we want necessary and sufficient signals.
     if (tab.last_active > purge_and_suspend_time_threshold)
       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();
+    }
   }
 }
 
 WebContents* TabManager::DiscardWebContentsAt(int index, TabStripModel* model) {
   // Can't discard active index.
   if (model->active_index() == index)
     return nullptr;
 
   WebContents* old_contents = model->GetWebContentsAt(index);
 
@@ skipping 231 matching lines @@
   // platform.
   std::string allow_multiple_discards = variations::GetVariationParamValue(
       features::kAutomaticTabDiscarding.name, "AllowMultipleDiscards");
   return (allow_multiple_discards != "true");
 #else
   return false;
 #endif
 }
 
 }  // namespace memory