Make MediaHTTPConnection thread safe.

MediaHTTPConnection's public methods are called from multiple Binder
threads. Since both HttpURLConnection and access to the various
connection related fields is not thread safe, this CL guards most
methods by a single lock. This means that the methods can now block
when called, although this should be rare:

 - there are two processes that call these methods. One process
   only calls getSize(), and the other process calls methods
   from a single thread (ie. at not overlapping clock times).
 - should lock contention unexpectedly increase in future, then
   that would be bad (because Binder thread pool threads would
   be blocked/unavailable), but it would not be easy to detect.
   It would be easy to detect if we could stop getSize() being
   called at overlapping clock times, since we could then use
   ReentrantLock.tryLock() to assert that the lock is never contended
   outside of disconnect().

Because it's a requirement for disconnect() to quickly stop another
thread that is blocked in readAt(), disconnect() is the only method
that doesn't acquire the lock immediately; the mConnection field
is marked volatile so that disconnect() has a high chance of reading
that field and calling disconnect() on it without waiting for
another thread (there's a small risk that another thread might
acquire the lock and start a new connection while disconnect()
is waiting for the lock; in that case, after acquiring the lock,
disconnect() will also disconnect that new connection; this is
subject to potential change in future.

Initially, a ReentrantLock object was considered but for now this
CL instead uses the synchronized lock on "this" because:

 - it minimizes churn on the lines of code in this file because
   synchronized (this) { } can be expressed by introduction of
   the word "synchronized" on the method header, whereas
   mLock.lock(); try { ... } finally { mLock.unlock(); } would
   indent all the lines in-between and thus pollute git annotate.
 - some methods were already synchronized.
 - ReentrantLock.tryLock() is not used for now; most of the time,
   lock acquisition should be uncontended but the two cases of
   lock contention mentioned above exist, which makes it difficult
   to distinguish surprising from unsurprising lock contention.
   While this is the case, it seems better to keep the code
   simple and to just unconditionally block.

Fixes: 114337214
Fixes: 119900000
Fixes: 129444137
Fixes: 128758794
Fixes: 63002045

Test: Checked manually that bug 114337214 no longer reproduces on
      Android API level 27 (Oreo MR1) after cherrypicking this CL.
Test: Ran the following on internal master with this CL:
      make cts && cts-tradefed run cts -m CtsMediaTestCases \
      -t android.media.cts.NativeDecoderTest#testAMediaDataSourceClose \
      --abi arm64-v8a
Test: Ran the following both on AOSP (158 tests) and internal master (178):
      atest CtsMediaTestCases:android.media.cts.{MediaPlayer{,2},Routing}Test

      All these tests pass except that on AOSP only, the following test
      fails both before and after my CL (appears unrelated):
      android.media.cts.RoutingTest#test_MediaPlayer_RoutingChangedCallback
Change-Id: I4e32a58891c3ce60ddfa72d36060486d37906f8d

diff --git a/media/java/android/media/MediaHTTPConnection.java b/media/java/android/media/MediaHTTPConnection.java
index d724762..b5c4cca 100644 (file)
--- a/media/java/android/media/MediaHTTPConnection.java
+++ b/media/java/android/media/MediaHTTPConnection.java
@@ -24,6 +24,7 @@ import android.os.IBinder;
 import android.os.StrictMode;
 import android.util.Log;
 
+import com.android.internal.annotations.GuardedBy;
 import java.io.BufferedInputStream;
 import java.io.IOException;
 import java.io.InputStream;
@@ -46,20 +47,35 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
     // connection timeout - 30 sec
     private static final int CONNECT_TIMEOUT_MS = 30 * 1000;
 
+    @GuardedBy("this")
     @UnsupportedAppUsage
     private long mCurrentOffset = -1;
+
+    @GuardedBy("this")
     @UnsupportedAppUsage
     private URL mURL = null;
+
+    @GuardedBy("this")
     @UnsupportedAppUsage
     private Map<String, String> mHeaders = null;
+
+    // volatile so that disconnect() can be called without acquiring a lock.
+    // All other access is @GuardedBy("this").
     @UnsupportedAppUsage
-    private HttpURLConnection mConnection = null;
+    private volatile HttpURLConnection mConnection = null;
+
+    @GuardedBy("this")
     @UnsupportedAppUsage
     private long mTotalSize = -1;
+
+    @GuardedBy("this")
     private InputStream mInputStream = null;
 
+    @GuardedBy("this")
     @UnsupportedAppUsage
     private boolean mAllowCrossDomainRedirect = true;
+
+    @GuardedBy("this")
     @UnsupportedAppUsage
     private boolean mAllowCrossProtocolRedirect = true;
 
@@ -79,7 +95,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
 
     @Override
     @UnsupportedAppUsage
-    public IBinder connect(String uri, String headers) {
+    public synchronized IBinder connect(String uri, String headers) {
         if (VERBOSE) {
             Log.d(TAG, "connect: uri=" + uri + ", headers=" + headers);
         }
@@ -96,7 +112,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
         return native_getIMemory();
     }
 
-    private boolean parseBoolean(String val) {
+    private static boolean parseBoolean(String val) {
         try {
             return Long.parseLong(val) != 0;
         } catch (NumberFormatException e) {
@@ -106,7 +122,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
     }
 
     /* returns true iff header is internal */
-    private boolean filterOutInternalHeaders(String key, String val) {
+    private synchronized boolean filterOutInternalHeaders(String key, String val) {
         if ("android-allow-cross-domain-redirect".equalsIgnoreCase(key)) {
             mAllowCrossDomainRedirect = parseBoolean(val);
             // cross-protocol redirects are also controlled by this flag
@@ -117,7 +133,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
         return true;
     }
 
-    private Map<String, String> convertHeaderStringToMap(String headers) {
+    private synchronized Map<String, String> convertHeaderStringToMap(String headers) {
         HashMap<String, String> map = new HashMap<String, String>();
 
         String[] pairs = headers.split("\r\n");
@@ -139,12 +155,23 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
     @Override
     @UnsupportedAppUsage
     public void disconnect() {
-        teardownConnection();
-        mHeaders = null;
-        mURL = null;
+        HttpURLConnection connectionToDisconnect = mConnection;
+        // Call disconnect() before blocking for the lock in order to ensure that any
+        // other thread that is blocked in readAt() will return quickly.
+        if (connectionToDisconnect != null) {
+            connectionToDisconnect.disconnect();
+        }
+        synchronized (this) {
+            // It's unlikely but possible that while we were waiting to acquire the lock, another
+            // thread concurrently started a new connection; if so, we're disconnecting that one
+            // here, too.
+            teardownConnection();
+            mHeaders = null;
+            mURL = null;
+        }
     }
 
-    private void teardownConnection() {
+    private synchronized void teardownConnection() {
         if (mConnection != null) {
             if (mInputStream != null) {
                 try {
@@ -184,7 +211,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
         return false;
     }
 
-    private void seekTo(long offset) throws IOException {
+    private synchronized void seekTo(long offset) throws IOException {
         teardownConnection();
 
         try {
@@ -323,21 +350,19 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
 
     @Override
     @UnsupportedAppUsage
-    public int readAt(long offset, int size) {
+    public synchronized int readAt(long offset, int size) {
         return native_readAt(offset, size);
     }
 
-    private int readAt(long offset, byte[] data, int size) {
+    private synchronized int readAt(long offset, byte[] data, int size) {
         StrictMode.ThreadPolicy policy =
             new StrictMode.ThreadPolicy.Builder().permitAll().build();
 
         StrictMode.setThreadPolicy(policy);
 
         try {
-            synchronized(this) {
-                if (offset != mCurrentOffset) {
-                    seekTo(offset);
-                }
+            if (offset != mCurrentOffset) {
+                seekTo(offset);
             }
 
             int n = mInputStream.read(data, 0, size);
@@ -407,7 +432,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
 
     @Override
     @UnsupportedAppUsage
-    public String getUri() {
+    public synchronized String getUri() {
         return mURL.toString();
     }