|
|
Created:
7 years, 2 months ago by Elly Fong-Jones Modified:
7 years, 2 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, cbentzel+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Visibility:
Public. |
Descriptionandroid: Use new proxy from PROXY_CHANGE intent
When we get a PROXY_CHANGE intent, the intent comes with the new proxy value;
use this value instead of ignoring it and asking the system property store for
the new proxy. This fixes a race condition where the system property store may
not have the new proxy by the time we receive the intent.
BUG=none yet
TEST=trybot
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230208
Patch Set 1 #
Total comments: 24
Patch Set 2 : Fixes for pliard #Patch Set 3 : Remove dead comment #
Total comments: 1
Patch Set 4 : Use ConvertJavaStringToUTF8 #
Total comments: 9
Patch Set 5 : Add bug number and test instructions #Patch Set 6 : Document arguments to ProxySettingsChanged #
Total comments: 1
Patch Set 7 : #Patch Set 8 : Fall back to old behavior #Patch Set 9 : Fall back to old mechanism #Patch Set 10 : Fall back to old mechanism #Patch Set 11 : Add findbugs entry for reflection goo #Patch Set 12 : Kill remaining findbugs warning #Patch Set 13 : Catch NPE from intent.getExtras() #
Messages
Total messages: 28 (0 generated)
I'm looking especially for advice on the findbugs warnings, since I can't think of how to avoid these. The fields it complains about are accessed from C via JNI.
You should get pliard to review this - he'll be far better than me. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:38: public String host_; Nit: Java uses mHost rather than host_; FindBugs may be complaining because these fields aren't private and get/set via accessor methods.
pliard, please take a look?
Thanks Elly! LGTM modulo one "major" comment and a few minor nits. I wasn't aware of the intent's parameters nor of the fact that the underlying class was private in the framework :/ https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:15: import java.util.Set; Nit: nor that one. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:33: private class ProxyConfig { Nit: you can make this 'private static class' since you never reference the outer ProxyChangeListener instance in this inner class. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:38: public String host_; On 2013/10/15 12:29:07, cbentzel wrote: > Nit: Java uses mHost rather than host_; > > FindBugs may be complaining because these fields aren't private and get/set via > accessor methods. +1 Also consider either making those fields 'final' or dropping the constructor (I would be more in favor of making them final personally). https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:82: Nit: other places in this file don't have this blank line. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:99: private ProxyConfig extractNewProxy(Intent intent) { Nit: you can make this method 'static' if you never reference 'this'. This will mainly improve readability since there is no way in Java to enforce immutability of the instance (other than making the method 'static' which seems to be possible here). https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:101: final String kClassName = "android.net.ProxyProperties"; Nit: I believe we use capital letters for constants. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:109: Method gethost = cls.getDeclaredMethod(kGetHostName); Nit: s/gethost/getHostMethod maybe (same below). https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:112: Object host_obj = gethost.invoke(props); Nit: feel free to drop these two lines (that shouldn't use '_' for identifiers) and directly write below: String host = (String) getHostMethod.invoke(props); int port = (Integer) getPortMethod.invoke(props); return new ProxyConfig(host, port); https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:163: ProxyConfig cfg); We only pass PODs or Java "primitive" types (e.g. String) to JNI. You can make this method take the host as a string and the port as an int. This will let you get rid of the reflection code in the .cc file (that we heavily discourage). This will also help you get rid of the findbugs warnings :) https://codereview.chromium.org/26763005/diff/1/net/proxy/proxy_config_servic... File net/proxy/proxy_config_service_android.cc (right): https://codereview.chromium.org/26763005/diff/1/net/proxy/proxy_config_servic... net/proxy/proxy_config_service_android.cc:163: void CreateStaticProxyConfig(ProxyConfig* config, const std::string& host, Nit: |config| should go last since it's an output parameter. https://codereview.chromium.org/26763005/diff/1/net/proxy/proxy_config_servic... net/proxy/proxy_config_service_android.cc:272: virtual void ProxySettingsChanged(JNIEnv* env, jobject jself, See my comment in the Java file (TL;DR: please directly pass the string and the int in).
PTAL? https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:15: import java.util.Set; On 2013/10/15 14:33:21, Philippe wrote: > Nit: nor that one. Done. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:33: private class ProxyConfig { On 2013/10/15 14:33:21, Philippe wrote: > Nit: you can make this 'private static class' since you never reference the > outer ProxyChangeListener instance in this inner class. Done. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:38: public String host_; On 2013/10/15 14:33:21, Philippe wrote: > On 2013/10/15 12:29:07, cbentzel wrote: > > Nit: Java uses mHost rather than host_; > > > > FindBugs may be complaining because these fields aren't private and get/set > via > > accessor methods. > > +1 > > Also consider either making those fields 'final' or dropping the constructor (I > would be more in favor of making them final personally). Done. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:82: On 2013/10/15 14:33:21, Philippe wrote: > Nit: other places in this file don't have this blank line. Done. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:99: private ProxyConfig extractNewProxy(Intent intent) { On 2013/10/15 14:33:21, Philippe wrote: > Nit: you can make this method 'static' if you never reference 'this'. This will > mainly improve readability since there is no way in Java to enforce immutability > of the instance (other than making the method 'static' which seems to be > possible here). Alas: ../net/android/java/src/org/chromium/net/ProxyChangeListener.java:96: inner classes cannot have static declarations https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:101: final String kClassName = "android.net.ProxyProperties"; On 2013/10/15 14:33:21, Philippe wrote: > Nit: I believe we use capital letters for constants. Done. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:109: Method gethost = cls.getDeclaredMethod(kGetHostName); On 2013/10/15 14:33:21, Philippe wrote: > Nit: s/gethost/getHostMethod maybe (same below). Done. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:112: Object host_obj = gethost.invoke(props); On 2013/10/15 14:33:21, Philippe wrote: > Nit: feel free to drop these two lines (that shouldn't use '_' for identifiers) > and directly write below: > > String host = (String) getHostMethod.invoke(props); > int port = (Integer) getPortMethod.invoke(props); > return new ProxyConfig(host, port); Done. https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:163: ProxyConfig cfg); On 2013/10/15 14:33:21, Philippe wrote: > We only pass PODs or Java "primitive" types (e.g. String) to JNI. You can make > this method take the host as a string and the port as an int. This will let you > get rid of the reflection code in the .cc file (that we heavily discourage). > This will also help you get rid of the findbugs warnings :) Done. https://codereview.chromium.org/26763005/diff/1/net/proxy/proxy_config_servic... File net/proxy/proxy_config_service_android.cc (right): https://codereview.chromium.org/26763005/diff/1/net/proxy/proxy_config_servic... net/proxy/proxy_config_service_android.cc:163: void CreateStaticProxyConfig(ProxyConfig* config, const std::string& host, On 2013/10/15 14:33:21, Philippe wrote: > Nit: |config| should go last since it's an output parameter. Done. https://codereview.chromium.org/26763005/diff/1/net/proxy/proxy_config_servic... net/proxy/proxy_config_service_android.cc:272: virtual void ProxySettingsChanged(JNIEnv* env, jobject jself, On 2013/10/15 14:33:21, Philippe wrote: > See my comment in the Java file (TL;DR: please directly pass the string and the > int in). Done.
Still LGTM, thanks Elly! https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/1/net/android/java/src/org/chro... net/android/java/src/org/chromium/net/ProxyChangeListener.java:99: private ProxyConfig extractNewProxy(Intent intent) { On 2013/10/15 16:37:15, Elly Jones wrote: > On 2013/10/15 14:33:21, Philippe wrote: > > Nit: you can make this method 'static' if you never reference 'this'. This > will > > mainly improve readability since there is no way in Java to enforce > immutability > > of the instance (other than making the method 'static' which seems to be > > possible here). > > Alas: > > ../net/android/java/src/org/chromium/net/ProxyChangeListener.java:96: inner > classes cannot have static declarations Wow, didn't know about that! https://codereview.chromium.org/26763005/diff/13001/net/proxy/proxy_config_se... File net/proxy/proxy_config_service_android.cc (right): https://codereview.chromium.org/26763005/diff/13001/net/proxy/proxy_config_se... net/proxy/proxy_config_service_android.cc:275: env->ReleaseStringUTFChars(jhost, chars); Sorry for not pointing you to that earlier, but you can remove a few lines here by using ConvertJavaStringToUTF8() from base/android/jni_string.h.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/22001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Please add a bug number.
https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/... net/android/java/src/org/chromium/net/ProxyChangeListener.java:83: ProxyConfig cfg = extractNewProxy(intent); Do older versions of Android set a "proxy" object in the intent? If not, we will miss signals about the proxy changing. https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/... net/android/java/src/org/chromium/net/ProxyChangeListener.java:96: private ProxyConfig extractNewProxy(Intent intent) { How hard would it be to test this? https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/... net/android/java/src/org/chromium/net/ProxyChangeListener.java:109: String host = (String)getHostMethod.invoke(props); Stupid java question: what happens if the getHost method exists but returns a non-String object? Will this raise an exception for the downcast? https://codereview.chromium.org/26763005/diff/22001/net/proxy/proxy_config_se... File net/proxy/proxy_config_service_android.h (right): https://codereview.chromium.org/26763005/diff/22001/net/proxy/proxy_config_se... net/proxy/proxy_config_service_android.h:42: // changed. Can you document this? In particular, the semantics of "0" as a port number indicating no proxy are a bit non-obvious.
https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/... net/android/java/src/org/chromium/net/ProxyChangeListener.java:83: ProxyConfig cfg = extractNewProxy(intent); On 2013/10/15 18:59:42, cbentzel wrote: > Do older versions of Android set a "proxy" object in the intent? If not, we will > miss signals about the proxy changing. All versions ICS and later do. https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/... net/android/java/src/org/chromium/net/ProxyChangeListener.java:96: private ProxyConfig extractNewProxy(Intent intent) { On 2013/10/15 18:59:42, cbentzel wrote: > How hard would it be to test this? Not difficult per se, just ugly. It would basically involve abusing Java reflection again to construct an object that looks like an Intent with the right property on it. Unfortunately my understanding of Java reflection is sketchy enough that doing that wouldn't add much confidence. https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/... net/android/java/src/org/chromium/net/ProxyChangeListener.java:109: String host = (String)getHostMethod.invoke(props); On 2013/10/15 18:59:42, cbentzel wrote: > Stupid java question: what happens if the getHost method exists but returns a > non-String object? Will this raise an exception for the downcast? I don't know. pliard? https://codereview.chromium.org/26763005/diff/22001/net/proxy/proxy_config_se... File net/proxy/proxy_config_service_android.h (right): https://codereview.chromium.org/26763005/diff/22001/net/proxy/proxy_config_se... net/proxy/proxy_config_service_android.h:42: // changed. On 2013/10/15 18:59:42, cbentzel wrote: > Can you document this? In particular, the semantics of "0" as a port number > indicating no proxy are a bit non-obvious. Done.
https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/22001/net/android/java/src/org/... net/android/java/src/org/chromium/net/ProxyChangeListener.java:109: String host = (String)getHostMethod.invoke(props); On 2013/10/15 19:50:42, Elly Jones wrote: > On 2013/10/15 18:59:42, cbentzel wrote: > > Stupid java question: what happens if the getHost method exists but returns a > > non-String object? Will this raise an exception for the downcast? > > I don't know. pliard? This would indeed throw an InvalidCastException (that would be caught below).
https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/... net/android/java/src/org/chromium/net/ProxyChangeListener.java:84: if (cfg != null) { I wonder if we should be safe and create a fake ProxyConfig (with empty string and 0 port) in the case of a null cfg from extractNewProxy. This will be consistent with the old behavior, but if we are able to extract details of the config from the intent we will use that instead.
On 2013/10/16 09:49:23, cbentzel wrote: > https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/... > File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): > > https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/... > net/android/java/src/org/chromium/net/ProxyChangeListener.java:84: if (cfg != > null) { > I wonder if we should be safe and create a fake ProxyConfig (with empty string > and 0 port) in the case of a null cfg from extractNewProxy. This will be > consistent with the old behavior, but if we are able to extract details of the > config from the intent we will use that instead. This actually isn't right, though - if we create a port-0 ProxyConfig, that signifies "direct", which we didn't get from the ProxyConfig. In the case where we can't we should really fall back to the old "ask the system" logic, but that's established to be racy, so... not sure what to do. Maybe "direct" is the right answer after all.
On 2013/10/16 15:24:40, Elly Jones wrote: > On 2013/10/16 09:49:23, cbentzel wrote: > > > https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/... > > File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): > > > > > https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/... > > net/android/java/src/org/chromium/net/ProxyChangeListener.java:84: if (cfg != > > null) { > > I wonder if we should be safe and create a fake ProxyConfig (with empty string > > and 0 port) in the case of a null cfg from extractNewProxy. This will be > > consistent with the old behavior, but if we are able to extract details of the > > config from the intent we will use that instead. > > This actually isn't right, though - if we create a port-0 ProxyConfig, that > signifies "direct", which we didn't get from the ProxyConfig. In the case where > we can't we should really fall back to the old "ask the system" logic, but > that's established to be racy, so... not sure what to do. Maybe "direct" is the > right answer after all. You are right. I think we may want to fall back to Ask the system (the old behavior, which worked most of the time...)
On 2013/10/16 16:03:47, cbentzel wrote: > On 2013/10/16 15:24:40, Elly Jones wrote: > > On 2013/10/16 09:49:23, cbentzel wrote: > > > > > > https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/... > > > File net/android/java/src/org/chromium/net/ProxyChangeListener.java (right): > > > > > > > > > https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/... > > > net/android/java/src/org/chromium/net/ProxyChangeListener.java:84: if (cfg > != > > > null) { > > > I wonder if we should be safe and create a fake ProxyConfig (with empty > string > > > and 0 port) in the case of a null cfg from extractNewProxy. This will be > > > consistent with the old behavior, but if we are able to extract details of > the > > > config from the intent we will use that instead. > > > > This actually isn't right, though - if we create a port-0 ProxyConfig, that > > signifies "direct", which we didn't get from the ProxyConfig. In the case > where > > we can't we should really fall back to the old "ask the system" logic, but > > that's established to be racy, so... not sure what to do. Maybe "direct" is > the > > right answer after all. > > You are right. I think we may want to fall back to Ask the system (the old > behavior, which worked most of the time...) Uhoh. I uploaded a broken version of my CL by accident. Fixing...
On 2013/10/16 20:20:10, Elly Jones wrote: > On 2013/10/16 16:03:47, cbentzel wrote: > > On 2013/10/16 15:24:40, Elly Jones wrote: > > > On 2013/10/16 09:49:23, cbentzel wrote: > > > > > > > > > > https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/... > > > > File net/android/java/src/org/chromium/net/ProxyChangeListener.java > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/26763005/diff/35001/net/android/java/src/org/... > > > > net/android/java/src/org/chromium/net/ProxyChangeListener.java:84: if (cfg > > != > > > > null) { > > > > I wonder if we should be safe and create a fake ProxyConfig (with empty > > string > > > > and 0 port) in the case of a null cfg from extractNewProxy. This will be > > > > consistent with the old behavior, but if we are able to extract details of > > the > > > > config from the intent we will use that instead. > > > > > > This actually isn't right, though - if we create a port-0 ProxyConfig, that > > > signifies "direct", which we didn't get from the ProxyConfig. In the case > > where > > > we can't we should really fall back to the old "ask the system" logic, but > > > that's established to be racy, so... not sure what to do. Maybe "direct" is > > the > > > right answer after all. > > > > You are right. I think we may want to fall back to Ask the system (the old > > behavior, which worked most of the time...) > > Uhoh. I uploaded a broken version of my CL by accident. Fixing... There, sorry. PTAL?
LGTM I get something about "old chunk failed" when doing diffs but perhaps that's a rietveld-only issue.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/56001
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/86001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/86002
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/86002
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/26763005/262001
Message was sent while issue was closed.
Change committed as 230208 |