Issue 10206014: Upstream Android proxy config service.

Philippe

Note that the original author is John Knottenbelt (CCed).

8 years, 8 months ago (2012-04-24 12:45:27 UTC) #1

Ryan Sleevi

First stab at review. I think there's some subtle threading issues, so I wanted to ...

8 years, 8 months ago (2012-04-24 18:20:04 UTC) #2

Philippe

Thanks Ryan for this first round of comments. I will have a deeper look at ...

8 years, 8 months ago (2012-04-25 15:39:08 UTC) #3

Yaron

https://chromiumcodereview.appspot.com/10206014/diff/4002/net/android/network_library.cc File net/android/network_library.cc (right): https://chromiumcodereview.appspot.com/10206014/diff/4002/net/android/network_library.cc#newcode87 net/android/network_library.cc:87: return RegisterNativesImpl(env) && drive-by: This doesn't belong and we ...

8 years, 8 months ago (2012-04-27 20:06:06 UTC) #4

Ryan Sleevi

I'll need to take another look at the threading issues later today, but since I ...

8 years, 7 months ago (2012-05-07 17:12:20 UTC) #5

I'll need to take another look at the threading issues later today, but since I
see you pushed a new version...

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:38: return
ProxyServer::FromURI(proxy_host + ":" + proxy_port, scheme);
What happens if |proxy_host| is an IPv6 host (a:b)?

net::HostPortPair::ToString() handles this, so you should instead pass around
one of those.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:42: const std::string& prefix,
Is it valid for prefix to be .empty()?

DCHECK for programmer error?

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:47: const std::string proxy_port =
delegate->GetProperty(prefix + ".proxyPort");
I don't believe I've seen in Chromium code the optimistic const-ing here. I
don't believe the style guide prohibits it, but as far as local style goes, it's
very rare to see this.

I'm guessing you're doing this simply for local documentation sake, as the
compiler should be able to optimize fine regardless of the annotation?

I don't feel strongly about this, but I did want to point it out that it's
uncommon.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:53: const std::string proxy_port =
delegate->GetProperty("proxyPort");
You check !proxy_host.empty(), but never check proxy_port.empty(). And then in
ConstructProxyServer, you always concat the two.

So if a proxy server was 'foo' with a port of '' for a scheme of http, your
returned setup would be 'HTTP foo:', which is a bit of an odd construction.

Same for all other occurrences of this pattern.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:91:
bypass_rules->AddRuleForHostname(scheme, pattern, -1);
This method returns bool. As it stands, you'll simply drop that host - eg:
*.foo|something_invalid|*.bar becomes "*.foo" and "*.bar" in the bypass rules

Is this what you want? Should you make AddBypassRules return a bool so that
GetProxyRules will also fail?

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:203: DCHECK(OnObserverThread());
I think it's still very subtle/unexpected that this can be created on thread A,
B, or C but must always be destroyed on thread X.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:34: virtual std::string
GetProperty(const std::string& property) = 0;
nit: Document what these methods do.

For example, with Start(), you're passing around a naked pointer. Is it an
ownership transferred pointer? It's not clear from just the header.

Similar, with GetProperty, if a property does not exist, what does it return? An
empty string? Is it valid for a property to be present but have an empty string?
If so, should they be distinguished?

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:38:
scoped_refptr<base::MessageLoopProxy> observer_loop);
Instead of using a MessageLoopProxy, I believe the new hotness is to pass around
a base::TaskRunner, of which a MLP derives from.

Also, this is a by-value copy, whereas given that it's ref-counted, it makes
more sense to either pass as a const-ref or a naked pointer. AIUI, only the
non-refcounted scoped classes (scoped_ptr/ptr_malloc/_array) support the C++11
move emulation.

Same with line 60

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:75: const scoped_refptr<SharedState>
shared_state_;
This means the scoped_refptr itself is const, but the SharedState is not.

Did you mean scoped_refptr<const SharedState> (which I don't know if it will
even work)

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:79: const
scoped_refptr<base::MessageLoopProxy> observer_loop_;
Drop the const?

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android_unittest.cc (right):

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:118: base::Thread::Stop();
This means you're calling a virtual function (CleanUp) from a destructor
(~TestObserverThread)

Here be dragons

Yaron

http://codereview.chromium.org/10206014/diff/30002/net/android/network_library.cc File net/android/network_library.cc (right): http://codereview.chromium.org/10206014/diff/30002/net/android/network_library.cc#newcode88 net/android/network_library.cc:88: ProxyConfigServiceAndroid::Init(env); So instead of doing this, you can add ...

8 years, 7 months ago (2012-05-07 19:15:30 UTC) #6

John Knottenbelt

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_service_android.cc File net/proxy/proxy_config_service_android.cc (right): http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_service_android.cc#newcode53 net/proxy/proxy_config_service_android.cc:53: const std::string proxy_port = delegate->GetProperty("proxyPort"); ConstructProxyServer checks for empty ...

8 years, 7 months ago (2012-05-08 11:43:12 UTC) #7

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:53: const std::string proxy_port =
delegate->GetProperty("proxyPort");
ConstructProxyServer checks for empty port to avoid the trailing ':'

On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> You check !proxy_host.empty(), but never check proxy_port.empty(). And then in
> ConstructProxyServer, you always concat the two.
> 
> So if a proxy server was 'foo' with a port of '' for a scheme of http, your
> returned setup would be 'HTTP foo:', which is a bit of an odd construction.
> 
> Same for all other occurrences of this pattern.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:91:
bypass_rules->AddRuleForHostname(scheme, pattern, -1);
The default java.net.ProxySelector (
eloper.android.com/reference/java/net/ProxySelector.html ) implementation in
Android is tolerant to invalid parts, so I think we should try to be too.

On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> This method returns bool. As it stands, you'll simply drop that host - eg:
> *.foo|something_invalid|*.bar becomes "*.foo" and "*.bar" in the bypass rules
> 
> Is this what you want? Should you make AddBypassRules return a bool so that
> GetProxyRules will also fail?

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:203: DCHECK(OnObserverThread());
I think it should be OK to relax this requirement now that the SharedState
struct has been introduced. We can just schedule a call to the stop method on
the observer thread if we're not already on it.

If there are still observers there would then be a possibility that observers
can get notifications after this object is destroyed. 

Ryan can you confirm if is an invariant that all observers are removed by the
time the Proxy Service object is destroyed? If so, we could add a DCHECK to that
effect in the method that runs on the observer thread.

On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> I think it's still very subtle/unexpected that this can be created on thread
A,
> B, or C but must always be destroyed on thread X.

Philippe

I'm publishing my comments for both the first round of comments and the recent one. ...

8 years, 7 months ago (2012-05-09 11:48:39 UTC) #8

I'm publishing my comments for both the first round of comments and the recent
one.

I initially uploaded a patch set so that John can take a look at the changes
before I publish my comments. I was hoping that Ryan didn't see the new changes
before I publish them. But it didn't work as expected :)

http://codereview.chromium.org/10206014/diff/4002/base/android/jni_generator/...
File base/android/jni_generator/jni_generator.py (right):

http://codereview.chromium.org/10206014/diff/4002/base/android/jni_generator/...
base/android/jni_generator/jni_generator.py:181:
'Lorg/chromium/chromeview/net/ProxyChangeListener',
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> sort

It is not obvious here, but it is already sorted.

http://codereview.chromium.org/10206014/diff/4002/net/android/java/org/chromi...
File net/android/java/org/chromium/net/ProxyChangeListener.java (right):

http://codereview.chromium.org/10206014/diff/4002/net/android/java/org/chromi...
net/android/java/org/chromium/net/ProxyChangeListener.java:35: assert mNativePtr
== 0;
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> assert() ?

In Java assert is a primary expression therefore we don't need parenthesis.

However, assertions in Java are disabled by default and very rarely used. I
didn't find any occurrence of '-ea' in build files to enable them.
I'm leaving that here in case we enable them later in debug builds.

http://codereview.chromium.org/10206014/diff/4002/net/android/java/org/chromi...
net/android/java/org/chromium/net/ProxyChangeListener.java:60: // that the C++
part will need to run the callbacks on the IO thread.
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> It's a layering violation to discuss /the/ UI thread and /the/ IO thread in
> net/.
> 
> // Note that this code currently runs on a MESSAGE_LOOP_UI
> // thread, but the C++ code must run the callbacks on the
> // network thread.

Done.

http://codereview.chromium.org/10206014/diff/4002/net/android/java/org/chromi...
net/android/java/org/chromium/net/ProxyChangeListener.java:65: * See
chrome/net/proxy/proxy_config_service_android.cc
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: chrome/ is cool and all, but you know what would be even cooler?
> 
> drop the chrome/

Done.

http://codereview.chromium.org/10206014/diff/4002/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/10206014/diff/4002/net/net.gyp#newcode1191
net/net.gyp:1191: 'proxy/proxy_config_service_android_unittest.cc',
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: sort

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:30: // This class implements the link
to Android's proxy broadcast mechanism.
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: Line break between 29 & 30

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:33: DelegateImpl();
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: Declare a destructor, add a line break between 33 & 34

Declaring the destructor here won't serve us in this case since we are in a cc
file (unless I'm missing something). You might have thought that this was in a
header since the implementation was not inlined (although nothing prevented it
from being). To avoid any confusion I have now inlined the implementation.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:83: }  // namespace
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: Line break between 82 & 83

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:97: DCHECK(OnObserverThread());
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> This is very surprising, and I think dangerously subtle.
> 
> Instead of matching "Create on thread X, delete on thread X", it's "Create on
> thread Y, call AddObserver on thread X, delete on thread X"

I agree. Unfortunately I can't change that.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:129: namespace {
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> This is very weird to read that you jump from an unnamed namespace into net
> (line 83) and then back into an unnamed namespace. Can you move these methods
> up?

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:132: const std::string& proxyHost,
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> Style: Please go through these methods and make sure they match Chromium's C++
> style (proxy_host / proxy_port) rather than Java naming style (proxyHost /
> proxyPort)

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:252: // Should be called from IO
thread.
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> network thread

I removed NotifyTask.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:261: // Should be called from IO
thread.
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> This doesn't seem to be met - ProxyConfigServiceAndroid::CancelNotifyTask is
> called in the destructor, which does DCHECK(OnObserverThread())

