Use Netlink instead of NetworkManager (via D-bus) to monitor network

Use Netlink instead of NetworkManager (via D-bus) to monitor network
connectivity on Linux.

BUG=117973
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170211

[pauljensen, 2012-11-09 22:27:42]

Can you take a brief look at this?  Its somewhat of a work in progress so I'm
just looking for overall comments.  I still need to do a lot of work to get unit
testing.

The code is a lot shorter than before and should work more reliably.  The whole
offline_state_initialized_ thing scares me, I copied it from the old code; maybe
I should Signal() it if Init() fails in either of its two "return" statements.

[szym, 2012-11-11 16:21:55]

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.cc:171: netlink_fd_ = -1;
Since you need to do this at least once, I suggest you set netlink_fd_ =
socket(...) up above and change CloseSocket(int) to CloseSocket() which closes
|netlink_fd_| and sets it to -1.

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.cc:183: // http://crbug.com/125097
Is it still needed? Can't we return offline until we know we are online?

I'm guessing the concern is around unnecessary aborts of HostResolver::Request
and flushing of SocketPools during initialization. My understanding of netlink
(seen in the current AddressTrackerLinux) is that if you sendto(kernel) the
response will be immediately available for reading.

If that's not the case, then we should create an explicit mechanism to suppress
the initial signal from reaching NCN Observers, or a way for the observers to
distinguish this signal from actual change signals. AFAIK, AddressSorterPosix is
the only observer which must be told when AddressMap is complete.

Also, if that's not the case, then I think blocking on initialization makes
http://crbug.com/157162 sad, because getifaddrs probably takes as much as
blocking for kernel's response over netlink, although Android has it's own
implementation of NetworkChangeNotifier.

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.cc:185: offline_state_initialized_.Wait();
I'm concerned that you Wait after locking. I think you must hold the lock before
waiting on the variable.

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.cc:187: // TODO(droger): Return something more
detailed than CONNECTION_UNKNOWN.
Could you find the bug for droger and link it here?

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.cc:202: // Block the first time through loop.
If we are ok with returning offline initially, you won't need to call
GetInitialSettings and ever block here.

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.h:50: NetworkChangeNotifier::ConnectionType
GetCurrentConnectionType();
const?

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.h:86: base::Lock is_offline_lock_;
If you make GetCurrentConnectionType const, then this needs to be mutable. 

More prevalent style is putting lock as first in the group, followed by any
state and conditional variables protected by the lock.

http://codereview.chromium.org/11359141/diff/4001/net/tools/net_watcher/net_w...
File net/tools/net_watcher/net_watcher.cc (right):

http://codereview.chromium.org/11359141/diff/4001/net/tools/net_watcher/net_w...
net/tools/net_watcher/net_watcher.cc:129: g_type_init();
This does not seem to belong in this CL. Is there a separate bug around toolkit
initialization for ProxyConfigService?

[pauljensen, 2012-11-12 16:45:21]

We covered some of this verbally, but here were my typed up thoughts.

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.cc:171: netlink_fd_ = -1;
On 2012/11/11 16:21:55, szym wrote:
> Since you need to do this at least once, I suggest you set netlink_fd_ =
> socket(...) up above and change CloseSocket(int) to CloseSocket() which closes
> |netlink_fd_| and sets it to -1.

Done.

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.cc:183: // http://crbug.com/125097
On 2012/11/11 16:21:55, szym wrote:
> Is it still needed? Can't we return offline until we know we are online?
I'm hesitant to return offline when uninitialized; I feel like this might result
in some funny Chrome startup behavior like the new tab icons going from grey to
colored or if something else goes wrong ERR_INTERNET_DISCONNECTED being blamed
or maybe sync being delayed.  We really need a PENDING result but that would be
a big change.  I could maybe see returning CONNECTION_UNKNOWN, but then do we
send out a network change event when we know its really something more specific
like CONNECTION_WIFI?  I think on linux the delay is going to be very short so
this shouldn't be such a big issue.  This waiting code is moved from
NetworkManagerApi.

> 
> I'm guessing the concern is around unnecessary aborts of HostResolver::Request
> and flushing of SocketPools during initialization. My understanding of netlink
> (seen in the current AddressTrackerLinux) is that if you sendto(kernel) the
> response will be immediately available for reading.
This seems to be the case but I could find no documentation of this, so I
switched to using a blocking socket to prevent a race.

