Add the ability for MDnsListener to actively refresh records

net/dns/mdns_client_impl.cc

Index: net/dns/mdns_client_impl.cc
diff --git a/net/dns/mdns_client_impl.cc b/net/dns/mdns_client_impl.cc
index c95b86c6d1735e610fd46beafc3c51c9939f0c56..b64ffa4e1151e3e03062ff7270dc2916aadfb18d 100644
--- a/net/dns/mdns_client_impl.cc
+++ b/net/dns/mdns_client_impl.cc
@@ -26,6 +26,9 @@ namespace net {
 namespace {
 
 const unsigned MDnsTransactionTimeoutSeconds = 3;
+const double kListenerRefreshPercentage1 = 0.85;

[szym, 2014/01/23 11:24:03]

Percentage suggests that this is divided by 100, i.e., percentage=0.85 means
0.85% or 0.0085. Suggest ListenerRefreshRatio.

Even then the name is unclear so this needs a comment.

// Refresh ratio is the time before the record expiration at which
// the listener should refresh it expressed as a fraction of
// the record TTL. For example, with refresh ratio of 0.85, a record
// with announced TTL of 100s should be refreshed 15s before expiration.

What's the reasoning for using time relative to TTL instead of absolute? I'm
concerned that the TTL received in DNS response is always the remaining TTL, so
it will be less if it's sent from a cache.

[Noam Samuel, 2014/01/23 17:50:42]

The time is relative to the record's original TTL, as per the mDNS spec. 
RecordParsed has separate fields for ttl and time_created so that the full
information on a packet's expiration time is maintained (which is another work
item for this CL, I don't actually use time_created, but will fix that).

+const double kListenerRefreshPercentage2 = 0.95;
+const unsigned kMillisecondsPerSecond = 1000;
 
 }  // namespace
 
@@ -192,7 +195,7 @@ void MDnsClientImpl::Core::HandlePacket(DnsResponse* response,
   // Note: We store cache keys rather than record pointers to avoid
   // erroneous behavior in case a packet contains multiple exclusive
   // records with the same type and name.
-  std::map<MDnsCache::Key, MDnsListener::UpdateType> update_keys;
+  std::map<MDnsCache::Key, MDnsCache::UpdateType> update_keys;
 
   if (!response->InitParseWithoutQuery(bytes_read)) {
     LOG(WARNING) << "Could not understand an mDNS packet.";
@@ -235,29 +238,10 @@ void MDnsClientImpl::Core::HandlePacket(DnsResponse* response,
     // Cleanup time may have changed.
     ScheduleCleanup(cache_.next_expiration());
 
-    if (update != MDnsCache::NoChange) {
-      MDnsListener::UpdateType update_external;
-
-      switch (update) {
-        case MDnsCache::RecordAdded:
-          update_external = MDnsListener::RECORD_ADDED;
-          break;
-        case MDnsCache::RecordChanged:
-          update_external = MDnsListener::RECORD_CHANGED;
-          break;
-        case MDnsCache::NoChange:
-        default:
-          NOTREACHED();
-          // Dummy assignment to suppress compiler warning.
-          update_external = MDnsListener::RECORD_CHANGED;
-          break;
-      }
-
-      update_keys.insert(std::make_pair(update_key, update_external));
-    }
+    update_keys.insert(std::make_pair(update_key, update));
   }
 
-  for (std::map<MDnsCache::Key, MDnsListener::UpdateType>::iterator i =
+  for (std::map<MDnsCache::Key, MDnsCache::UpdateType>::iterator i =
            update_keys.begin(); i != update_keys.end(); i++) {
     const RecordParsed* record = cache_.LookupKey(i->first);
     if (!record)
@@ -311,14 +295,14 @@ void MDnsClientImpl::Core::OnConnectionError(int error) {
 }
 
 void MDnsClientImpl::Core::AlertListeners(
-    MDnsListener::UpdateType update_type,
+    MDnsCache::UpdateType update_type,
     const ListenerKey& key,
     const RecordParsed* record) {
   ListenerMap::iterator listener_map_iterator = listeners_.find(key);
   if (listener_map_iterator == listeners_.end()) return;
 
   FOR_EACH_OBSERVER(MDnsListenerImpl, *listener_map_iterator->second,
-                    AlertDelegate(update_type, record));
+                    HandleRecordUpdate(update_type, record));
 }
 
 void MDnsClientImpl::Core::AddListener(
@@ -392,7 +376,7 @@ void MDnsClientImpl::Core::DoCleanup() {
 
 void MDnsClientImpl::Core::OnRecordRemoved(
     const RecordParsed* record) {
-  AlertListeners(MDnsListener::RECORD_REMOVED,
+  AlertListeners(MDnsCache::RecordRemoved,
                  ListenerKey(record->name(), record->type()), record);
 }
 
@@ -449,7 +433,14 @@ MDnsListenerImpl::MDnsListenerImpl(
     MDnsListener::Delegate* delegate,
     MDnsClientImpl* client)
     : rrtype_(rrtype), name_(name), client_(client), delegate_(delegate),
-      started_(false) {
+      started_(false), active_refresh_(false) {
+}
+
+MDnsListenerImpl::~MDnsListenerImpl() {
+  if (started_) {
+    DCHECK(client_->core());
+    client_->core()->RemoveListener(this);
+  }
 }
 
 bool MDnsListenerImpl::Start() {
@@ -463,10 +454,15 @@ bool MDnsListenerImpl::Start() {
   return true;
 }
 
-MDnsListenerImpl::~MDnsListenerImpl() {
+void MDnsListenerImpl::SetActiveRefresh(bool active_refresh) {
+  active_refresh_ = active_refresh;
+
   if (started_) {
-    DCHECK(client_->core());
-    client_->core()->RemoveListener(this);
+    if (!active_refresh_) {
+      next_refresh_.Cancel();
+    } else if (last_update_ != base::Time()) {
+      ScheduleNextRefresh();
+    }
   }
 }
 
@@ -478,10 +474,40 @@ uint16 MDnsListenerImpl::GetType() const {
   return rrtype_;
 }
 
-void MDnsListenerImpl::AlertDelegate(MDnsListener::UpdateType update_type,
-                                     const RecordParsed* record) {
+void MDnsListenerImpl::HandleRecordUpdate(MDnsCache::UpdateType update_type,
+                                          const RecordParsed* record) {
   DCHECK(started_);
-  delegate_->OnRecordUpdate(update_type, record);
+
+  if (update_type != MDnsCache::RecordRemoved) {
+    ttl_ = record->ttl();
+    last_update_ = base::Time::Now();
+
+    ScheduleNextRefresh();
+  }
+
+  if (update_type != MDnsCache::NoChange) {
+    MDnsListener::UpdateType update_external;
+
+    switch (update_type) {
+      case MDnsCache::RecordAdded:
+        update_external = MDnsListener::RECORD_ADDED;
+        break;
+      case MDnsCache::RecordChanged:
+        update_external = MDnsListener::RECORD_CHANGED;
+        break;
+      case MDnsCache::RecordRemoved:
+        update_external = MDnsListener::RECORD_REMOVED;
+        break;
+      case MDnsCache::NoChange:
+      default:
+        NOTREACHED();
+        // Dummy assignment to suppress compiler warning.
+        update_external = MDnsListener::RECORD_CHANGED;
+        break;
+    }
+
+    delegate_->OnRecordUpdate(update_external, record);
+  }
 }
 
 void MDnsListenerImpl::AlertNsecRecord() {
@@ -489,6 +515,41 @@ void MDnsListenerImpl::AlertNsecRecord() {
   delegate_->OnNsecRecord(name_, rrtype_);
 }
 
+void MDnsListenerImpl::ScheduleNextRefresh() {

[szym, 2014/01/23 11:24:03]

This function silently assumes that if active_refresh_ then last_update_ !=
Time().

I suggest you either add DCHECK or explicit check and exit if last_update_ ==
Time().

[Noam Samuel, 2014/01/23 17:50:42]

Done.

+  if (active_refresh_) {

[szym, 2014/01/23 11:24:03]

By convention this should be:
if (!active_refresh_)
  return;
...

[Noam Samuel, 2014/01/23 17:50:42]

Done.

+    // A zero TTL is a goodbye packet and should not be refreshed.
+    if (ttl_ == 0) {
+      next_refresh_.Cancel();
+      return;
+    }
+
+    next_refresh_.Reset(base::Bind(&MDnsListenerImpl::DoRefresh,
+                                   AsWeakPtr()));
+
+    base::Time next_refresh1 = last_update_ + base::TimeDelta::FromMilliseconds(
+        static_cast<int>(kMillisecondsPerSecond *
+                         kListenerRefreshPercentage1 * ttl_));
+
+    base::Time next_refresh2 = last_update_ + base::TimeDelta::FromMilliseconds(

[szym, 2014/01/23 11:24:03]

You need a comment somewhere why two refresh tasks are scheduled at the same
time.

[Noam Samuel, 2014/01/23 17:50:42]

Done.

+        static_cast<int>(kMillisecondsPerSecond *
+                         kListenerRefreshPercentage2 * ttl_));
+
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        next_refresh_.callback(),
+        next_refresh1 - base::Time::Now());
+
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        next_refresh_.callback(),
+        next_refresh2 - base::Time::Now());
+  }
+}
+
+void MDnsListenerImpl::DoRefresh() {
+  client_->core()->SendQuery(rrtype_, name_);
+}
+
 MDnsTransactionImpl::MDnsTransactionImpl(
     uint16 rrtype,
     const std::string& name,