Implement NetworkChangeNotifier::IsCurrentlyOffline() for Mac.

Implement NetworkChangeNotifier::IsCurrentlyOffline() for Mac.

R=willchan@chromium.org
BUG=7469, 53473
TEST=load onlineTest.html attachment on bug 7469; disconnect/connect network cable

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

[adamk, 2011-05-31 23:51:01]

I'm sad about the lack of a unittest, but I don't know if such a thing is
even possible.  Let me know if you have any suggestions for testing this in
a non-manual manner.

On Tue, May 31, 2011 at 4:49 PM, <adamk@chromium.org> wrote:

> Reviewers: willchan,
>
> Description:
> Implement NetworkChangeNotifier::IsCurrentlyOffline() for Mac.
>
> R=willchan@chromium.org
> BUG=7469,53473
> TEST=load onlineTest.html attachment on bug 7469; disconnect/connect
> network
> cable
>
>
> Please review this at http://codereview.chromium.org/7006015/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M net/base/network_change_notifier_mac.h
>  M net/base/network_change_notifier_mac.cc
>  M net/base/network_config_watcher_mac.h
>  M net/base/network_config_watcher_mac.cc
>
>
> Index: net/base/network_change_notifier_mac.cc
> diff --git a/net/base/network_change_notifier_mac.cc
> b/net/base/network_change_notifier_mac.cc
> index
>
7521c90bd649117a2d7b4400e2e29424d652564e..1fb3fb741a66893eff4322a53dc5e1982a6a750e
> 100644
> --- a/net/base/network_change_notifier_mac.cc
> +++ b/net/base/network_change_notifier_mac.cc
> @@ -4,7 +4,11 @@
>
>  #include "net/base/network_change_notifier_mac.h"
>
> +#include <netinet/in.h>
> +#include <string.h>
> +
>  #include <SystemConfiguration/SCDynamicStoreKey.h>
> +#include <SystemConfiguration/SCNetworkReachability.h>
>  #include <SystemConfiguration/SCSchemaDefinitions.h>
>
>  #include "base/mac/scoped_cftyperef.h"
> @@ -13,12 +17,12 @@ namespace net {
>
>  NetworkChangeNotifierMac::NetworkChangeNotifierMac()
>     : forwarder_(this),
> -      config_watcher_(&forwarder_) {}
> +      config_watcher_(&forwarder_),
> +      network_reachable_(true) {}
>  NetworkChangeNotifierMac::~NetworkChangeNotifierMac() {}
>
>  bool NetworkChangeNotifierMac::IsCurrentlyOffline() const {
> -  // TODO(eroman): http://crbug.com/53473
> -  return false;
> +  return !network_reachable_;
>  }
>
>  void NetworkChangeNotifierMac::SetDynamicStoreNotificationKeys(
> @@ -42,11 +46,41 @@ void
> NetworkChangeNotifierMac::SetDynamicStoreNotificationKeys(
>       store, notification_keys.get(), NULL);
>   // TODO(willchan): Figure out a proper way to handle this rather than
> crash.
>   CHECK(ret);
> +
> +  // Try to reach 0.0.0.0. This is the approach taken by Firefox:
> +  //
> +  //
>
http://mxr.mozilla.org/mozilla2.0/source/netwerk/system/mac/nsNetworkLinkServ...
> +  //
> +  // From my (adamk) testing on Snow Leopard, 0.0.0.0
> +  // seems to be reachable if any network connection is available.
> +  struct sockaddr_in addr;
> +  memset(&addr, 0, sizeof(addr));
> +  addr.sin_len = sizeof(addr);
> +  addr.sin_family = AF_INET;
> +  reachability_.reset(SCNetworkReachabilityCreateWithAddress(
> +      kCFAllocatorDefault, reinterpret_cast<struct sockaddr*>(&addr)));
> +  SCNetworkReachabilityContext reachability_context = {
> +      0,     // version
> +      this,  // user data
> +      NULL,  // retain
> +      NULL,  // release
> +      NULL   // description
> +  };
> +  // TODO(adamk): Either of these calls could fail, can we do any better
> +  // in handling this?
> +  ret = SCNetworkReachabilitySetCallback(
> +      reachability_.get(),
> +      &NetworkChangeNotifierMac::ReachabilityCallback,
> +      &reachability_context);
> +  DCHECK(ret) << "Could not set network reachability callback";
> +  ret = SCNetworkReachabilityScheduleWithRunLoop(reachability_.get(),
> +                                                 CFRunLoopGetCurrent(),
> +                                                 kCFRunLoopCommonModes);
> +  DCHECK(ret) << "Could not schedule network reachability on run loop";
>  }
>
>  void NetworkChangeNotifierMac::OnNetworkConfigChange(CFArrayRef
> changed_keys) {
>   // Called on notifier thread.
> -
>   for (CFIndex i = 0; i < CFArrayGetCount(changed_keys); ++i) {
>     CFStringRef key = static_cast<CFStringRef>(
>         CFArrayGetValueAtIndex(changed_keys, i));
> @@ -64,4 +98,28 @@ void
> NetworkChangeNotifierMac::OnNetworkConfigChange(CFArrayRef changed_keys) {
>   }
>  }
>
> +// static
> +void NetworkChangeNotifierMac::ReachabilityCallback(
> +    SCNetworkReachabilityRef target,
> +    SCNetworkConnectionFlags flags,
> +    void* notifier) {
> +  // Called on notifier thread.
> +  bool reachable = (flags & kSCNetworkFlagsReachable);
> +  bool connection_required = (flags & kSCNetworkFlagsConnectionRequired);
> +  NetworkChangeNotifierMac* notifier_mac =
> +      static_cast<NetworkChangeNotifierMac*>(notifier);
> +  notifier_mac->network_reachable_ = (reachable && !connection_required);
> +  notifier_mac->NotifyObserversOfOnlineStateChange();
> +}
> +
> +void NetworkChangeNotifierMac::CleanUp() {
> +  // Called on notifier thread.
> +  if (!reachability_.get())
> +    return;
> +  SCNetworkReachabilityUnscheduleFromRunLoop(reachability_.get(),
> +                                             CFRunLoopGetCurrent(),
> +                                             kCFRunLoopCommonModes);
> +  reachability_.reset();
> +}
> +
>  }  // namespace net
> Index: net/base/network_change_notifier_mac.h
> diff --git a/net/base/network_change_notifier_mac.h
> b/net/base/network_change_notifier_mac.h
> index
>
f46a6666f64b4169a32ac468583b7d9959907ab7..a5e4a6097f2b4b2b0dbf761099f189fdf20c6485
> 100644
> --- a/net/base/network_change_notifier_mac.h
> +++ b/net/base/network_change_notifier_mac.h
> @@ -7,8 +7,10 @@
>  #pragma once
>
>  #include <SystemConfiguration/SCDynamicStore.h>
> +#include <SystemConfiguration/SCNetworkReachability.h>
>
>  #include "base/basictypes.h"
> +#include "base/mac/scoped_cftyperef.h"
>  #include "net/base/network_change_notifier.h"
>  #include "net/base/network_config_watcher_mac.h"
>
> @@ -37,6 +39,9 @@ class NetworkChangeNotifierMac: public
> NetworkChangeNotifier {
>     virtual void OnNetworkConfigChange(CFArrayRef changed_keys) {
>       net_config_watcher_->OnNetworkConfigChange(changed_keys);
>     }
> +    virtual void CleanUp() {
> +      net_config_watcher_->CleanUp();
> +    }
>
>    private:
>     NetworkChangeNotifierMac* const net_config_watcher_;
> @@ -47,8 +52,15 @@ class NetworkChangeNotifierMac: public
> NetworkChangeNotifier {
>   void SetDynamicStoreNotificationKeys(SCDynamicStoreRef store);
>   void OnNetworkConfigChange(CFArrayRef changed_keys);
>
> +  static void ReachabilityCallback(SCNetworkReachabilityRef target,
> +                                   SCNetworkConnectionFlags flags,
> +                                   void* notifier);
> +  void CleanUp();
> +
>   Forwarder forwarder_;
>   const NetworkConfigWatcherMac config_watcher_;
> +  base::mac::ScopedCFTypeRef<SCNetworkReachabilityRef> reachability_;
> +  bool network_reachable_;
>
>   DISALLOW_COPY_AND_ASSIGN(NetworkChangeNotifierMac);
>  };
> Index: net/base/network_config_watcher_mac.cc
> diff --git a/net/base/network_config_watcher_mac.cc
> b/net/base/network_config_watcher_mac.cc
> index
>
bd3528a75009edabd1596cc68b4203c162da1a26..b4ea3c4a888337079c56103970acf66c134e1811
> 100644
> --- a/net/base/network_config_watcher_mac.cc
> +++ b/net/base/network_config_watcher_mac.cc
> @@ -78,6 +78,7 @@ void NetworkConfigWatcherMacThread::Init() {
>  }
>
>  void NetworkConfigWatcherMacThread::CleanUp() {
> +  delegate_->CleanUp();
>   if (!run_loop_source_.get())
>     return;
>
> Index: net/base/network_config_watcher_mac.h
> diff --git a/net/base/network_config_watcher_mac.h
> b/net/base/network_config_watcher_mac.h
> index
>
5193a98d5811315f74db71784c89bd7d13379da7..33bb3a691452ae2138c07d09e6832770e62518fc
> 100644
> --- a/net/base/network_config_watcher_mac.h
> +++ b/net/base/network_config_watcher_mac.h
> @@ -35,6 +35,10 @@ class NetworkConfigWatcherMac {
>     // Called when one of the notification keys has changed.
>     // Will be called on the notifier thread.
>     virtual void OnNetworkConfigChange(CFArrayRef changed_keys) = 0;
> +
> +    // Called when the notifier thread is being destroyed.
> +    // Will be called on the notifier thread.
> +    virtual void CleanUp() {}
>   };
>
>   explicit NetworkConfigWatcherMac(Delegate* delegate);
>
>
>

[willchan no longer on Chromium, 2011-06-01 20:46:04]

I usually add a Mac guy to help me with these OS X specific APIs. Mark, mind
taking a look or adding someone who can?

I'll go validate any generic net/ stuff here.

[willchan no longer on Chromium, 2011-06-01 20:48:47]

LGTM, but wait for Mark or someone he delegates to approve the OS X specific API
calls.

[Mark Mentovai, 2011-06-01 21:05:59]

LGTM from a Mac perspective.

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:7: #include <netinet/in.h>
These shouldn’t be in a separate group from the SystemConfiguration headers. The
group for “C system headers” doesn’t distinguish between POSIX and non-POSIX.

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:56: struct sockaddr_in addr;
I would just say |struct sockaddr_in addr = { 0 };| and let the compiler memset
it for me.

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:74: &reachability_context);
I hope SC copies what it needs from this structure, since reachability_context
is invalid as soon as this function goes out of scope.

(I think you’re OK here, and we have other uses that do this too.)

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:76: ret =
SCNetworkReachabilityScheduleWithRunLoop(reachability_.get(),
I think you’d want to avoid doing this (and maybe do reachability_.reset()) if
the preceding call failed.

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:107: bool reachable = (flags &
kSCNetworkFlagsReachable);
No parentheses on this line, the next, or line 111.

[adamk, 2011-06-01 21:42:53]

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:7: #include <netinet/in.h>
On 2011/06/01 21:05:59, Mark Mentovai wrote:
> These shouldn’t be in a separate group from the SystemConfiguration headers.
The
> group for “C system headers” doesn’t distinguish between POSIX and non-POSIX.

Done.

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:56: struct sockaddr_in addr;
On 2011/06/01 21:05:59, Mark Mentovai wrote:
> I would just say |struct sockaddr_in addr = { 0 };| and let the compiler
memset
> it for me.

I don't think the 0 does anything here.  I've rewritten it as "= {};" (which
should default-initialize the whole thing.

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:74: &reachability_context);
On 2011/06/01 21:05:59, Mark Mentovai wrote:
> I hope SC copies what it needs from this structure, since reachability_context
> is invalid as soon as this function goes out of scope.
> 
> (I think you’re OK here, and we have other uses that do this too.)

I have to imagine they copy what they need...

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:76: ret =
SCNetworkReachabilityScheduleWithRunLoop(reachability_.get(),
On 2011/06/01 21:05:59, Mark Mentovai wrote:
> I think you’d want to avoid doing this (and maybe do reachability_.reset()) if
> the preceding call failed.

Changed the structure around and added some resets (also switched to
LOG(DFATAL).  PTAL and see if this meets what you had in mind.

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:107: bool reachable = (flags &
kSCNetworkFlagsReachable);
On 2011/06/01 21:05:59, Mark Mentovai wrote:
> No parentheses on this line, the next, or line 111.

Done.

[Mark Mentovai, 2011-06-01 22:02:45]

LGTM

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:56: struct sockaddr_in addr;
adamk wrote:
> On 2011/06/01 21:05:59, Mark Mentovai wrote:
> > I would just say |struct sockaddr_in addr = { 0 };| and let the compiler
> memset
> > it for me.
> 
> I don't think the 0 does anything here.  I've rewritten it as "= {};" (which
> should default-initialize the whole thing.

We normally use { 0 }, which does in fact zero out the entire structure.

http://groups.google.com/group/chromium-dev/browse_thread/thread/498891cd7b06...

There are a couple of weak reasons to use this instead of {}:

1. { 0 }, as a pattern, is understood to mean and look like “zero.”

2. We normally use this when we’re trying to zero-initialize C structs, and are
thinking in C-ish terms, and the empty {} is not valid C.

I don’t mind if you want to leave it as {}, but I mention this because you
asserted that { 0 } wouldn’t do anything.

http://codereview.chromium.org/7006015/diff/8001/net/base/network_change_noti...
net/base/network_change_notifier_mac.cc:67: NULL   // description
Is the TODO still relevant? It’s fine to leave it if you think it still is, but
if the .reset()s account for it now, maybe it’s no longer necessary.

[adamk, 2011-06-01 22:09:34]

Thanks for the quick review!

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:56: struct sockaddr_in addr;
On 2011/06/01 22:02:45, Mark Mentovai wrote:
> adamk wrote:
> > On 2011/06/01 21:05:59, Mark Mentovai wrote:
> > > I would just say |struct sockaddr_in addr = { 0 };| and let the compiler
> > memset
> > > it for me.
> > 
> > I don't think the 0 does anything here.  I've rewritten it as "= {};" (which
> > should default-initialize the whole thing.
> 
> We normally use { 0 }, which does in fact zero out the entire structure.
> 
>
http://groups.google.com/group/chromium-dev/browse_thread/thread/498891cd7b06...
> 
> There are a couple of weak reasons to use this instead of {}:
> 
> 1. { 0 }, as a pattern, is understood to mean and look like “zero.”
> 
> 2. We normally use this when we’re trying to zero-initialize C structs, and
are
> thinking in C-ish terms, and the empty {} is not valid C.
> 
> I don’t mind if you want to leave it as {}, but I mention this because you
> asserted that { 0 } wouldn’t do anything.

Sorry, I meant to say "{ 0 }" wouldn't do anything different: it'll set the
first member of the struct to 0 and the rest of the members to
default-initialized (which happens to be 0 for this case).  I'm happy to put the
0 there for aesthetic purposes, as it seems more common in Chromium.

[Mark Mentovai, 2011-06-01 22:10:43]

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/1/net/base/network_change_notifie...
net/base/network_change_notifier_mac.cc:56: struct sockaddr_in addr;
adamk wrote:
> Sorry, I meant to say "{ 0 }" wouldn't do anything different: it'll set the
> first member of the struct to 0 and the rest of the members to
> default-initialized (which happens to be 0 for this case).  I'm happy to put
the
> 0 there for aesthetic purposes, as it seems more common in Chromium.

Gotcha, I misread what you meant by “don’t think the 0 does anything.” Like I
said, your call on this one.

[adamk, 2011-06-01 22:11:28]

http://codereview.chromium.org/7006015/diff/8001/net/base/network_change_noti...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/8001/net/base/network_change_noti...
net/base/network_change_notifier_mac.cc:67: // in handling this?
On 2011/06/01 22:02:45, Mark Mentovai wrote:
> Is the TODO still relevant? It’s fine to leave it if you think it still is,
but
> if the .reset()s account for it now, maybe it’s no longer necessary.

Done.

[adamk, 2011-06-01 23:16:51]

Hmm, some worrisome tryjob output.  Will need to investigate:

http://build.chromium.org/p/tryserver.chromium/builders/mac_sync/builds/2817/...

[Mark Mentovai, 2011-06-01 23:28:50]

http://codereview.chromium.org/7006015/diff/12001/net/base/network_change_not...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/12001/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:110:
notifier_mac->NotifyObserversOfOnlineStateChange();
Also: Do you only want to make this call when network_reachable_ is changing?

[adamk, 2011-06-01 23:31:07]

On Wed, Jun 1, 2011 at 4:28 PM, <mark@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/7006015/diff/12001/net/base/network_change_not...
>
> File net/base/network_change_notifier_mac.cc (right):
>
>
>
http://codereview.chromium.org/7006015/diff/12001/net/base/network_change_not...
> net/base/network_change_notifier_mac.cc:110:
> notifier_mac->NotifyObserversOfOnlineStateChange();
> Also: Do you only want to make this call when network_reachable_ is
> changing?


Sure, that could save the occasional IPC here and there.  Done.


>
> http://codereview.chromium.org/7006015/
>

[Mark Mentovai, 2011-06-01 23:36:31]

LGTM again, but the real reason I was rereading this was to try to figure out
the cause of your chronic double-free problem that the try servers uncovered. No
luck. :(

[adamk, 2011-06-01 23:48:59]

My guess: the NetworkChangeNotifier is getting deleted too early, and so
CleanUp() is being called after the thing is already deleted.  Not sure why
we ever bother destroying the notifier, since it's a singleton, perhaps Will
has some insight?  It's owned by BrowserMainParts (in
chrome/browser/browser_main.cc).

On Wed, Jun 1, 2011 at 4:36 PM, <mark@chromium.org> wrote:

> LGTM again, but the real reason I was rereading this was to try to figure
> out
> the cause of your chronic double-free problem that the try servers
> uncovered. No
> luck. :(
>
>
> http://codereview.chromium.org/7006015/
>

[adamk, 2011-06-02 00:51:41]

Indeed, adding a call to CleanUp() in the destructor causes the tests to
pass. I'm inclined to say this is a unittest-only problem unless I hear
otherwise.  But it disconcerts me a bit.

On Wed, Jun 1, 2011 at 4:48 PM, Adam Klein <adamk@chromium.org> wrote:

> My guess: the NetworkChangeNotifier is getting deleted too early, and so
> CleanUp() is being called after the thing is already deleted.  Not sure why
> we ever bother destroying the notifier, since it's a singleton, perhaps Will
> has some insight?  It's owned by BrowserMainParts (in
> chrome/browser/browser_main.cc).
>
>
> On Wed, Jun 1, 2011 at 4:36 PM, <mark@chromium.org> wrote:
>
>> LGTM again, but the real reason I was rereading this was to try to figure
>> out
>> the cause of your chronic double-free problem that the try servers
>> uncovered. No
>> luck. :(
>>
>>
>> http://codereview.chromium.org/7006015/
>>
>
>

[Mark Mentovai, 2011-06-02 16:26:57]

http://codereview.chromium.org/7006015/diff/12003/net/base/network_change_not...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/12003/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:22: CleanUp();
Which thread does this happen on?

http://codereview.chromium.org/7006015/diff/12003/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:31: // Called on notifier thread.
Every time you have this comment:

Is this something you can DCHECK for?

[adamk, 2011-06-02 16:43:50]

http://codereview.chromium.org/7006015/diff/12003/net/base/network_change_not...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/12003/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:22: CleanUp();
On 2011/06/02 16:26:57, Mark Mentovai wrote:
> Which thread does this happen on?

Ugh, good point, this is why I put it in the delegate interface in the first
place.  So this is clearly not the right answer.

http://codereview.chromium.org/7006015/diff/12003/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:31: // Called on notifier thread.
On 2011/06/02 16:26:57, Mark Mentovai wrote:
> Every time you have this comment:
> 
> Is this something you can DCHECK for?

Not without some additional machinery.  Possibly this method could save a
MessageLoopProxy of that thread, which could allow the destructor to at least
attempt to run CleanUp() on the proper thread.  But that still isn't quite
right, as it'd have to Join the thread.  More thought necessary...I think the
ideal would be that this object is simply never destroyed.

[Mark Mentovai, 2011-06-02 16:47:19]

http://codereview.chromium.org/7006015/diff/12003/net/base/network_change_not...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/12003/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:31: // Called on notifier thread.
adamk wrote:
> Not without some additional machinery.  Possibly this method could save a
> MessageLoopProxy of that thread, which could allow the destructor to at least
> attempt to run CleanUp() on the proper thread.  But that still isn't quite
> right, as it'd have to Join the thread.  More thought necessary...I think the
> ideal would be that this object is simply never destroyed.

Since this is platform-specific code, you can save a pthread_t from
pthread_self(). That’s probably most lightweight of all, and is certainly easy
to do, and a very small amount of code.

But I’ll leave this to you to rethink a bit, since the threading model as it
stands now is off (and any DCHECK you introduce would wind up getting tripped).

[adamk, 2011-06-02 19:14:00]

I've done away with the CleanUp() method, and instead I retain the CFRunLoop in
order to unschedule the callback in NetworkChangeNotifierMac's destructor. 
PTAL.  try jobs seem green.

[adamk, 2011-06-02 19:37:07]

I've also added thread ID DCHECKs (using PlatformThread even though this is
Mac-only since it seems that uses mach_thread_self instead of pthread_self
and I don't know enough to know the difference; happy to change to
pthread_self/pthread_equal if that's the right thing).

On Thu, Jun 2, 2011 at 12:14 PM, <adamk@chromium.org> wrote:

> I've done away with the CleanUp() method, and instead I retain the
> CFRunLoop in
> order to unschedule the callback in NetworkChangeNotifierMac's destructor.
> PTAL.  try jobs seem green.
>
>
> http://codereview.chromium.org/7006015/
>

[Mark Mentovai, 2011-06-02 19:46:36]

http://codereview.chromium.org/7006015/diff/16001/net/base/network_change_not...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/16001/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:23: if (reachability_.get() &&
run_loop_) {
Two alternatives here.

1. Rather than checking run_loop_ twice, you can structure this as

  if (run_loop_) {
    if (reachability_.get()) {
      // unschedule
    }
    // release
  }

2. You can hold run_loop_ in a ScopedCFTypeRef and relieve yourself of the
responsibility for releasing it manually in the destructor.

http://codereview.chromium.org/7006015/diff/16001/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:94: // Called on notifier thread.
I suppose that now that you’re tracking run_loop_, you can write the DCHECK we
were discussing so that it compares run_loop_ to CFRunLoopGetCurrent().

I just got your most recent e-mail as I was writing this comment, though, so
let’s see how things look in the next patch set.

http://codereview.chromium.org/7006015/diff/16002/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:100: NotifyObserversOfIPAddressChange();
You can use DCHECK_EQ here and on line 127.

This is fine with the PlatformThreadId if you’d prefer to use this over the
CFRunLoopGetCurrent thing I had suggested.

[adamk, 2011-06-02 19:55:50]

http://codereview.chromium.org/7006015/diff/16001/net/base/network_change_not...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/16001/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:23: if (reachability_.get() &&
run_loop_) {
On 2011/06/02 19:46:36, Mark Mentovai wrote:
> Two alternatives here.
> 
> 1. Rather than checking run_loop_ twice, you can structure this as
> 
>   if (run_loop_) {
>     if (reachability_.get()) {
>       // unschedule
>     }
>     // release
>   }
> 
> 2. You can hold run_loop_ in a ScopedCFTypeRef and relieve yourself of the
> responsibility for releasing it manually in the destructor.

Took option (2).

http://codereview.chromium.org/7006015/diff/16001/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:94: // Called on notifier thread.
On 2011/06/02 19:46:36, Mark Mentovai wrote:
> I suppose that now that you’re tracking run_loop_, you can write the DCHECK we
> were discussing so that it compares run_loop_ to CFRunLoopGetCurrent().
> 
> I just got your most recent e-mail as I was writing this comment, though, so
> let’s see how things look in the next patch set.

Went with CFRunLoopGetCurrent() to avoid having to store extra stuff.  Seems
simplest this way.

[Mark Mentovai, 2011-06-02 20:02:43]

LGTM

http://codereview.chromium.org/7006015/diff/14003/net/base/network_change_not...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/7006015/diff/14003/net/base/network_change_not...
net/base/network_change_notifier_mac.cc:91: DCHECK(run_loop_.get() ==
CFRunLoopGetCurrent());
You can still change this and line 118 to DCHECK_EQ instead of writing ==. That
has the advantage of printing each side’s value in the message if the DCHECK is
tripped.

[adamk, 2011-06-02 20:28:24]

On Thu, Jun 2, 2011 at 1:02 PM, <mark@chromium.org> wrote:

> LGTM
>
>
>
>
http://codereview.chromium.org/7006015/diff/14003/net/base/network_change_not...
>
> File net/base/network_change_notifier_mac.cc (right):
>
>
>
http://codereview.chromium.org/7006015/diff/14003/net/base/network_change_not...
> net/base/network_change_notifier_mac.cc:91: DCHECK(run_loop_.get() ==
> CFRunLoopGetCurrent());
> You can still change this and line 118 to DCHECK_EQ instead of writing
> ==. That has the advantage of printing each side’s value in the message
> if the DCHECK is tripped.


Will do. My initial thought was that this wouldn't provide any interesting
information, but I suppose it would if run_loop_ is NULL.


>
> http://codereview.chromium.org/7006015/
>

[Mark Mentovai, 2011-06-02 20:29:10]

Exactly!

[Mark Mentovai, 2011-06-02 20:56:49]

LGTM

[eroman, 2011-06-07 19:18:38]

I have two concerns with this implementation:

(1) Does it work correctly if you launch Chrome while offline? 

It looks to me like since |network_reachable_| is initialized to false, and is
only subsequently updated by the change callback, we will think Chrome is
"online" until the next online/offline transition.

(2) The "network_reachable_" boolean is updated non-atomically from a different
thread than where it might be accessed from:

notifier_mac->network_reachable_ = reachable && !connection_required;

Note that implementations are supposed to be threadsafe --> this bool may be
accessed from other threads. Depending how the above line gets compiled, you
could end up reading the intermediate value of the above expression...


Otherwise LGTM, thanks for writing this.

[eroman, 2011-06-07 19:19:16]

(i obviously meant "initialized to true" in the above comment :)

[adamk, 2011-06-07 19:25:53]

On Tue, Jun 7, 2011 at 12:18 PM, <eroman@chromium.org> wrote:

> I have two concerns with this implementation:
>
> (1) Does it work correctly if you launch Chrome while offline?
>
> It looks to me like since |network_reachable_| is initialized to false, and
> is
> only subsequently updated by the change callback, we will think Chrome is
> "online" until the next online/offline transition.
>

This is very true, though I just noticed it yesterday.  Sorry, should have
seen it before.  There's actually a sync version of this call; where would
be a good place to make that call? Should it happen on construction? I just
worry about slowing down startup...


> (2) The "network_reachable_" boolean is updated non-atomically from a
> different
> thread than where it might be accessed from:
>
> notifier_mac->network_**reachable_ = reachable && !connection_required;
>
> Note that implementations are supposed to be threadsafe --> this bool may
> be
> accessed from other threads. Depending how the above line gets compiled,
> you
> could end up reading the intermediate value of the above expression...
>

Hmm, good point. I was only considering the OnlineStateObserver use-case,
but this is clearly wrong.  Would you prefer this be switched to use an
atomic op, or a mutex?


>
> Otherwise LGTM, thanks for writing this.
>
>
>
http://codereview.chromium.**org/7006015/<http://codereview.chromium.org/7006...
>