I removed NotifyTask.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:284:
observer_loop_->PostTask(FROM_HERE,
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> Possible crash here, AFAICT
> 
> Two threads: T1, T2
> T1 is in ProxySettingsChanged
> T2 is in ~ProxyConfigServiceAndroid()
> 
> T1 wakes, runs, gets lock (line 278)
> T1 creates task, releases lock (line 283), OS puts to sleep
> T2 wakes, runs, begins tearing down. Calls to CancelNotifyTask (line 102).
Gets
> lock, tears down, object is now destroyed.
> T1 wakes, runs line 284, |observer_loop_| is now destroyed, mayhem ensues.
> 
> Having the lock cover line 284 would ensure that the OS puts it to sleep (line
> 283), the lock is still held, so T1 will block destruction in
CancelNotifyTask,
> since it's trying to grab the lock (305)
> 
> However, I think there are many a subtle threading semantic here that I think
> can be largely simplified.

This code is now much simpler.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:316: return !observer_loop_ ||
observer_loop_->BelongsToCurrentThread();
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> This is equally subtle and surprising.
> 
> I think these comments belong in the header in some form (here and line
> 320-322), but I think it's very non-intuitive that OnObserverThread == true
when
> there is no ObserverThread

Since the message loop proxy is now injected in the class' constructor we don't
have these subtleties anymore.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:322: // thread, will see the value of
observer_loop_.
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> Lock? I don't see any lock.

Indeed. I removed this method since the observer loop is injected in the class'
constructor.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.cc:326: DCHECK(OnObserverThread());
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> It seems like this could be written:
> if (!observer_loop_)
>   observer_loop_ = base::MessageLoopProxy::current();
> DCHECK(observer_loop_->BelongsToCurrentThread())

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.h:11: #include
"base/memory/ref_counted.h"
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: Make sure the headers are sorted

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.h:19: #include <string>
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: C++ system headers should appear before any Chromium headers

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.h:40: ProxyConfigServiceAndroid(Delegate*
delegate);  // For tests; takes ownership.
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: explicit ProxyConfigServiceAndroid(Delegate* delegate)
> 
> And if it's just for tests, make it private and use either friend or
> FRIEND_TEST_ALL_PREFIXES to really make clear it's just for testing.

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.h:52: // changed. AddObserver() must have
been called at least once before.
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> I must admit, I'm not sure your threading guarantee - does this mean it can be
> called from worker threads?
> 
> Should this be "on the network thread" ?

This method is actually called from the Java side which runs on the UI thread. I
removed this comment since the one below already provides enough information.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.h:71: 
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: Delete extra line

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.h:73: NotifyTask* notify_task_;  //
Accessed from any thread, guarded by lock_.
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> Using Locks, particularly for threading, is generally seen as an anti-pattern
> within the code.
> 
> Are you just trying to do ObserverListThreadSafe ?
> 

NotifyTask was mainly used to handle the case where AndroidProxyConfigService
gets deleted before the callback runs on the network thread.

We removed that and used a shared pointer instead.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android.h:78: // methods should be called on this
thread.
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> This behaviour strikes me as a little surprising, in that it's one of those
> things where it can be subtle to figure out what the 'true' thread this
> observer_loop_ refers to (eg: it's dependent on whether or not someone 'else'
> has called AddObserver)
> 
> Can you make this explicit from the ctor, rather than implicit?

The message loop proxy is now injected through the constructor.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
File net/proxy/proxy_config_service_android_unittest.cc (right):

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android_unittest.cc:35: base::Lock lock_;
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> See previous comments about locks and double check this is correct

We simplified significantly ProxyConfigServiceAndroid. However I think we might
have to keep this lock since the two threads perform read/write operations on
this object concurrently.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android_unittest.cc:42: }
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: One line
> 
> TestDelegate::TestDelegate() : service_(NULL) {}

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android_unittest.cc:48: void TestDelegate::Stop()
{
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: one line

Done.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android_unittest.cc:71: // Construct this on the
UI thread.
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> See previous comments about "UI thread" references

We don't mention network nor UI anymore.

http://codereview.chromium.org/10206014/diff/4002/net/proxy/proxy_config_serv...
net/proxy/proxy_config_service_android_unittest.cc:206: static std::string
ToString(ProxyInfo& proxy_info) {
On 2012/04/24 18:20:04, Ryan Sleevi wrote:
> nit: Use ProxyInfo::ToPacString() instead?

The code using ToString() is generated from a Python script which also generates
test cases for Java. I don't think we have a ProxyInfo::ToPacString() equivalent
in Java.

http://codereview.chromium.org/10206014/diff/30002/net/android/network_librar...
File net/android/network_library.cc (right):

http://codereview.chromium.org/10206014/diff/30002/net/android/network_librar...
net/android/network_library.cc:88: ProxyConfigServiceAndroid::Init(env);
On 2012/05/07 19:15:30, Yaron wrote:
> So instead of doing this, you can add your register to net_jni_registrar in
> http://codereview.chromium.org/10178023

I didn't forget you Yaron :) I uploaded a new patch set so that John (the
original author) can take a look at the changes before everyone gets notified
(after I publish my comments). But that didn't work as expected, Ryan saw the
changes before :)

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:38: return
ProxyServer::FromURI(proxy_host + ":" + proxy_port, scheme);
On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> What happens if |proxy_host| is an IPv6 host (a:b)?
> 
> net::HostPortPair::ToString() handles this, so you should instead pass around
> one of those.

Done.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:42: const std::string& prefix,
On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> Is it valid for prefix to be .empty()?
> 
> DCHECK for programmer error?

Done.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:47: const std::string proxy_port =
delegate->GetProperty(prefix + ".proxyPort");
On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> I don't believe I've seen in Chromium code the optimistic const-ing here. I
> don't believe the style guide prohibits it, but as far as local style goes,
it's
> very rare to see this.
> 
> I'm guessing you're doing this simply for local documentation sake, as the
> compiler should be able to optimize fine regardless of the annotation?
> 
> I don't feel strongly about this, but I did want to point it out that it's
> uncommon.

It's mainly for local documentation sake as you said. I think it lets the reader
more quickly catch the mutable state. It seems that it is more common indeed in
Google3 code than in Chromium. I removed the consts here (that I previously
added) as they don't add much value in this particular case (very restricted
scope). However I think that "const" really adds value when used to make
class/struct state immutable.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:203: DCHECK(OnObserverThread());
On 2012/05/08 11:43:12, John Knottenbelt wrote:
> I think it should be OK to relax this requirement now that the SharedState
> struct has been introduced. We can just schedule a call to the stop method on
> the observer thread if we're not already on it.
> 
> If there are still observers there would then be a possibility that observers
> can get notifications after this object is destroyed. 
> 
> Ryan can you confirm if is an invariant that all observers are removed by the
> time the Proxy Service object is destroyed? If so, we could add a DCHECK to
that
> effect in the method that runs on the observer thread.
> 
> 
> 
> On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> > I think it's still very subtle/unexpected that this can be created on thread
> A,
> > B, or C but must always be destroyed on thread X.
> 

I'm waiting for Ryan's feedback before I address this issue.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:34: virtual std::string
GetProperty(const std::string& property) = 0;
On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> nit: Document what these methods do.
> 
> For example, with Start(), you're passing around a naked pointer. Is it an
> ownership transferred pointer? It's not clear from just the header.
> 
> Similar, with GetProperty, if a property does not exist, what does it return?
An
> empty string? Is it valid for a property to be present but have an empty
string?
> If so, should they be distinguished?

Good points.

Currently, ProxyConfigServiceAndroid does not use this distinction. In this
particular case, I don't really see how we can benefit from making the
distinction.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:38:
scoped_refptr<base::MessageLoopProxy> observer_loop);
On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> Instead of using a MessageLoopProxy, I believe the new hotness is to pass
around
> a base::TaskRunner, of which a MLP derives from.
> 
> Also, this is a by-value copy, whereas given that it's ref-counted, it makes
> more sense to either pass as a const-ref or a naked pointer. AIUI, only the
> non-refcounted scoped classes (scoped_ptr/ptr_malloc/_array) support the C++11
> move emulation.
> 
> Same with line 60

Thanks for the tip :) I used SingleThreadTaskRunner so that we can keep calling
BelongsToCurrentThread().

I'm not sure I see your point about const-ref/naked pointer passing. I don't
take (unique) ownership of the pointer (which a unique_ptr or scoped_ptr with
move emulation would have  expressed nicely). I still need the guarantee that
the message loop proxy won't be deleted before me which implies the use of a
shared pointer. What am I missing?

While we are talking about ownership, is that common in the Chromium code base
to use a scoped_ptr (with Pass()) in place of a unique_ptr? I see that
scoped_ptr "supports move semantic" through Pass(). I'm just wondering if people
use it since it might be confusing to have a scoped_ptr used like a unique_ptr
knowing that scoped_ptr is also part of C++ 11.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:75: const scoped_refptr<SharedState>
shared_state_;
On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> This means the scoped_refptr itself is const, but the SharedState is not.
> 
> Did you mean scoped_refptr<const SharedState> (which I don't know if it will
> even work)

It only prevents us from reassigning the pointer. It documents the code by
expressing that once we constructed the instance (and DCHECKed), we can "safely"
dereference the pointer.
That way it is the semantic equivalent of non-const reference except that we
keep a syntax that makes mutations explicit (which is good).
"const SharedState" would have been best unfortunately we still need to perform
mutations on the ObserverList which is in SharedState.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:79: const
scoped_refptr<base::MessageLoopProxy> observer_loop_;
On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> Drop the const?

I would like to keep it for the same reason as above if you don't mind.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android_unittest.cc (right):

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:118: base::Thread::Stop();
On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> This means you're calling a virtual function (CleanUp) from a destructor
> (~TestObserverThread)
> 
> Here be dragons

There is a non-trivial comment in thread.h for Thread::~Thread() which explains
why it is needed and "safe" in this particular case.

Here it is:
NOTE: All subclasses of Thread must call Stop() in their destructor, or
otherwise ensure Stop() is called before the
subclass is destructed.  This is required to avoid a data race
between the destructor modifying the vtable, and the thread's
ThreadMain calling the virtual method Run.  It also ensures that
the CleanUp() virtual method is called on the subclass before it
is destructed.

Ryan Sleevi

Thanks Philippe. More comments - certainly not trying to drag this out, but I am ...

8 years, 7 months ago (2012-05-14 18:00:24 UTC) #9

Thanks Philippe.

More comments - certainly not trying to drag this out, but I am wanting to make
sure I've got a good grasp on this and it's threading edge cases.

