Execute operations of ChildProcessLauncher on launcher thread

content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java

Index: content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java b/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
index 2eaf20eb1a23a47564349189b4176f946254269f..aa4770d06516a41a52282b263404f3799d40405b 100644
--- a/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
+++ b/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
@@ -35,47 +35,68 @@ class BindingManagerImpl implements BindingManager {
     private static final long MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS = 10 * 1000;
     private static final long MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS_ON_TESTING = 100;
 
+    // ManagedConnection is accessed even after it is cleared because whether it was OOM protected
+    // or not is queried later. So we need to keep it in memory for a while.
+    private static final long MANAGED_CONNECTION_DELETE_DELAY_MILLS = 3 * 1000;
+
     // These fields allow to override the parameters for testing - see
     // createBindingManagerForTesting().
     private final long mRemoveStrongBindingDelay;
     private final boolean mIsLowMemoryDevice;
 
+    interface LauncherThreadExecutor {

[Yaron, 2015/09/04 16:04:04]

This is basically a static but you're wrapping to avoid threading in tests
(presumably?). If we make the threading model actually work on tests, then I'd
prefer to see this be a static and avoid passing it around everywhere.

[Jaekyun Seok (inactive), 2015/09/08 09:45:12]

This is only for BindingManagerImplTest which is actually unittests for
BindingManagerImp; we don't need to test the threading model there.
So I want to keep this logic.

+        void execute(Runnable runnable);
+    }
+
     private static class ModerateBindingPool
             extends LruCache<Integer, ManagedConnection> implements ComponentCallbacks2 {
         private final float mLowReduceRatio;
         private final float mHighReduceRatio;
+        private final LauncherThreadExecutor mLauncherThreadExecutor;
         private final Object mDelayedClearerLock = new Object();
         private Runnable mDelayedClearer;
         private final Handler mHandler = new Handler(ThreadUtils.getUiThreadLooper());
 
-        public ModerateBindingPool(int maxSize, float lowReduceRatio, float highReduceRatio) {
+        public ModerateBindingPool(int maxSize, float lowReduceRatio, float highReduceRatio,
+                LauncherThreadExecutor executor) {
             super(maxSize);
 
             mLowReduceRatio = lowReduceRatio;
             mHighReduceRatio = highReduceRatio;
+            mLauncherThreadExecutor = executor;
         }
 
         @Override
-        public void onTrimMemory(int level) {
-            Log.i(TAG, "onTrimMemory: level=%d, size=%d", level, size());
-            if (size() > 0) {
-                if (level <= TRIM_MEMORY_RUNNING_MODERATE) {
-                    reduce(mLowReduceRatio);
-                } else if (level <= TRIM_MEMORY_RUNNING_LOW) {
-                    reduce(mHighReduceRatio);
-                } else if (level == TRIM_MEMORY_UI_HIDDEN) {
-                    // This will be handled by |mDelayedClearer|.
-                    return;
-                } else {
-                    evictAll();
+        public void onTrimMemory(final int level) {
+            mLauncherThreadExecutor.execute(new Runnable() {
+                @Override

[Yaron, 2015/09/04 16:04:04]

Here you're posting to launcher thread within the function but addConnection
you've done it in the caller. Can you do this more consistently so it's easier
to reason about?

[Jaekyun Seok (inactive), 2015/09/08 09:45:12]

I separated ComponentCallbacks2 from ModerateBindingPool for clear
understanding.

[Yaron, 2015/09/09 01:00:20]

Nice, this helps

+                public void run() {
+                    Log.i(TAG, "onTrimMemory: level=%d, size=%d", level, size());
+                    if (size() > 0) {
+                        if (level <= TRIM_MEMORY_RUNNING_MODERATE) {
+                            reduce(mLowReduceRatio);
+                        } else if (level <= TRIM_MEMORY_RUNNING_LOW) {
+                            reduce(mHighReduceRatio);
+                        } else if (level == TRIM_MEMORY_UI_HIDDEN) {
+                            // This will be handled by |mDelayedClearer|.
+                            return;
+                        } else {
+                            evictAll();
+                        }
+                    }
                 }
-            }
+            });
         }
 
         @Override
         public void onLowMemory() {
-            Log.i(TAG, "onLowMemory: evict %d bindings", size());
-            evictAll();
+            mLauncherThreadExecutor.execute(new Runnable() {
+                @Override
+                public void run() {
+                    Log.i(TAG, "onLowMemory: evict %d bindings", size());
+                    evictAll();
+                }
+            });
         }
 
         @Override
@@ -139,12 +160,17 @@ class BindingManagerImpl implements BindingManager {
                             if (mDelayedClearer == null) return;
                             mDelayedClearer = null;
                         }
-                        Log.i(TAG, "Release moderate connections: %d", size());
-                        if (!onTesting) {
-                            RecordHistogram.recordCountHistogram(
-                                    "Android.ModerateBindingCount", size());
-                        }
-                        evictAll();
+                        mLauncherThreadExecutor.execute(new Runnable() {
+                            @Override
+                            public void run() {
+                                Log.i(TAG, "Release moderate connections: %d", size());
+                                if (!onTesting) {
+                                    RecordHistogram.recordCountHistogram(
+                                            "Android.ModerateBindingCount", size());
+                                }
+                                evictAll();
+                            }
+                        });
                     }
                 };
                 mHandler.postDelayed(mDelayedClearer, onTesting

[Yaron, 2015/09/04 16:04:04]

This function is now very peculiar. We have an mHandler for the UIThread. We
post a task for it to run after some delay. As soon as it runs, it grabs a mutex
to ensure it can run but then posts back to the launcher thread to do its work.
I know there was confusion abou t loopers/handlers so it might not be possible
to post a delayed task for the current thread. (Maybe it's worth creating a
native wrapper for Runnable so you can post to the native message loop). 

Regardless of that, I'm not sure the mutex is doing the right thing anymore.
mDelayedClearer is now already cleared when the task is just posted to the
process launcher thread. Is evictAll guaranteed to do the right thing?

[Jaekyun Seok (inactive), 2015/09/08 09:45:11]

I will add a native wrapper for posting, but in this case I want to use java
Handler because the task can be canceled.

@@ -214,7 +240,7 @@ class BindingManagerImpl implements BindingManager {
 
             // This runnable performs the actual unbinding. It will be executed synchronously when
             // on low-end devices and posted with a delay otherwise.
-            Runnable doUnbind = new Runnable() {
+            final Runnable doUnbind = new Runnable() {
                 @Override
                 public void run() {
                     if (connection.isStrongBindingBound()) {
@@ -234,7 +260,12 @@ class BindingManagerImpl implements BindingManager {
             if (mIsLowMemoryDevice) {
                 doUnbind.run();
             } else {
-                ThreadUtils.postOnUiThreadDelayed(doUnbind, mRemoveStrongBindingDelay);
+                ThreadUtils.postOnUiThreadDelayed(new Runnable() {

[Yaron, 2015/09/04 16:04:03]

General question: it seems like in many places we are now posting to the ui
thread and then immediately going to process_launcher. Why bother going through
the UI thread anymore?

[Jaekyun Seok (inactive), 2015/09/08 09:45:11]

Added a native wrapper to post a task on launcher thread.

+                    @Override
+                    public void run() {
+                        mLauncherThreadExecutor.execute(doUnbind);
+                    }
+                }, mRemoveStrongBindingDelay);
             }
         }
 
@@ -347,20 +378,24 @@ class BindingManagerImpl implements BindingManager {
     // Whether this instance is used on testing.
     private final boolean mOnTesting;
 
+    // Executor that executes a task on LauncherThread.
+    private final LauncherThreadExecutor mLauncherThreadExecutor;
+
     /**
      * The constructor is private to hide parameters exposed for testing from the regular consumer.
      * Use factory methods to create an instance.
      */
-    private BindingManagerImpl(
-            boolean isLowMemoryDevice, long removeStrongBindingDelay, boolean onTesting) {
+    private BindingManagerImpl(boolean isLowMemoryDevice, long removeStrongBindingDelay,
+            boolean onTesting, LauncherThreadExecutor executor) {
         mIsLowMemoryDevice = isLowMemoryDevice;
         mRemoveStrongBindingDelay = removeStrongBindingDelay;
         mOnTesting = onTesting;
+        mLauncherThreadExecutor = executor;
     }
 
-    public static BindingManagerImpl createBindingManager() {
+    public static BindingManagerImpl createBindingManager(LauncherThreadExecutor executor) {
         return new BindingManagerImpl(
-                SysUtils.isLowEndDevice(), DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS, false);
+                SysUtils.isLowEndDevice(), DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS, false, executor);
     }
 
     /**
@@ -369,7 +404,13 @@ class BindingManagerImpl implements BindingManager {
      * @param isLowEndDevice true iff the created instance should apply low-end binding policies
      */
     public static BindingManagerImpl createBindingManagerForTesting(boolean isLowEndDevice) {
-        return new BindingManagerImpl(isLowEndDevice, 0, true);
+        LauncherThreadExecutor executor = new LauncherThreadExecutor() {
+            @Override
+            public void execute(Runnable runnable) {
+                runnable.run();
+            }
+        };
+        return new BindingManagerImpl(isLowEndDevice, 0, true, executor);
     }
 
     @Override
@@ -464,19 +505,36 @@ class BindingManagerImpl implements BindingManager {
     }
 
     @Override
-    public void clearConnection(int pid) {
+    public void clearConnection(final int pid) {
         ManagedConnection managedConnection;
         synchronized (mManagedConnections) {
             managedConnection = mManagedConnections.get(pid);
         }
-        if (managedConnection != null) managedConnection.clearConnection();
+        if (managedConnection != null) {
+            managedConnection.clearConnection();
+            final Runnable cleaner = new Runnable() {
+                @Override
+                public void run() {
+                    synchronized (mManagedConnections) {
+                        mManagedConnections.delete(pid);
+                    }
+                }
+            };
+            ThreadUtils.postOnUiThreadDelayed(new Runnable() {
+                @Override
+                public void run() {
+                    mLauncherThreadExecutor.execute(cleaner);
+                }
+            }, MANAGED_CONNECTION_DELETE_DELAY_MILLS);

[Yaron, 2015/09/04 16:04:04]

This was intentionally leaky (specifically for the case of mWasOomProtected).
See the class-level comments for ManagedConnection. Maybe that's not the best
strategy, but how did you choose 3 seconds?

[Jaekyun Seok (inactive), 2015/09/08 09:45:12]

I picked 3s because I thought that it was big enough to keep the closed
connection for the last access.
Is there any case to access the closed connection other than checking
mWasOomProtected to show a sad tab?

[Yaron, 2015/09/09 01:00:20]

I think that's the only reason. Still, I think we have enough going on in this
CL that we can do this separately

+        }
     }
 
     /** @return true iff the connection reference is no longer held */
     @VisibleForTesting
     public boolean isConnectionCleared(int pid) {
         synchronized (mManagedConnections) {
-            return mManagedConnections.get(pid).isConnectionCleared();
+            ManagedConnection managedConnection = mManagedConnections.get(pid);
+            return managedConnection == null || managedConnection.isConnectionCleared();
         }
     }
 
@@ -488,8 +546,8 @@ class BindingManagerImpl implements BindingManager {
 
             Log.i(TAG, "Moderate binding enabled: maxSize=%d lowReduceRatio=%f highReduceRatio=%f",
                     maxSize, lowReduceRatio, highReduceRatio);
-            mModerateBindingPool =
-                    new ModerateBindingPool(maxSize, lowReduceRatio, highReduceRatio);
+            mModerateBindingPool = new ModerateBindingPool(
+                    maxSize, lowReduceRatio, highReduceRatio, mLauncherThreadExecutor);
             if (context != null) context.registerComponentCallbacks(mModerateBindingPool);
         }
     }