Listen to OnNetworkChanged instead of OnIPAddressChanged

components/google/core/browser/google_url_tracker.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/google/core/browser/google_url_tracker.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
@@ skipping 227 matching lines @@
   ++fetcher_id_;
   // We don't want this fetch to set new entries in the cache or cookies, lest
   // we alarm the user.
   fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE |
                          net::LOAD_DO_NOT_SAVE_COOKIES);
   fetcher_->SetRequestContext(client_->GetRequestContext());
 
   // Configure to max_retries at most kMaxRetries times for 5xx errors.
   static const int kMaxRetries = 5;
   fetcher_->SetMaxRetriesOn5xx(kMaxRetries);
+#if defined(OS_ANDROID)
+  // Configure to max_retries at most kMaxRetries times for network changes.
+  fetcher_->SetAutomaticallyRetryOnNetworkChanges(kMaxRetries);

[Peter Kasting, 2014/08/27 19:52:46]

Let me understand this.  When the network changes, OnIPAddressChanged() gets
called, which calls StartFetchIfDesirable(), but the fetch we subsequently
trigger here can still fail with ERR_NETWORK_CHANGED after that?  Why?

Also, why is this Android-specific?  Wouldn't we want this retry behavior on all
platforms?

[Jaekyun Seok (inactive), 2014/08/28 02:00:52]

Yes, that is correctly what happened on clank.
I confirmed that when this problem happened HostResolverImpl::OnDNSChanged() was
called right after GoogleURLTracker::OnIPAddressChanged() was called, and so
HostResolverImpl::OnDNSChanged() aborted GoogleURLTracker's request. But this
doesn't always happen, and so I think that this is timing problem.
Actually, I tested this only on clank, and so I applied this only on android to
avoid any regression on other platforms. But I believe that this isn't harmful
at all. I will remove if macro.

[Peter Kasting, 2014/08/28 03:28:19]

Hmm.  Here are two ideas for a possible better fix:
(1) Ensure that HostResolverImpl::OnDNSChanged() gets called before any
OnIPAddressChanged() methods get called, so no on tries to do network accesses
that will immediately get aborted.
(2) Have OnIPAddressChanged() post a task to do the network access, so we finish
calling all the various handlers before we try to do this.

(1) is better than (2), if it would work, since (2) is basically a hack.  Either
one seems better than the current fix, though, since the current fix doesn't
prevent us from doing extra work that will fail, it just retries if we do so.

The network stack folks should probably weigh in.

+#endif
 
   fetcher_->Start();
 }
 
 void GoogleURLTracker::OnNavigationPending(
     scoped_ptr<GoogleURLTrackerNavigationHelper> nav_helper,
     infobars::InfoBarManager* infobar_manager,
     int pending_id) {
   GoogleURLTrackerMapEntry* map_entry = NULL;
 
@@ skipping 136 matching lines @@
   }
   if (client_->IsListeningForNavigationStart()) {
     DCHECK(!search_committed_);
     client_->SetListeningForNavigationStart(false);
   }
 }
 
 void GoogleURLTracker::NotifyGoogleURLUpdated() {
   callback_list_.Notify();
 }