Adding non-tracking mode support to AddressTracker.

The non-tracking mode in AddressTracker will be used as a way to get a snapshot of current system configuration from GetNetworkList whose current implementation doesn't provide IPv6 address attributes.

BUG=411086
Committed: https://crrev.com/f09e9971c053661b2113f38881381678af6f62b3
Cr-Commit-Position: refs/heads/master@{#296048}

[guoweis_left_chromium, 2014-09-13 16:25:47]

guoweis@chromium.org changed reviewers:
+ pauljensen@chromium.org

[guoweis_left_chromium, 2014-09-13 16:25:48]

@Paul, this is not final but mainly to start the design discussion.

My design goal is to instantiate very short-lived AddressTrackerLinux in each
invocation of GetNetworkList.

2 things that I'd like to get your review/opinion.

1. nl_pid has to be changed since it has to be unique. According to man page,
setting nl_pid to 0 before bind() should have kernel assign a unique one and
ret\
urn to us. http://man7.org/linux/man-pages/man7/netlink.7.html However, this is
not what I saw. I still see nl_pid back as 0. But the address notification mec\
hanism through netlink still seems to work fine in my test app on linux.

2. Is there any concern about AddressTrackerLinux being so short-lived? Could
there be any race condition b/w destructor of my local AddressTracker and a
netl\
ink event?

The alternative designs that I could see:

1. expose the global single instance of AddressTracker. However, on Android,
there seems to be complex threading issue w.r.t. JNI which could be risky
2. modify the AddressTracker to allow "non-listening" AddressTracker. i.e.
Netlink socket without bind/multicasting group/notification.

I'm porting my test app on Android to see how it behaves.

[pauljensen, 2014-09-17 16:31:38]

I could really go either way:
1. simply add a "bool track" option to the constructor of AddressTrackerLinux
that determines whether it should register to track the events
2. expose the global single instance of AddressTracker.  This is already exposed
on NetworkChangeNotifierLinux::GetAddressTrackerInternal() and we'd just be
exposing to net::GetNetworkList(), so it wouldn't really be exposed outside of
net::

[guoweis2, 2014-09-17 16:37:55]

On 2014/09/17 16:31:38, pauljensen wrote:
> I could really go either way:
> 1. simply add a "bool track" option to the constructor of AddressTrackerLinux
> that determines whether it should register to track the events
> 2. expose the global single instance of AddressTracker.  This is already
exposed
> on NetworkChangeNotifierLinux::GetAddressTrackerInternal() and we'd just be
> exposing to net::GetNetworkList(), so it wouldn't really be exposed outside of
> net::

Thanks. I also feel that adding a bool track would be the cleanest way to go
about this. I'll modify it accordingly and get back.

[guoweis2, 2014-09-17 21:38:16]

On 2014/09/17 16:37:55, guoweis2 wrote:
> On 2014/09/17 16:31:38, pauljensen wrote:
> > I could really go either way:
> > 1. simply add a "bool track" option to the constructor of
AddressTrackerLinux
> > that determines whether it should register to track the events
> > 2. expose the global single instance of AddressTracker.  This is already
> exposed
> > on NetworkChangeNotifierLinux::GetAddressTrackerInternal() and we'd just be
> > exposing to net::GetNetworkList(), so it wouldn't really be exposed outside
of
> > net::
> 
> Thanks. I also feel that adding a bool track would be the cleanest way to go
> about this. I'll modify it accordingly and get back.

Added a bool track option in AddressTrackerLinux::Init(). In theory, I could
even go further to only do locking when we're tracking but want to have you take
a look at this before I do that.

[pauljensen, 2014-09-18 13:56:15]

I'm liking the "bool track" option.  That seems like a great way to go.  As you
say, I think we should get rid of the extra new locking.  Maybe make a
constructor with "bool track" and then instead of locking do CHECK(!track).
I think we should change comment by the definition of Init() to say that in the
!track case all state should be available when Init() returns.

https://codereview.chromium.org/571743002/diff/20001/net/base/address_tracker...
File net/base/address_tracker_linux.cc (right):

https://codereview.chromium.org/571743002/diff/20001/net/base/address_tracker...
net/base/address_tracker_linux.cc:112: DCHECK(!link_callback.is_null());
I think these two lines will give you troubles when !track? or is there an easy
way to pass in null/empty Closures?

[guoweis2, 2014-09-18 19:41:11]

guoweis@google.com changed reviewers:
+ guoweis@google.com

[guoweis2, 2014-09-18 19:41:12]

PTAL. Add a non-tracking version of constructor.

https://codereview.chromium.org/571743002/diff/20001/net/base/address_tracker...
File net/base/address_tracker_linux.cc (right):

https://codereview.chromium.org/571743002/diff/20001/net/base/address_tracker...
net/base/address_tracker_linux.cc:112: DCHECK(!link_callback.is_null());
On 2014/09/18 13:56:15, pauljensen wrote:
> I think these two lines will give you troubles when !track? or is there an
easy
> way to pass in null/empty Closures?

There is a do nothing version used by unit tests.

[pauljensen, 2014-09-19 14:50:17]

I think we should add DCHECK(CalledOnValidThread()) in AddressTrackerAutoLock
when !tracking.
To do this I think it would be simplest to make AddressTrackerAutoLock a nested
private class of AddressTrackerLinux, and pass "this" to
AddressTrackerAutoLock's construtor.  I think this would allow
AddressTrackerAutoLock to access  |tracking| and |CalledOnValidThread()|.

Can you please update the description of this CL a bit.  "AddressTracker
change." is rather vague.
"addresstracker" should be CamelCased.  Please also reference a BUG= bug.

[chromium-reviews, 2014-09-19 17:39:47]

Thanks,
Guowei

On Fri, Sep 19, 2014 at 7:50 AM, <pauljensen@chromium.org> wrote:

> I think we should add DCHECK(CalledOnValidThread()) in
> AddressTrackerAutoLock
> when !tracking.
> To do this I think it would be simplest to make AddressTrackerAutoLock a
> nested
> private class of AddressTrackerLinux, and pass "this" to
> AddressTrackerAutoLock's construtor.  I think this would allow
> AddressTrackerAutoLock to access  |tracking| and |CalledOnValidThread()|.
>

Like the idea of adding the CalledOnValidThread() for non-tracking case to
ensure there is no unexpected race condition. However, AddressTrackerLinux
doesn't contain or inherit from base::ThreadChecker. I'm adding one into
the AddressTrackerAutoLock itself.


>
> Can you please update the description of this CL a bit.  "AddressTracker
> change." is rather vague.
> "addresstracker" should be CamelCased.  Please also reference a BUG= bug.
>
>
Will do.


>
>
>
> https://codereview.chromium.org/571743002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-09-19 18:03:59]

Thanks,
Guowei

On Fri, Sep 19, 2014 at 10:39 AM, Guo-wei Shieh <guoweis@google.com> wrote:

>
>
> Thanks,
> Guowei
>
> On Fri, Sep 19, 2014 at 7:50 AM, <pauljensen@chromium.org> wrote:
>
>> I think we should add DCHECK(CalledOnValidThread()) in
>> AddressTrackerAutoLock
>> when !tracking.
>> To do this I think it would be simplest to make AddressTrackerAutoLock a
>> nested
>> private class of AddressTrackerLinux, and pass "this" to
>> AddressTrackerAutoLock's construtor.  I think this would allow
>> AddressTrackerAutoLock to access  |tracking| and |CalledOnValidThread()|.
>>
>
> Like the idea of adding the CalledOnValidThread() for non-tracking case to
> ensure there is no unexpected race condition. However, AddressTrackerLinux
> doesn't contain or inherit from base::ThreadChecker. I'm adding one into
> the AddressTrackerAutoLock itself.
>

Actually, to think it more, if the purpose for adding this to non-tracking
mode is to prevent unexpected race condition (like a call back does happen
in non-tracking mode). How does this help? Each thread will get its own
stack object and each one will validate just fine, unless we make this a
member of AddressTrackLinux, but this loses the "auto" part of being on
stack.

Maybe I misunderstood your point?


>
>
>>
>> Can you please update the description of this CL a bit.  "AddressTracker
>> change." is rather vague.
>> "addresstracker" should be CamelCased.  Please also reference a BUG= bug.
>>
>>
> Will do.
>
>
>>
>>
>>
>> https://codereview.chromium.org/571743002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-09-19 18:05:21]

I think I understand what you meant now. The thread_checker_ will be a
member class of AddressTrackerLinux which will achieve the effect. Will
code accordingly.

Thanks,
Guowei

On Fri, Sep 19, 2014 at 11:03 AM, Guo-wei Shieh <guoweis@google.com> wrote:

>
>
> Thanks,
> Guowei
>
> On Fri, Sep 19, 2014 at 10:39 AM, Guo-wei Shieh <guoweis@google.com>
> wrote:
>
>>
>>
>> Thanks,
>> Guowei
>>
>> On Fri, Sep 19, 2014 at 7:50 AM, <pauljensen@chromium.org> wrote:
>>
>>> I think we should add DCHECK(CalledOnValidThread()) in
>>> AddressTrackerAutoLock
>>> when !tracking.
>>> To do this I think it would be simplest to make AddressTrackerAutoLock a
>>> nested
>>> private class of AddressTrackerLinux, and pass "this" to
>>> AddressTrackerAutoLock's construtor.  I think this would allow
>>> AddressTrackerAutoLock to access  |tracking| and |CalledOnValidThread()|.
>>>
>>
>> Like the idea of adding the CalledOnValidThread() for non-tracking case
>> to ensure there is no unexpected race condition. However,
>> AddressTrackerLinux doesn't contain or inherit from base::ThreadChecker.
>> I'm adding one into the AddressTrackerAutoLock itself.
>>
>
> Actually, to think it more, if the purpose for adding this to non-tracking
> mode is to prevent unexpected race condition (like a call back does happen
> in non-tracking mode). How does this help? Each thread will get its own
> stack object and each one will validate just fine, unless we make this a
> member of AddressTrackLinux, but this loses the "auto" part of being on
> stack.
>
> Maybe I misunderstood your point?
>
>
>>
>>
>>>
>>> Can you please update the description of this CL a bit.  "AddressTracker
>>> change." is rather vague.
>>> "addresstracker" should be CamelCased.  Please also reference a BUG= bug.
>>>
>>>
>> Will do.
>>
>>
>>>
>>>
>>>
>>> https://codereview.chromium.org/571743002/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[guoweis2, 2014-09-19 18:30:47]

PTAL.

On 2014/09/19 18:05:21, chromium-reviews wrote:
> I think I understand what you meant now. The thread_checker_ will be a
> member class of AddressTrackerLinux which will achieve the effect. Will
> code accordingly.
> 
> Thanks,
> Guowei
> 
> On Fri, Sep 19, 2014 at 11:03 AM, Guo-wei Shieh <mailto:guoweis@google.com>
wrote:
> 
> >
> >
> > Thanks,
> > Guowei
> >
> > On Fri, Sep 19, 2014 at 10:39 AM, Guo-wei Shieh <mailto:guoweis@google.com>
> > wrote:
> >
> >>
> >>
> >> Thanks,
> >> Guowei
> >>
> >> On Fri, Sep 19, 2014 at 7:50 AM, <mailto:pauljensen@chromium.org> wrote:
> >>
> >>> I think we should add DCHECK(CalledOnValidThread()) in
> >>> AddressTrackerAutoLock
> >>> when !tracking.
> >>> To do this I think it would be simplest to make AddressTrackerAutoLock a
> >>> nested
> >>> private class of AddressTrackerLinux, and pass "this" to
> >>> AddressTrackerAutoLock's construtor.  I think this would allow
> >>> AddressTrackerAutoLock to access  |tracking| and |CalledOnValidThread()|.
> >>>
> >>
> >> Like the idea of adding the CalledOnValidThread() for non-tracking case
> >> to ensure there is no unexpected race condition. However,
> >> AddressTrackerLinux doesn't contain or inherit from base::ThreadChecker.
> >> I'm adding one into the AddressTrackerAutoLock itself.
> >>
> >
> > Actually, to think it more, if the purpose for adding this to non-tracking
> > mode is to prevent unexpected race condition (like a call back does happen
> > in non-tracking mode). How does this help? Each thread will get its own
> > stack object and each one will validate just fine, unless we make this a
> > member of AddressTrackLinux, but this loses the "auto" part of being on
> > stack.
> >
> > Maybe I misunderstood your point?
> >
> >
> >>
> >>
> >>>
> >>> Can you please update the description of this CL a bit.  "AddressTracker
> >>> change." is rather vague.
> >>> "addresstracker" should be CamelCased.  Please also reference a BUG= bug.
> >>>
> >>>
> >> Will do.
> >>
> >>
> >>>
> >>>
> >>>
> >>> https://codereview.chromium.org/571743002/
> >>>
> >>
> >>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[pauljensen, 2014-09-19 18:39:42]

https://codereview.chromium.org/571743002/diff/80001/net/base/address_tracker...
File net/base/address_tracker_linux.h (right):

https://codereview.chromium.org/571743002/diff/80001/net/base/address_tracker...
net/base/address_tracker_linux.h:76: class AddressTrackerAutoLock {
Please move all this code into address_tracker_linux.cc; the declaration can
stay here.

https://codereview.chromium.org/571743002/diff/80001/net/base/address_tracker...
net/base/address_tracker_linux.h:89:
DCHECK(tracker_.thread_checker_.CalledOnValidThread());
let's move this to the constructor so it's a little more obvious when the check
is hit (i.e. rather than a hidden call to the destructor).

https://codereview.chromium.org/571743002/diff/80001/net/base/address_tracker...
net/base/address_tracker_linux.h:159: // A |ThreadChecker| to validate that in
the non-tracking mode, the
Eh how about something like "Used to verify single-threaded access in
non-tracking mode."

[guoweis2, 2014-09-19 18:48:30]

PTAL.

https://codereview.chromium.org/571743002/diff/80001/net/base/address_tracker...
File net/base/address_tracker_linux.h (right):

https://codereview.chromium.org/571743002/diff/80001/net/base/address_tracker...
net/base/address_tracker_linux.h:76: class AddressTrackerAutoLock {
On 2014/09/19 18:39:42, pauljensen wrote:
> Please move all this code into address_tracker_linux.cc; the declaration can
> stay here.

Done.

https://codereview.chromium.org/571743002/diff/80001/net/base/address_tracker...
net/base/address_tracker_linux.h:89:
DCHECK(tracker_.thread_checker_.CalledOnValidThread());
On 2014/09/19 18:39:42, pauljensen wrote:
> let's move this to the constructor so it's a little more obvious when the
check
> is hit (i.e. rather than a hidden call to the destructor).

Done.

https://codereview.chromium.org/571743002/diff/80001/net/base/address_tracker...
net/base/address_tracker_linux.h:159: // A |ThreadChecker| to validate that in
the non-tracking mode, the
On 2014/09/19 18:39:42, pauljensen wrote:
> Eh how about something like "Used to verify single-threaded access in
> non-tracking mode."

Done.

[pauljensen, 2014-09-19 19:04:30]

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
File net/base/address_tracker_linux.h (right):

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
net/base/address_tracker_linux.h:24: #include
"base/threading/thread_restrictions.h"
unnecessary?

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
net/base/address_tracker_linux.h:39: // constructed without callbacks, it takes
a snapshot of current
Remove "When AddressTrackerLinux is constructed without callbacks, " as I think
this is redundant with the constructor declared on the following line.

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
net/base/address_tracker_linux.h:41: // available through GetOnlineLinks() and
GetAddressMap (). Note
GetAddressMap ()->GetAddressMap()

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
net/base/address_tracker_linux.h:44: // assert on CalledOnValidThread().
I don't think this note is necessary.  I think classes in chromium are generally
considered single-threaded unless otherwise noted.

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
net/base/address_tracker_linux.h:73: // A private nested class to facilitate
different locking behaviors
Remove "A private nested class " as that's redundant with where we are in the
file.
I don't really like "facilitate different locking behaviors" as in non-tracking
mode we don't actually lock.  How about "In tracking mode, holds |lock| while
alive.  In non-tracking mode, enforces single-threaded access."

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
net/base/address_tracker_linux.h:150: // happening in the non-tracking mode.
Remove "This is to catch any unexpected callback happening in the non-tracking
mode." as there are no callbacks used.

[guoweis2, 2014-09-19 19:15:38]

PTAL

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
File net/base/address_tracker_linux.h (right):

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
net/base/address_tracker_linux.h:24: #include
"base/threading/thread_restrictions.h"
On 2014/09/19 19:04:29, pauljensen wrote:
> unnecessary?

Done.

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
net/base/address_tracker_linux.h:39: // constructed without callbacks, it takes
a snapshot of current
On 2014/09/19 19:04:29, pauljensen wrote:
> Remove "When AddressTrackerLinux is constructed without callbacks, " as I
think
> this is redundant with the constructor declared on the following line.

Done.

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
net/base/address_tracker_linux.h:41: // available through GetOnlineLinks() and
GetAddressMap (). Note
On 2014/09/19 19:04:29, pauljensen wrote:
> GetAddressMap ()->GetAddressMap()

Done.

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
net/base/address_tracker_linux.h:44: // assert on CalledOnValidThread().
On 2014/09/19 19:04:29, pauljensen wrote:
> I don't think this note is necessary.  I think classes in chromium are
generally
> considered single-threaded unless otherwise noted.

Done.

https://codereview.chromium.org/571743002/diff/100001/net/base/address_tracke...
net/base/address_tracker_linux.h:73: // A private nested class to facilitate
different locking behaviors
On 2014/09/19 19:04:29, pauljensen wrote:
> Remove "A private nested class " as that's redundant with where we are in the
> file.
> I don't really like "facilitate different locking behaviors" as in
non-tracking
> mode we don't actually lock.  How about "In tracking mode, holds |lock| while
> alive.  In non-tracking mode, enforces single-threaded access."

Done.

[pauljensen, 2014-09-19 19:25:40]

This needs some tests.

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
File net/base/address_tracker_linux.h (right):

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux.h:38: // system configuration once Init()
returns. The configuration is
This is incorrect, the snapshot is not taken when Init() returns, it's take
before that.  How about "it takes a snapshot of the current system
configuration.  Once Init() returns, the configuration is available through
GetOnlineLinks() and GetAddressMap()."

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux.h:51: // In tracking mode, it starts watching
system configuration for
system->the system

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux.h:55: // returns.
|AddressMap| is a type but |online_links_| is a private variable...  How about
"In non-tracking mode, once Init() returns, a snapshot of the system
configuration is available through GetOnlineLinks() and GetAddressMap()."

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux.h:59: base::hash_set<int> GetOnlineLinks() const;
This needs a comment, maybe "Returns set of interface indicies for online
interfaces."

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux.h:142: // A |ThreadChecker| used to verify
single-threaded access in
Remove "A |ThreadChecker|" as this is redundant with the code.

[guoweis2, 2014-09-19 22:10:07]

Patchset #8 (id:140001) has been deleted

[guoweis2, 2014-09-19 22:12:55]

PTAL. 

I was hoping I could remove friend status from AddressTrackerLinuxTest but still
need it for HandleMessages().

To exercise the non-tracking mode, I have to separate the creation of
AddressTrackerLinux into a separate function.

The new test case is just to exercise the constructor and AddressTrackerAutoLock
in non-tracking mode.

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
File net/base/address_tracker_linux.h (right):

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux.h:38: // system configuration once Init()
returns. The configuration is
On 2014/09/19 19:25:39, pauljensen wrote:
> This is incorrect, the snapshot is not taken when Init() returns, it's take
> before that.  How about "it takes a snapshot of the current system
> configuration.  Once Init() returns, the configuration is available through
> GetOnlineLinks() and GetAddressMap()."

Done.

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux.h:51: // In tracking mode, it starts watching
system configuration for
On 2014/09/19 19:25:40, pauljensen wrote:
> system->the system

Done.

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux.h:55: // returns.
On 2014/09/19 19:25:40, pauljensen wrote:
> |AddressMap| is a type but |online_links_| is a private variable...  How about
> "In non-tracking mode, once Init() returns, a snapshot of the system
> configuration is available through GetOnlineLinks() and GetAddressMap()."

Done.

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux.h:59: base::hash_set<int> GetOnlineLinks() const;
On 2014/09/19 19:25:39, pauljensen wrote:
> This needs a comment, maybe "Returns set of interface indicies for online
> interfaces."

Done.

https://codereview.chromium.org/571743002/diff/120001/net/base/address_tracke...
net/base/address_tracker_linux.h:142: // A |ThreadChecker| used to verify
single-threaded access in
On 2014/09/19 19:25:40, pauljensen wrote:
> Remove "A |ThreadChecker|" as this is redundant with the code.

Done.

[guoweis2, 2014-09-20 00:09:52]

Patchset #8 (id:160001) has been deleted

[pauljensen, 2014-09-22 11:08:38]

Looking like it's almost done.  Could we add one more test that just calls
AddressTrackerLinux() and Init() to make sure it doesn't crash.

[guoweis2, 2014-09-22 16:53:43]

On 2014/09/22 11:08:38, pauljensen wrote:
> Looking like it's almost done.  Could we add one more test that just calls
> AddressTrackerLinux() and Init() to make sure it doesn't crash.

PTAL

[pauljensen, 2014-09-22 18:29:12]

lgtm

[guoweis2, 2014-09-22 19:52:48]

The CQ bit was checked by guoweis@google.com

[commit-bot: I haz the power, 2014-09-22 19:53:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/571743002/200001

[commit-bot: I haz the power, 2014-09-22 20:47:50]

Committed patchset #9 (id:200001) as bca4ac30ad0f03b7dbbd4ad3a7dec3bdbc1054ca

[commit-bot: I haz the power, 2014-09-22 20:48:31]

Patchset 9 (id:??) landed as
https://crrev.com/f09e9971c053661b2113f38881381678af6f62b3
Cr-Commit-Position: refs/heads/master@{#296048}