Fix race condition in NetworkChangeNotifier on Android.

Fix race condition in NetworkChangeNotifier on Android.

NetworkChangeNotifier::GetCurrentConnectionType() is called on any thread,
including the network thread.
Its implementation on Android calls some Java code (from any thread) which
indirectly reads a primitive member variable set from the UI thread.

The lack of synchronization could make
NetworkChangeNotifier::GetCurrentConnectionType() return an out-dated result on
multi-core devices.

BUG=143433
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157804

[digit1, 2012-09-14 09:13:44]

https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:79:
mAutoDetectorObserver = null;
Are you sure you want to set this field to null? It seems there is no way to
recreate the object once this is done. This might be a problem when calling
setAutoDetectConnectivityStateInternal() twice (once with false, then another
time with true).

https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:124: return
mConnectionType.get();
This patch is good, but I believe that this function is only called from native
code through JNI. As such, it might be more efficient to store the atomic
integer value in a native method. This would avoid the JNI call everytime
reading it is required.

[digit1, 2012-09-14 09:19:26]

lgtm (with philippe's remark). Thanks.

[digit1, 2012-09-14 09:26:27]

On 2012/09/14 09:19:26, digit1 wrote:
> lgtm (with philippe's remark). Thanks.

Aarg, sorry, this was for a different patch :-p

[Philippe, 2012-09-14 09:27:39]

https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:79:
mAutoDetectorObserver = null;
On 2012/09/14 09:13:45, digit1 wrote:
> Are you sure you want to set this field to null? It seems there is no way to
> recreate the object once this is done. This might be a problem when calling
> setAutoDetectConnectivityStateInternal() twice (once with false, then another
> time with true).

Indeed!

https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:124: return
mConnectionType.get();
On 2012/09/14 09:13:45, digit1 wrote:
> This patch is good, but I believe that this function is only called from
native
> code through JNI. As such, it might be more efficient to store the atomic
> integer value in a native method. This would avoid the JNI call everytime
> reading it is required.

Good point. I'm preparing a new patch set for that (which will be quite
different from this one).
Performance seems important here since network_change_notifier.h explicitly
states that GetCurrentConnectionType() must be cheap.

[Philippe, 2012-09-14 11:23:56]

On 2012/09/14 09:27:39, Philippe wrote:
>
https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
> File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):
> 
>
https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
> net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:79:
> mAutoDetectorObserver = null;
> On 2012/09/14 09:13:45, digit1 wrote:
> > Are you sure you want to set this field to null? It seems there is no way to
> > recreate the object once this is done. This might be a problem when calling
> > setAutoDetectConnectivityStateInternal() twice (once with false, then
another
> > time with true).
> 
> Indeed!
> 
>
https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
> net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:124: return
> mConnectionType.get();
> On 2012/09/14 09:13:45, digit1 wrote:
> > This patch is good, but I believe that this function is only called from
> native
> > code through JNI. As such, it might be more efficient to store the atomic
> > integer value in a native method. This would avoid the JNI call everytime
> > reading it is required.
> 
> Good point. I'm preparing a new patch set for that (which will be quite
> different from this one).
> Performance seems important here since network_change_notifier.h explicitly
> states that GetCurrentConnectionType() must be cheap.

After I double checked, there is a bit more work to do.
NetworkChangeNotifier::NetworkChangeNotifier() can also be called on any thread
according to network_change_notifier.h. Right now the Java side assumes that all
the calls are done on a single thread.
I'm addressing that and will upload a new patch set.

[Philippe, 2012-09-14 12:24:49]

On 2012/09/14 11:23:56, Philippe wrote:
> On 2012/09/14 09:27:39, Philippe wrote:
> >
>
https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
> > File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java
(right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
> > net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:79:
> > mAutoDetectorObserver = null;
> > On 2012/09/14 09:13:45, digit1 wrote:
> > > Are you sure you want to set this field to null? It seems there is no way
to
> > > recreate the object once this is done. This might be a problem when
calling
> > > setAutoDetectConnectivityStateInternal() twice (once with false, then
> another
> > > time with true).
> > 
> > Indeed!
> > 
> >
>
https://chromiumcodereview.appspot.com/10905264/diff/1/net/android/java/src/o...
> > net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:124: return
> > mConnectionType.get();
> > On 2012/09/14 09:13:45, digit1 wrote:
> > > This patch is good, but I believe that this function is only called from
> > native
> > > code through JNI. As such, it might be more efficient to store the atomic
> > > integer value in a native method. This would avoid the JNI call everytime
> > > reading it is required.
> > 
> > Good point. I'm preparing a new patch set for that (which will be quite
> > different from this one).
> > Performance seems important here since network_change_notifier.h explicitly
> > states that GetCurrentConnectionType() must be cheap.
> 
> After I double checked, there is a bit more work to do.
> NetworkChangeNotifier::NetworkChangeNotifier() can also be called on any
thread
> according to network_change_notifier.h. Right now the Java side assumes that
all
> the calls are done on a single thread.
> I'm addressing that and will upload a new patch set.

