Throttle media decoding after excessive Android media server crashes

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

Index: content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java b/content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
new file mode 100644
index 0000000000000000000000000000000000000000..1c008c404221fdb3bc0e8f6e05acab7ff6a9f624
--- /dev/null
+++ b/content/public/android/java/src/org/chromium/content/browser/MediaThrottler.java
@@ -0,0 +1,199 @@
+// Copyright 2015 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.
+
+package org.chromium.content.browser;
+
+import android.content.Context;
+import android.media.MediaPlayer;
+import android.os.AsyncTask;
+import android.os.Handler;
+import android.os.Looper;
+import android.os.SystemClock;
+
+import org.chromium.base.Log;
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.JNINamespace;
+import org.chromium.content.R;
+
+/**
+ * Class for listening to Android MediaServer Crashes to throttle media decoding
+ * when needed.
+ */
+@JNINamespace("content")
+class MediaThrottler implements MediaPlayer.OnErrorListener {
+    private static final String TAG = "cr_MediaThrottler";
+    private static final long UNKNOWN_LAST_SERVER_CRASH_TIME = -1;
+
+    // Number of active decode requests.
+    private int mRequestCount;
+
+    // Application context.
+    private final Context mContext;
+
+    // Watch dog player. Used to listen to all media server crashes.
+    private MediaPlayer mPlayer;
+
+    // The last media server crash time since Chrome lauches.
+    private long mLastCrashTime = UNKNOWN_LAST_SERVER_CRASH_TIME;
+
+    // Server crash count since last reset() call.
+    private int mServerCrashCount;
+
+    // Object for synchronized access to memeber variables.
+    private final Object mLock = new Object();
+
+    // Handler for posting delayed tasks.
+    private Handler mHandler;
+
+    // Intervals between media server crashes that are considered normal.

[Raymond Toy, 2015/09/30 22:37:54]

Why are crashes considered "normal"?  And why 60 sec?  How did you come with
this number?  An explanation of how you got the magic 60 sec would be nice.

[qinmin, 2015/09/30 22:56:04]

This value is based on the discussion with the security team. We don't have
anything substantial to support the 60 seconds interval. However, since
restarting the media server takes about 5 seconds, so anything larger than 5
seconds should be reasonable. We can add some UMAs to study the interval between
2 decoder requests, but that can come in a later patch.

[Raymond Toy, 2015/10/01 21:58:20]

I think just saying anything larger than 5 sec or so is a reasonable value.  But
I'm not going to require you to say anything. I just hate random constants that
don't give a hint on how they were chosen.

[qinmin, 2015/10/01 22:53:34]

Done.

+    private static final long SERVER_CRASH_INTERVAL_THRESHOLD_IN_MILLIS = 60000;
+
+    // Delay to keep the watch dog player alive When there are no decoding
+    // requests. This is introduced to avoid recreating the watch dog over and
+    // over if a burst of small decoding requests arrive.
+    private static final int RELEASE_WATCH_DOG_PLAYER_DELAY_IN_MILLIS = 5000;
+
+    // When |mServerCrashCount| reaches this threshold, throttling will start.
+    private static final int SERVER_CRASH_COUNT_THRESHOLD_FOR_THROTTLING = 4;

[Raymond Toy, 2015/09/30 22:37:54]

An explanation on how you decided 4 was enough would be very helpful.

[qinmin, 2015/09/30 22:56:04]

same as above. This value is just introduced to kick off the throttling, there
is nothing to back whether 4 is the optimal number. However, for ASLR attacks,
normally it will involve many more than 4 crashes.

[Raymond Toy, 2015/10/01 21:58:20]

As above, just a short comment saying what you just said here would satisfy me.

But I'm not blocking the CL on this.

[qinmin, 2015/10/01 22:53:34]

Done.

+
+    /**
+     * A background task to release the watch dog player.
+     */
+    private class ReleaseWatchDogTask extends AsyncTask<Void, Void, Void> {
+        @Override
+        protected Void doInBackground(Void... voids) {
+            synchronized (mLock) {
+                if (mRequestCount == 0 && mPlayer != null) {
+                    mPlayer.release();
+                    mPlayer = null;
+                }
+            }
+            return null;
+        }
+    }
+
+    private final Runnable mDelayedReleaseRunnable = new Runnable() {
+        @Override
+        public void run() {
+            new ReleaseWatchDogTask().execute();
+        }
+    };
+
+    @CalledByNative
+    private static MediaThrottler create(Context context) {
+        return new MediaThrottler(context);
+    }
+
+    private MediaThrottler(Context context) {
+        mContext = context;
+        mHandler = new Handler(Looper.getMainLooper());
+    }
+
+    /**
+     * A background task to start the watch dog player.
+     */
+    private class StartWatchDogTask extends AsyncTask<Void, Void, Void> {
+        @Override
+        protected Void doInBackground(Void... voids) {
+            synchronized (mLock) {
+                if (mPlayer != null || mRequestCount == 0) return null;
+                mPlayer = MediaPlayer.create(mContext, R.raw.empty);
+                if (mPlayer == null) {
+                    Log.e(TAG, "Unable to create watch dog player, treat it as server crash.");
+                    onMediaServerCrash();
+                } else {
+                    mPlayer.setOnErrorListener(MediaThrottler.this);
+                }
+            }
+            return null;
+        }
+    }
+
+    /**
+     * Called to request the permission to decode media data.
+     *
+     * @return true if the request is permitted, or false otherwise.
+     */
+    @CalledByNative
+    private boolean requestDecoderResources() {
+        synchronized (mLock) {
+            long currentTime = SystemClock.elapsedRealtime();
+            if (mLastCrashTime != UNKNOWN_LAST_SERVER_CRASH_TIME
+                    && (currentTime - mLastCrashTime < SERVER_CRASH_INTERVAL_THRESHOLD_IN_MILLIS)
+                    && mServerCrashCount >= SERVER_CRASH_COUNT_THRESHOLD_FOR_THROTTLING) {
+                Log.e(TAG, "Request to decode media data denied due to throttling.");
+                return false;
+            }
+            mRequestCount++;
+            if (mRequestCount == 1) {
+                mHandler.removeCallbacks(mDelayedReleaseRunnable);

[Ted C, 2015/10/01 02:19:57]

What happens if ReleaseWatchDogTask had already been executed on a background
thread by mDelayedReleaseRunnable?  Is this supposed to prevent that code from
happening?

If you want to prevent that, you'd need to call
http://developer.android.com/reference/android/os/AsyncTask.html#cancel(boolean)
on the async task itself (and check isCancelled() before and after the lock)

[qinmin, 2015/10/01 18:14:02]

If ReleaseWatchDogTask had already been executed, that should be ok.
There are 3 scenarios on the ordering:
1. ReleaseWatchDogTask is posted to the background thread, but not yet executed.
  In this case, when ReleaseWatchDogTask runs, it will find the requestCount ==
1,and aborting early.
2. ReleaseWatchDogTask has already been executed on the backgroundd thread.
  In this case, the watch dog player is already released, and this function will
just recreate it here.
3. ReleaseWatchDogTask is still not yet executed.
  In this case, the task will be cancelled here, so the watch dog player will
not be released.

On 2015/10/01 02:19:57, Ted C wrote:

+                mHandler.post(new Runnable() {
+                    @Override
+                    public void run() {
+                        new StartWatchDogTask().execute();

[Ted C, 2015/10/01 02:19:57]

Why post the async task?  As opposed to the runnable, it will be done in the
background.  Why can't you just run it?  Do you want it to be the end of the UI
message queue?

[qinmin, 2015/10/01 18:14:02]

Creating the watch dog player will need to initialize the mediaplayer, that
blocks the UI thread for a while. So moving it to an async task frees the UI
thread from such temporary hangs. Alternatively, we can create a dedicated loop
thread for this. But since most of the time the looper thread will be idle, so
having it doesn't justify the cost.

[Ted C, 2015/10/02 17:23:38]

Not blocking the UI thread is good, so an AsyncTask makes sense.  But still
doesn't explain why you need to post the task.  new
ReleaseWatchDogTask().execute();
will have it run in a background thread already.

So posting of the runnable just puts it to the back of the UI message queue,
and I'm not sure why that is needed.

[qinmin, 2015/10/02 17:31:55]

the issue is that async task has to be executed on the UI thread, and
requestDecoderResources can be called on any thread.

[Ted C, 2015/10/02 18:34:20]

Ah ha!  Gotcha!

+                    }
+                });
+            }
+        }
+        return true;
+    }
+
+    /**
+     * Called to signal that a decode request has been completed.
+     */
+    @CalledByNative
+    private void onDecodeRequestFinished() {
+        synchronized (mLock) {
+            mRequestCount--;
+            if (mRequestCount == 0) {
+                // Don't release the watch dog immediately, there could be a
+                // number of small requests coming together.
+                prepareToStopWatchDog();
+            }
+        }
+    }
+
+    /**
+     * Posts a delayed task to stop the watch dog player.
+     */
+    private void prepareToStopWatchDog() {
+        mHandler.postDelayed(mDelayedReleaseRunnable, RELEASE_WATCH_DOG_PLAYER_DELAY_IN_MILLIS);
+    }
+
+    @Override
+    public boolean onError(MediaPlayer mp, int what, int extra) {
+        if (what == MediaPlayer.MEDIA_ERROR_SERVER_DIED) {
+            synchronized (mLock) {
+                onMediaServerCrash();
+            }
+        }
+        return true;
+    }
+
+    /**
+     * Called when media server crashes.
+     */
+    private void onMediaServerCrash() {
+        long currentTime = SystemClock.elapsedRealtime();

[Ted C, 2015/10/01 02:19:57]

do you need to lock here?  I guess you are already locked in both cases, so it
would be just defensive programming to have it.

Maybe we could add an assert for
http://developer.android.com/reference/java/lang/Thread.html#holdsLock(java.l...

[qinmin, 2015/10/01 18:14:02]

Done.

+        if (mLastCrashTime != UNKNOWN_LAST_SERVER_CRASH_TIME
+                && (currentTime - mLastCrashTime < SERVER_CRASH_INTERVAL_THRESHOLD_IN_MILLIS)) {
+            mServerCrashCount++;
+        } else {
+            mServerCrashCount = 1;
+        }
+        mLastCrashTime = currentTime;
+    }
+
+    /**
+     * Resets the MediaThrottler to its initial state so that subsequent requests
+     * will not be throttled.
+     */
+    @CalledByNative
+    private void reset() {
+        synchronized (mLock) {
+            mServerCrashCount = 0;

[Ted C, 2015/10/01 02:19:57]

should you cancel mDelayedReleaseRunnable?

[qinmin, 2015/10/01 18:14:02]

No need. If mDelayedReleaseRunnable is already scheduled or posted on the
background thread while this is called, we should let it run so that the watch
dog player will get released.

+            mLastCrashTime = UNKNOWN_LAST_SERVER_CRASH_TIME;
+        }
+    }
+}