Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23684004:
Fix broken duration logic in MediaResourceGetter for non-video media. (Closed)
|
Created:
7 years, 3 months ago by Andrew Hayden (chromium.org) Modified:
6 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFix broken duration logic in MediaResourceGetter for non-video media.
Also closes a security hole: the filesystem checks did not properly canonicalize the path, with the result that paths such as "/mnt/sdcard/../../data/data/yourdata" would have been accepted.
BUG=278117
NOPRESUBMIT=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262766
Patch Set 1 #Patch Set 2 : Fix minor typo in toString #
Total comments: 5
Patch Set 3 : Add unit tests and restructure the class to be testable. #
Total comments: 7
Patch Set 4 : rebase onto latest master #Patch Set 5 : Address Min's comments #
Total comments: 6
Patch Set 6 : Nits and findbugs warnings #Patch Set 7 : Add equals/hashcode to make findbugs happy #Patch Set 8 : Rebase onto latest master #Patch Set 9 : No more findbugs warnings. #
Messages
Total messages: 34 (0 generated)
https://codereview.chromium.org/23684004/diff/3001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/23684004/diff/3001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:123: // Audio should have a duration, but no width or height; How do we distinguish between a valid audio and an invalid video stream that doesn't have height and width but have duration? Are we supposed to report an error later in the process?
https://codereview.chromium.org/23684004/diff/3001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/23684004/diff/3001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:45: // then we must return positive infinity. the spec mentioned the duration should be NaN https://codereview.chromium.org/23684004/diff/3001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:134: if (videoWidth != null && videoHeight != null) { use retriever.extractMetadata(METADATA_KEY_HAS_VIDEO) to determine if this is really a video
https://codereview.chromium.org/23684004/diff/3001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/23684004/diff/3001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:45: // then we must return positive infinity. On 2013/08/28 15:56:06, qinmin wrote: > the spec mentioned the duration should be NaN Sorry. If we don't know then it is NaN. If it is unbounded (not sure how to check that), then positive infinity. Quoth the spec: media. duration Returns the length of the media resource, in seconds, assuming that the start of the media resource is at time zero. Returns NaN if the duration isn't available. Returns Infinity for unbounded streams. https://codereview.chromium.org/23684004/diff/3001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:123: // Audio should have a duration, but no width or height; On 2013/08/28 15:11:38, whywhat wrote: > How do we distinguish between a valid audio and an invalid video stream that > doesn't have height and width but have duration? Are we supposed to report an > error later in the process? Qinmin's comment below should be the answer, though I'm not sure why we didn't do this to begin with: we can check if this is a video with METADATA_KEY_HAS_VIDEO, apparently.
New patchset adds 27 unit tests and restructures the class so that it is testable. Unit tests revealed a security hole in the use of File.getAbsolutePath() instead of File.getCanonicalPath(), which I have also fixed. This class should now make debugging media-related issues simpler as well, with increased debug logging.
-yfriedman (ooto), +joth
https://codereview.chromium.org/23684004/diff/13001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/23684004/diff/13001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:34: private static final MediaMetadata EMPTY_METADATA = new MediaMetadata(0,0,0,false); don't use static final on non-primitive objects. they hit performance https://codereview.chromium.org/23684004/diff/13001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:81: result = prime * result + mDurationInMilliseconds; why we need this hash function? mDurationInMilliseconds can be very big, and we can get int overflow below https://codereview.chromium.org/23684004/diff/13001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:224: boolean networkEnvironmentOk(Context context) { the functionName is very misleading. what about IsNetworkReliable
https://codereview.chromium.org/23684004/diff/13001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/23684004/diff/13001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:34: private static final MediaMetadata EMPTY_METADATA = new MediaMetadata(0,0,0,false); On 2013/08/30 16:53:34, qinmin wrote: > don't use static final on non-primitive objects. they hit performance I will make it non-static. https://codereview.chromium.org/23684004/diff/13001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:81: result = prime * result + mDurationInMilliseconds; On 2013/08/30 16:53:34, qinmin wrote: > why we need this hash function? mDurationInMilliseconds can be very big, and we > can get int overflow below 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. https://codereview.chromium.org/23684004/diff/13001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:224: boolean networkEnvironmentOk(Context context) { On 2013/08/30 16:53:34, qinmin wrote: > the functionName is very misleading. what about IsNetworkReliable 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!
https://codereview.chromium.org/23684004/diff/13001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/23684004/diff/13001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:224: boolean networkEnvironmentOk(Context context) { 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: > On 2013/08/30 16:53:34, qinmin wrote: > > the functionName is very misleading. what about IsNetworkReliable > > 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!
Min, PTAL and let me know if you're ready for me to submit this.
lgtm, please fix the findbugs warning
lgtm https://codereview.chromium.org/23684004/diff/28001/content/public/android/ja... File content/public/android/javatests/src/org/chromium/content/browser/MediaResourceGetterTest.java (right): https://codereview.chromium.org/23684004/diff/28001/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/MediaResourceGetterTest.java:67: Map<Integer, String> mMetadata = null; Nit: Use SparseArray https://codereview.chromium.org/23684004/diff/28001/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/MediaResourceGetterTest.java:169: assertEquals(sEmptyMetadata, new MediaMetadata(0,0,0,false)); nit: spaces in a bunch of places in the file after ","s https://codereview.chromium.org/23684004/diff/28001/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/MediaResourceGetterTest.java:345: assertEquals(TEST_FILE_PATH, mFakeMRG.mPath); // i.e., we configured successfully. Nit: use a failure message instead of a comment.
On 2013/09/11 17:20:00, qinmin wrote: > lgtm, please fix the findbugs warning The findbugs warnings: 1. Complaining about the hard-coded path. This seems legit, but the old code had the same problem and I don't want to introduce the possibility of a regression in this change. I'm going to file a crbug to use android.os.Environment.getExternalStorageDirectory(): http://developer.android.com/reference/android/os/Environment.html 2. Complaining that we don't override File.equals() in the totally-baked File subclass in the test code. This is a non-issue. I am going to run findbugs --rebaseline and simply exclude this one. Implementing equals() here is pointless. Let me just address Yaron's nits below and we'll get this rolling.
I've let this languish for a long time, but the other day I picked up my phone and saw a broken time number and remembered this bug. It's time to put this to bed. I've just rebased and will update the tests, fix the nits, and get this done.
Rebased onto master, absorbed the fix for the broken handset special-case, absorbed the User Agent change, modified unit tests accordingly. All passing again, all good. Now, on to the previous nits.
Nits killed. https://codereview.chromium.org/23684004/diff/28001/content/public/android/ja... File content/public/android/javatests/src/org/chromium/content/browser/MediaResourceGetterTest.java (right): https://codereview.chromium.org/23684004/diff/28001/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/MediaResourceGetterTest.java:67: Map<Integer, String> mMetadata = null; On 2013/09/12 02:10:59, Yaron wrote: > Nit: Use SparseArray Done. https://codereview.chromium.org/23684004/diff/28001/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/MediaResourceGetterTest.java:169: assertEquals(sEmptyMetadata, new MediaMetadata(0,0,0,false)); On 2013/09/12 02:10:59, Yaron wrote: > nit: spaces in a bunch of places in the file after ","s Spaces of doom eliminated. https://codereview.chromium.org/23684004/diff/28001/content/public/android/ja... content/public/android/javatests/src/org/chromium/content/browser/MediaResourceGetterTest.java:345: assertEquals(TEST_FILE_PATH, mFakeMRG.mPath); // i.e., we configured successfully. On 2013/09/12 02:10:59, Yaron wrote: > Nit: use a failure message instead of a comment. Done.
+jdduke as OWNER. Can you guys take another look? Sorry this has sat for so long. I think the use of paths in this code is ok, since we're explicitly ALSO checking getExternalStorageDirectory(); there shouldn't be any danger here, so I've added a findbugs exclusion. Let me know if there are any remaining issues, I believe this addresses everything. All tests updated and passing.
On 2014/03/27 16:15:49, Andrew Hayden wrote: > +jdduke as OWNER. Can you guys take another look? Sorry this has sat for so > long. > > I think the use of paths in this code is ok, since we're explicitly ALSO > checking getExternalStorageDirectory(); there shouldn't be any danger here, so > I've added a findbugs exclusion. Let me know if there are any remaining issues, > I believe this addresses everything. All tests updated and passing. I believe yfriedman@ is already an owner here, and certainly a better one than me for this code =/
Sorry, jdduke@: was going on "git cl" suggestions. I'll remove you.
Go, trybots, go!
Rebased onto latest master without any merge failures, kicking off trybots again. I believe this is good to go, PTAL.
lgtm sorry for delay
lgtm
The CQ bit was checked by andrewhayden@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/23684004/79001
You have nothing to apologize for, I'm the one that let this languish in the code review pile for 5 months. Bad on me. On 2 April 2014 17:36, <yfriedman@chromium.org> wrote: > lgtm > > sorry for delay > > https://codereview.chromium.org/23684004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by andrewhayden@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/23684004/79001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by andrewhayden@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/23684004/99001
Message was sent while issue was closed.
Change committed as 262766 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