Please see the two comments marked "BUG?" below. The rest is mostly style nits

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:38:
scoped_refptr<base::MessageLoopProxy> observer_loop);
On 2012/05/09 11:48:40, Philippe wrote:
> On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> > Instead of using a MessageLoopProxy, I believe the new hotness is to pass
> around
> > a base::TaskRunner, of which a MLP derives from.
> > 
> > Also, this is a by-value copy, whereas given that it's ref-counted, it makes
> > more sense to either pass as a const-ref or a naked pointer. AIUI, only the
> > non-refcounted scoped classes (scoped_ptr/ptr_malloc/_array) support the
C++11
> > move emulation.
> > 
> > Same with line 60
> 
> Thanks for the tip :) I used SingleThreadTaskRunner so that we can keep
calling
> BelongsToCurrentThread().
> 
> I'm not sure I see your point about const-ref/naked pointer passing. I don't
> take (unique) ownership of the pointer (which a unique_ptr or scoped_ptr with
> move emulation would have  expressed nicely). I still need the guarantee that
> the message loop proxy won't be deleted before me which implies the use of a
> shared pointer. What am I missing?

Just pass the naked pointer and store a scoped_refptr<> as a member. Don't pass
a scoped_refptr<> by-val.

Passing it by-val just adds an extra AddRef()/Release() pair - first to copy it
into the temporary passed as the argument, second to pass it to the actual
stored data member. Passing the naked pointer but storing in a scoped_refptr<>
eliminates that by only calling AddRef() once - and while the pointer is still
valid.

> 
> While we are talking about ownership, is that common in the Chromium code base
> to use a scoped_ptr (with Pass()) in place of a unique_ptr? I see that
> scoped_ptr "supports move semantic" through Pass(). I'm just wondering if
people
> use it since it might be confusing to have a scoped_ptr used like a unique_ptr
> knowing that scoped_ptr is also part of C++ 11.

We don't use C++11 in Chromium code, and likely won't for some time yet. As far
as unique_ptr<> vs scoped_ptr<>, scoped_ptr<> is used (almost? totally?)
exclusively, in part due to shared heritage in google3 land.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:75: const scoped_refptr<SharedState>
shared_state_;
On 2012/05/09 11:48:40, Philippe wrote:
> On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> > This means the scoped_refptr itself is const, but the SharedState is not.
> > 
> > Did you mean scoped_refptr<const SharedState> (which I don't know if it will
> > even work)
> 
> It only prevents us from reassigning the pointer. It documents the code by
> expressing that once we constructed the instance (and DCHECKed), we can
"safely"
> dereference the pointer.

But I mean, that's not really true. Once you construct the object, the
SharedState remains mutable and so any decision made at time of construction may
be wrong. Yes, it prevents reassignment of pointers, but as far as const
assumptions go, or about the validity of the members, it doesn't really old
much.

> That way it is the semantic equivalent of non-const reference except that we
> keep a syntax that makes mutations explicit (which is good).

Right, which we explicitly don't allow (const or non-const references).

> "const SharedState" would have been best unfortunately we still need to
perform
> mutations on the ObserverList which is in SharedState.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:79: const
scoped_refptr<base::MessageLoopProxy> observer_loop_;
On 2012/05/09 11:48:40, Philippe wrote:
> On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> > Drop the const?
> 
> I would like to keep it for the same reason as above if you don't mind.

I don't feel terribly strongly about this, but I still don't think it buys all
that much =) Protecting members from re-assignment, ok - but eh, I've yet to see
a Chromium bug caused by that, as opposed to doing some sort of mutative action
on the underlying object.

http://codereview.chromium.org/10206014/diff/31004/base/android/jni_generator...
File base/android/jni_generator/jni_generator.py (right):

http://codereview.chromium.org/10206014/diff/31004/base/android/jni_generator...
base/android/jni_generator/jni_generator.py:181:
'Lorg/chromium/chromeview/net/ProxyChangeListener',
Should this be on line 174 instead?

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:17: #include
"jni/proxy_change_listener_jni.h"
Is this absolute to the source tree? Or is this really
"<(SHARED_INTERMEDIATE_DIR)/net/jni/proxy_change_listener_jni.h" but with a
hidden -I "<(SHARED_INTERMEDIATE_DIR)/net"

For generated headers, we try to generate them with their full names, and then
just -I the <(SHARED_INTERMEDIATE_DIR) that they were generated in, rather than
-I the more specific path. This makes it easier to cross-reference the source
tree and find where the thing-that-generates-the-header is stored.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:21: #include <sys/system_properties.h>
C headers before C++ & Project headers (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
)

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:39: return
ProxyServer::FromURI(proxy_host, scheme);
I'm still not convinced this is handling IPv6 properly - but I'll readily admit
I may not be familiar with how Android is storing things.

if .proxyHost == "a:b:c::d:1", and .proxyPort == "", then when this is called,
it will try to construct as "[a:b:c::d]:1" rather than "[a:b:c::d:1]"

Since you've got unit tests - can you add and see what would happen with an IPv6
proxy?

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:41: base::StringToUint(proxy_port,
&port_as_uint);
BUG? Using a GURL seems more appropriate here for parsing, especially since you
can end up with a port > UINT16_MAX with this approach

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:106: // semantics we're trying to
match.
ocd nit: It's been raised by reviewers of my code in the past to avoid writing
comments in first-person ("we're"). This is not a hard rule, but I think it's
justified, so I like to ask others to consider as well :)

// See libcore/luni/src/main/java/java/net/ProxySelectorImpl.java for
// the equivalent Android implementation.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:165: // Question: Conversion to/from
UTF8 ok here?
Should there be a TODO and a bug followed for follow-up?

Otherwise, this comment will never go answered and just serve to confuse future
readers as to whether or not there is or isn't a bug a here.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:170: return result.is_null() ? "" :
ConvertJavaStringToUTF8(env, result.obj());
ocd style: std::string() rather than ""

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:174: ProxyConfigServiceAndroid*
service_;
Did I miss where |service_| is actually used here? Doesn't seem like you need it
(or need it to be passed in Start())

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:181: struct
ProxyConfigServiceAndroid::SharedState
style: Classes that are RefCountedThreadSafe<> should have private destructors,
which argues in favour of class vs struct (generally).

Also, DISALLOW_COPY_AND_ASSIGN(SharedState)

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:183: SharedState(Delegate* delegate) :
delegate_(delegate) {
style: explicit SharedState(...) {

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:206: }
style: Make sure that the order matches the header - that is, this should appear
after ProxySettingsChanged (line 247)

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:231: shared_state_->delegate_->Stop();
style nit: Remove the braces around one-line ifs for consistency with the rest
of the file

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:11: #include
"base/android/scoped_java_ref.h"
nit: I don't see this being used in this header

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:13: #include "base/memory/scoped_ptr.h"
nit: nor this?

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:14: #include "base/observer_list.h"
or this

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:16: #include "net/proxy/proxy_config.h"
nit: You can forward declare net::ProxyConfig - it's only used as a pointer
(line 57)

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:55: virtual void
RemoveObserver(Observer* observer) OVERRIDE;
BUG? AddObserver/RemoveObserver are called on the IO thread, not the UI thread -
at least, this is true for ProxyConfigServiceMac and ProxyConfigServiceLinux.

It sounds like the TaskRunner you take should be for the JNI runner, not the
observer runner.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:62: void ProxySettingsChanged(JNIEnv*,
jobject) { ProxySettingsChanged(); }
Avoid overloads (Line 59 & 62) -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:69:
scoped_refptr<base::SingleThreadTaskRunner> observer_runner,
Same here - take a naked pointer.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:81: scoped_refptr<SharedState>
callback_state);
const scoped_refptr<SharedState>& callback_state

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:88: const
scoped_refptr<base::SingleThreadTaskRunner> observer_runner_;
DISALLOW_COPY_AND_ASSIGN(ProxyConfigServiceAndroid)

Philippe

Thanks Ryan for spending so much time reviewing this! Hopefully we will land that soon. ...

8 years, 7 months ago (2012-05-15 16:12:47 UTC) #10

Thanks Ryan for spending so much time reviewing this!

Hopefully we will land that soon.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:38:
scoped_refptr<base::MessageLoopProxy> observer_loop);
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> On 2012/05/09 11:48:40, Philippe wrote:
> > On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> > > Instead of using a MessageLoopProxy, I believe the new hotness is to pass
> > around
> > > a base::TaskRunner, of which a MLP derives from.
> > > 
> > > Also, this is a by-value copy, whereas given that it's ref-counted, it
makes
> > > more sense to either pass as a const-ref or a naked pointer. AIUI, only
the
> > > non-refcounted scoped classes (scoped_ptr/ptr_malloc/_array) support the
> C++11
> > > move emulation.
> > > 
> > > Same with line 60
> > 
> > Thanks for the tip :) I used SingleThreadTaskRunner so that we can keep
> calling
> > BelongsToCurrentThread().
> > 
> > I'm not sure I see your point about const-ref/naked pointer passing. I don't
> > take (unique) ownership of the pointer (which a unique_ptr or scoped_ptr
with
> > move emulation would have  expressed nicely). I still need the guarantee
that
> > the message loop proxy won't be deleted before me which implies the use of a
> > shared pointer. What am I missing?
> 
> Just pass the naked pointer and store a scoped_refptr<> as a member. Don't
pass
> a scoped_refptr<> by-val.
> 
> Passing it by-val just adds an extra AddRef()/Release() pair - first to copy
it
> into the temporary passed as the argument, second to pass it to the actual
> stored data member. Passing the naked pointer but storing in a scoped_refptr<>
> eliminates that by only calling AddRef() once - and while the pointer is still
> valid.
> 
> > 
> > While we are talking about ownership, is that common in the Chromium code
base
> > to use a scoped_ptr (with Pass()) in place of a unique_ptr? I see that
> > scoped_ptr "supports move semantic" through Pass(). I'm just wondering if
> people
> > use it since it might be confusing to have a scoped_ptr used like a
unique_ptr
> > knowing that scoped_ptr is also part of C++ 11.
> 
> We don't use C++11 in Chromium code, and likely won't for some time yet. As
far
> as unique_ptr<> vs scoped_ptr<>, scoped_ptr<> is used (almost? totally?)
> exclusively, in part due to shared heritage in google3 land.

