I'm not sure the proposed solution works, because the two classes are looking at different things.  ChromeOmniboxNavigationObserver merely needs to know whether the target exists in some form.  IntranetRedirectDetector wants to know where requests are being directed to so it can see if disparate requests go to the same place.  Thus it doesn't just care about "a redirect happened so this exists" but "where did it go".

For IntranetRedirectDetector to stop on the first redirect, it would need to treat the target of the redirect as the destination.  This in turn would mean it would detect as "bad ISPs" all cases where Chrome starts up in a captive portal or has some sort of proxy or extension redirecting traffic.  (This has been a problem for ChromeOmniboxNavigationObserver too.)

There are a few things I can think of to deal with this:

* Change from HEAD to GET.  While servers are supposed to send the same headers in response to both, this is not the only case where they don't (though it's the first one that looks intentional/malicious to me).  We may burn more bandwidth this way, though since the IntranetRedirectDetector requests garbage hostnames, that's probably rare.

* Provide users some sort of way of telling Chrome they're on a bad ISP.  The couple UI treatments I can think of for this wouldn't be used very much or work very well, though, so it's probably not worth doing.

A third option:

Currently the code does:
  // If any two fetches result in the same domain/host, we set the redirect
  // origin to that; otherwise we set it to nothing.
but it restricts this analysis only to successful fetches.

What's the harm in looking at the file URL attempted to be fetched and seeing if those final URLs are the same domain/host?

Failed fetches that all get sent to a central location may be some legit "no site for this intranet host" handling?

Isn't that effectively the same thing as what the intranet redirect detector is trying to catch?  Some "no site for this intranet host" handling (in typical case showing search results at the same time)...

Hmm.  I guess the assumption is that a failed load won't trigger the infobar.  But as noted elsewhere, a redirect to something that fails _will_ trigger the infobar.  That is, maybe the original proposed solution is the right one after all, because it keeps the two classes in sync.  Though maybe doing GET instead of HEAD is also a good change.

showing suggestions for localhost seems a bit risky 

Deleted: localhost suggestions.jpg 11.6 KB

	localhost suggestions.jpg 11.6 KB View Download