Listen to OnNetworkChanged instead of OnIPAddressChanged

Listen to OnNetworkChanged instead of OnIPAddressChanged
    
On android platform, it often fails to fetch searchdomaincheck url right
after network changes.
To fix the failure, this patch modifies GoogleURLTracker to listen to
OnNetworkChanged instead of OnIPAddressChanged.

BUG=407442
Committed: https://crrev.com/22b47697f6940fd76b8cb5fe5470abdd50c1a8ad
Cr-Commit-Position: refs/heads/master@{#293371}

[Jaekyun Seok (inactive), 2014-08-27 15:52:39]

jaekyun@chromium.org changed reviewers:
+ blundell@chromium.org, tedchoc@chromium.org

[Jaekyun Seok (inactive), 2014-08-27 15:52:39]

Please review this patch.

[blundell, 2014-08-27 16:57:25]

blundell@chromium.org changed reviewers:
+ pkasting@chromium.org

[blundell, 2014-08-27 16:57:25]

blundell -> pkasting

PK is better-suited than me for the OWNERS review here.

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

https://codereview.chromium.org/512843002/diff/1/components/google/core/brows...
File components/google/core/browser/google_url_tracker.cc (right):

https://codereview.chromium.org/512843002/diff/1/components/google/core/brows...
components/google/core/browser/google_url_tracker.cc:250:
fetcher_->SetAutomaticallyRetryOnNetworkChanges(kMaxRetries);
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]

https://codereview.chromium.org/512843002/diff/1/components/google/core/brows...
File components/google/core/browser/google_url_tracker.cc (right):

https://codereview.chromium.org/512843002/diff/1/components/google/core/brows...
components/google/core/browser/google_url_tracker.cc:250:
fetcher_->SetAutomaticallyRetryOnNetworkChanges(kMaxRetries);
On 2014/08/27 19:52:46, Peter Kasting wrote:
> 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?

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.

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

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:20]

https://codereview.chromium.org/512843002/diff/1/components/google/core/brows...
File components/google/core/browser/google_url_tracker.cc (right):

https://codereview.chromium.org/512843002/diff/1/components/google/core/brows...
components/google/core/browser/google_url_tracker.cc:250:
fetcher_->SetAutomaticallyRetryOnNetworkChanges(kMaxRetries);
On 2014/08/28 02:00:52, Jaekyun Seok wrote:
> On 2014/08/27 19:52:46, Peter Kasting wrote:
> > 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?
> 
> 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.

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.

[Jaekyun Seok (inactive), 2014-08-28 05:17:26]

On 2014/08/28 03:28:20, Peter Kasting wrote:
>
https://codereview.chromium.org/512843002/diff/1/components/google/core/brows...
> File components/google/core/browser/google_url_tracker.cc (right):
> 
>
https://codereview.chromium.org/512843002/diff/1/components/google/core/brows...
> components/google/core/browser/google_url_tracker.cc:250:
> fetcher_->SetAutomaticallyRetryOnNetworkChanges(kMaxRetries);
> On 2014/08/28 02:00:52, Jaekyun Seok wrote:
> > On 2014/08/27 19:52:46, Peter Kasting wrote:
> > > 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?
> > 
> > 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.
> 
> 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.

For solution (1),
HostResolverImpl::OnDNSChanged() is called due to DNSObserver::OnDNSChanged
event, and GoogleURLTracker::OnIPAddressChanged() is called due to
IPAddressObserver::OnIPAddressChanged event. But the two events are generated by
two different sources; DnsConfigService (jni) and NetworkChangeNotifier (java)
on android. So serializing them doesn't seem easy.

For solution (2),
I don't understand this. For now GoogleURLTracker::OnIPAddressChanged()
internally is posting a task to do the network access, and the task is aborted
by HostResolverImpl::OnDNSChanged() in this problem.

I want to suggest another solution which handles DNSObserver::OnDNSChanged event
separately inside GoogleURLTracker. This solution will fix the problem
regardless of the timing of DNSObserver::OnDNSChanged and
IPAddressObserver::OnIPAddressChanged events because StartFetchIfDesirable()
will be called again by DNSObserver::OnDNSChanged event.

What do you think of this?

And could you please add any network stack folks as reviewers if you know some?

[Peter Kasting, 2014-08-28 18:51:19]

pkasting@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Peter Kasting, 2014-08-28 18:52:11]

Ryan, I'm looking for some network stack guidance on the best solution to the
problem here.  Please see the last few comments on this review for the relevant
discussion.

