BindingManager: fix an edge-case of the background period binding.

Since http://crrev.com/237333, BindingManager is responsible for keeping an
additional, strong binding on the foreground renderer while the embedder
application remains in background.

To implement that, we reused the existing |mLastOomBoundPid| field that keeps
track of the renderer that was most recently bound with an oom binding. When
the app goes to background, we put the additional binding on whatever is
indicated by |mLastOomBoundPid|.

But, |mLastOomBoundPid| is set not only when a strong binding is added for a
connection, but also when a new connection (holding the initial binding) is
created. That means that it will point to a background rendererer right after a
tab is opened and loaded in background.

Therefore it is currently possible to have BindingManager attach the background
period binding on a wrong renderer.

This patch fixes that by not setting the |mLastOomBoundPid| when a new
connection is started. We can do that, because:
- on the high-end we use the field only for the background period binding
- on the low-end a new renderer is always started in foreground, so the values
  stored in the field will not change

Testing for the BindingManager is also modified to cover this case.

BUG=317963

Review URL: https://codereview.chromium.org/108203004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@239848 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 45 insertions, 22 deletions

diff --git a/content/public/android/java/src/org/chromium/content/browser/BindingManager.java b/content/public/android/java/src/org/chromium/content/browser/BindingManager.java
index 699cb44..003995b 100644
--- a/content/public/android/java/src/org/chromium/content/browser/BindingManager.java
+++ b/content/public/android/java/src/org/chromium/content/browser/BindingManager.java
@@ -99,16 +99,16 @@ class BindingManager {
         return managedConnection.getConnection();
     }
 
-    // Pid of the renderer that was most recently oom bound. This is used on low-memory devices
-    // to drop oom bindings of a process when another one acquires them, making sure that only
-    // one renderer process at a time is oom bound.
+    // Pid of the renderer that was most recently bound using bindAsHighPriority(). This is used on
+    // low-memory devices to drop oom bindings of a process when another one acquires them, making
+    // sure that only one renderer process at a time is oom bound.
     private int mLastOomPid = -1;
 
     // Pid of the renderer that is bound with a strong binding for the background period. Equals
     // -1 when there is no such renderer.
     private int mBoundForBackgroundPeriodPid = -1;
 
-    // Synchronizes operations that modify mLastOomPid: addNewConnection() and bindAsHighPriority().
+    // Synchronizes operations that access mLastOomPid: addNewConnection() and bindAsHighPriority().
     private final Object mLastOomPidLock = new Object();
 
     /**
@@ -149,13 +149,11 @@ class BindingManager {
                 ChildProcessConnection lastOomConnection = getConnectionForPid(mLastOomPid);
                 if (lastOomConnection != null) lastOomConnection.dropOomBindings();
             }
-            // This will reset the previous entry for the pid in the unlikely event of the OS
-            // reusing renderer pids.
-            synchronized (mManagedConnections) {
-                mManagedConnections.put(pid, new ManagedConnection(connection));
-            }
-            // Every new connection is bound with initial oom binding.
-            mLastOomPid = pid;
+        }
+        // This will reset the previous entry for the pid in the unlikely event of the OS
+        // reusing renderer pids.
+        synchronized (mManagedConnections) {
+            mManagedConnections.put(pid, new ManagedConnection(connection));
         }
     }
 
@@ -230,11 +228,13 @@ class BindingManager {
      */
     void onSentToBackground() {
         assert mBoundForBackgroundPeriodPid == -1;
-        // mLastOomPid can be -1 at this point as the embedding application could be used in
-        // foreground without spawning any renderers.
-        if (mLastOomPid >= 0) {
-            bindAsHighPriority(mLastOomPid);
-            mBoundForBackgroundPeriodPid = mLastOomPid;
+        synchronized (mLastOomPidLock) {
+            // mLastOomPid can be -1 at this point as the embedding application could be used in
+            // foreground without spawning any renderers.
+            if (mLastOomPid >= 0) {
+                bindAsHighPriority(mLastOomPid);
+                mBoundForBackgroundPeriodPid = mLastOomPid;
+            }
         }
     }
 
diff --git a/content/public/android/javatests/src/org/chromium/content/browser/BindingManagerTest.java b/content/public/android/javatests/src/org/chromium/content/browser/BindingManagerTest.java
index c53ed13..8cdf321 100644
--- a/content/public/android/javatests/src/org/chromium/content/browser/BindingManagerTest.java
+++ b/content/public/android/javatests/src/org/chromium/content/browser/BindingManagerTest.java
@@ -299,8 +299,14 @@ public class BindingManagerTest extends InstrumentationTestCase {
     }
 
     /**
-     * Verifies that onSentToBackground() / onBroughtToForeground() correctly attach and remove a
-     * strong binding that keeps the most recently used renderer bound for the background period.
+     * Verifies that onSentToBackground() / onBroughtToForeground() correctly attach and remove
+     * additional strong binding keept on the most recently bound renderer for the background
+     * period.
+     *
+     * The renderer that will be bound for the background period should be the one that was most
+     * recendly bound using .bindAsHighPriority(), even if there is one that was added using
+     * .addNewConnection() after that. Otherwise we would bound a background renderer when user
+     * loads a new tab in background and leaves the browser.
      */
     @SmallTest
     @Feature({"ProcessManagement"})
@@ -310,25 +316,42 @@ public class BindingManagerTest extends InstrumentationTestCase {
             BindingManager manager = managerEntry.mManager;
             String message = managerEntry.getErrorMessage();
 
-            // Add two connections without strong bindings.
+            // Add two connections, bind and release each.
             MockChildProcessConnection firstConnection = new MockChildProcessConnection(1);
             manager.addNewConnection(firstConnection.getPid(), firstConnection);
-            MockChildProcessConnection secondConnection = new MockChildProcessConnection(1);
+            manager.bindAsHighPriority(firstConnection.getPid());
+            manager.unbindAsHighPriority(firstConnection.getPid());
+
+            MockChildProcessConnection secondConnection = new MockChildProcessConnection(2);
             manager.addNewConnection(secondConnection.getPid(), secondConnection);
+            manager.bindAsHighPriority(secondConnection.getPid());
+            manager.unbindAsHighPriority(secondConnection.getPid());
+
+            // Add third connection, do not bind it.
+            MockChildProcessConnection thirdConnection = new MockChildProcessConnection(3);
+            manager.addNewConnection(thirdConnection.getPid(), thirdConnection);
+
+            // Sanity check: verify that no connection has a strong binding.
+            getInstrumentation().waitForIdleSync();
             assertFalse(message, firstConnection.isStrongBindingBound());
             assertFalse(message, secondConnection.isStrongBindingBound());
+            assertFalse(message, thirdConnection.isStrongBindingBound());
 
-            // Call onSentToBackground() and verify that a strong binding was added for the most
-            // recent connection (but not for the other one).
+            // Call onSentToBackground() and verify that a strong binding was added for the second
+            // connection:
+            // - not the first one, because it was bound earlier than the second
+            // - not the thirs one, because it was never bound at all
             manager.onSentToBackground();
             assertFalse(message, firstConnection.isStrongBindingBound());
             assertTrue(message, secondConnection.isStrongBindingBound());
+            assertFalse(message, thirdConnection.isStrongBindingBound());
 
             // Call onBroughtToForeground() and verify that the strong binding was removed.
             manager.onBroughtToForeground();
             getInstrumentation().waitForIdleSync();
             assertFalse(message, firstConnection.isStrongBindingBound());
             assertFalse(message, secondConnection.isStrongBindingBound());
+            assertFalse(message, thirdConnection.isStrongBindingBound());
         }
     }
 }