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..75e8fcfeca31ac9289b3868060f79c9fbaf74540 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,54 +35,39 @@ 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;
 
-    private static class ModerateBindingPool
-            extends LruCache<Integer, ManagedConnection> implements ComponentCallbacks2 {
+    interface LauncherThreadExecutor {
+        void execute(Runnable runnable);
+        void executeDelayed(Runnable runnable, long delayInMS);
+    }
+
+    private static class ModerateBindingPool extends LruCache<Integer, ManagedConnection> {
         private final float mLowReduceRatio;
         private final float mHighReduceRatio;
-        private final Object mDelayedClearerLock = new Object();
+        private final LauncherThreadExecutor mLauncherThreadExecutor;
         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();
-                }
-            }
-        }
-
-        @Override
-        public void onLowMemory() {
-            Log.i(TAG, "onLowMemory: evict %d bindings", size());
-            evictAll();
-        }
-
-        @Override
-        public void onConfigurationChanged(Configuration configuration) {}
-
         @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1)
-        private void reduce(float reduceRatio) {
+        void reduce(boolean useHighReduceRatio) {
+            final float reduceRatio = useHighReduceRatio ? mHighReduceRatio : mLowReduceRatio;
             int oldSize = size();
             int newSize = (int) (oldSize * (1f - reduceRatio));
             Log.i(TAG, "Reduce connections from %d to %d", oldSize, newSize);
@@ -131,38 +116,37 @@ class BindingManagerImpl implements BindingManager {
 
         void onSentToBackground(final boolean onTesting) {
             if (size() == 0) return;
-            synchronized (mDelayedClearerLock) {
-                mDelayedClearer = new Runnable() {
-                    @Override
-                    public void run() {
-                        synchronized (mDelayedClearerLock) {
+            mDelayedClearer = new Runnable() {

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

I think you were following my suggestion but as-is it's still accessed on
multiple threads so the lock is needed.
What I was hoping for was getting rid of mDelayedClearer (or having all
mutations on one thread) and then you can get rid of the lock but if you need
the cancellation on UI thread that means this is racey.

[Jaekyun Seok (inactive), 2015/09/09 02:02:41]

mDelayedClearer is only accessed on launcher thread because onSentToBackground()
and onBroughtToForeground() are called on launcher thread.

+                @Override
+                public void run() {
+                    mLauncherThreadExecutor.execute(new Runnable() {
+                        @Override
+                        public void run() {
                             if (mDelayedClearer == null) return;
                             mDelayedClearer = null;
+
+                            Log.i(TAG, "Release moderate connections: %d", size());
+                            if (!onTesting) {
+                                RecordHistogram.recordCountHistogram(
+                                        "Android.ModerateBindingCount", size());
+                            }
+                            evictAll();
                         }
-                        Log.i(TAG, "Release moderate connections: %d", size());
-                        if (!onTesting) {
-                            RecordHistogram.recordCountHistogram(
-                                    "Android.ModerateBindingCount", size());
-                        }
-                        evictAll();
-                    }
-                };
-                mHandler.postDelayed(mDelayedClearer, onTesting
-                                ? MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS_ON_TESTING
-                                : MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS);
-            }
+                    });
+                }
+            };
+            mHandler.postDelayed(mDelayedClearer, onTesting
+                            ? MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS_ON_TESTING
+                            : MODERATE_BINDING_POOL_CLEARER_DELAY_MILLIS);
         }
 
         void onBroughtToForeground() {
-            synchronized (mDelayedClearerLock) {
-                if (mDelayedClearer == null) return;
-                mHandler.removeCallbacks(mDelayedClearer);
-                mDelayedClearer = null;
-            }
+            if (mDelayedClearer == null) return;
+            mHandler.removeCallbacks(mDelayedClearer);
+            mDelayedClearer = null;
         }
     }
 
-    private final Object mModerateBindingPoolLock = new Object();
     private ModerateBindingPool mModerateBindingPool;
 
     /**
@@ -194,39 +178,28 @@ class BindingManagerImpl implements BindingManager {
 
         /** Adds a strong service binding. */
         private void addStrongBinding() {
-            ChildProcessConnection connection = mConnection;
-            if (connection == null) return;
+            if (mConnection == null) return;
 
-            connection.addStrongBinding();
-            ModerateBindingPool moderateBindingPool;
-            synchronized (mModerateBindingPoolLock) {
-                moderateBindingPool = mModerateBindingPool;
-            }
-            if (moderateBindingPool != null) moderateBindingPool.removeConnection(this);
+            mConnection.addStrongBinding();
+            if (mModerateBindingPool != null) mModerateBindingPool.removeConnection(this);
         }
 
         /** Removes a strong service binding. */
         private void removeStrongBinding(final boolean keepsAsModerate) {
-            final ChildProcessConnection connection = mConnection;
             // We have to fail gracefully if the strong binding is not present, as on low-end the
             // binding could have been removed by dropOomBindings() when a new service was started.
-            if (connection == null || !connection.isStrongBindingBound()) return;
+            if (mConnection == null || !mConnection.isStrongBindingBound()) return;
 
             // 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()) {
-                        connection.removeStrongBinding();
-                        ModerateBindingPool moderateBindingPool;
-                        synchronized (mModerateBindingPoolLock) {
-                            moderateBindingPool = mModerateBindingPool;
-                        }
-                        if (moderateBindingPool != null && !connection.isStrongBindingBound()
-                                && keepsAsModerate) {
-                            moderateBindingPool.addConnection(ManagedConnection.this);
-                        }
+                    if (mConnection == null || !mConnection.isStrongBindingBound()) return;
+                    mConnection.removeStrongBinding();
+                    if (mModerateBindingPool != null && !mConnection.isStrongBindingBound()
+                            && keepsAsModerate) {
+                        mModerateBindingPool.addConnection(ManagedConnection.this);
                     }
                 }
             };
@@ -234,7 +207,12 @@ class BindingManagerImpl implements BindingManager {
             if (mIsLowMemoryDevice) {
                 doUnbind.run();

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

doesn't this affect mModerateBindingPool on a different thread now?

[Jaekyun Seok (inactive), 2015/09/09 02:02:41]

No. All accesses to mModerateBindingPool happen on launcher thread, and all
accesses to ManagedConnection except isOomProtected() happen on launcher thread
as well.

             } else {
-                ThreadUtils.postOnUiThreadDelayed(doUnbind, mRemoveStrongBindingDelay);
+                mLauncherThreadExecutor.executeDelayed(new Runnable() {
+                    @Override
+                    public void run() {
+                        doUnbind.run();
+                    }
+                }, mRemoveStrongBindingDelay);
             }
         }
 
@@ -246,10 +224,9 @@ class BindingManagerImpl implements BindingManager {
 
         /** Adds the moderate service binding. */
         private void addModerateBinding() {
-            ChildProcessConnection connection = mConnection;
-            if (connection == null) return;
+            if (mConnection == null) return;
 
-            connection.addModerateBinding();
+            mConnection.addModerateBinding();
         }
 
         /**
@@ -258,10 +235,9 @@ class BindingManagerImpl implements BindingManager {
          */
         private void dropBindings() {
             assert mIsLowMemoryDevice;
-            ChildProcessConnection connection = mConnection;
-            if (connection == null) return;
+            if (mConnection == null) return;
 
-            connection.dropOomBindings();
+            mConnection.dropOomBindings();
         }
 
         ManagedConnection(ChildProcessConnection connection) {
@@ -302,21 +278,21 @@ class BindingManagerImpl implements BindingManager {
             mBoundForBackgroundPeriod = nextBound;
         }
 
+        /** This may be called from any thread. */
         boolean isOomProtected() {
             // When a process crashes, we can be queried about its oom status before or after the
             // connection is cleared. For the latter case, the oom status is stashed in
             // mWasOomProtected.
-            return mConnection != null
-                    ? mConnection.isOomProtectedOrWasWhenDied() : mWasOomProtected;
+            ChildProcessConnection connection = mConnection;

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

Why was this changed?

[Jaekyun Seok (inactive), 2015/09/09 02:02:41]

isOomProtected() could be called from any thread, and so mConnection could be
null unexpectedly.

[Yaron, 2015/09/11 22:15:24]

Right. Hmm, so shouldn't mConnection be declared as volatile then?

[Jaekyun Seok (inactive), 2015/09/14 01:41:45]

Done.

+            if (connection == null) return mWasOomProtected;
+
+            return connection.isOomProtectedOrWasWhenDied();
         }
 
         void clearConnection() {
+            if (mConnection == null) return;
             mWasOomProtected = mConnection.isOomProtectedOrWasWhenDied();
-            ModerateBindingPool moderateBindingPool;
-            synchronized (mModerateBindingPoolLock) {
-                moderateBindingPool = mModerateBindingPool;
-            }
-            if (moderateBindingPool != null) moderateBindingPool.removeConnection(this);
+            if (mModerateBindingPool != null) mModerateBindingPool.removeConnection(this);
             mConnection = null;
         }
 
@@ -337,30 +313,47 @@ class BindingManagerImpl implements BindingManager {
     // renderer process at a time is protected from oom killing.
     private ManagedConnection mLastInForeground;
 
-    // Synchronizes operations that access mLastInForeground: setInForeground() and

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

This removal looks good

-    // addNewConnection().
-    private final Object mLastInForegroundLock = new Object();
-
     // The connection bound with additional binding in onSentToBackground().
     private ManagedConnection mBoundForBackgroundPeriod;
 
     // Whether this instance is used on testing.
     private final boolean mOnTesting;
 
+    // Executor that executes a task on LauncherThread.
+    private final LauncherThreadExecutor mLauncherThreadExecutor;
+
+    private final Object mThreadIdLock = new Object();
+    private Long mThreadId;
+
+    void checkIfCalledOnValidThread() {

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

why not re-use NonThreadSafe? Because of the testing aspect again?

[Jaekyun Seok (inactive), 2015/09/09 02:02:41]

The class can't be used here because it is under chrome/. Moreover its initial
thread id is determined in creator, which isn't proper in this case.

[Yaron, 2015/09/11 22:15:24]

Well the simple solution would be to move it out of chrome/. It's not clear to
me why it's there and not in base/. I see what you're saying though cause the
binding manager is static so it's hard to control it being created on the
process launcher thread

+        if (mOnTesting) return;
+
+        synchronized (mThreadIdLock) {
+            long currentThreadId = Thread.currentThread().getId();
+            if (mThreadId == null) {
+                mThreadId = currentThreadId;
+            } else if (mThreadId != currentThreadId) {
+                throw new IllegalStateException(
+                        "Method is not called on valid thread: " + Thread.currentThread());
+            }
+        }
+    }
+
     /**
      * 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,11 +362,24 @@ 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) {
+                ThreadUtils.runOnUiThread(runnable);
+            }
+
+            @Override
+            public void executeDelayed(Runnable runnable, long delayInMS) {
+                ThreadUtils.postOnUiThreadDelayed(runnable, delayInMS);
+            }
+        };
+        return new BindingManagerImpl(isLowEndDevice, 0, true, executor);
     }
 
     @Override
     public void addNewConnection(int pid, ChildProcessConnection connection) {
+        checkIfCalledOnValidThread();
+
         // This will reset the previous entry for the pid in the unlikely event of the OS
         // reusing renderer pids.
         synchronized (mManagedConnections) {
@@ -383,6 +389,8 @@ class BindingManagerImpl implements BindingManager {
 
     @Override
     public void setInForeground(int pid, boolean inForeground) {
+        checkIfCalledOnValidThread();
+
         ManagedConnection managedConnection;
         synchronized (mManagedConnections) {
             managedConnection = mManagedConnections.get(pid);
@@ -393,19 +401,19 @@ class BindingManagerImpl implements BindingManager {
             return;
         }
 
-        synchronized (mLastInForegroundLock) {
-            if (inForeground && mIsLowMemoryDevice && mLastInForeground != null
-                    && mLastInForeground != managedConnection) {
-                mLastInForeground.dropBindings();
-            }
-
-            managedConnection.setInForeground(inForeground);
-            if (inForeground) mLastInForeground = managedConnection;
+        if (inForeground && mIsLowMemoryDevice && mLastInForeground != null
+                && mLastInForeground != managedConnection) {
+            mLastInForeground.dropBindings();
         }
+
+        managedConnection.setInForeground(inForeground);
+        if (inForeground) mLastInForeground = managedConnection;
     }
 
     @Override
     public void determinedVisibility(int pid) {
+        checkIfCalledOnValidThread();
+
         ManagedConnection managedConnection;
         synchronized (mManagedConnections) {
             managedConnection = mManagedConnections.get(pid);
@@ -422,35 +430,32 @@ class BindingManagerImpl implements BindingManager {
 
     @Override
     public void onSentToBackground() {
+        checkIfCalledOnValidThread();
+
         assert mBoundForBackgroundPeriod == null;
-        synchronized (mLastInForegroundLock) {
-            // mLastInForeground can be null at this point as the embedding application could be
-            // used in foreground without spawning any renderers.
-            if (mLastInForeground != null) {
-                mLastInForeground.setBoundForBackgroundPeriod(true);
-                mBoundForBackgroundPeriod = mLastInForeground;
-            }
-        }
-        ModerateBindingPool moderateBindingPool;
-        synchronized (mModerateBindingPoolLock) {
-            moderateBindingPool = mModerateBindingPool;
+
+        // mLastInForeground can be null at this point as the embedding application could be
+        // used in foreground without spawning any renderers.
+        if (mLastInForeground != null) {
+            mLastInForeground.setBoundForBackgroundPeriod(true);
+            mBoundForBackgroundPeriod = mLastInForeground;
         }
-        if (moderateBindingPool != null) moderateBindingPool.onSentToBackground(mOnTesting);
+
+        if (mModerateBindingPool != null) mModerateBindingPool.onSentToBackground(mOnTesting);
     }
 
     @Override
     public void onBroughtToForeground() {
+        checkIfCalledOnValidThread();
+
         if (mBoundForBackgroundPeriod != null) {
             mBoundForBackgroundPeriod.setBoundForBackgroundPeriod(false);
             mBoundForBackgroundPeriod = null;
         }
-        ModerateBindingPool moderateBindingPool;
-        synchronized (mModerateBindingPoolLock) {
-            moderateBindingPool = mModerateBindingPool;
-        }
-        if (moderateBindingPool != null) moderateBindingPool.onBroughtToForeground();
+        if (mModerateBindingPool != null) mModerateBindingPool.onBroughtToForeground();
     }
 
+    /** This may be called from any thread. */
     @Override
     public boolean isOomProtected(int pid) {
         // In the unlikely event of the OS reusing renderer pid, the call will refer to the most
@@ -464,45 +469,97 @@ class BindingManagerImpl implements BindingManager {
     }
 
     @Override
-    public void clearConnection(int pid) {
+    public void clearConnection(final int pid) {
+        checkIfCalledOnValidThread();
+
         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);
+                    }
+                }
+            };
+            mLauncherThreadExecutor.executeDelayed(cleaner, MANAGED_CONNECTION_DELETE_DELAY_MILLS);
+        }
     }
 
     /** @return true iff the connection reference is no longer held */

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

Also called on any thread?

[Jaekyun Seok (inactive), 2015/09/09 02:02:42]

Yes.

[Yaron, 2015/09/11 22:15:24]

add comment

[Jaekyun Seok (inactive), 2015/09/14 01:41:45]

Done.

     @VisibleForTesting
     public boolean isConnectionCleared(int pid) {
         synchronized (mManagedConnections) {
-            return mManagedConnections.get(pid).isConnectionCleared();
+            ManagedConnection managedConnection = mManagedConnections.get(pid);
+            return managedConnection == null || managedConnection.isConnectionCleared();
         }
     }
 
     @Override
     public void startModerateBindingManagement(
             Context context, int maxSize, float lowReduceRatio, float highReduceRatio) {
-        synchronized (mModerateBindingPoolLock) {
-            if (mIsLowMemoryDevice || mModerateBindingPool != null) return;
+        checkIfCalledOnValidThread();
+
+        if (mIsLowMemoryDevice || mModerateBindingPool != null) return;
+
+        Log.i(TAG, "Moderate binding enabled: maxSize=%d lowReduceRatio=%f highReduceRatio=%f",
+                maxSize, lowReduceRatio, highReduceRatio);
+        mModerateBindingPool = new ModerateBindingPool(
+                maxSize, lowReduceRatio, highReduceRatio, mLauncherThreadExecutor);
+        if (context != null) {
+            context.registerComponentCallbacks(new ComponentCallbacks2() {
+                @Override
+                public void onTrimMemory(final int level) {
+                    mLauncherThreadExecutor.execute(new Runnable() {
+                        @Override
+                        public void run() {
+                            Log.i(TAG, "onTrimMemory: level=%d, size=%d", level,
+                                    mModerateBindingPool.size());
+                            if (mModerateBindingPool.size() > 0) {
+                                if (level <= TRIM_MEMORY_RUNNING_MODERATE) {
+                                    mModerateBindingPool.reduce(false);
+                                } else if (level <= TRIM_MEMORY_RUNNING_LOW) {
+                                    mModerateBindingPool.reduce(true);
+                                } else if (level == TRIM_MEMORY_UI_HIDDEN) {
+                                    // This will be handled by onSentToBackground().
+                                    return;
+                                } else {
+                                    mModerateBindingPool.evictAll();
+                                }
+                            }
+                        }
+                    });
+                }
+
+                @Override
+                public void onLowMemory() {
+                    mLauncherThreadExecutor.execute(new Runnable() {
+                        @Override
+                        public void run() {
+                            Log.i(TAG, "onLowMemory: evict %d bindings",
+                                    mModerateBindingPool.size());
+                            mModerateBindingPool.evictAll();
+                        }
+                    });
+                }
 
-            Log.i(TAG, "Moderate binding enabled: maxSize=%d lowReduceRatio=%f highReduceRatio=%f",
-                    maxSize, lowReduceRatio, highReduceRatio);
-            mModerateBindingPool =
-                    new ModerateBindingPool(maxSize, lowReduceRatio, highReduceRatio);
-            if (context != null) context.registerComponentCallbacks(mModerateBindingPool);
+                @Override
+                public void onConfigurationChanged(Configuration configuration) {}
+            });
         }
     }
 
     @Override
     public void releaseAllModerateBindings() {
-        ModerateBindingPool moderateBindingPool;
-        synchronized (mModerateBindingPoolLock) {
-            moderateBindingPool = mModerateBindingPool;
-        }
-        if (moderateBindingPool != null) {
-            Log.i(TAG, "Release all moderate bindings: %d", moderateBindingPool.size());
-            moderateBindingPool.evictAll();
+        checkIfCalledOnValidThread();
+
+        if (mModerateBindingPool != null) {
+            Log.i(TAG, "Release all moderate bindings: %d", mModerateBindingPool.size());
+            mModerateBindingPool.evictAll();
         }
     }
 }