Revert "MediaHTTPConnection: move connection states into an inner class"

This reverts commit 621e7968adf0253d5e22406f02ccc8bcc0eda5ec.

Many of the fields that were moved are annotated @UnsupportedAppUsage,
so the CL would have had undesirable app compat impact. Further,
because investigation has revealed that lock contention *is* possible,
we need to always acquire the lock anyway so there is no longer a
benefit in keeping all of the mutable state in a single field that
can be atomically set to null.

Bug: 114337214
Test: Treehugger

(cherry picked from commit dc9f4b4d5d28fc68b1b5e4e8500bf67d4b11621d)

Change-Id: I202c5653cb086d99228491e161a159bad640105a
Merged-In: I202c5653cb086d99228491e161a159bad640105a

2 files changed, 93 insertions, 125 deletions

diff --git a/media/java/android/media/MediaHTTPConnection.java b/media/java/android/media/MediaHTTPConnection.java
index 3838a99969f0..d72476269e18 100644
--- a/media/java/android/media/MediaHTTPConnection.java
+++ b/media/java/android/media/MediaHTTPConnection.java
@@ -37,7 +37,6 @@ import java.net.URL;
 import java.net.UnknownServiceException;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.concurrent.atomic.AtomicReference;
 
 /** @hide */
 public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
@@ -47,23 +46,27 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
     // connection timeout - 30 sec
     private static final int CONNECT_TIMEOUT_MS = 30 * 1000;
 
+    @UnsupportedAppUsage
+    private long mCurrentOffset = -1;
+    @UnsupportedAppUsage
+    private URL mURL = null;
+    @UnsupportedAppUsage
+    private Map<String, String> mHeaders = null;
+    @UnsupportedAppUsage
+    private HttpURLConnection mConnection = null;
+    @UnsupportedAppUsage
+    private long mTotalSize = -1;
+    private InputStream mInputStream = null;
+
+    @UnsupportedAppUsage
+    private boolean mAllowCrossDomainRedirect = true;
+    @UnsupportedAppUsage
+    private boolean mAllowCrossProtocolRedirect = true;
+
     // from com.squareup.okhttp.internal.http
     private final static int HTTP_TEMP_REDIRECT = 307;
     private final static int MAX_REDIRECTS = 20;
 
-    class ConnectionState {
-        public HttpURLConnection mConnection = null;
-        public InputStream mInputStream = null;
-        public long mCurrentOffset = -1;
-        public Map<String, String> mHeaders = null;
-        public URL mURL = null;
-        public long mTotalSize = -1;
-        public boolean mAllowCrossDomainRedirect = true;
-        public boolean mAllowCrossProtocolRedirect = true;
-    }
-    private final AtomicReference<ConnectionState> mConnectionStateHolder =
-            new AtomicReference<ConnectionState>();
-
     @UnsupportedAppUsage
     public MediaHTTPConnection() {
         CookieHandler cookieHandler = CookieHandler.getDefault();
@@ -81,23 +84,13 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
             Log.d(TAG, "connect: uri=" + uri + ", headers=" + headers);
         }
 
-        ConnectionState connectionState = mConnectionStateHolder.get();
-        synchronized (this) {
-            if (connectionState == null) {
-                connectionState = new ConnectionState();
-                mConnectionStateHolder.set(connectionState);
-            }
-        }
-
         try {
             disconnect();
-            connectionState.mAllowCrossDomainRedirect = true;
-            connectionState.mURL = new URL(uri);
-            connectionState.mHeaders = convertHeaderStringToMap(headers, connectionState);
+            mAllowCrossDomainRedirect = true;
+            mURL = new URL(uri);
+            mHeaders = convertHeaderStringToMap(headers);
         } catch (MalformedURLException e) {
             return null;
-        } finally {
-            mConnectionStateHolder.set(connectionState);
         }
 
         return native_getIMemory();
@@ -113,21 +106,18 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
     }
 
     /* returns true iff header is internal */