On 2014/08/28 05:17:26, Jaekyun Seok wrote:
> For solution (1),
> HostResolverImpl::OnDNSChanged() is called due to DNSObserver::OnDNSChanged
> event, and GoogleURLTracker::OnIPAddressChanged() is called due to
> IPAddressObserver::OnIPAddressChanged event. But the two events are generated
by
> two different sources; DnsConfigService (jni) and NetworkChangeNotifier (java)
> on android. So serializing them doesn't seem easy.

Ah... that's likely going to break (2) as well, since the callbacks aren't all
happening during the same callstack.

> I want to suggest another solution which handles DNSObserver::OnDNSChanged
event
> separately inside GoogleURLTracker. This solution will fix the problem
> regardless of the timing of DNSObserver::OnDNSChanged and
> IPAddressObserver::OnIPAddressChanged events because StartFetchIfDesirable()
> will be called again by DNSObserver::OnDNSChanged event.

Hmm.  Maybe.  In theory, GoogleURLTracker probably doesn't really care about DNS
changes, just IP changes, since the latter can change the searchdomaincheck
result but the former (in the absence of the latter) presumably can't.  Which
would imply that your current fix is better.

[Ryan Sleevi, 2014-08-29 01:49:50]

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

[Ryan Sleevi, 2014-08-29 01:49:50]

I believe Paul may be better able to answer this. If not he, then mmenke@, cc'd.

The best I can share is the general statement that "DNS and IP changes tend to
mess everything up and make Chrome unusuable because so many systems start
flapping", so thanks at least for trying to make something less flappybad.

[pauljensen, 2014-08-29 12:05:20]

(I just hit some magic key that closed my Chrome window with all my tabs and
erased my comment....starting again....very angry....)
I suggest changing from OnIpAddressChanged() to using OnNetworkChanged() and
considering this use case a "constructive" use case.  Here's my doc on the
subject from long ago:
https://docs.google.com/document/d/1ch22_dRHPQJ9QArFMmWl_UyVpl-EDXA0PdljGh2kD...
I never got around to the very destabilizing work of converting all observers to
OnNetworkChanged.
Here's what I recommend:
OnNetworkChanged(ConnectionType type) {
  // Ignore destructive signals.
  if (type == CONNECTION_NONE)
    return;
  already_fetched_ = false;
  StartFetchIfDesirable();
}

[pauljensen, 2014-08-29 12:06:47]

I should also mention, I'm fine if you want your URLFetcher to retry on
ERR_NETWORK_CHANGED too, but I'd recommend trying OnNetworkChanged first as I
think it more directly addresses your concern, rather than papers over it.

[Peter Kasting, 2014-08-29 18:16:17]

Thanks Paul, that sounds like a good plan.

Jaekyun, want to implement this?

[Jaekyun Seok (inactive), 2014-08-29 21:06:55]

On 2014/08/29 18:16:17, Peter Kasting wrote:
> Thanks Paul, that sounds like a good plan.
> 
> Jaekyun, want to implement this?

Yes, I will try next week.

[Jaekyun Seok (inactive), 2014-09-01 01:45:05]

PTAL.

I modified this patch to listen to OnNetworkChanged instead of
OnIPAddressChanged.
I confirmed that it resolved the original issue.

But I left the logic to configure max retries for ERR_NETWORK_CHANGED for some
occasional cases.

[Jaekyun Seok (inactive), 2014-09-01 05:17:48]

After testing more without configuring max retries for ERR_NETWORK_CHANGED, I
confirmed that the original issue sometimes happened even after listening to
OnNetworkChanged.

So, I believe that configuring max retries for ERR_NETWORK_CHANGED is needed
anyway.

[pauljensen, 2014-09-02 11:05:45]

Can you elaborate on the circumstances that cause it to fail and require
increasing the retry count?  A net-internals log of an example failure without
retrying enabled would be very helpful, you can use chrome://net-export on
Android to collect such a log.

[Peter Kasting, 2014-09-02 19:33:16]

I agree. The network change observer stuff looks fine, but continuing to need
retries on ERR_NETWORK_CHANGED seems unfortunate.

[Jaekyun Seok (inactive), 2014-09-02 23:23:06]

You can see the net-internals log between network change and failure of fetching
in
https://drive.google.com/a/google.com/file/d/0B0rlutRlVuQKT2lwU2ZybVJnclk/vie...
.

And I confirmed that the failure sequence was as same as before.
HostResolverImpl::OnDNSChanged() was
triggered by DNSObserver::OnDNSChanged event right after
GoogleURLTracker::OnNetworkChanged() was triggered by
NetworkChangeObserver::OnNetworkChanged event, and so
HostResolverImpl::OnDNSChanged() aborted GoogleURLTracker's request.

