Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
289653002:
Skip network availabity check for localhost in MediaResourceGetter. (Closed)
|
Created:
6 years, 7 months ago by ycheo (away) Modified:
6 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, boliu Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSkip network availabity check for localhost in MediaResourceGetter.
This causes the test ExternalVideoSurfaceContainerTest#testEnableVideoOverlayForEmbeddedVideo
to fail if the target device is in airplane mode.
BUG=372174
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270921
Patch Set 1 #
Total comments: 2
Patch Set 2 : addressed Andrew's comment. #
Total comments: 6
Patch Set 3 : Added the tests. #
Messages
Total messages: 18 (0 generated)
Thanks for fixing this! Please add test cases for 127.0.0.1, localhost and ::1 to MediaResourceGetterTest to ensure that things are going to work properly. Let's make sure this doesn't creep back into a later regression. https://codereview.chromium.org/289653002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/289653002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:272: return InetAddress.getByName(host).isLoopbackAddress(); I'm slightly concerned about the possibility of a network lookup hidden in this code. We are trying to avoid the possibility of accessing the network if it is unreliable, yet we might potentially incur a network hit to resolve the name. I traced around code in the OpenJDK for quite a while, and it's surprisingly complex in the case of "localhost" versus "127.0.0.1". We might actually be better off doing a stupid check here to see if the host just equals one of those two strings. This would probably catch >99% of all use cases correctly, and would not require a network lookup under any conditions. I'm all for using the Java APIs, but I don't see either of these conventions changing. To be forward looking, also allow "::1", which is apparently the IPv6 equivalent to IPv4's localhost.
https://codereview.chromium.org/289653002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/289653002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:272: return InetAddress.getByName(host).isLoopbackAddress(); On 2014/05/14 12:08:04, Andrew Hayden wrote: > I'm slightly concerned about the possibility of a network lookup hidden in this > code. We are trying to avoid the possibility of accessing the network if it is > unreliable, yet we might potentially incur a network hit to resolve the name. I > traced around code in the OpenJDK for quite a while, and it's surprisingly > complex in the case of "localhost" versus "127.0.0.1". > > We might actually be better off doing a stupid check here to see if the host > just equals one of those two strings. This would probably catch >99% of all use > cases correctly, and would not require a network lookup under any conditions. > I'm all for using the Java APIs, but I don't see either of these conventions > changing. To be forward looking, also allow "::1", which is apparently the IPv6 > equivalent to IPv4's localhost. I had the same concern if I'm overengineering. According to the javadoc of InetAddress, InetAddress has DNS caching. So I guess the performance difference can be not significant, because the host name should be resolved anyway later in the android MediaPlayer. Anyway, I'm fine with either way.
On 2014/05/14 12:36:13, Yuncheol Heo wrote: > https://codereview.chromium.org/289653002/diff/1/content/public/android/java/... > File > content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java > (right): > > https://codereview.chromium.org/289653002/diff/1/content/public/android/java/... > content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:272: > return InetAddress.getByName(host).isLoopbackAddress(); > On 2014/05/14 12:08:04, Andrew Hayden wrote: > > I'm slightly concerned about the possibility of a network lookup hidden in > this > > code. We are trying to avoid the possibility of accessing the network if it is > > unreliable, yet we might potentially incur a network hit to resolve the name. > I > > traced around code in the OpenJDK for quite a while, and it's surprisingly > > complex in the case of "localhost" versus "127.0.0.1". > > > > We might actually be better off doing a stupid check here to see if the host > > just equals one of those two strings. This would probably catch >99% of all > use > > cases correctly, and would not require a network lookup under any conditions. > > I'm all for using the Java APIs, but I don't see either of these conventions > > changing. To be forward looking, also allow "::1", which is apparently the > IPv6 > > equivalent to IPv4's localhost. > > I had the same concern if I'm overengineering. > According to the javadoc of InetAddress, InetAddress has DNS caching. > So I guess the performance difference can be not significant, because the host > name should be resolved anyway later in the android MediaPlayer. > > Anyway, I'm fine with either way. I found that the trybot tests are failed. I guess the target device on the bot has no network and so the hostname resolving is failed. Your suggestion doesn't have this problem. So I'll go with your suggestion.
> I found that the trybot tests are failed. > I guess the target device on the bot has no network and so the hostname > resolving is failed. > Your suggestion doesn't have this problem. So I'll go with your suggestion. Sounds good. It's actually kind of cool that you found that; perhaps in the future, airplane mode will do something similar to the eth/wlan devices on Android. So it's reasonable to assume that our attempt might insta-fail in the same way.
https://codereview.chromium.org/289653002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/289653002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:267: return host != null && (host.equals("localhost") // typical hostname && has higher operator precendence than ||. So after the first line is evaluated, it will continue to evaluate the remainder of this statement. If host == null, doesn't that cause an NPE?
The change looks good, but we should be updating the unit tests before we check this in. https://codereview.chromium.org/289653002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/289653002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:265: // not to break the testing in the device without the network I/F. I think you can legitimately state that this solves all known use cases without having to worry about whether or not a DNS lookup could result from parsing the hostname. It's not JUST for testing the device, it makes sense in general. https://codereview.chromium.org/289653002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:267: return host != null && (host.equals("localhost") // typical hostname On 2014/05/14 16:27:08, qinmin wrote: > && has higher operator precendence than ||. So after the first line is > evaluated, it will continue to evaluate the remainder of this statement. > If host == null, doesn't that cause an NPE? He's parenthesized the entire or-clause, so this should be fine. If host == null, the if- statement will short circuit. https://codereview.chromium.org/289653002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:267: return host != null && (host.equals("localhost") // typical hostname For pedantic correctness, you should normalize the host string first, e.g.: host = host.toLowerCase()
https://codereview.chromium.org/289653002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/289653002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:267: return host != null && (host.equals("localhost") // typical hostname ah...I missed the "(" before host on the first line. On 2014/05/14 16:32:49, Andrew Hayden wrote: > On 2014/05/14 16:27:08, qinmin wrote: > > && has higher operator precendence than ||. So after the first line is > > evaluated, it will continue to evaluate the remainder of this statement. > > If host == null, doesn't that cause an NPE? > > He's parenthesized the entire or-clause, so this should be fine. If host == > null, the if- statement will short circuit.
I Changed android.net.Uri to java.net.URI, because the android api has a bug to handle in ipv6 square form: filed the bug, http://b/14976593
LGTM, and thanks again for this fix!
https://codereview.chromium.org/289653002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java (right): https://codereview.chromium.org/289653002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/MediaResourceGetter.java:265: // not to break the testing in the device without the network I/F. On 2014/05/14 16:32:49, Andrew Hayden wrote: > I think you can legitimately state that this solves all known use cases without > having to worry about whether or not a DNS lookup could result from parsing the > hostname. It's not JUST for testing the device, it makes sense in general. Done.
@qinmin, PTAL.
lgtm
@yfriedman, Could you approve this?
lgtm
The CQ bit was checked by ycheo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ycheo@chromium.org/289653002/40001
Message was sent while issue was closed.
Change committed as 270921 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