-    private boolean filterOutInternalHeaders(
-            String key, String val, ConnectionState connectionState) {
+    private boolean filterOutInternalHeaders(String key, String val) {
         if ("android-allow-cross-domain-redirect".equalsIgnoreCase(key)) {
-            connectionState.mAllowCrossDomainRedirect = parseBoolean(val);
+            mAllowCrossDomainRedirect = parseBoolean(val);
             // cross-protocol redirects are also controlled by this flag
-            connectionState.mAllowCrossProtocolRedirect =
-                    connectionState.mAllowCrossDomainRedirect;
+            mAllowCrossProtocolRedirect = mAllowCrossDomainRedirect;
         } else {
             return false;
         }
         return true;
     }
 
-    private Map<String, String> convertHeaderStringToMap(String headers,
-            ConnectionState connectionState) {
+    private Map<String, String> convertHeaderStringToMap(String headers) {
         HashMap<String, String> map = new HashMap<String, String>();
 
         String[] pairs = headers.split("\r\n");
@@ -137,7 +127,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
                 String key = pair.substring(0, colonPos);
                 String val = pair.substring(colonPos + 1);
 
-                if (!filterOutInternalHeaders(key, val, connectionState)) {
+                if (!filterOutInternalHeaders(key, val)) {
                     map.put(key, val);
                 }
             }
@@ -149,28 +139,25 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
     @Override
     @UnsupportedAppUsage
     public void disconnect() {
-        ConnectionState connectionState = mConnectionStateHolder.getAndSet(null);
-        if (connectionState != null) {
-            teardownConnection(connectionState);
-            connectionState.mHeaders = null;
-            connectionState.mURL = null;
-        }
+        teardownConnection();
+        mHeaders = null;
+        mURL = null;
     }
 
-    private void teardownConnection(ConnectionState connectionState) {
-        if (connectionState.mConnection != null) {
-            if (connectionState.mInputStream != null) {
+    private void teardownConnection() {
+        if (mConnection != null) {
+            if (mInputStream != null) {
                 try {
-                    connectionState.mInputStream.close();
+                    mInputStream.close();
                 } catch (IOException e) {
                 }
-                connectionState.mInputStream = null;
+                mInputStream = null;
             }
 
-            connectionState.mConnection.disconnect();
-            connectionState.mConnection = null;
+            mConnection.disconnect();
+            mConnection = null;
 
-            connectionState.mCurrentOffset = -1;
+            mCurrentOffset = -1;
         }
     }
 
@@ -197,44 +184,42 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
         return false;
     }
 
-    private void seekTo(long offset, ConnectionState connectionState) throws IOException {
-        teardownConnection(connectionState);
+    private void seekTo(long offset) throws IOException {
+        teardownConnection();
 
         try {
             int response;
             int redirectCount = 0;
 
-            URL url = connectionState.mURL;
+            URL url = mURL;
 
             // do not use any proxy for localhost (127.0.0.1)
             boolean noProxy = isLocalHost(url);
 
             while (true) {
                 if (noProxy) {
-                    connectionState.mConnection =
-                            (HttpURLConnection) url.openConnection(Proxy.NO_PROXY);
+                    mConnection = (HttpURLConnection)url.openConnection(Proxy.NO_PROXY);
                 } else {
-                    connectionState.mConnection = (HttpURLConnection) url.openConnection();
+                    mConnection = (HttpURLConnection)url.openConnection();
                 }
-                connectionState.mConnection.setConnectTimeout(CONNECT_TIMEOUT_MS);
+                mConnection.setConnectTimeout(CONNECT_TIMEOUT_MS);
 
                 // handle redirects ourselves if we do not allow cross-domain redirect
-                connectionState.mConnection.setInstanceFollowRedirects(
-                        connectionState.mAllowCrossDomainRedirect);
+                mConnection.setInstanceFollowRedirects(mAllowCrossDomainRedirect);
 
-                if (connectionState.mHeaders != null) {
-                    for (Map.Entry<String, String> entry : connectionState.mHeaders.entrySet()) {
-                        connectionState.mConnection.setRequestProperty(
+                if (mHeaders != null) {
+                    for (Map.Entry<String, String> entry : mHeaders.entrySet()) {
+                        mConnection.setRequestProperty(
                                 entry.getKey(), entry.getValue());
                     }
                 }
 
                 if (offset > 0) {
-                    connectionState.mConnection.setRequestProperty(
+                    mConnection.setRequestProperty(
                             "Range", "bytes=" + offset + "-");
                 }
 
-                response = connectionState.mConnection.getResponseCode();
+                response = mConnection.getResponseCode();
                 if (response != HttpURLConnection.HTTP_MULT_CHOICE &&
                         response != HttpURLConnection.HTTP_MOVED_PERM &&
                         response != HttpURLConnection.HTTP_MOVED_TEMP &&
@@ -248,7 +233,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
                     throw new NoRouteToHostException("Too many redirects: " + redirectCount);
                 }
 
-                String method = connectionState.mConnection.getRequestMethod();
+                String method = mConnection.getRequestMethod();
                 if (response == HTTP_TEMP_REDIRECT &&
                         !method.equals("GET") && !method.equals("HEAD")) {
                     // "If the 307 status code is received in response to a
@@ -256,35 +241,34 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
                     // automatically redirect the request"
                     throw new NoRouteToHostException("Invalid redirect");
                 }
-                String location = connectionState.mConnection.getHeaderField("Location");
+                String location = mConnection.getHeaderField("Location");
                 if (location == null) {
                     throw new NoRouteToHostException("Invalid redirect");
                 }
-                url = new URL(connectionState.mURL /* TRICKY: don't use url! */, location);
+                url = new URL(mURL /* TRICKY: don't use url! */, location);
                 if (!url.getProtocol().equals("https") &&
                         !url.getProtocol().equals("http")) {
                     throw new NoRouteToHostException("Unsupported protocol redirect");
                 }
-                boolean sameProtocol =
-                        connectionState.mURL.getProtocol().equals(url.getProtocol());
-                if (!connectionState.mAllowCrossProtocolRedirect && !sameProtocol) {
+                boolean sameProtocol = mURL.getProtocol().equals(url.getProtocol());
+                if (!mAllowCrossProtocolRedirect && !sameProtocol) {
                     throw new NoRouteToHostException("Cross-protocol redirects are disallowed");
                 }
-                boolean sameHost = connectionState.mURL.getHost().equals(url.getHost());
-                if (!connectionState.mAllowCrossDomainRedirect && !sameHost) {
+                boolean sameHost = mURL.getHost().equals(url.getHost());
+                if (!mAllowCrossDomainRedirect && !sameHost) {
                     throw new NoRouteToHostException("Cross-domain redirects are disallowed");
                 }
 
                 if (response != HTTP_TEMP_REDIRECT) {
                     // update effective URL, unless it is a Temporary Redirect
-                    connectionState.mURL = url;
+                    mURL = url;
                 }
             }
 
-            if (connectionState.mAllowCrossDomainRedirect) {
+            if (mAllowCrossDomainRedirect) {
                 // remember the current, potentially redirected URL if redirects
                 // were handled by HttpURLConnection
-                connectionState.mURL = connectionState.mConnection.getURL();
+                mURL = mConnection.getURL();
             }
 
             if (response == HttpURLConnection.HTTP_PARTIAL) {
@@ -292,9 +276,10 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
                 // because what we want is not just the length of the range
                 // returned but the size of the full content if available.
 
-                String contentRange = connectionState.mConnection.getHeaderField("Content-Range");
+                String contentRange =
+                    mConnection.getHeaderField("Content-Range");
 
-                connectionState.mTotalSize = -1;
+                mTotalSize = -1;
                 if (contentRange != null) {
                     // format is "bytes xxx-yyy/zzz
                     // where "zzz" is the total number of bytes of the
@@ -306,7 +291,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
                             contentRange.substring(lastSlashPos + 1);
 
                         try {
-                            connectionState.mTotalSize = Long.parseLong(total);
+                            mTotalSize = Long.parseLong(total);
                         } catch (NumberFormatException e) {
                         }
                     }
@@ -314,7 +299,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
             } else if (response != HttpURLConnection.HTTP_OK) {
                 throw new IOException();
             } else {
-                connectionState.mTotalSize = connectionState.mConnection.getContentLength();
+                mTotalSize = mConnection.getContentLength();
             }
 
             if (offset > 0 && response != HttpURLConnection.HTTP_PARTIAL) {
@@ -323,14 +308,14 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
                 throw new ProtocolException();
             }
 
-            connectionState.mInputStream =
-                new BufferedInputStream(connectionState.mConnection.getInputStream());
+            mInputStream =
+                new BufferedInputStream(mConnection.getInputStream());
 
-            connectionState.mCurrentOffset = offset;
+            mCurrentOffset = offset;
         } catch (IOException e) {
-            connectionState.mTotalSize = -1;
-            teardownConnection(connectionState);
-            connectionState.mCurrentOffset = -1;
+            mTotalSize = -1;
+            teardownConnection();
+            mCurrentOffset = -1;
 
             throw e;
         }
@@ -339,14 +324,10 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
     @Override
     @UnsupportedAppUsage
     public int readAt(long offset, int size) {
-        ConnectionState connectionState = mConnectionStateHolder.get();
-        if (connectionState != null) {
-            return native_readAt(offset, size, connectionState);
-        }
-        return -1;
+        return native_readAt(offset, size);
     }
 
-    private int readAt(long offset, byte[] data, int size, ConnectionState connectionState) {
+    private int readAt(long offset, byte[] data, int size) {
         StrictMode.ThreadPolicy policy =
             new StrictMode.ThreadPolicy.Builder().permitAll().build();
 
@@ -354,12 +335,12 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
 
         try {
             synchronized(this) {
-                if (offset != connectionState.mCurrentOffset) {
-                    seekTo(offset, connectionState);
+                if (offset != mCurrentOffset) {
+                    seekTo(offset);
                 }
             }
 
-            int n = connectionState.mInputStream.read(data, 0, size);
+            int n = mInputStream.read(data, 0, size);
 
             if (n == -1) {
                 // InputStream signals EOS using a -1 result, our semantics
@@ -367,7 +348,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
                 n = 0;
             }
 
-            connectionState.mCurrentOffset += n;
+            mCurrentOffset += n;
 
             if (VERBOSE) {
                 Log.d(TAG, "readAt " + offset + " / " + size + " => " + n);
@@ -399,47 +380,35 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
 
     @Override
     public synchronized long getSize() {
-        ConnectionState connectionState = mConnectionStateHolder.get();
-        if (connectionState != null) {
-            if (connectionState.mConnection == null) {
-                try {
-                    seekTo(0, connectionState);
-                } catch (IOException e) {
-                    return -1;
-                }
+        if (mConnection == null) {
+            try {
+                seekTo(0);
+            } catch (IOException e) {
+                return -1;
             }
-            return connectionState.mTotalSize;
         }
 
-        return -1;
+        return mTotalSize;
     }
 
     @Override
     @UnsupportedAppUsage
     public synchronized String getMIMEType() {
-        ConnectionState connectionState = mConnectionStateHolder.get();
-        if (connectionState != null) {
-            if (connectionState.mConnection == null) {
-                try {
-                    seekTo(0, connectionState);
-                } catch (IOException e) {
-                    return "application/octet-stream";
-                }
+        if (mConnection == null) {
+            try {
+                seekTo(0);
+            } catch (IOException e) {
+                return "application/octet-stream";
             }
-            return connectionState.mConnection.getContentType();
         }
 
-        return null;
+        return mConnection.getContentType();
     }
 
     @Override
     @UnsupportedAppUsage
     public String getUri() {
-        ConnectionState connectionState = mConnectionStateHolder.get();
-        if (connectionState != null) {
-            return connectionState.mURL.toString();
-        }
-        return null;
+        return mURL.toString();
     }
 
     @Override
@@ -452,7 +421,7 @@ public class MediaHTTPConnection extends IMediaHTTPConnection.Stub {
     private native final void native_finalize();
 
     private native final IBinder native_getIMemory();
-    private native int native_readAt(long offset, int size, ConnectionState connectionState);
+    private native final int native_readAt(long offset, int size);
 
     static {
         System.loadLibrary("media_jni");
diff --git a/media/jni/android_media_MediaHTTPConnection.cpp b/media/jni/android_media_MediaHTTPConnection.cpp
index d28c15c98d16..365e045689f0 100644
--- a/media/jni/android_media_MediaHTTPConnection.cpp
+++ b/media/jni/android_media_MediaHTTPConnection.cpp
@@ -109,8 +109,7 @@ static void android_media_MediaHTTPConnection_native_init(JNIEnv *env) {
     gFields.context = env->GetFieldID(clazz.get(), "mNativeContext", "J");
     CHECK(gFields.context != NULL);
 
-    gFields.readAtMethodID = env->GetMethodID(
-            clazz.get(), "readAt", "(J[BILandroid/media/MediaHTTPConnection$ConnectionState;)I");
+    gFields.readAtMethodID = env->GetMethodID(clazz.get(), "readAt", "(J[BI)I");
 }
 
 static void android_media_MediaHTTPConnection_native_setup(
@@ -133,7 +132,7 @@ static jobject android_media_MediaHTTPConnection_native_getIMemory(
 }
 
 static jint android_media_MediaHTTPConnection_native_readAt(
-        JNIEnv *env, jobject thiz, jlong offset, jint size, jobject connectionState) {
+        JNIEnv *env, jobject thiz, jlong offset, jint size) {
     sp<JMediaHTTPConnection> conn = getObject(env, thiz);
     if (size > JMediaHTTPConnection::kBufferSize) {
         size = JMediaHTTPConnection::kBufferSize;
@@ -142,7 +141,7 @@ static jint android_media_MediaHTTPConnection_native_readAt(
     jbyteArray byteArrayObj = conn->getByteArrayObj();
 
     jint n = env->CallIntMethod(
-            thiz, gFields.readAtMethodID, offset, byteArrayObj, size, connectionState);
+            thiz, gFields.readAtMethodID, offset, byteArrayObj, size);
 
     if (n > 0) {
         env->GetByteArrayRegion(
@@ -159,7 +158,7 @@ static const JNINativeMethod gMethods[] = {
     { "native_getIMemory", "()Landroid/os/IBinder;",
       (void *)android_media_MediaHTTPConnection_native_getIMemory },
 
-    { "native_readAt", "(JILandroid/media/MediaHTTPConnection$ConnectionState;)I",
+    { "native_readAt", "(JI)I",
       (void *)android_media_MediaHTTPConnection_native_readAt },
 
     { "native_init", "()V",