I see now. I guess that I was not used to having the refcount part of the object
itself as opposed to the shared pointer (like std::shared_ptr). The good point
with that is that you can pass the object through a raw pointer without loosing
any refcount information.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:75: const scoped_refptr<SharedState>
shared_state_;
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> On 2012/05/09 11:48:40, Philippe wrote:
> > On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> > > This means the scoped_refptr itself is const, but the SharedState is not.
> > > 
> > > Did you mean scoped_refptr<const SharedState> (which I don't know if it
will
> > > even work)
> > 
> > It only prevents us from reassigning the pointer. It documents the code by
> > expressing that once we constructed the instance (and DCHECKed), we can
> "safely"
> > dereference the pointer.
> 
> But I mean, that's not really true. Once you construct the object, the
> SharedState remains mutable and so any decision made at time of construction
may
> be wrong. Yes, it prevents reassignment of pointers, but as far as const
> assumptions go, or about the validity of the members, it doesn't really old
> much.
> 
> > That way it is the semantic equivalent of non-const reference except that we
> > keep a syntax that makes mutations explicit (which is good).
> 
> Right, which we explicitly don't allow (const or non-const references).
> 
> > "const SharedState" would have been best unfortunately we still need to
> perform
> > mutations on the ObserverList which is in SharedState.
> 

I removed the const here and below since it might be confusing for the reader.

http://codereview.chromium.org/10206014/diff/30002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:79: const
scoped_refptr<base::MessageLoopProxy> observer_loop_;
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> On 2012/05/09 11:48:40, Philippe wrote:
> > On 2012/05/07 17:12:20, Ryan Sleevi wrote:
> > > Drop the const?
> > 
> > I would like to keep it for the same reason as above if you don't mind.
> 
> I don't feel terribly strongly about this, but I still don't think it buys all
> that much =) Protecting members from re-assignment, ok - but eh, I've yet to
see
> a Chromium bug caused by that, as opposed to doing some sort of mutative
action
> on the underlying object.
> 

Done.

http://codereview.chromium.org/10206014/diff/31004/base/android/jni_generator...
File base/android/jni_generator/jni_generator.py (right):

http://codereview.chromium.org/10206014/diff/31004/base/android/jni_generator...
base/android/jni_generator/jni_generator.py:181:
'Lorg/chromium/chromeview/net/ProxyChangeListener',
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> Should this be on line 174 instead?

By default VIM uses case-sensitive sort (through :sort). I don't know what is
the preferred way of sorting (case-sensitive vs case-insensitive). Google C++
coding standards seem to only mention 'alphabetically'. I don't feel strongly
about that so I followed your suggestion.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:17: #include
"jni/proxy_change_listener_jni.h"
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> Is this absolute to the source tree? Or is this really
> "<(SHARED_INTERMEDIATE_DIR)/net/jni/proxy_change_listener_jni.h" but with a
> hidden -I "<(SHARED_INTERMEDIATE_DIR)/net"
> 
> For generated headers, we try to generate them with their full names, and then
> just -I the <(SHARED_INTERMEDIATE_DIR) that they were generated in, rather
than
> -I the more specific path. This makes it easier to cross-reference the source
> tree and find where the thing-that-generates-the-header is stored.

Indeed, it is "<(SHARED_INTERMEDIATE_DIR)/net/jni/proxy_change_listener_jni.h"
with a hidden -I "<(SHARED_INTERMEDIATE_DIR)/net".

But I don't see where "<(SHARED_INTERMEDIATE_DIR)/net" is being included.
<(SHARED_INTERMEDIATE_DIR) does not seem to be added to the include dirs either
which means that I can't include "net/jni/proxy_change_listener_jni.h" instead.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:21: #include <sys/system_properties.h>
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> C headers before C++ & Project headers (
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
> )

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:39: return
ProxyServer::FromURI(proxy_host, scheme);
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> I'm still not convinced this is handling IPv6 properly - but I'll readily
admit
> I may not be familiar with how Android is storing things.
> 
> if .proxyHost == "a:b:c::d:1", and .proxyPort == "", then when this is called,
> it will try to construct as "[a:b:c::d]:1" rather than "[a:b:c::d:1]"
> 
> Since you've got unit tests - can you add and see what would happen with an
IPv6
> proxy?

I fixed the implementation to use HostPortPair in all cases and added some unit
tests.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:41: base::StringToUint(proxy_port,
&port_as_uint);
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> BUG? Using a GURL seems more appropriate here for parsing, especially since
you
> can end up with a port > UINT16_MAX with this approach

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:106: // semantics we're trying to
match.
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> ocd nit: It's been raised by reviewers of my code in the past to avoid writing
> comments in first-person ("we're"). This is not a hard rule, but I think it's
> justified, so I like to ask others to consider as well :)
> 
> // See libcore/luni/src/main/java/java/net/ProxySelectorImpl.java for
> // the equivalent Android implementation.

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:165: // Question: Conversion to/from
UTF8 ok here?
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> Should there be a TODO and a bug followed for follow-up?
> 
> Otherwise, this comment will never go answered and just serve to confuse
future
> readers as to whether or not there is or isn't a bug a here.

I made it a TODO and will file a bug.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:170: return result.is_null() ? "" :
ConvertJavaStringToUTF8(env, result.obj());
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> ocd style: std::string() rather than ""

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:174: ProxyConfigServiceAndroid*
service_;
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> Did I miss where |service_| is actually used here? Doesn't seem like you need
it
> (or need it to be passed in Start())

No you didn't :) It was used before when we had NotifyTask. We don't need it
anymore. Start() uses |service| though when calling
Java_ProxyChangeListener_start() but we don't need to store it anymore.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:181: struct
ProxyConfigServiceAndroid::SharedState
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> style: Classes that are RefCountedThreadSafe<> should have private
destructors,
> which argues in favour of class vs struct (generally).
> 
> Also, DISALLOW_COPY_AND_ASSIGN(SharedState)

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:183: SharedState(Delegate* delegate) :
delegate_(delegate) {
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> style: explicit SharedState(...) {

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:206: }
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> style: Make sure that the order matches the header - that is, this should
appear
> after ProxySettingsChanged (line 247)

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:231: shared_state_->delegate_->Stop();
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> style nit: Remove the braces around one-line ifs for consistency with the rest
> of the file

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:11: #include
"base/android/scoped_java_ref.h"
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> nit: I don't see this being used in this header

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:13: #include "base/memory/scoped_ptr.h"
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> nit: nor this?

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:14: #include "base/observer_list.h"
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> or this

Indeed. It seems that I forgot to remove these includes after I added the new
SharedState class.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:16: #include "net/proxy/proxy_config.h"
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> nit: You can forward declare net::ProxyConfig - it's only used as a pointer
> (line 57)

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:55: virtual void
RemoveObserver(Observer* observer) OVERRIDE;
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> BUG? AddObserver/RemoveObserver are called on the IO thread, not the UI thread
-
> at least, this is true for ProxyConfigServiceMac and ProxyConfigServiceLinux.
> 
> It sounds like the TaskRunner you take should be for the JNI runner, not the
> observer runner.

I might be missing something but the observer thread is the IO thread. I can
rename observer_runner_ if you want (io_runner_?).

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:62: void ProxySettingsChanged(JNIEnv*,
jobject) { ProxySettingsChanged(); }
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> Avoid overloads (Line 59 & 62) -
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:69:
scoped_refptr<base::SingleThreadTaskRunner> observer_runner,
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> Same here - take a naked pointer.

Done.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:81: scoped_refptr<SharedState>
callback_state);
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> const scoped_refptr<SharedState>& callback_state

I might be wrong, but it seems safer in this particular case to make a copy of
the scoped_refptr since ProxyConfigServiceAndroid can be deleted before the
asynchronous task runs (which is why we have SharedState). In that case
&shared_state_ would be a pointer to a "random" place.

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:88: const
scoped_refptr<base::SingleThreadTaskRunner> observer_runner_;
On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> DISALLOW_COPY_AND_ASSIGN(ProxyConfigServiceAndroid)

Done.

Philippe

http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_service_android.h File net/proxy/proxy_config_service_android.h (right): http://codereview.chromium.org/10206014/diff/31004/net/proxy/proxy_config_service_android.h#newcode81 net/proxy/proxy_config_service_android.h:81: scoped_refptr<SharedState> callback_state); On 2012/05/15 16:12:48, Philippe wrote: > On ...

8 years, 7 months ago (2012-05-16 08:01:05 UTC) #11

Ryan Sleevi

Right, Bind uses BindStorage, which uses CallbackStorageTraits. The CallbackStorageTraits for all concrete types is store ...

8 years, 7 months ago (2012-05-19 01:26:31 UTC) #12

Right, Bind uses BindStorage, which uses CallbackStorageTraits. The
CallbackStorageTraits for all concrete types is store as T, pass as const T&
[with the exception of move emulation and a few others]

That said, I think there's still a threading issue here, please see below.

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:59: return ProxyServer::FromURI(
use the constructor

return ProxyServer(scheme, HostPortPair(proxy_host, ...));

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:60: HostPortPair(proxy_host,
static_cast<uint>(port_as_int)).ToString(),
uint16

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:185: ? std::string()
the ? should be on the previous line

See http://www.chromium.org/developers/coding-style "When expressions are
wrapped"

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:277:
ProxyConfigService::CONFIG_VALID));
I don't believe the threading is correct here.

The observers should only have OnProxyConfigChanged called on the IO thread (see
for example
http://src.chromium.org/viewvc/chrome/trunk/src/net/proxy/proxy_config_servic...
- OnProxyConfigChanged should be called on the IO thread)

You call this thread on the observer_runner_ (see line 257), which suggests that
|observer_runner_| should be the IO thread.

However, you call GetLatestProxyConfigInternal on this thread (line 274), which
calls GetProxyRules (line 145), which calls LookupProxy (line 126), which calls
GetProperty (line 69), which attaches JNI to the thread (line 180), which should
only be done on the UI thread.

If you look in the ProxyConfigServiceLinux (
http://src.chromium.org/viewvc/chrome/trunk/src/net/proxy/proxy_config_servic...
), you'll see the SettingsGetter, which takes the /ui/ and /file/ loops.

I believe what you want is:
1) Receive a STTR for the UI thread and the IO thread.
2) Post a task to the UI thread to do JNI registration & setup, and start
observing (on the UI thread)
3) When the UI thread callback is called, fetch all of the proxy settings
4) Post a task to the IO thread to wake it
5) Have the IO thread receive the settings, (optionally compare with existing),
then notify observers (on the IO thread)

Since you also need to do some de-registration, when the object goes out of
scope (on the IO thread), post a task to the UI thread to stop monitoring to the
SharedState/Delegate, then continue destruction. Eventually the UI thread will
notice this, deregister, and then destroy the refcounted shared-state on the UI
thread.

