ONC: Fix handling of invalid proxy scheme.

chromeos/network/onc/onc_utils.cc

Index: chromeos/network/onc/onc_utils.cc
diff --git a/chromeos/network/onc/onc_utils.cc b/chromeos/network/onc/onc_utils.cc
index a696ca5984855e530b1cfc9653571f5d1762193e..c47bb1a0a562e8bf1cf62af884c8471c15a7d09d 100644
--- a/chromeos/network/onc/onc_utils.cc
+++ b/chromeos/network/onc/onc_utils.cc
@@ -36,6 +36,7 @@
 #include "components/prefs/pref_service.h"
 #include "components/proxy_config/proxy_config_dictionary.h"
 #include "components/signin/core/account_id/account_id.h"
+#include "components/url_formatter/url_fixer.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "crypto/encryptor.h"
@@ -931,7 +932,14 @@ std::unique_ptr<base::DictionaryValue> ConvertOncProxySettingsToProxyConfig(
     onc_proxy_settings.GetStringWithoutPathExpansion(::onc::proxy::kPAC,
                                                      &pac_url);
     GURL url(pac_url);
-    DCHECK(url.is_valid()) << "Invalid URL in ProxySettings.PAC";
+    if (!url.is_valid()) {

[Peter Kasting, 2017/01/06 01:59:33]

Why check for an invalid GURL before running it through fixup?  Why not just:

  GURL url(url_formatter::FixupURL(pac_url, std::string()));

[stevenjb, 2017/01/06 17:36:14]

Done.

+      // Assume the scheme is missing, use http.
+      url = GURL(url_formatter::FixupURL(pac_url, std::string()));
+      if (!url.is_valid()) {
+        LOG(WARNING) << "Invalid URL in ProxySettings.PAC: " << pac_url;

[Peter Kasting, 2017/01/06 01:59:33]

Generally, I suggest people avoid LOGs, as they bloat the binary and we don't
normally collect and act on them in some meaningful way.  Is there something
useful you're going to do with this logging data?  If not, avoid logging.

[stevenjb, 2017/01/06 17:36:14]

Done.

+        url = GURL();

[Peter Kasting, 2017/01/06 01:59:33]

Do you need to do this?  Is it safe to just call .possibly_invalid_spec() below
instead, or will something later blow up if the URL is invalid?  If the latter,
we could save a line by doing:

    proxy_dict.reset(ProxyConfigDictionary::CreatePacScript(
        url.is_valid() ? url.spec() : std::string(), false));

[stevenjb, 2017/01/06 17:36:14]

Something later will blow up if this is invalid. Also, this will provide
immediate feedback in the UI that the url is invalid.

Combined the lines.

+      }
+    }
     proxy_dict.reset(ProxyConfigDictionary::CreatePacScript(url.spec(), false));
   } else if (type == ::onc::proxy::kManual) {
     const base::DictionaryValue* manual_dict = nullptr;