> 
> If that's not the case, then we should create an explicit mechanism to
suppress
> the initial signal from reaching NCN Observers, or a way for the observers to
> distinguish this signal from actual change signals. AFAIK, AddressSorterPosix
is
> the only observer which must be told when AddressMap is complete.
I prevent initial signaling by calling ReadMessages() in GetInitialSettings().

> 
> Also, if that's not the case, then I think blocking on initialization makes
> http://crbug.com/157162 sad, because getifaddrs probably takes as much as
> blocking for kernel's response over netlink, although Android has it's own
> implementation of NetworkChangeNotifier.
Ya, on Linux this should be quite fast versus Android, and Android doesn't use
this code.

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.cc:185: offline_state_initialized_.Wait();
On 2012/11/11 16:21:55, szym wrote:
> I'm concerned that you Wait after locking. I think you must hold the lock
before
> waiting on the variable.
I'm confused; your two sentences contradict each other.  I think we need to wait
then lock to avoid deadlocking.

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.cc:187: // TODO(droger): Return something more
detailed than CONNECTION_UNKNOWN.
On 2012/11/11 16:21:55, szym wrote:
> Could you find the bug for droger and link it here?
I don't think there is one.  I filed one.

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.h:50: NetworkChangeNotifier::ConnectionType
GetCurrentConnectionType();
On 2012/11/11 16:21:55, szym wrote:
> const?

This would require making is_offline_lock_ and offline_state_initialized_
mutable which isn't safe with multiple threads and this function is called from
multiple threads.  I'm just moving this from NetworkManagerApi.

http://codereview.chromium.org/11359141/diff/4001/net/base/address_tracker_li...
net/base/address_tracker_linux.h:86: base::Lock is_offline_lock_;
On 2012/11/11 16:21:55, szym wrote:
> If you make GetCurrentConnectionType const, then this needs to be mutable. 
> 
> More prevalent style is putting lock as first in the group, followed by any
> state and conditional variables protected by the lock.

I was just moving this from NetworkManagerApi.  I'll move the lock before the
bool.

http://codereview.chromium.org/11359141/diff/4001/net/tools/net_watcher/net_w...
File net/tools/net_watcher/net_watcher.cc (right):

http://codereview.chromium.org/11359141/diff/4001/net/tools/net_watcher/net_w...
net/tools/net_watcher/net_watcher.cc:129: g_type_init();
On 2012/11/11 16:21:55, szym wrote:
> This does not seem to belong in this CL. Is there a separate bug around
toolkit
> initialization for ProxyConfigService?
I moved to another CL.

[pauljensen, 2012-11-16 15:16:50]

Szymon, could you take a more thorough look now that I think we've finalized the
implementation details?  When you're happy with it I'll rework the unit tests
appropriately.

[szym, 2012-11-16 17:26:17]

lgtm

http://codereview.chromium.org/11359141/diff/16001/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/11359141/diff/16001/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:160: base::AutoLock lock(is_offline_lock_);
This should be closer to where is_offline_ is first written. That means at the
end of GetInitialSettings() or inside ReadMessages.

You could just set it (and Signal) every time you write is_offline_ (in
ReadMessages). Or add if(!is_offline_initialized_), but if this extra work is a
concern, at the end of GetInitialSettings is fine.

http://codereview.chromium.org/11359141/diff/16001/net/base/address_tracker_l...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/11359141/diff/16001/net/base/address_tracker_l...
net/base/address_tracker_linux.h:22: #include
"base/synchronization/condition_variable.h"
nit: order

[pauljensen, 2012-11-21 18:11:25]

Could you please review my unittest changes?

[szym, 2012-11-21 20:14:10]

https://codereview.chromium.org/11359141/diff/25001/net/base/address_tracker_...
File net/base/address_tracker_linux_unittest.cc (right):