If you need to ensure that from point of destruction (on the IO thread) no
further messages are delivered on the IO thread, use a WeakPtr for the
ProxyConfigServiceAndroid. Any tasks posted to the IO thread that run after the
PCSA has gone out of scope will then not run, because on PCSA destruction, it
invalidated all weak ptrs.

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:45:
ProxyConfigServiceAndroid(base::SingleThreadTaskRunner* observer_runner);
explicit

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:55: 
Delete this blank line, per http://www.chromium.org/developers/coding-style
("When you implement an interface")

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:77: struct SharedState;
nit: Not always consistently, but there was a big push to treat forward decl's
like typedefs/enums, and thus moved  between lines 66 and 68 per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

I understand why it was placed here (first usage on line 80), but worth noting.

Ryan Sleevi

oh, and for naming, "network_task_runner" and "jni_task_runner" are the suggested names. I used IO/UI for ...

8 years, 7 months ago (2012-05-19 01:28:32 UTC) #13

Philippe

I moved delegate_ out of the SharedState class since I don't use it in the ...

8 years, 7 months ago (2012-05-22 15:23:04 UTC) #14

I moved delegate_ out of the SharedState class since I don't use it in the
aynchronous callback anymore.
Also, I renamed SharedState to SharedObserverList since this wrapper class now
contains only the observer list.

I hope that I correctly addressed the threading issues this time.

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:59: return ProxyServer::FromURI(
On 2012/05/19 01:26:31, Ryan Sleevi wrote:
> use the constructor
> 
> return ProxyServer(scheme, HostPortPair(proxy_host, ...));

Done.

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:60: HostPortPair(proxy_host,
static_cast<uint>(port_as_int)).ToString(),
On 2012/05/19 01:26:31, Ryan Sleevi wrote:
> uint16

Done.

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:185: ? std::string()
On 2012/05/19 01:26:31, Ryan Sleevi wrote:
> the ? should be on the previous line
> 
> See http://www.chromium.org/developers/coding-style "When expressions are
> wrapped"

Done.

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:277:
ProxyConfigService::CONFIG_VALID));
On 2012/05/19 01:26:31, Ryan Sleevi wrote:
> I don't believe the threading is correct here.
> 
> The observers should only have OnProxyConfigChanged called on the IO thread
(see
> for example
>
http://src.chromium.org/viewvc/chrome/trunk/src/net/proxy/proxy_config_servic...
> - OnProxyConfigChanged should be called on the IO thread)
> 
> You call this thread on the observer_runner_ (see line 257), which suggests
that
> |observer_runner_| should be the IO thread.
> 
> However, you call GetLatestProxyConfigInternal on this thread (line 274),
which
> calls GetProxyRules (line 145), which calls LookupProxy (line 126), which
calls
> GetProperty (line 69), which attaches JNI to the thread (line 180), which
should
> only be done on the UI thread.
> 
> If you look in the ProxyConfigServiceLinux (
>
http://src.chromium.org/viewvc/chrome/trunk/src/net/proxy/proxy_config_servic...
> ), you'll see the SettingsGetter, which takes the /ui/ and /file/ loops.
> 
> I believe what you want is:
> 1) Receive a STTR for the UI thread and the IO thread.
> 2) Post a task to the UI thread to do JNI registration & setup, and start
> observing (on the UI thread)
> 3) When the UI thread callback is called, fetch all of the proxy settings
> 4) Post a task to the IO thread to wake it
> 5) Have the IO thread receive the settings, (optionally compare with
existing),
> then notify observers (on the IO thread)
> 
> Since you also need to do some de-registration, when the object goes out of
> scope (on the IO thread), post a task to the UI thread to stop monitoring to
the
> SharedState/Delegate, then continue destruction. Eventually the UI thread will
> notice this, deregister, and then destroy the refcounted shared-state on the
UI
> thread.
> 
> If you need to ensure that from point of destruction (on the IO thread) no
> further messages are delivered on the IO thread, use a WeakPtr for the
> ProxyConfigServiceAndroid. Any tasks posted to the IO thread that run after
the
> PCSA has gone out of scope will then not run, because on PCSA destruction, it
> invalidated all weak ptrs.

Indeed.

Now I fetch the latest proxy config in ProxySettingsChangedInternal() running on
the JNI thread (UI thread). The function also stores this config (which
unfortunately requires a new Lock) so that clients can quickly and safely fetch
it back later through GetLatestProxyConfig().

Regarding WeakPtr, we considered that with John a few weeks ago but it seems
that we would be exposed to threading issues. From weak_ptr.h's top-level
comment, a WeakPtr can't be constructed on a thread and then dereferenced on
another one. That is unfortunate.

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:45:
ProxyConfigServiceAndroid(base::SingleThreadTaskRunner* observer_runner);
On 2012/05/19 01:26:31, Ryan Sleevi wrote:
> explicit

Done. The constructor now takes two parameters.

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:55: 
On 2012/05/19 01:26:31, Ryan Sleevi wrote:
> Delete this blank line, per http://www.chromium.org/developers/coding-style
> ("When you implement an interface")

Done.

http://codereview.chromium.org/10206014/diff/37005/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:77: struct SharedState;
On 2012/05/19 01:26:31, Ryan Sleevi wrote:
> nit: Not always consistently, but there was a big push to treat forward decl's
> like typedefs/enums, and thus moved  between lines 66 and 68 per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> 
> I understand why it was placed here (first usage on line 80), but worth
noting.

Thanks for the information.

Ryan Sleevi

Philippe: I feel like we've taken a step back with the locking. You can/should be ...

8 years, 7 months ago (2012-05-22 18:26:19 UTC) #15

Philippe:

I feel like we've taken a step back with the locking. You can/should be able to
get away with purely a delegate interface :)

Note: This is more or less the exact same strategy of
proxy_config_service_linux, which is why I recommended it be used as a model.
This avoids any locks, and keeps the RefCounting to a minimum and with minimum
forward declares.

I'd consider the |Delegate| to be an implementation detail (and thus only
forward declared in the .h), but if you wanted to mock out the delegate, then
yes, it will also need to appear in the .h.

If you do decide to put the Delegate's class definition in the header, please
only include the minimum set of public functions (Start,
ShutdownOnNetworkThread, Add/RemoveObserver, GetLatestProxyConfig) - the rest
are all impl. details :)

Please feel free to gchat/mail, and if necessary we can setup a VC.

class Delegate : public base::RefCountedThreadSafe<Delegate> {
 public:
  Delegate(base::SequencedTaskRunner* network_thread,
           base::SequencedTaskRunner* jni_thread);
 
  // Called only on the network thread
  void ShutdownOnNetworkThread();
  void AddObserver(Observer* observer);
  void RemoveObserver(Observer* observer);
  ProxyConfigService::ConfigAvailability GetLatestProxyConfig(ProxyConfig*
config);

  // Called on the JNI thread.
  // Mention the Java interface being implemented here.
  // Should this be virtual? Not necessary with JNI?
  void ProxySettingsChanged(JNIEnv*, jobject) {
    ProxySettingsChangedInternal();
  }

 private:
  friend class base::RefCountedThreadSafe<Delegate>;
  ~Delegate() {}

  // Called on the JNI thread
  void SetupOnJNIThread();
  void ShutdownOnJNIThread();
  void ProxySettingsChangedInternal();

  // Called on the network thread
  void SetNewProxyConfig(const ProxyConfig& config);

  // Accessed only on the JNI thread.
  scoped_refptr<...> network_thread_;

  // Accessed only on the network thread.
  scoped_refptr<...> jni_thread_;
  ProxyConfig config_;
};

Delegate::Delegate(...) {
  // This is probably always true and thus you can just call SetupOnJNIThread
  // directly. Still, for testing, if you needed it. May be overkill.
  if (OnJNIThread())
    return SetupOnJNIThread();

  jni_thread_->PostTask(FROM_HERE, &Delegate::SetupOnJNIThread, this);
}

Delegate::ShutdownOnNetworkThread(...) {
  DCHECK(OnNetworkThread());
  jni_thread_->PostTask(
      FROM_HERE, base::Bind(&Delegate::ShutdownOnJNIThread, this));
}

// AddObserver
// RemoveObserver
// GetLatestProxyConfig

Delegate::SetupOnJNIThread() {
  DCHECK(OnJNIThread());
  // Register the JNI objects
  // Grab the first config synchronously. 
  network_thread_->PostTask(
      FROM_HERE,
      &Delegate::SetNewProxyConfig, this, config);
}

Delegate::ShutdownOnJNIThread() {
  DCHECK(OnJNIThread());
  // Unregister from JNI
}

Delegate::ProxySettingsChangedInternal(...) {
  DCHECK(OnJNIThread());
  // Get config via JNI
  network_thread_->PostTask(
      FROM_HERE,
      base::Bind(&Delegate::SetNewProxyConfig, this, config));
}

Delegate::SetNewProxyConfig(...) {
  DCHECK(OnNetworkThread());
  config_ = config;
  FOR_EACH_OBSERVER(..., ..., config);
}

ProxyConfigServiceAndroid::ProxyConfigServiceAndroid(
    base::SequencedTaskRunner* network_thread,
    base::SequencedTaskRunner* jni_thread)
    : delegate_(new Delegate(network_thread, jni_thread)) {
}

ProxyConfigServiceAndroid::~ProxyConfigServiceAndroid() {
  delegate_->StopOnNetworkThread();
}

ProxyConfigServiceAndroid::AddObserver(...) {
  delegate_->AddObserver(...);
}

ProxyConfigServiceAndroid::RemoveObserver(...) {
  delegate_->RemoveObserver(...);
}

ConfigAvailability ProxyConfigServiceAndroid::GetLatestProxyConfig(ProxyConfig*
config) {
  return delegate_->GetLatestProxyConfig(config);
}

http://codereview.chromium.org/10206014/diff/35037/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/35037/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:93: scoped_refptr<SharedObserverList>
observers_;
I don't believe you need to protect this by forcing scoped_refptr<> ness. See
comments below.

http://codereview.chromium.org/10206014/diff/35037/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:101: base::Lock lock_;  // Protects the
state below.
This isn't needed

Philippe

Hi Ryan, I made quite a lot of changes following your comments. That was quite ...

8 years, 7 months ago (2012-05-25 16:20:09 UTC) #16

Ryan Sleevi

Thanks for putting up with the review comments. I think this looks much better, and ...

8 years, 7 months ago (2012-05-25 17:42:14 UTC) #17

Thanks for putting up with the review comments. I think this looks much better,
and as you noted, it's much simpler in the implementation.