I see at least two approaches to address these thread-safety issues:

1) Making sure on the native side that calls to Java are always done on the UI
thread. Although this means no lock, it would rely on Chromium's heavy weapons
(e.g. PostTask, maybe a ref-counted thread-safe delegate) which would make the
code much more complicated (I'm ok with it since these are common practices).

2) Adding synchronization (lock) to the Java side. I know that some of you don't
like locks :).

I was starting with 1).

What do you guys think?

[bulach, 2012-09-14 14:26:19]

if I understood digit's suggestion, we could try to split like:

- java class works entirely in the thread it's created. it doesn't hold any
value there.
- it notifies the native side, passing the new integer by value.

- the native side is then made thread safe: the "set" and the "get" will have
locks around its "connection_type_" member (since the contract says it can be
called from _any_ thread... if this wasn't the case, we could just PostTask with
the specific value from the native side of the JNI, into the specific thread for
the get... but that doesn't seem to be possible here)

wdyt?

[Philippe, 2012-09-14 14:46:30]

On 2012/09/14 14:26:19, bulach wrote:
> if I understood digit's suggestion, we could try to split like:
> 
> - java class works entirely in the thread it's created. it doesn't hold any
> value there.
> - it notifies the native side, passing the new integer by value.
> 
> - the native side is then made thread safe: the "set" and the "get" will have
> locks around its "connection_type_" member (since the contract says it can be
> called from _any_ thread... if this wasn't the case, we could just PostTask
with
> the specific value from the native side of the JNI, into the specific thread
for
> the get... but that doesn't seem to be possible here)
> 
> wdyt?

Your third point is right. I have already a patch I was about to send using this
approach.
The problem (which I might address in another CL) is that the Java class can be
instantiated on any thread (thus its unprotected state set on any thread)
although it is then used by the UI thread. In practice that might not be a big
problem.
I will fix the connection type issue in this CL first anyway and we can address
other issues in next CLs.

[digit1, 2012-09-14 14:46:30]

On 2012/09/14 14:26:19, bulach wrote:
> if I understood digit's suggestion, we could try to split like:
> 
> - java class works entirely in the thread it's created. it doesn't hold any
> value there.

I'm looking into the issue with philippe. It's slightly more complicated than
that, because the Java class constructor (and destructor) are called from C++
code, which itself can be called from either the UI or network thread. The
current code is definitely missing synchronization primitives to avoid issues
when this happens (even though this should be rare in practice, compared to
actually using the class).

> - it notifies the native side, passing the new integer by value.
> 
Yes.

> - the native side is then made thread safe: the "set" and the "get" will have
> locks around its "connection_type_" member (since the contract says it can be
> called from _any_ thread...

Exactly, and this ensures that the C++ code that implements the
net::base::NetworkChangeNotifier::GetConnectionType() is 'thread-safe and
cheap', as stated by its documentation in net/base/network_change_notifier.h.

In other words, this saves a costly JNI call, compared to the proposed path set
2 above (which only solves the 'thread-safe' part).

 if this wasn't the case, we could just PostTask with
> the specific value from the native side of the JNI, into the specific thread
for
> the get... but that doesn't seem to be possible here)
> 
I don't think this will be necessary at all :)

> wdyt?

[Philippe, 2012-09-14 15:38:22]

I've sent a new patch set addressing David's comment. The current connection
type is now stored on the native side so that GetCurrentConnectionType() is as
cheap as possible.

I'm not 100% sure on the use of atomicops. Let me know if you prefer a lock
instead.

We still might have threading issues at  construction/destruction as discussed
before.

[Ryan Sleevi, 2012-09-14 15:51:52]

Please do not use AtomicOps here.

Please do not use a lock either (see Chromium-dev thread on locks and
threading)
On Sep 14, 2012 8:38 AM, <pliard@chromium.org> wrote:

> I've sent a new patch set addressing David's comment. The current
> connection
> type is now stored on the native side so that GetCurrentConnectionType()
> is as
> cheap as possible.
>
> I'm not 100% sure on the use of atomicops. Let me know if you prefer a lock
> instead.
>
> We still might have threading issues at  construction/destruction as
> discussed
> before.
>
>
http://codereview.chromium.**org/10905264/<http://codereview.chromium.org/109...
>

[bulach, 2012-09-14 15:51:53]

thanks phillippe! a couple of suggestions below, hopefully it'll simplify the
java side:

http://codereview.chromium.org/10905264/diff/11001/net/android/java/src/org/c...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

http://codereview.chromium.org/10905264/diff/11001/net/android/java/src/org/c...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:75:
mAutoDetectorObserver = new NetworkChangeNotifierAutoDetect.Observer() {
nit: isn't 74 passing null as mAutoDetectorObserver? should swap these two
lines?
or even more java-ish, the member mAutoDetectorObserver doesn't seem to be used
at all, so just pass an anonymous class here..

http://codereview.chromium.org/10905264/diff/11001/net/android/java/src/org/c...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:105:
nativeGetConnectionType(mNativeChangeNotifier) != CONNECTION_NONE;
this is going down to native twice, maybe best consolidate in one single call,
and pass the "force" param down to native?

[felipeg_google, 2012-09-14 16:02:46]

Hi Ryan,
If I read correctly the thread in Chromium-dev, the whole issue regarding
locks is how they are used in each situation.
There are situations where makes sense to use the PostTask pattern, while
other situations we can use locks.
And in every situation we should try to avoid having a shared state when we
can afford that.

Can you clarify why you don't want pliard to use locks neither AtomicOps
here ?
What is the reason for that ?


On Fri, Sep 14, 2012 at 5:51 PM, Ryan Sleevi <rsleevi@chromium.org> wrote:

> Please do not use AtomicOps here.
>
> Please do not use a lock either (see Chromium-dev thread on locks and
> threading)
> On Sep 14, 2012 8:38 AM, <pliard@chromium.org> wrote:
>
>> I've sent a new patch set addressing David's comment. The current
>> connection
>> type is now stored on the native side so that GetCurrentConnectionType()
>> is as
>> cheap as possible.
>>
>> I'm not 100% sure on the use of atomicops. Let me know if you prefer a
>> lock
>> instead.
>>
>> We still might have threading issues at  construction/destruction as
>> discussed
>> before.
>>
>>
http://codereview.chromium.**org/10905264/<http://codereview.chromium.org/109...
>>
>

[felipeg_google, 2012-09-14 16:03:56]

Sorry, sent the email too early.

Could you also propose a solution that doesn't use AtomicOps neither Locks ?
A design with no shared state would be good.


On Fri, Sep 14, 2012 at 6:02 PM, Felipe Goldstein <felipeg@google.com>wrote:

> Hi Ryan,
> If I read correctly the thread in Chromium-dev, the whole issue regarding
> locks is how they are used in each situation.
> There are situations where makes sense to use the PostTask pattern, while
> other situations we can use locks.
> And in every situation we should try to avoid having a shared state when
> we can afford that.
>
> Can you clarify why you don't want pliard to use locks neither AtomicOps
> here ?
> What is the reason for that ?
>
>
> On Fri, Sep 14, 2012 at 5:51 PM, Ryan Sleevi <rsleevi@chromium.org> wrote:
>
>> Please do not use AtomicOps here.
>>
>> Please do not use a lock either (see Chromium-dev thread on locks and
>> threading)
>> On Sep 14, 2012 8:38 AM, <pliard@chromium.org> wrote:
>>
>>> I've sent a new patch set addressing David's comment. The current
>>> connection
>>> type is now stored on the native side so that GetCurrentConnectionType()
>>> is as
>>> cheap as possible.
>>>
>>> I'm not 100% sure on the use of atomicops. Let me know if you prefer a
>>> lock
>>> instead.
>>>
>>> We still might have threading issues at  construction/destruction as
>>> discussed
>>> before.
>>>
>>>
http://codereview.chromium.**org/10905264/<http://codereview.chromium.org/109...
>>>
>>
>

[digit1, 2012-09-14 16:04:35]

On 2012/09/14 15:51:52, Ryan Sleevi wrote:
> Please do not use AtomicOps here.
> 
> Please do not use a lock either (see Chromium-dev thread on locks and
> threading)

Ryan, a lock is perfectly adequate here, since it's only used to synchonize
writes and reads to a single integer value. This will never block the UI thread
more than the duration of a write.

Atomic ops are a small optimizations in this specific case, but I'm pretty sure
the performance increase will be minimal (simple pthread mutexes are very fast
on Android, hopefully the Chromium wrappers don't add too much overhead).

[szym, 2012-09-14 16:10:21]

On 2012/09/14 16:04:35, digit1 wrote:
> On 2012/09/14 15:51:52, Ryan Sleevi wrote:
> > Please do not use AtomicOps here.
> > 
> > Please do not use a lock either (see Chromium-dev thread on locks and
> > threading)
> 
> Ryan, a lock is perfectly adequate here, since it's only used to synchonize
> writes and reads to a single integer value. This will never block the UI
thread
> more than the duration of a write.
> 
> Atomic ops are a small optimizations in this specific case, but I'm pretty
sure
> the performance increase will be minimal (simple pthread mutexes are very fast
> on Android, hopefully the Chromium wrappers don't add too much overhead).

I agree that passing a single int via message loop tasks is overkill. Passing
tasks does not really avoid mutexes (incoming_queue_lock_ is used to add
protected incoming_queue_ in MessageLoop).

Any other action (like calling observers) should never be done in a critical
section. One common approach is to simply pass the relevant piece of data to the
observers via Callbacks, especially if the data is lightweight.

[Philippe, 2012-09-14 16:19:28]

It seems that there is no way other than lock/atomicops in this specific case
since GetCurrentConnectionType() can be called from any thread.
If there was a single thread calling it (e.g. the network thread) then we could
indeed use message passing (post a task from the UI thread to the network
thread).

This is GetCurrentConnectionType()'s comment in network_change_notifier.h:
"Implementations must be thread-safe. Implementations must also be cheap as this
could be called (repeatedly) from the network thread.".
So it does mention the network thread but it also suggests that other threads
can call it.

http://codereview.chromium.org/10905264/diff/11001/net/android/java/src/org/c...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

http://codereview.chromium.org/10905264/diff/11001/net/android/java/src/org/c...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:75:
mAutoDetectorObserver = new NetworkChangeNotifierAutoDetect.Observer() {
On 2012/09/14 15:51:53, bulach wrote:
> nit: isn't 74 passing null as mAutoDetectorObserver? should swap these two
> lines?
> or even more java-ish, the member mAutoDetectorObserver doesn't seem to be
used
> at all, so just pass an anonymous class here..

Indeed. It was instantiated before in the constructor. It does not make sense
anymore.

http://codereview.chromium.org/10905264/diff/11001/net/android/java/src/org/c...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:105:
nativeGetConnectionType(mNativeChangeNotifier) != CONNECTION_NONE;
On 2012/09/14 15:51:53, bulach wrote:
> this is going down to native twice, maybe best consolidate in one single call,
> and pass the "force" param down to native?

This might make the native side a bit odd, what do you think?

Something like MaybeNotifyObservers(int newConnectionType, bool forceOnline)?
I'm ok with doing that although I'm not sure that a Java->native call is that
expensive (I was tempted to think it was cheap).
The main thing I'm concerned about is that we use notifyNativeObservers() in two
different scenarios: this one which does not use the auto-detector, where the
forceOnline makes sense and the other one using the auto-detector where it does
not make much sense.

What do you think?

[cbentzel, 2012-09-14 16:26:25]

+pauljensen

On Fri, Sep 14, 2012 at 12:19 PM, <pliard@chromium.org> wrote:

> It seems that there is no way other than lock/atomicops in this specific
> case
> since GetCurrentConnectionType() can be called from any thread.
> If there was a single thread calling it (e.g. the network thread) then we
> could
> indeed use message passing (post a task from the UI thread to the network
> thread).
>
> This is GetCurrentConnectionType()'s comment in network_change_notifier.h:
> "Implementations must be thread-safe. Implementations must also be cheap
> as this
> could be called (repeatedly) from the network thread.".
> So it does mention the network thread but it also suggests that other
> threads
> can call it.
>
>
>
> http://codereview.chromium.**org/10905264/diff/11001/net/**
>
android/java/src/org/chromium/**net/NetworkChangeNotifier.java<http://codereview.chromium.org/10905264/diff/11001/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java>
> File net/android/java/src/org/**chromium/net/**NetworkChangeNotifier.java
> (right):
>
> http://codereview.chromium.**org/10905264/diff/11001/net/**
>
android/java/src/org/chromium/**net/NetworkChangeNotifier.**java#newcode75<http://codereview.chromium.org/10905264/diff/11001/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java#newcode75>
> net/android/java/src/org/**chromium/net/**NetworkChangeNotifier.java:75:
> mAutoDetectorObserver = new NetworkChangeNotifierAutoDetec**t.Observer() {
> On 2012/09/14 15:51:53, bulach wrote:
>
>> nit: isn't 74 passing null as mAutoDetectorObserver? should swap these
>>
> two
>
>> lines?
>> or even more java-ish, the member mAutoDetectorObserver doesn't seem
>>
> to be used
>
>> at all, so just pass an anonymous class here..
>>
>
> Indeed. It was instantiated before in the constructor. It does not make
> sense anymore.
>
>
> http://codereview.chromium.**org/10905264/diff/11001/net/**
> android/java/src/org/chromium/**net/NetworkChangeNotifier.**
>
java#newcode105<http://codereview.chromium.org/10905264/diff/11001/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java#newcode105>
> net/android/java/src/org/**chromium/net/**NetworkChangeNotifier.java:**
> 105:
> nativeGetConnectionType(**mNativeChangeNotifier) != CONNECTION_NONE;
> On 2012/09/14 15:51:53, bulach wrote:
>
>> this is going down to native twice, maybe best consolidate in one
>>
> single call,
>
>> and pass the "force" param down to native?
>>
>
> This might make the native side a bit odd, what do you think?
>
> Something like MaybeNotifyObservers(int newConnectionType, bool
> forceOnline)?
> I'm ok with doing that although I'm not sure that a Java->native call is
> that expensive (I was tempted to think it was cheap).
> The main thing I'm concerned about is that we use
> notifyNativeObservers() in two different scenarios: this one which does
> not use the auto-detector, where the forceOnline makes sense and the
> other one using the auto-detector where it does not make much sense.
>
> What do you think?
>
>
http://codereview.chromium.**org/10905264/<http://codereview.chromium.org/109...
>

[szym, 2012-09-14 18:25:22]

http://codereview.chromium.org/10905264/diff/1007/net/android/network_change_...
File net/android/network_change_notifier_android.cc (right):

http://codereview.chromium.org/10905264/diff/1007/net/android/network_change_...
net/android/network_change_notifier_android.cc:45:
CheckConnectionType(new_connection_type) ?
Do this jint -> int and jint -> AtomicWord implicit conversions work? I recall
there was some reason for the explicit switch statement in
GetCurrentConnectionType.

nit: Create a new temporary variable (e.g., |known_connection_type|) to make it
more readable.

http://codereview.chromium.org/10905264/diff/1007/net/android/network_change_...
File net/android/network_change_notifier_android.h (right):

http://codereview.chromium.org/10905264/diff/1007/net/android/network_change_...
net/android/network_change_notifier_android.h:40: base::subtle::AtomicWord
connection_type_;
In an offline conversation with pauljensen@ we pondered if you could wrap all
the "subtlety" in a class, like ThreadsafeWord { Read(), Write(...) } Probably
best to keep it in this source file for now.

That said, I wonder if this memory barrier is actually buying you anything.
There are two categories of GetConnectionType callers: OnConnectionTypeChanged,
where the value is passed via callback, and NCN::IsOffline. This CL does not
affect the first category, and I'm not sure if makes a difference for the second
either.

[Ryan Sleevi, 2012-09-14 19:03:58]

On 2012/09/14 16:26:25, cbentzel wrote:
> +pauljensen

It is "unfortunate", but it looks like the existing Mac & Linux implementations
are already using heavy-handed locking to handle GetCurrentConnectionType().

In general, we try to avoid the use of AtomicWord as a threading solution (which
the fact that it's in base::subtle:: is trying to hint at). Outside of base/, it
is used in only 4 files. Oh those, two are specifically and explicitly in places
where it's unsafe to use other functionality from base/ (chromoting crash
handling & installer stub), one is in Java bridge code, and one is in thread
management code.

To resolve this, base/ provides wrappers for the subtle code (eg:
StaticAtomicSequenceNumber, AtomicRefCount, AtomicSequenceNum, Singleton, etc),
and callers are expected to use those.

jar and I were recently discussing this in the context of another review, and in
casually mentioning this use, we're in agreement that, just like the file header
says, it's better for readability, reasoning, and maintainability to use a
base::Lock here (same as Mac and Linux). Per the comments in base::subtle

http://src.chromium.org/viewvc/chrome/trunk/src/base/atomicops.h?annotate=152...

// The routines exported by this module are subtle.  If you use them, even if
// you get the code right, it will depend on careful reasoning about atomicity
// and memory ordering; it will be less readable, and harder to maintain.  If
// you plan to use these routines, you should have a good reason, such as solid
// evidence that performance would otherwise suffer, or there being no
// alternative.

[pauljensen, 2012-09-14 19:53:54]

I tend to agree that the base::subtle:: usage here is not justified because of
readability, reasoning, and maintainability concerns; and, using a lock like the
other hosts do is probably the best solution at the moment.

That being said I think Philippe's code is so wonderfully optimal and efficient
that its evidence that we really need a base/ wrapper for a thread-safe integer
so we have a clean way to do this going forward.  I've only been working on
Chrome two months and this is the second time I've seen the need for this sort
of thing; here's an example of needing to lock a bool:
https://chromiumcodereview.appspot.com/10377092/patch/1085/13038
On x86 there is essentially zero overhead for what Philippe was doing.  In the
best case locks are significantly slower than Philippe's code, and in the worst
case, however rare, locks can block which is at least an order of magnitude
slower, even if the unblocking occurs immediately.  Calls that randomly run an
order of magnitude slower can make for very hard to reproduce bugs.

[Ryan Sleevi, 2012-09-14 20:53:30]

On 2012/09/14 19:53:54, pauljensen wrote:
> I tend to agree that the base::subtle:: usage here is not justified because of
> readability, reasoning, and maintainability concerns; and, using a lock like
the
> other hosts do is probably the best solution at the moment.
> 
> That being said I think Philippe's code is so wonderfully optimal and
efficient
> that its evidence that we really need a base/ wrapper for a thread-safe
integer
> so we have a clean way to do this going forward.  I've only been working on
> Chrome two months and this is the second time I've seen the need for this sort
> of thing; here's an example of needing to lock a bool:
> https://chromiumcodereview.appspot.com/10377092/patch/1085/13038
> On x86 there is essentially zero overhead for what Philippe was doing.  In the
> best case locks are significantly slower than Philippe's code, and in the
worst
> case, however rare, locks can block which is at least an order of magnitude
> slower, even if the unblocking occurs immediately.  Calls that randomly run an
> order of magnitude slower can make for very hard to reproduce bugs.

Yes, I agree, which is why I mentioned "unfortunate" - all three implementations
*could* be done using atomics. Unfortunately, it means that anyone who maintains
the code has to understand the atomic semantics AND then has to reason about the
performance characteristics (are these atomics because they needed to be, or
because they could be?)

C++11 provides somewhat suitable wrappers for atomics, via std::atomic<>, but
that doesn't really help with the mental modelling (since now you have to reason
about the value of std::memory_order).

An Atomic<Foo> type that had Acquire_Load+Release_Store semantics may make
sense, and people like jar and willchan should review. The threshold for getting
code into base/ is fairly high, so I think it'd need to be demonstrated that the
existing uses are substantial enough to need a generic way (and I think it may
be - and that some existing usages of base::subtle: should be converted over to
existing types)

[Philippe, 2012-09-19 17:35:14]

I uploaded a new patch set. Sorry guys for the delay, I got distracted with perf
and traveling (I'm now in MTV with Digit and Felipe for two weeks).

I'm using a lock in the new patch set. We can see what people think on
chromium-dev about the addition of a non-subtle atomic integer (or even
Atomic<T> with sizeof(T) <= sizeof(AtomicWord)?) once this CL is submitted (the
discussion might take a while).

http://chromiumcodereview.appspot.com/10905264/diff/1007/net/android/network_...
File net/android/network_change_notifier_android.h (right):

http://chromiumcodereview.appspot.com/10905264/diff/1007/net/android/network_...
net/android/network_change_notifier_android.h:40: base::subtle::AtomicWord
connection_type_;
On 2012/09/14 18:25:23, szym wrote:
> In an offline conversation with pauljensen@ we pondered if you could wrap all
> the "subtlety" in a class, like ThreadsafeWord { Read(), Write(...) } Probably
> best to keep it in this source file for now.
> 
> That said, I wonder if this memory barrier is actually buying you anything.
> There are two categories of GetConnectionType callers:
OnConnectionTypeChanged,
> where the value is passed via callback, and NCN::IsOffline. This CL does not
> affect the first category, and I'm not sure if makes a difference for the
second
> either.

I used a Lock with a TODO instead after Ryan and Paul's replies.
I think the barrier/mutex is needed regardless of the clients. The issue was
that there was no guarantee that the value returned by
GetCurrentConnectionType() was up-to-date (i.e. the one just set from the UI
thread) on multi-core ARM. That might not happen in practice but it seemed
pretty dangerous. I might be misunderstanding something though.

[Ryan Sleevi, 2012-09-19 17:43:27]

https://chromiumcodereview.appspot.com/10905264/diff/16001/net/android/networ...
File net/android/network_change_notifier_android.cc (right):

https://chromiumcodereview.appspot.com/10905264/diff/16001/net/android/networ...
net/android/network_change_notifier_android.cc:45: new_connection_type :
CONNECTION_UNKNOWN);
nit: I feel this might read easier with a temporary (which should be optimized
out all the same anyways)

int connection_type = CheckConnectionType(...) ? ... : ...;
SetConnectionType(connection_type);

https://chromiumcodereview.appspot.com/10905264/diff/16001/net/android/networ...
net/android/network_change_notifier_android.cc:80: return
static_cast<net::NetworkChangeNotifier::ConnectionType>(
No need for net:: on line 77 or here (because of line 11)

[Philippe, 2012-09-19 17:57:04]

http://chromiumcodereview.appspot.com/10905264/diff/16001/net/android/network...
File net/android/network_change_notifier_android.cc (right):

http://chromiumcodereview.appspot.com/10905264/diff/16001/net/android/network...
net/android/network_change_notifier_android.cc:45: new_connection_type :
CONNECTION_UNKNOWN);
On 2012/09/19 17:43:27, Ryan Sleevi wrote:
> nit: I feel this might read easier with a temporary (which should be optimized
> out all the same anyways)
> 
> int connection_type = CheckConnectionType(...) ? ... : ...;
> SetConnectionType(connection_type);

Done.

http://chromiumcodereview.appspot.com/10905264/diff/16001/net/android/network...
net/android/network_change_notifier_android.cc:80: return
static_cast<net::NetworkChangeNotifier::ConnectionType>(
On 2012/09/19 17:43:27, Ryan Sleevi wrote:
> No need for net:: on line 77 or here (because of line 11)

Indeed.

[szym, 2012-09-19 18:46:55]

https://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/networ...
File net/android/network_change_notifier_android.cc (right):

https://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/networ...
net/android/network_change_notifier_android.cc:64: void
NetworkChangeNotifier::SetConnectionType(int connection_type) {
nit: make order of SetConnectionType and CreateJavaObject the same here and in
the header file.

https://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/networ...
net/android/network_change_notifier_android.cc:77:
net::NetworkChangeNotifier::ConnectionType
No need for net:: here either.

https://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/networ...
File net/android/network_change_notifier_android.h (right):

https://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/networ...
net/android/network_change_notifier_android.h:43: // never happen though.
Add relevant reference to crbug.com/

https://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/networ...
net/android/network_change_notifier_android.h:45: // Wrote from the UI thread,
read from any thread.
nit: "Written"

[Philippe, 2012-09-19 20:57:56]

http://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/network...
File net/android/network_change_notifier_android.cc (right):

http://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/network...
net/android/network_change_notifier_android.cc:64: void
NetworkChangeNotifier::SetConnectionType(int connection_type) {
On 2012/09/19 18:46:55, szym wrote:
> nit: make order of SetConnectionType and CreateJavaObject the same here and in
> the header file.

Done.

http://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/network...
net/android/network_change_notifier_android.cc:77:
net::NetworkChangeNotifier::ConnectionType
On 2012/09/19 18:46:55, szym wrote:
> No need for net:: here either.

Done.

http://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/network...
File net/android/network_change_notifier_android.h (right):

http://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/network...
net/android/network_change_notifier_android.h:43: // never happen though.
On 2012/09/19 18:46:55, szym wrote:
> Add relevant reference to crbug.com/

Done.

http://chromiumcodereview.appspot.com/10905264/diff/21001/net/android/network...
net/android/network_change_notifier_android.h:45: // Wrote from the UI thread,
read from any thread.
On 2012/09/19 18:46:55, szym wrote:
> nit: "Written"

Oops :)

[Ryan Sleevi, 2012-09-19 21:16:31]

LGTM, but one question below.

https://chromiumcodereview.appspot.com/10905264/diff/19002/net/android/networ...
File net/android/network_change_notifier_android.cc (right):

https://chromiumcodereview.appspot.com/10905264/diff/19002/net/android/networ...
net/android/network_change_notifier_android.cc:64: void
NetworkChangeNotifier::CreateJavaObject(JNIEnv* env) {
Is there a reason CreateJavaObject needs to take the JNIEnv*, rather than just
having it call AttachCurrentThread itself (rather than line 60).

I just wasn't sure if this was a JNI idiom. If not, it seems like it would make
sense just to move the AttachCurrentThread inside here.

[Philippe, 2012-09-19 21:34:00]

http://chromiumcodereview.appspot.com/10905264/diff/19002/net/android/network...
File net/android/network_change_notifier_android.cc (right):

http://chromiumcodereview.appspot.com/10905264/diff/19002/net/android/network...
net/android/network_change_notifier_android.cc:64: void
NetworkChangeNotifier::CreateJavaObject(JNIEnv* env) {
On 2012/09/19 21:16:31, Ryan Sleevi wrote:
> Is there a reason CreateJavaObject needs to take the JNIEnv*, rather than just
> having it call AttachCurrentThread itself (rather than line 60).
> 
> I just wasn't sure if this was a JNI idiom. If not, it seems like it would
make
> sense just to move the AttachCurrentThread inside here.

Good point.

I think that the main motivation behind that, which is not the case here, is to
minimize the calls to AttachCurrentThread(). Sometimes the caller also uses
|env| so people tend to make their JNI functions take it in rather than calling
AttachCurrentThread() again.
This might be premature optimization though. I have no idea how expensive
AttachCurrentThread() is.

I think it's good not to unnecessarily expose an extra JNIEnv* in the header
especially in this case since the caller doesn't use it.
I got rid of CreateJavaObject() since it's used only once in the constructor.
Let me know if you want to keep it.

[digit1, 2012-09-19 21:54:25]

Please always assume that JNI operations are costly, even AttachCurrentThread().

The performance of JNI operations is very dependent on the VM itself, and since
our code is supposed to run on various releases of the Android platform, better
be prepared for the worst.

As an anecdote, the performance of most JNI operations degraded when the
concurrent garbage collector was introduced in Gingerbread. They were later
degraded in ICS, iirc, due to other optimizations that resulted in modifications
to the implementation of local and global references.

Even if calling AttachCurrentThread on an already attached thread is a no-op, it
is not free in terms of performance.

[Philippe, 2012-09-19 21:57:08]

On 2012/09/19 21:54:25, digit1 wrote:
> Please always assume that JNI operations are costly, even
AttachCurrentThread().
> 
> The performance of JNI operations is very dependent on the VM itself, and
since
> our code is supposed to run on various releases of the Android platform,
better
> be prepared for the worst.
> 
> As an anecdote, the performance of most JNI operations degraded when the
> concurrent garbage collector was introduced in Gingerbread. They were later
> degraded in ICS, iirc, due to other optimizations that resulted in
modifications
> to the implementation of local and global references.
> 
> Even if calling AttachCurrentThread on an already attached thread is a no-op,
it
> is not free in terms of performance.

Good to know, thanks David for the details.

[gone, 2012-09-19 21:57:31]

On 2012/09/19 21:34:00, Philippe wrote:
> I got rid of CreateJavaObject() since it's used only once in the constructor.
> Let me know if you want to keep it.

Made the same call in my related CL.  Not sure why it was separated in the first
place.

[pasko-google - do not use, 2012-09-19 22:26:34]

On 2012/09/19 21:54:25, digit1 wrote:
> Please always assume that JNI operations are costly, even
AttachCurrentThread().
> 
> The performance of JNI operations is very dependent on the VM itself, and
since
> our code is supposed to run on various releases of the Android platform,
better
> be prepared for the worst.
> 
> As an anecdote, the performance of most JNI operations degraded when the
> concurrent garbage collector was introduced in Gingerbread. They were later
> degraded in ICS, iirc, due to other optimizations that resulted in
modifications
> to the implementation of local and global references.
> 
> Even if calling AttachCurrentThread on an already attached thread is a no-op,
it
> is not free in terms of performance.

2c:

my intuition suggests that AttachCurrentThread() would be even slower than
normal JNI calls since it's talking to other thread(s).

[Philippe, 2012-09-19 22:36:13]

On 2012/09/19 22:26:34, pasko wrote:
> On 2012/09/19 21:54:25, digit1 wrote:
> > Please always assume that JNI operations are costly, even
> AttachCurrentThread().
> > 
> > The performance of JNI operations is very dependent on the VM itself, and
> since
> > our code is supposed to run on various releases of the Android platform,
> better
> > be prepared for the worst.
> > 
> > As an anecdote, the performance of most JNI operations degraded when the
> > concurrent garbage collector was introduced in Gingerbread. They were later
> > degraded in ICS, iirc, due to other optimizations that resulted in
> modifications
> > to the implementation of local and global references.
> > 
> > Even if calling AttachCurrentThread on an already attached thread is a
no-op,
> it
> > is not free in terms of performance.
> 
> 2c:
> 
> my intuition suggests that AttachCurrentThread() would be even slower than
> normal JNI calls since it's talking to other thread(s).

I'm testing this in our internal repository and will submit this CL once you
approved it too Szymon.
After that I will send an e-mail to chromium-dev about the addition of
base::Atomic<T>.

[digit1, 2012-09-19 22:36:46]

fwiw, the *current* implementation in Dalvik essentially does a
pthread_getspecific(), and returns if the thread is already attached (the no-op
case). However, this is wrapped by three nested function calls (each one adding
some parameters), the first one being performed through a double indirection
(like a C++ virtual method call).

I wouldn't call this "light" (in terms of cache trashing / stack operations),
and there is no guarantee that this won't get worse in the future.

Ok, this is getting  bit off-topic, let's get back to the patch itself ;)

[szym, 2012-09-19 22:38:27]

lgtm

[Yaron, 2012-09-20 00:14:42]

This seems fine but you'll have to rebase on top of
http://codereview.chromium.org/10928193/.

[commit-bot: I haz the power, 2012-09-20 16:49:15]

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

[commit-bot: I haz the power, 2012-09-20 18:52:05]

Change committed as 157804

[gone, 2012-09-20 21:52:01]

https://chromiumcodereview.appspot.com/10905264/diff/34001/net/android/networ...
File net/android/network_change_notifier_android.cc (right):

https://chromiumcodereview.appspot.com/10905264/diff/34001/net/android/networ...
net/android/network_change_notifier_android.cc:44: new_connection_type :
CONNECTION_UNKNOWN;
Just saw this when rebasing my patch; this overloads the CONNECTION_UNKNOWN
type.  CONNECTION_UNKNOWN implies that there is a network connection, but that
we don't know what kind it is.  Is this intentional?

[szym, 2012-09-20 21:54:19]

https://chromiumcodereview.appspot.com/10905264/diff/34001/net/android/networ...
File net/android/network_change_notifier_android.cc (right):

https://chromiumcodereview.appspot.com/10905264/diff/34001/net/android/networ...
net/android/network_change_notifier_android.cc:44: new_connection_type :
CONNECTION_UNKNOWN;
On 2012/09/20 21:52:01, dfalcantara wrote:
> Just saw this when rebasing my patch; this overloads the CONNECTION_UNKNOWN
> type.  CONNECTION_UNKNOWN implies that there is a network connection, but that
> we don't know what kind it is.  Is this intentional?

I believe so. CONNECTION_NONE is used to indicate we are offline. Paul?

[pauljensen, 2012-09-20 22:05:25]

On 2012/09/20 21:54:19, szym wrote:
>
https://chromiumcodereview.appspot.com/10905264/diff/34001/net/android/networ...
> File net/android/network_change_notifier_android.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10905264/diff/34001/net/android/networ...
> net/android/network_change_notifier_android.cc:44: new_connection_type :
> CONNECTION_UNKNOWN;
> On 2012/09/20 21:52:01, dfalcantara wrote:
> > Just saw this when rebasing my patch; this overloads the CONNECTION_UNKNOWN
> > type.  CONNECTION_UNKNOWN implies that there is a network connection, but
that
> > we don't know what kind it is.  Is this intentional?
> 
> I believe so. CONNECTION_NONE is used to indicate we are offline. Paul?

We'll only use the CONNECTION_UNKNOWN from line 44 when we hit NOTREACHED() in
CheckConnectionType() so that should be fine.  It should be fine to fall back on
CONNECTION_UNKNOWN anyhow.  Other targets only ever produce CONNECTION_UNKNOWN
and CONNECTION_NONE.