[Peter Kasting, 2014-09-02 23:36:33]

On 2014/09/02 23:23:06, Jaekyun Seok wrote:
> You can see the net-internals log between network change and failure of
fetching
> in
>
https://drive.google.com/a/google.com/file/d/0B0rlutRlVuQKT2lwU2ZybVJnclk/vie...
> .
> 
> And I confirmed that the failure sequence was as same as before.
> HostResolverImpl::OnDNSChanged() was
> triggered by DNSObserver::OnDNSChanged event right after
> GoogleURLTracker::OnNetworkChanged() was triggered by
> NetworkChangeObserver::OnNetworkChanged event, and so
> HostResolverImpl::OnDNSChanged() aborted GoogleURLTracker's request.

Should we be making HostResolverImpl respond to network change events instead? 
Should we ensure that network change events are sent later than DNS change
events?

[Ryan Sleevi, 2014-09-03 00:31:42]

rsleevi@chromium.org changed reviewers:
- rsleevi@chromium.org

[pauljensen, 2014-09-03 14:53:56]

I filed http://crbug.com/410304 to make the network change events.

[blundell, 2014-09-03 14:55:00]

On 2014/09/03 14:53:56, pauljensen wrote:
> I filed http://crbug.com/410304 to make the network change events.

to make the network change events ... ?

[pauljensen, 2014-09-03 14:56:30]

I filed http://crbug.com/410304 to make the network change events come after the
DNS changed events. (Sorry dunno what happened to the end of that sentence.)

[Jaekyun Seok (inactive), 2014-09-03 21:27:21]

Then, will this patch keep going separately, or do you want me to abandon it?

[Peter Kasting, 2014-09-03 21:40:12]

On 2014/09/03 21:27:21, Jaekyun Seok wrote:
> Then, will this patch keep going separately, or do you want me to abandon it?

Why don't you commit the portions of it that switch to listening to
NetworkChanged events.  That should at least improve the code.  The network
changed retry bit we can leave out unless it becomes clear we'll end up needing
it.

[Jaekyun Seok (inactive), 2014-09-03 21:56:38]

Without the network changed retry bit this change isn't related to the original
bug at all. And I think it isn't harmful anyway even though the network change
events come after the DNS changed events.
Maybe we can use it as a workaround until http://crbug.com/410304 is fixed.

So my suggestion is to keep the line with TODO comments that its necessity
should be re-checked after http://crbug.com/410304 is fixed.

How about my suggestion?

[Peter Kasting, 2014-09-03 22:07:44]

I would really rather not add that line, even if it works around this bug for
now, until we at least have a schedule for fixing the other bug and Paul has a
chance to respond to my question there.

This isn't unrelated to the original bug.  Changing which event we listen to may
make things work better, especially on non-Android platforms.

[Jaekyun Seok (inactive), 2014-09-03 22:27:24]

OK. I will remove the line for the network changed retry bit in this change.

And after seeing to Paul's response I will make a separate change for the
network changed retry bit if necessary.

[Jaekyun Seok (inactive), 2014-09-03 22:31:42]

PTAL.

I removed the network changed retry.

[Peter Kasting, 2014-09-03 22:34:32]

LGTM

[Jaekyun Seok (inactive), 2014-09-03 22:41:41]

The CQ bit was checked by jaekyun@chromium.org

[commit-bot: I haz the power, 2014-09-03 23:24:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jaekyun@chromium.org/512843002/60001

[commit-bot: I haz the power, 2014-09-04 02:22:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 02:22:04]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jaekyun Seok (inactive), 2014-09-04 04:18:46]

PTAL.

I updated tests to fix failures on unittests.

[Jaekyun Seok (inactive), 2014-09-04 04:22:59]

Paul, please take a look at changes on network_change_notifier.h/cc.

[pauljensen, 2014-09-04 11:38:01]

net/ LGTM

[Jaekyun Seok (inactive), 2014-09-04 21:17:24]

The CQ bit was checked by jaekyun@chromium.org

[commit-bot: I haz the power, 2014-09-04 21:19:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jaekyun@chromium.org/512843002/80001

[commit-bot: I haz the power, 2014-09-04 23:23:41]

Committed patchset #5 (id:80001) as f5cff07e7f19d02993e06062d14282978beed20b

[commit-bot: I haz the power, 2014-09-10 03:33:52]

Patchset 5 (id:??) landed as
https://crrev.com/22b47697f6940fd76b8cb5fe5470abdd50c1a8ad
Cr-Commit-Position: refs/heads/master@{#293371}