The good news is that your unit tests can be as simple - if not moreso - than
the actual implementation. Just what you were hoping for, I'm sure :)

Rather than using multiple threads, signals, and task marshalling, you can run
everything on a single MessageLoop on the current test thread, without any
ill-effect. Effectively, the whole thing will run single-threaded via task
posting, and it will just work. Probably right up your alley :)

class MyTestFixture : public ::testing::Test {
 protected:
  MyTestFixture()
    : service_(message_loop_.message_loop_proxy(),
               message_loop_.message_loop_proxy(),
               base::Bind(&GetProperty, this));

  std::string GetProperty(const std::string& property) {
    return properties_[property];
  }

  void AddProperty(const std::string& key, const std::string& value) {
    properties_[key] = value;
  }

  MessageLoop message_loop_;
  ProxyConfigServiceAndroid service_;
  std::map<std::string, std::string> properties_;
};

TEST_F(MyTestFixture, SomeTest) {
  // Setup what you want the initial proxy config
  // callbacks to return or what have you

  // Initialize the service. This 'registers' with JNI,
  // etc. It will run until there are no more tasks -
  // meaning everything is now set up :)
  message_loop_.RunAllPending();

  // Check that the initial config is as you expect.

  // Set new settings
  AddProperty("http.proxyHost", "localhost");
  AddProperty(..., ...);

  // Notify the service
  ProxySettingsChanged();

  // Give it a chance to pick up the config. Check to
  // ensure your observers are called, as you expect.
  message_loop_.RunAllPending();

  // Check that the new config is what you want.
}


Hopefully that makes it MUCH easier to unit test - no threading issues, no event
signalling, no thread joining, no task posting.

https://chromiumcodereview.appspot.com/10206014/diff/57013/net/proxy/proxy_co...
File net/proxy/proxy_config_service_android.cc (right):

https://chromiumcodereview.appspot.com/10206014/diff/57013/net/proxy/proxy_co...
net/proxy/proxy_config_service_android.cc:175:
base::Bind(&Delegate::SetupOnJNIThread, this, service));
nit: uses curly braces around these ifs because they span multiple lines. Same
on 179-184

https://chromiumcodereview.appspot.com/10206014/diff/57013/net/proxy/proxy_co...
net/proxy/proxy_config_service_android.cc:260: java_proxy_change_listener_));
braces

https://chromiumcodereview.appspot.com/10206014/diff/57013/net/proxy/proxy_co...
File net/proxy/proxy_config_service_android_unittest.cc (right):

https://chromiumcodereview.appspot.com/10206014/diff/57013/net/proxy/proxy_co...
net/proxy/proxy_config_service_android_unittest.cc:83: return result;
Use ProxyInfo.ToPacString() - this whole loop is a dependency on behaviour not
part of your test.

Philippe

Indeed, using a single message loop makes testing MUCH easier :) I was missing RunAllPending(). ...

8 years, 6 months ago (2012-05-29 10:05:29 UTC) #18

Philippe

On 2012/05/29 10:05:29, Philippe wrote: > Indeed, using a single message loop makes testing MUCH ...

8 years, 6 months ago (2012-05-29 13:10:27 UTC) #19

Ryan Sleevi

Almost! One potential use-after-free here, and a few nits about the tests, but nothing too ...

8 years, 6 months ago (2012-05-29 17:34:42 UTC) #20

Almost!

One potential use-after-free here, and a few nits about the tests, but nothing
too substantial :)

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:175:
base::Bind(&Delegate::SetupOnJNIThread, this, service));
There is a slight timing/use-after-free issue here, in that |service| may be
invalidated by the time SetupOnJNIThread is called.

Likewise, there's a similar race between ~Delegate() (Implicitly invoked line
302) and Shutdown (line 223).

The way to resolve this would be to ensure that you're registering the
/Delegate/ with the JNI thread, rather than the /service/.

