Fix broken duration logic in MediaResourceGetter for non-video media.

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

Index: content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java b/content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java
index b8f208553dea4afde581ce6a2d68afc4d39b51cd..de9f67b82731521ed6c21f727be2909ecbf2d2e1 100644
--- a/content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java
+++ b/content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java
@@ -13,12 +13,16 @@ import android.net.Uri;
 import android.text.TextUtils;
 import android.util.Log;
 
+import com.google.common.annotations.VisibleForTesting;
+
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
 import org.chromium.base.PathUtils;
 
 import java.io.File;
+import java.io.IOException;
 import java.util.HashMap;
+import java.util.Map;
 
 /**
  * Java counterpart of android MediaResourceGetter.
@@ -27,104 +31,290 @@ import java.util.HashMap;
 class MediaResourceGetter {
 
     private static final String TAG = "MediaResourceGetter";
+    private static final MediaMetadata EMPTY_METADATA = new MediaMetadata(0,0,0,false);

[qinmin, 2013/08/30 16:53:34]

don't use static final on non-primitive objects. they hit performance

[Andrew Hayden (chromium.org), 2013/08/30 17:10:09]

I will make it non-static.

+
+    private final MediaMetadataRetriever mRetriever = new MediaMetadataRetriever();
 
-    private static class MediaMetadata {
+    @VisibleForTesting
+    static class MediaMetadata {
         private final int mDurationInMilliseconds;
         private final int mWidth;
         private final int mHeight;
         private final boolean mSuccess;
 
-        private MediaMetadata(int durationInMilliseconds, int width, int height, boolean success) {
+        MediaMetadata(int durationInMilliseconds, int width, int height, boolean success) {
             mDurationInMilliseconds = durationInMilliseconds;
             mWidth = width;
             mHeight = height;
             mSuccess = success;
         }
 
+        // TODO(andrewhayden): according to the spec, if duration is unknown
+        // then we must return NaN. If it is unbounded, then positive infinity.
+        // http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html
         @CalledByNative("MediaMetadata")
-        private int getDurationInMilliseconds() { return mDurationInMilliseconds; }
+        int getDurationInMilliseconds() { return mDurationInMilliseconds; }
 
         @CalledByNative("MediaMetadata")
-        private int getWidth() { return mWidth; }
+        int getWidth() { return mWidth; }
 
         @CalledByNative("MediaMetadata")
-        private int getHeight() { return mHeight; }
+        int getHeight() { return mHeight; }
 
         @CalledByNative("MediaMetadata")
-        private boolean isSuccess() { return mSuccess; }
+        boolean isSuccess() { return mSuccess; }
+
+        @Override
+        public String toString() {
+            return "MediaMetadata["
+                    + "durationInMilliseconds=" + mDurationInMilliseconds
+                    + ", width=" + mWidth
+                    + ", height=" + mHeight
+                    + ", success=" + mSuccess
+                    + "]";
+        }
+
+        @Override
+        public int hashCode() {
+            final int prime = 31;
+            int result = 1;
+            result = prime * result + mDurationInMilliseconds;

[qinmin, 2013/08/30 16:53:34]

why we need this hash function? mDurationInMilliseconds can be very big, and we
can get int overflow below

[Andrew Hayden (chromium.org), 2013/08/30 17:10:09]

Java best practices dictate always generating hashCode() when generating
equals(). Generating just one or the other leads to extremely confusing
behavior. I added equals() because it saves a stupid about of boilerplate code
in the unit tests; I added hashCode() because I added equals. Both were
autogenerated by Eclipse. The overflow case for the integer shouldn't matter;
all that matters is generating a nice uniform distribution of values for
hashing, which this accomplishes regardless of overflow.

+            result = prime * result + mHeight;
+            result = prime * result + (mSuccess ? 1231 : 1237);
+            result = prime * result + mWidth;
+            return result;
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+            if (this == obj)
+                return true;
+            if (obj == null)
+                return false;
+            if (getClass() != obj.getClass())
+                return false;
+            MediaMetadata other = (MediaMetadata)obj;
+            if (mDurationInMilliseconds != other.mDurationInMilliseconds)
+                return false;
+            if (mHeight != other.mHeight)
+                return false;
+            if (mSuccess != other.mSuccess)
+                return false;
+            if (mWidth != other.mWidth)
+                return false;
+            return true;
+        }
     }
 
     @CalledByNative
     private static MediaMetadata extractMediaMetadata(Context context, String url, String cookies) {
-        int durationInMilliseconds = 0;
-        int width = 0;
-        int height = 0;
-        boolean success = false;
-        // TODO(qinmin): use ConnectionTypeObserver to listen to the network type change.
-        ConnectivityManager mConnectivityManager =
-                (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
-        if (mConnectivityManager != null) {
-            if (context.checkCallingOrSelfPermission(
-                    android.Manifest.permission.ACCESS_NETWORK_STATE) !=
-                    PackageManager.PERMISSION_GRANTED) {
-                return new MediaMetadata(0, 0, 0, false);
-            }
+        return new MediaResourceGetter().extract(context, url, cookies);
+    }
 
-            NetworkInfo info = mConnectivityManager.getActiveNetworkInfo();
-            if (info == null) {
-                return new MediaMetadata(durationInMilliseconds, width, height, success);
-            }
-            switch (info.getType()) {
-                case ConnectivityManager.TYPE_ETHERNET:
-                case ConnectivityManager.TYPE_WIFI:
-                    break;
-                case ConnectivityManager.TYPE_WIMAX:
-                case ConnectivityManager.TYPE_MOBILE:
-                default:
-                    return new MediaMetadata(durationInMilliseconds, width, height, success);
-            }
+    @VisibleForTesting
+    MediaMetadata extract(Context context, String url, String cookies) {
+        if (!configure(context, url, cookies)) {
+            Log.e(TAG, "Unable to configure metadata extractor");
+            return EMPTY_METADATA;
         }
 
-        MediaMetadataRetriever retriever = new MediaMetadataRetriever();
         try {
-            Uri uri = Uri.parse(url);
-            String scheme = uri.getScheme();
-            if (scheme == null || scheme.equals("file")) {
-                File file = new File(uri.getPath());
-                String path = file.getAbsolutePath();
-                if (file.exists() && (path.startsWith("/mnt/sdcard/") ||
-                        path.startsWith("/sdcard/") ||
-                        path.startsWith(PathUtils.getExternalStorageDirectory()))) {
-                    retriever.setDataSource(path);
-                } else {
-                    Log.e(TAG, "Unable to read file: " + url);
-                    return new MediaMetadata(durationInMilliseconds, width, height, success);
+            String durationString = extractMetadata(
+                    MediaMetadataRetriever.METADATA_KEY_DURATION);
+            if (durationString == null) {
+                Log.w(TAG, "missing duration metadata");
+                return EMPTY_METADATA;
+            }
+
+            int durationMillis = 0;
+            try {
+                durationMillis = Integer.parseInt(durationString);
+            } catch (NumberFormatException e) {
+                Log.w(TAG, "non-numeric duration: " + durationString);
+                return EMPTY_METADATA;
+            }
+
+            int width = 0;
+            int height = 0;
+            boolean hasVideo = "yes".equals(extractMetadata(
+                    MediaMetadataRetriever.METADATA_KEY_HAS_VIDEO));
+            Log.d(TAG, (hasVideo ? "resource has video" : "resource doesn't have video"));
+            if (hasVideo) {
+                String widthString = extractMetadata(
+                        MediaMetadataRetriever.METADATA_KEY_VIDEO_WIDTH);
+                if (widthString == null) {
+                    Log.w(TAG, "missing video width metadata");
+                    return EMPTY_METADATA;
                 }
-            } else {
-                HashMap<String, String> headersMap = new HashMap<String, String>();
-                if (!TextUtils.isEmpty(cookies)) {
-                    headersMap.put("Cookie", cookies);
+                try {
+                    width = Integer.parseInt(widthString);
+                } catch (NumberFormatException e) {
+                    Log.w(TAG, "non-numeric width: " + widthString);
+                    return EMPTY_METADATA;
+                }
+
+                String heightString = extractMetadata(
+                        MediaMetadataRetriever.METADATA_KEY_VIDEO_HEIGHT);
+                if (heightString == null) {
+                    Log.w(TAG, "missing video height metadata");
+                    return EMPTY_METADATA;
+                }
+                try {
+                    height = Integer.parseInt(heightString);
+                } catch (NumberFormatException e) {
+                    Log.w(TAG, "non-numeric height: " + heightString);
+                    return EMPTY_METADATA;
                 }
-                retriever.setDataSource(url, headersMap);
-            }
-            String duration = retriever.extractMetadata(
-                    MediaMetadataRetriever.METADATA_KEY_DURATION);
-            String videoWidth = retriever.extractMetadata(
-                    MediaMetadataRetriever.METADATA_KEY_VIDEO_WIDTH);
-            String videoHeight = retriever.extractMetadata(
-                    MediaMetadataRetriever.METADATA_KEY_VIDEO_HEIGHT);
-            if (duration == null || videoWidth == null || videoHeight == null) {
-                return new MediaMetadata(durationInMilliseconds, width, height, success);
             }
-            durationInMilliseconds = Integer.parseInt(duration);
-            width = Integer.parseInt(videoWidth);
-            height = Integer.parseInt(videoHeight);
-            success = true;
-        } catch (IllegalArgumentException e) {
-            Log.e(TAG, "Invalid url: " + e);
+            MediaMetadata result = new MediaMetadata(durationMillis, width, height, true);
+            Log.d(TAG, "extracted valid metadata: " + result.toString());
+            return result;
         } catch (RuntimeException e) {
-            Log.e(TAG, "Invalid url: " + e);
+            Log.e(TAG, "Unable to extract medata", e);
+            return EMPTY_METADATA;
         }
-        return new MediaMetadata(durationInMilliseconds, width, height, success);
     }
-}
+
+    @VisibleForTesting
+    boolean configure(Context context, String url, String cookies) {
+        Uri uri = Uri.parse(url);
+        String scheme = uri.getScheme();
+        if (scheme == null || scheme.equals("file")) {
+            File file = uriToFile(uri.getPath());
+            if (!file.exists()) {
+                Log.e(TAG, "File does not exist.");
+                return false;
+            }
+            if (!filePathAcceptable(file)) {
+                Log.e(TAG, "Refusing to read from unsafe file location.");
+                return false;
+            }
+            try {
+                configure(file.getAbsolutePath());
+                return true;
+            } catch (RuntimeException e) {
+                Log.e(TAG, "Error configuring data source", e);
+                return false;
+            }
+        } else {
+            if (!networkEnvironmentOk(context)) {
+                Log.w(TAG, "non-file URI can't be read due to unsuitable network conditions");
+                return false;
+            }
+            Map<String, String> headersMap = new HashMap<String, String>();
+            if (!TextUtils.isEmpty(cookies)) {
+                headersMap.put("Cookie", cookies);
+            }
+            try {
+                configure(url, headersMap);
+                return true;
+            } catch (RuntimeException e) {
+                Log.e(TAG, "Error configuring data source", e);
+                return false;
+            }
+        }
+    }
+
+    /**
+     * @return true if the device is on an ethernet or wifi network.
+     * If anything goes wrong (e.g., permission denied while trying to access
+     * the network state), returns false.
+     */
+    @VisibleForTesting
+    boolean networkEnvironmentOk(Context context) {

[qinmin, 2013/08/30 16:53:34]

the functionName is very misleading. what about IsNetworkReliable

[Andrew Hayden (chromium.org), 2013/08/30 17:10:09]

Works for me. Good suggestion, thanks. Long term I hope we plan to allow
streaming on mobile in lite of 4G/LTE, but that's a discussion for another day!

[qinmin, 2013/08/30 17:25:19]

This won't impact streaming. It only saves some unnecessary data cost for the
user if he don't really interested in the video.

On 2013/08/30 17:10:09, Andrew Hayden wrote:

+        if (context.checkCallingOrSelfPermission(
+                android.Manifest.permission.ACCESS_NETWORK_STATE) !=
+                PackageManager.PERMISSION_GRANTED) {
+            Log.w(TAG, "permission denied to access network state");
+            return false;
+        }
+
+        Integer networkType = getNetworkType(context);
+        if (networkType == null) {
+            return false;
+        }
+        switch (networkType.intValue()) {
+            case ConnectivityManager.TYPE_ETHERNET:
+            case ConnectivityManager.TYPE_WIFI:
+                Log.d(TAG, "ethernet/wifi connection detected");
+                return true;
+            case ConnectivityManager.TYPE_WIMAX:
+            case ConnectivityManager.TYPE_MOBILE:
+            default:
+                Log.d(TAG, "no ethernet/wifi connection detected");
+                return false;
+        }
+    }
+
+    /**
+     * @param file the file whose path should be checked
+     * @return true if and only if the file is in a location that we consider
+     * safe to read from, such as /mnt/sdcard.
+     */
+    @VisibleForTesting
+    boolean filePathAcceptable(File file) {
+        try {
+            // We must canonicalize the file. However, in order to properly
+            // match the roots we must also canonicalize the well-known
+            // paths we are matching against. If we don't, then we can
+            // get unusual results in testing systems or possibly on rooted
+            // devices.
+            //
+            // Note that canonicalized directory paths always end with '/'.
+            final String canonicalMntSdcard = new File("/mnt/sdcard/").getCanonicalPath();
+            final String canonicalSdcard = new File("/sdcard/").getCanonicalPath();
+            String path = file.getCanonicalPath();
+            Log.d(TAG, "canonicalized file path: " + path);
+            return path.startsWith(canonicalMntSdcard) ||
+                    path.startsWith(canonicalSdcard) ||
+                    path.startsWith(getExternalStorageDirectory());
+        } catch (IOException e) {
+            // Canonicalization has failed. Assume malicious, give up.
+            Log.w(TAG, "canonicalization of file path failed");
+            return false;
+        }
+    }
+
+    // The methods below can be used by unit tests to fake functionality.
+    @VisibleForTesting
+    File uriToFile(String path) {
+        return new File(path);
+    }
+
+    @VisibleForTesting
+    Integer getNetworkType(Context context) {
+        // TODO(qinmin): use ConnectionTypeObserver to listen to the network type change.
+        ConnectivityManager mConnectivityManager =
+                (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
+        if (mConnectivityManager == null) {
+            Log.w(TAG, "no connectivity manager available");
+            return null;
+        }
+        NetworkInfo info = mConnectivityManager.getActiveNetworkInfo();
+        if (info == null) {
+            Log.d(TAG, "no active network");
+            return null;
+        }
+        return info.getType();
+    }
+
+    @VisibleForTesting
+    String getExternalStorageDirectory() {
+        return PathUtils.getExternalStorageDirectory();
+    }
+
+    @VisibleForTesting
+    void configure(String url, Map<String,String> headers) {
+        mRetriever.setDataSource(url, headers);
+    }
+
+    @VisibleForTesting
+    void configure(String path) {
+        mRetriever.setDataSource(path);
+    }
+
+    @VisibleForTesting
+    String extractMetadata(int key) {
+        return mRetriever.extractMetadata(key);
+    }
+}