https://codereview.chromium.org/11359141/diff/25001/net/base/address_tracker_...
net/base/address_tracker_linux_unittest.cc:24: char* buf, size_t length, bool*
address_changed, bool* link_changed) {
Style says that in multi-line function declarations one argument per line.

I also suggest moving the typedef Buffer from line 41 above and accepting that
as input.

https://codereview.chromium.org/11359141/diff/25001/net/base/address_tracker_...
net/base/address_tracker_linux_unittest.cc:139: MakeAddrMessage(RTM_NEWADDR,
This does not have to be one arg per line. I think two lines would do just fine.

https://codereview.chromium.org/11359141/diff/25001/net/base/address_tracker_...
net/base/address_tracker_linux_unittest.cc:162: address_changed = false;
Suggest adding two helper functions to the fixture instead of the current
HandleMessage proxy:

bool CheckAddrChange(char* buffer, size_t size) {
  bool address_changed;
  bool link_changed;
  HandleMessage(buffer, size, &address_changed, &link_changed);
  EXPECT_FALSE(link_changed);
  return address_changed;
}

bool CheckLinkChange(...)

Failures are not going to be as informative as before, but it should suffice.

In fact, I suggest CheckAddrChange(const Buffer& buffer);

https://codereview.chromium.org/11359141/diff/25001/net/base/address_tracker_...
net/base/address_tracker_linux_unittest.cc:425:
EXPECT_TRUE(GetOnlineLinks()->find(0) != GetOnlineLinks()->end());
Suggest: |count(value) != 0|

https://codereview.chromium.org/11359141/diff/25001/net/base/address_tracker_...
net/base/address_tracker_linux_unittest.cc:437:
EXPECT_TRUE(GetOnlineLinks()->find(0) != GetOnlineLinks()->end());
Suggest also EXPECT_EQ(1, GetOnlineLinks()->size());

https://codereview.chromium.org/11359141/diff/25001/net/base/address_tracker_...
net/base/address_tracker_linux_unittest.cc:450:
EXPECT_TRUE(GetOnlineLinks()->find(1) != GetOnlineLinks()->end());
Suggest also EXPECT_EQ(2, GetOnlineLinks()->size())

https://codereview.chromium.org/11359141/diff/25001/net/base/address_tracker_...
net/base/address_tracker_linux_unittest.cc:463: HandleMessage(&buffer[0],
buffer.size(), &address_changed, &link_changed);
Add expectation that GetOnlineLinks is not empty.

https://codereview.chromium.org/11359141/diff/25001/net/base/address_tracker_...
net/base/address_tracker_linux_unittest.cc:504: HandleMessage(&buffer[0],
buffer.size(), &address_changed, &link_changed);
ditto

[pauljensen, 2012-11-26 17:15:30]

I addressed your comments.  The test code is half as big now :)  Care to look it
over again?

[szym, 2012-11-26 17:34:20]

This already LGTM, so you are just humoring me :)

https://codereview.chromium.org/11359141/diff/23012/net/base/address_tracker_...
File net/base/address_tracker_linux_unittest.cc (right):

https://codereview.chromium.org/11359141/diff/23012/net/base/address_tracker_...
net/base/address_tracker_linux_unittest.cc:155: EXPECT_TRUE(map.find(kAddr0) !=
map.end());
ugh, I suggested using EXPECT_EQ(1u, count(key)), but now I see I used
EXPECT_TRUE(find(key) != end()) myself.

If it's not too much hassle, could you fix all the uses in this file to be the
same? I still think count(key) is the better way.

https://codereview.chromium.org/11359141/diff/23012/net/base/address_tracker_...
net/base/address_tracker_linux_unittest.cc:296: // Ignores loopback
nit: Add '.' at end of comments to make them more like sentences :)

https://codereview.chromium.org/11359141/diff/23012/net/base/address_tracker_...
net/base/address_tracker_linux_unittest.cc:316:
EXPECT_EQ(GetOnlineLinks()->count(0), 1u);
EXPECT_EQ(expected, tested) so that when it fails it prints:
Actual: <actual>
Expected: <expected>

[pauljensen, 2012-11-26 23:00:10]

Brett, could you review base/threading/thread_restrictions.h?
I essentially moved the code from one class to another so the class name
changed.  The new implementation (using Netlink) should be significantly faster
than the old implementation (using dbus) so its unlikely to ever block the UI
thread; it never does on my machine.

[brettw, 2012-11-26 23:37:55]

base lgtm

[commit-bot: I haz the power, 2012-11-29 14:01:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/11359141/19015

[commit-bot: I haz the power, 2012-11-29 17:11:57]

Change committed as 170211