Then you'd simply need to keep a reference to the Delegate alive long enough to
shut it down on the JNI thread. This would require an explicit call to
Delegate::Shutdown (on line 302), while |service| still holds a reference, so
that the Delegate can then PostTask to the JNI task runner (which will bump up
the Delegate's refcount to 2) before the ProxyConfigServiceAndroid's
scoped_refptr releases (dropping the Delegate's Refcount to 1 - meaning it will
be deleted once the task runs on the JNI thread that unregisters it).

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android_unittest.cc (right):

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:33: // Called from main
thread.
nit: still applicable? No threads involved now

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:47: std::string
ToString(const ProxyInfo& proxy_info) {
The naming of this method is not ideal - but more importantly, it's also not
clear why it's needed. It only seems to be used on line 111, in which case, just
call proxy_info.ToPacString()

If it is part of some auto-testing piece, then ProxyInfoToPacString would be
better, if only for verbosity sake.

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:58: virtual void SetUp()
OVERRIDE {
Note: Every test runs as a new instance, so you do not need to put this in
SetUp(). Instead, you can move much of this to being in the constructor, with
the variables declared on the stack, rather than on the heap.

ProxyConfigServiceAndroidTest()
  : service_(message_loop_.message_loop_proxy(),
             message_loop_.message_loop_proxy(),
             base::Bind(...)) {
}

virtual void SetUp() OVERRIDE {
  message_loop.RunAllPending();
  service_.AddObserver(&observer);
}

virtual void TearDown() OVERRIDE {
  service_.RemoveObserver(&observer);
}

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:60: message_loop_ =
MessageLoop::current();
BUG: There is no guarantee that there will be a current MessageLoop at the time
this test runs. You need to explicitly create a MessageLoop (heap or stack)
before assuming MessageLoop::current().

If there is a MessageLoop::current(), one or more tests may be leaking. The
general proof of this is to run with a --gtest_filter for just this test; it
should presumably crash.

Or is this part of the JNI test runner infrastructure, where net_unittests is
built as a shared library? If so, please clearly comment about why
MessageLoop::current() is safe in this test.

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:72: service_.reset();
BUG: You don't RemoveObserver (line 67)

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:107: ProxyConfig*
proxy_config) {
nit: You only seem to call this method on line 100. Should this just be rolled
into the above TestMapping?

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:118: typedef
std::map<std::string, std::string> StringMap;
Is this typedef still necessary? Seems to only be used on line 85

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:131:
ProxyConfigServiceAndroidTest::SetUp();
nit: You should generally always call the parent method first.

Philippe

I'm glad to see that we are almost there :) http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_service_android.cc File net/proxy/proxy_config_service_android.cc (right): http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_service_android.cc#newcode175 ...

8 years, 6 months ago (2012-06-01 13:06:36 UTC) #21

I'm glad to see that we are almost there :)

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:175:
base::Bind(&Delegate::SetupOnJNIThread, this, service));
On 2012/05/29 17:34:43, Ryan Sleevi wrote:
> There is a slight timing/use-after-free issue here, in that |service| may be
> invalidated by the time SetupOnJNIThread is called.
> 
> Likewise, there's a similar race between ~Delegate() (Implicitly invoked line
> 302) and Shutdown (line 223).
> 
> The way to resolve this would be to ensure that you're registering the
> /Delegate/ with the JNI thread, rather than the /service/.
> 
> Then you'd simply need to keep a reference to the Delegate alive long enough
to
> shut it down on the JNI thread. This would require an explicit call to
> Delegate::Shutdown (on line 302), while |service| still holds a reference, so
> that the Delegate can then PostTask to the JNI task runner (which will bump up
> the Delegate's refcount to 2) before the ProxyConfigServiceAndroid's
> scoped_refptr releases (dropping the Delegate's Refcount to 1 - meaning it
will
> be deleted once the task runs on the JNI thread that unregisters it).

Good catch!

I got your point for the first issue. I also had to add a JNICallback class in
the header. We were registering the |service| rather than the |Delegate| because
we needed to have the method called by Java exposed in the header. Here is the
piece of generated JNI code calling ProxySettingsChanged:

static void ProxySettingsChanged(JNIEnv* env, jobject obj,
    jint nativePtr) {
  DCHECK(nativePtr) << "ProxySettingsChanged";
  net::ProxyConfigServiceAndroid* native =
      reinterpret_cast<net::ProxyConfigServiceAndroid*>(nativePtr);
  return native->ProxySettingsChanged(env, obj);
}

Obviously we could have left the Delegate declaration in the header too (please
let me know if you want me to do that instead).

Regarding the second issue, at destruction, I might be wrong but the callback
that we post on the JNI thread does not capture any state by reference
(ShutdownOnJNIThread() is static and |java_proxy_change_listener_| is captured
by copy). Therefore it can run independently of |Delegate| and |service|
lifecycles. I should have documented this subtlety.
I might be missing something important here.

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android_unittest.cc (right):

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:33: // Called from main
thread.
On 2012/05/29 17:34:43, Ryan Sleevi wrote:
> nit: still applicable? No threads involved now

Done.

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:47: std::string
ToString(const ProxyInfo& proxy_info) {
On 2012/05/29 17:34:43, Ryan Sleevi wrote:
> The naming of this method is not ideal - but more importantly, it's also not
> clear why it's needed. It only seems to be used on line 111, in which case,
just
> call proxy_info.ToPacString()
> 
> If it is part of some auto-testing piece, then ProxyInfoToPacString would be
> better, if only for verbosity sake.

Not needed (anymore) indeed.

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:58: virtual void SetUp()
OVERRIDE {
On 2012/05/29 17:34:43, Ryan Sleevi wrote:
> Note: Every test runs as a new instance, so you do not need to put this in
> SetUp(). Instead, you can move much of this to being in the constructor, with
> the variables declared on the stack, rather than on the heap.
> 
> ProxyConfigServiceAndroidTest()
>   : service_(message_loop_.message_loop_proxy(),
>              message_loop_.message_loop_proxy(),
>              base::Bind(...)) {
> }
> 
> virtual void SetUp() OVERRIDE {
>   message_loop.RunAllPending();
>   service_.AddObserver(&observer);
> }
> 
> virtual void TearDown() OVERRIDE {
>   service_.RemoveObserver(&observer);
> }

I didn't know that a new instance was created for each unit test. Thanks for the
information.

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:60: message_loop_ =
MessageLoop::current();
On 2012/05/29 17:34:43, Ryan Sleevi wrote:
> BUG: There is no guarantee that there will be a current MessageLoop at the
time
> this test runs. You need to explicitly create a MessageLoop (heap or stack)
> before assuming MessageLoop::current().
> 
> If there is a MessageLoop::current(), one or more tests may be leaking. The
> general proof of this is to run with a --gtest_filter for just this test; it
> should presumably crash.
> 
> Or is this part of the JNI test runner infrastructure, where net_unittests is
> built as a shared library? If so, please clearly comment about why
> MessageLoop::current() is safe in this test.

I created a new message loop. Thanks for the details.

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:72: service_.reset();
On 2012/05/29 17:34:43, Ryan Sleevi wrote:
> BUG: You don't RemoveObserver (line 67)

Done.

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:107: ProxyConfig*
proxy_config) {
On 2012/05/29 17:34:43, Ryan Sleevi wrote:
> nit: You only seem to call this method on line 100. Should this just be rolled
> into the above TestMapping?

Indeed. I had a unit test using it before.

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:118: typedef
std::map<std::string, std::string> StringMap;
On 2012/05/29 17:34:43, Ryan Sleevi wrote:
> Is this typedef still necessary? Seems to only be used on line 85

It wasn't indeed. I had to keep it though since I'm now using it more often.

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android_unittest.cc:131:
ProxyConfigServiceAndroidTest::SetUp();
On 2012/05/29 17:34:43, Ryan Sleevi wrote:
> nit: You should generally always call the parent method first. 

This is a little subtle. I set the configuration before so that when the service
is constructed, in ProxyConfigServiceAndroidTest::SetUp(), and more importantly
the initial config fetched, we use a non-empty configuration map.
I had to make some small changes here anyway since we don't create the service
in SetUp() anymore.

Philippe

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_service_android.cc File net/proxy/proxy_config_service_android.cc (right): http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_service_android.cc#newcode175 net/proxy/proxy_config_service_android.cc:175: base::Bind(&Delegate::SetupOnJNIThread, this, service)); On 2012/06/01 13:06:37, Philippe wrote: > ...

8 years, 6 months ago (2012-06-01 14:11:21 UTC) #22

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/52026/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:175:
base::Bind(&Delegate::SetupOnJNIThread, this, service));
On 2012/06/01 13:06:37, Philippe wrote:
> On 2012/05/29 17:34:43, Ryan Sleevi wrote:
> > There is a slight timing/use-after-free issue here, in that |service| may be
> > invalidated by the time SetupOnJNIThread is called.
> > 
> > Likewise, there's a similar race between ~Delegate() (Implicitly invoked
line
> > 302) and Shutdown (line 223).
> > 
> > The way to resolve this would be to ensure that you're registering the
> > /Delegate/ with the JNI thread, rather than the /service/.
> > 
> > Then you'd simply need to keep a reference to the Delegate alive long enough
> to
> > shut it down on the JNI thread. This would require an explicit call to
> > Delegate::Shutdown (on line 302), while |service| still holds a reference,
so
> > that the Delegate can then PostTask to the JNI task runner (which will bump
up
> > the Delegate's refcount to 2) before the ProxyConfigServiceAndroid's
> > scoped_refptr releases (dropping the Delegate's Refcount to 1 - meaning it
> will
> > be deleted once the task runs on the JNI thread that unregisters it).
> 
> Good catch!
> 
> I got your point for the first issue. I also had to add a JNICallback class in
> the header. We were registering the |service| rather than the |Delegate|
because
> we needed to have the method called by Java exposed in the header. Here is the
> piece of generated JNI code calling ProxySettingsChanged:
> 
> static void ProxySettingsChanged(JNIEnv* env, jobject obj,
>     jint nativePtr) {
>   DCHECK(nativePtr) << "ProxySettingsChanged";
>   net::ProxyConfigServiceAndroid* native =
>       reinterpret_cast<net::ProxyConfigServiceAndroid*>(nativePtr);
>   return native->ProxySettingsChanged(env, obj);
> }
> 
> Obviously we could have left the Delegate declaration in the header too
(please
> let me know if you want me to do that instead).
> 
> Regarding the second issue, at destruction, I might be wrong but the callback
> that we post on the JNI thread does not capture any state by reference
> (ShutdownOnJNIThread() is static and |java_proxy_change_listener_| is captured
> by copy). Therefore it can run independently of |Delegate| and |service|
> lifecycles. I should have documented this subtlety.
> I might be missing something important here.

Ok, I think that I got your second point after I talked to John :)
I'm addressing that currently.

Philippe

Hi Ryan, This last patch set should have fixed the threading issue at destruction. When ...

8 years, 6 months ago (2012-06-01 17:14:09 UTC) #23

Ryan Sleevi

On 2012/06/01 17:14:09, Philippe wrote: > Hi Ryan, > > This last patch set should ...

8 years, 6 months ago (2012-06-01 18:09:49 UTC) #24

Ryan Sleevi

Great! I think you've fixed just about everything. I have one concern that a bit ...

8 years, 6 months ago (2012-06-01 18:10:58 UTC) #25

Great! I think you've fixed just about everything. I have one concern that a bit
too much of the implementation is leaking into the public header, and based on
your most recent patchset, I think you can get away with hiding those details.
The less the merrier ;-)

a few style nits too

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:169:
get_property_callback_(get_property_callback) {}
style: } belongs on a new line when the ctor arg list or initializer list spans
multiple lines.

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:172: if (OnJNIThread()) {
Is this ever not true? Should it be a DCHECK instead?

I read the contract as being this object must always be first constructed on the
JNI thread. If that's so, you should just be able to inline both SetupJNI and
FetchInitialConfig into the constructor (or into an Init method called by the
constructor), and avoid the extra message passing.

proxy_config_service_linux for the example of this :)

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:192: if (OnJNIThread()) {
This one would stay, I would assume.

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:56: class JNICallback {
nit: Rather than explain this from the perspective of justification, just
explain what it does.

nit: The approach you've taken with this, requiring it take a Delegate in ctor,
has forced you to expose an implementation detail (Delegate) as a public type
(line 29), even though no consumer can actually use either it or this class.

The better approach would be:

class JNIDelegate {
 public:
  virtual void ProxySettingsChanged(JNIEnv*, jobject) = 0;
};

And then have a concrete implementation in the .cc.

From the perspective of the JNI bindings (where this interface really lives, but
understandably can't due to the auto-generated nature), all you care about is
that this method exists - and any users of the PCSA do not have to be exposed to
the Delegate details (line 29).

finally, style-nit: typenames & member classes before methods ( conceptually
lumped under typedefs/enums under
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
)

Philippe

You were right, the message loop is initialized by the net test suite (in NetTestSuite::InitializeTestThreadNoNetworkChangeNotifier() ...

8 years, 6 months ago (2012-06-04 10:02:37 UTC) #26

You were right, the message loop is initialized by the net test suite (in
NetTestSuite::InitializeTestThreadNoNetworkChangeNotifier() in
net/base/net_test_suite.cc). I added a comment for that.

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.cc (right):

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:169:
get_property_callback_(get_property_callback) {}
On 2012/06/01 18:10:58, Ryan Sleevi wrote:
> style: } belongs on a new line when the ctor arg list or initializer list
spans
> multiple lines.

Done.

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:172: if (OnJNIThread()) {
On 2012/06/01 18:10:58, Ryan Sleevi wrote:
> Is this ever not true? Should it be a DCHECK instead?
> 
> I read the contract as being this object must always be first constructed on
the
> JNI thread. If that's so, you should just be able to inline both SetupJNI and
> FetchInitialConfig into the constructor (or into an Init method called by the
> constructor), and avoid the extra message passing.
> 
> proxy_config_service_linux for the example of this :)

Done. I had to keep two separate methods though, SetupJNI() and
FetchInitialConfig(), since we don't setup JNI for testing. But that removed
quite a lot of functions/message passing anyway.

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.cc:192: if (OnJNIThread()) {
On 2012/06/01 18:10:58, Ryan Sleevi wrote:
> This one would stay, I would assume.

Indeed.

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/51022/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:56: class JNICallback {
On 2012/06/01 18:10:58, Ryan Sleevi wrote:
> nit: Rather than explain this from the perspective of justification, just
> explain what it does.
> 
> nit: The approach you've taken with this, requiring it take a Delegate in
ctor,
> has forced you to expose an implementation detail (Delegate) as a public type
> (line 29), even though no consumer can actually use either it or this class.
> 
> The better approach would be:
> 
> class JNIDelegate {
>  public:
>   virtual void ProxySettingsChanged(JNIEnv*, jobject) = 0;
> };
> 
> And then have a concrete implementation in the .cc.
> 
> From the perspective of the JNI bindings (where this interface really lives,
but
> understandably can't due to the auto-generated nature), all you care about is
> that this method exists - and any users of the PCSA do not have to be exposed
to
> the Delegate details (line 29).
> 
> finally, style-nit: typenames & member classes before methods ( conceptually
> lumped under typedefs/enums under
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> )

Done. Good point. I made the implementation class nested in the Delegate class
to avoid adding a 'friend class JNIDelegateImpl' below. Please let me know if
you want me instead to add this friend declaration below and make the
implementation class part of the anonymous namespace.

Ryan Sleevi

LGTM, but with a sharp edge and some nits below. http://codereview.chromium.org/10206014/diff/89002/net/android/java/org/chromium/net/ProxyChangeListener.java File net/android/java/org/chromium/net/ProxyChangeListener.java (right): http://codereview.chromium.org/10206014/diff/89002/net/android/java/org/chromium/net/ProxyChangeListener.java#newcode68 ...

8 years, 6 months ago (2012-06-04 21:49:18 UTC) #27

Philippe

Thanks a lot Ryan for spending so much time and reviewing this so carefully! I ...

8 years, 6 months ago (2012-06-05 09:07:54 UTC) #28

Thanks a lot Ryan for spending so much time and reviewing this so carefully! I
learned a lot about threading in Chromium.

I'm waiting for your final LGTM before I commit this.

http://codereview.chromium.org/10206014/diff/89002/net/android/java/org/chrom...
File net/android/java/org/chromium/net/ProxyChangeListener.java (right):

http://codereview.chromium.org/10206014/diff/89002/net/android/java/org/chrom...
net/android/java/org/chromium/net/ProxyChangeListener.java:68: int nativePtr /*
net::ProxyConfigServiceAndroid::JNIDelegate */);
On 2012/06/04 21:49:18, Ryan Sleevi wrote:
> Does the JNI generator let you forward declare like this? In C++, you can't
> forward declare derived classes (eg: it expects ProxyConfigServiceAndroid is a
> namespace, rather than a class).
> 
> If this works, its only working because
net/proxy/proxy_config_service_android.h
> has already been included, which seems dangerous because the JNI file here
> cannot express that.
> 
> net::JNIDelegate (or net::jni::delegate, or jni::net::delegate, or whatever) -
> that is, removing it from being a derived interface and moving it into the
> top-level namespace, should avoid this.

Good point.

There is no forward declaration in the generated code. The type
net::ProxyConfigServiceAndroid::JNIDelegate is used only once to do a
reinterpret_cast. Therefore the generated code does rely indeed on the fact that
its client must include the appropriate headers. More importantly, after the
reinterpret_cast, the generated code calls a method of ProxyConfigServiceAndroid
thus can't rely on a forward declaration:

static void ProxySettingsChanged(JNIEnv* env, jobject obj,
    jint nativePtr) {
  DCHECK(nativePtr) << "ProxySettingsChanged";
  net::ProxyConfigServiceAndroid::JNIDelegate* native =
      reinterpret_cast<net::ProxyConfigServiceAndroid::JNIDelegate*>(nativePtr);
  return native->ProxySettingsChanged(env, obj);
}

We are including this generated header only in our cc file. The only risk I can
see is that the JNI generator changes significantly and starts generating
forward declarations. This would be a major change since the current
implementation, calling a method of the user-provided type, can't rely on a
forward declaration (inability to call a method from an incomplete type). In the
worst case, if that happened (seems unlikely), this would produce an error when
compiling proxy_config_service_android.cc. 

I'm just a little concerned about adding some extra friend declarations and more
importantly using the namespace net (I would probably have to find a name more
specific than "JNIDelegate"). I think that keeping the class as it is currently
should be reasonably safe for the reasons I mentioned above.

What do you think?

http://codereview.chromium.org/10206014/diff/89002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/89002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:39: // the header.
On 2012/06/04 21:49:18, Ryan Sleevi wrote:
> Does this second half of the comment still apply... "without exposing the
> Delegate here in the header"?
> 
> Seems like the comment should just explain /what/ the class does, not /why/ :)

I removed the second sentence.

http://codereview.chromium.org/10206014/diff/89002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:50: base::SingleThreadTaskRunner*
jni_task_runner);
On 2012/06/04 21:49:18, Ryan Sleevi wrote:
> Do these need to be SingleThreadTaskRunners? That is, is there thread-local
> storage involved?
> 
> Otherwise, using a SequencedTaskRunner might be more idiomatic here.

I was using SingleThreadTaskRunners for BelongsToCurrentThread() but this method
is apparently nothing more than an alias to RunsTasksOnCurrentThread(). I made
the change.

Ryan Sleevi

LGTM! One comment nit, but no need to wait for my re-review. Address as/if you ...

8 years, 6 months ago (2012-06-05 16:50:13 UTC) #29

LGTM!

One comment nit, but no need to wait for my re-review. Address as/if you see
fit.

Thanks for the patience in the review. I know it was a long way getting here,
hopefully it wasn't too painful :) Please let me know if there's anything I
could have done that would have made it easier. Thanks!

http://codereview.chromium.org/10206014/diff/89002/net/android/java/org/chrom...
File net/android/java/org/chromium/net/ProxyChangeListener.java (right):

http://codereview.chromium.org/10206014/diff/89002/net/android/java/org/chrom...
net/android/java/org/chromium/net/ProxyChangeListener.java:68: int nativePtr /*
net::ProxyConfigServiceAndroid::JNIDelegate */);
On 2012/06/05 09:07:54, Philippe wrote:
> On 2012/06/04 21:49:18, Ryan Sleevi wrote:
> > Does the JNI generator let you forward declare like this? In C++, you can't
> > forward declare derived classes (eg: it expects ProxyConfigServiceAndroid is
a
> > namespace, rather than a class).
> > 
> > If this works, its only working because
> net/proxy/proxy_config_service_android.h
> > has already been included, which seems dangerous because the JNI file here
> > cannot express that.
> > 
> > net::JNIDelegate (or net::jni::delegate, or jni::net::delegate, or whatever)
-
> > that is, removing it from being a derived interface and moving it into the
> > top-level namespace, should avoid this.
> 
> Good point.
> 
> There is no forward declaration in the generated code. The type
> net::ProxyConfigServiceAndroid::JNIDelegate is used only once to do a
> reinterpret_cast. Therefore the generated code does rely indeed on the fact
that
> its client must include the appropriate headers. More importantly, after the
> reinterpret_cast, the generated code calls a method of
ProxyConfigServiceAndroid
> thus can't rely on a forward declaration:
> 
> static void ProxySettingsChanged(JNIEnv* env, jobject obj,
>     jint nativePtr) {
>   DCHECK(nativePtr) << "ProxySettingsChanged";
>   net::ProxyConfigServiceAndroid::JNIDelegate* native =
>      
reinterpret_cast<net::ProxyConfigServiceAndroid::JNIDelegate*>(nativePtr);
>   return native->ProxySettingsChanged(env, obj);
> }
> 
> We are including this generated header only in our cc file. The only risk I
can
> see is that the JNI generator changes significantly and starts generating
> forward declarations. This would be a major change since the current
> implementation, calling a method of the user-provided type, can't rely on a
> forward declaration (inability to call a method from an incomplete type). In
the
> worst case, if that happened (seems unlikely), this would produce an error
when
> compiling proxy_config_service_android.cc. 

I see. The important take-away here is that all of the code is within the
generated header, rather than being a .h/.cc file. As such, and because this
generated header is only included in the proxy_config_service_android.cc
(avoiding ODR violations), this is "safe."

> 
> I'm just a little concerned about adding some extra friend declarations and
more
> importantly using the namespace net (I would probably have to find a name more
> specific than "JNIDelegate"). I think that keeping the class as it is
currently
> should be reasonably safe for the reasons I mentioned above.
> 
> What do you think?

I think it's fine to leave as-is. This particular edge case of the build system
surprises me, but that's what happens when you glue languages together :)

http://codereview.chromium.org/10206014/diff/95002/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_android.h (right):

http://codereview.chromium.org/10206014/diff/95002/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_android.h:37: // the header.
nit: "implemented in the header" ?

Philippe

Thanks Ryan! I addressed the small comment. Otherwise I think you did a great job. ...

8 years, 6 months ago (2012-06-06 08:45:20 UTC) #30

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/10206014/102002

8 years, 6 months ago (2012-06-06 09:53:15 UTC) #31

commit-bot: I haz the power

Presubmit check for 10206014-102002 failed and returned exit status 1. Running presubmit commit checks ...

8 years, 6 months ago (2012-06-06 09:53:19 UTC) #32

bulach

lgtm, just a few nits: http://codereview.chromium.org/10206014/diff/102002/base/android/jni_generator/jni_generator.py File base/android/jni_generator/jni_generator.py (right): http://codereview.chromium.org/10206014/diff/102002/base/android/jni_generator/jni_generator.py#newcode188 base/android/jni_generator/jni_generator.py:188: 'Lorg/chromium/net/NetworkChangeNotifier', nit: seems duplicated ...

8 years, 6 months ago (2012-06-06 10:57:38 UTC) #34

Philippe

Thanks Marcus! http://codereview.chromium.org/10206014/diff/102002/base/android/jni_generator/jni_generator.py File base/android/jni_generator/jni_generator.py (right): http://codereview.chromium.org/10206014/diff/102002/base/android/jni_generator/jni_generator.py#newcode188 base/android/jni_generator/jni_generator.py:188: 'Lorg/chromium/net/NetworkChangeNotifier', On 2012/06/06 10:57:38, bulach wrote: > ...

8 years, 6 months ago (2012-06-06 11:36:58 UTC) #35

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/10206014/97005

8 years, 6 months ago (2012-06-06 11:37:18 UTC) #36

joth

https://chromiumcodereview.appspot.com/10206014/diff/31004/net/proxy/proxy_config_service_android.cc File net/proxy/proxy_config_service_android.cc (right): https://chromiumcodereview.appspot.com/10206014/diff/31004/net/proxy/proxy_config_service_android.cc#newcode165 net/proxy/proxy_config_service_android.cc:165: // Question: Conversion to/from UTF8 ok here? On 2012/05/15 ...

8 years, 6 months ago (2012-06-21 01:12:43 UTC) #38

Philippe

8 years, 6 months ago (2012-06-21 11:32:55 UTC) #39

https://chromiumcodereview.appspot.com/10206014/diff/31004/net/proxy/proxy_co...
File net/proxy/proxy_config_service_android.cc (right):

https://chromiumcodereview.appspot.com/10206014/diff/31004/net/proxy/proxy_co...
net/proxy/proxy_config_service_android.cc:165: // Question: Conversion to/from
UTF8 ok here?
On 2012/06/21 01:12:43, joth wrote:
> On 2012/05/15 16:12:48, Philippe wrote:
> > On 2012/05/14 18:00:25, Ryan Sleevi wrote:
> > > Should there be a TODO and a bug followed for follow-up?
> > > 
> > > Otherwise, this comment will never go answered and just serve to confuse
> > future
> > > readers as to whether or not there is or isn't a bug a here.
> > 
> > I made it a TODO and will file a bug.
> 
> Was there a bug?
> AFAICT UTF8 should be fine: the property names are well known and all simple
> ascii.
> The results should only be hostnames (with possible wild card or | delim) and
so
> again ascii. If we do get invalid uf8 back, the device is broken, but anyway
the
> converter will drop it and give us an empty string thereby disabling that part
> of proxy support which is probably the least worse thing to do in that
scenario.

No, it seems that I forgot to create the bug :/ I'm glad that there is finally
no bug here. I will create a CL to remove the TODO.

Thanks Jonathan!

Issue 10206014: Upstream Android proxy config service. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Sync #

Patch Set 3 : Add proxy_test_cases.py #

Patch Set 4 : Address Ryan's comments #

Patch Set 5 : Address Ryan and Yaron's comments #

Patch Set 6 : Address Ryan's comments #

Patch Set 7 : Pass scoped_refptr by const reference #

Patch Set 8 : Address Ryan's comments #

Patch Set 9 : Address Ryan's comments #

Patch Set 10 : Fix comment #

Patch Set 11 : Address Ryan's comments #

Patch Set 12 : Remove a DCHECK for consistency #

Patch Set 13 : Make Java methods non-synchronized #

Patch Set 14 : Address Ryan's comments #

Patch Set 15 : Fix two threading issues + Sync #

Patch Set 16 : Address Ryan's comments #

Patch Set 17 : Address Ryan's comments #

Patch Set 18 : Fix comment #

Patch Set 19 : Address Marcus' comments #

Messages