ONC: Fix handling of invalid proxy scheme.

ONC: Fix handling of invalid proxy scheme.

It is common for a proxy configuration URL to be added as 'foo.com'
instead of 'http://foo.com'. Rather than relying on all UI to
validate the input, we should do so in the ONC conversion.

BUG=677357
Review-Url: https://codereview.chromium.org/2608803002
Cr-Commit-Position: refs/heads/master@{#442378}
Committed: https://chromium.googlesource.com/chromium/src/+/a8a349eb13605f30e1aa6c096f4c4ea28ba11bc9

[stevenjb, 2016-12-29 21:17:04]

stevenjb@chromium.org changed reviewers:
+ emaxx@chromium.org, mnissler@chromium.org

[emaxx, 2016-12-29 22:05:36]

https://codereview.chromium.org/2608803002/diff/1/chromeos/network/onc/onc_ut...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/2608803002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:936: url = GURL(std::string(url::kHttpScheme)
+ url::kStandardSchemeSeparator +
This logic looks a bit crude to me.
Could we use here the standard utility for fixing the human-input URLs - 
url_formatter::FixupURL?

[stevenjb, 2016-12-29 22:44:17]

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

[stevenjb, 2016-12-29 22:44:18]

+pkasting@ for owner of components/url_formatter/ (adding DEP)

https://codereview.chromium.org/2608803002/diff/1/chromeos/network/onc/onc_ut...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/2608803002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:936: url = GURL(std::string(url::kHttpScheme)
+ url::kStandardSchemeSeparator +
On 2016/12/29 22:05:36, emaxx wrote:
> This logic looks a bit crude to me.
> Could we use here the standard utility for fixing the human-input URLs - 
> url_formatter::FixupURL?

Ah, I was looking for something like that but didn't find it. Done.

[stevenjb, 2016-12-29 22:44:28]

The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 22:44:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2016-12-29 22:56:02]

lgtm

[commit-bot: I haz the power, 2016-12-29 23:47:40]

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

[commit-bot: I haz the power, 2016-12-29 23:47:41]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2608803002/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/2608803002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:935: if (!url.is_valid()) {
Why check for an invalid GURL before running it through fixup?  Why not just:

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

https://codereview.chromium.org/2608803002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:939: LOG(WARNING) << "Invalid URL in
ProxySettings.PAC: " << pac_url;
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.

https://codereview.chromium.org/2608803002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:940: url = GURL();
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]

Also added a couple of test cases. PTAL.

https://codereview.chromium.org/2608803002/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/2608803002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:935: if (!url.is_valid()) {
On 2017/01/06 01:59:33, Peter Kasting (backlogged) wrote:
> Why check for an invalid GURL before running it through fixup?  Why not just:
> 
>   GURL url(url_formatter::FixupURL(pac_url, std::string()));

Done.

https://codereview.chromium.org/2608803002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:939: LOG(WARNING) << "Invalid URL in
ProxySettings.PAC: " << pac_url;
On 2017/01/06 01:59:33, Peter Kasting (backlogged) wrote:
> 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.

Done.

https://codereview.chromium.org/2608803002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:940: url = GURL();
On 2017/01/06 01:59:33, Peter Kasting (backlogged) wrote:
> 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));

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.

[Peter Kasting, 2017-01-06 20:58:21]

LGTM

[stevenjb, 2017-01-06 21:13:34]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-01-06 21:13:34]

The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2608803002/#ps40001
(title: "Feedback + add unit test")

[commit-bot: I haz the power, 2017-01-06 21:13:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 21:42:48]

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

[commit-bot: I haz the power, 2017-01-06 21:42:49]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[stevenjb, 2017-01-09 20:54:35]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-01-09 20:54:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2608803002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-09 20:55:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 22:38:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483995275169570,
"parent_rev": "c9a06c811d1fcbad634cd66ef0fe9871018ea738", "commit_rev":
"a8a349eb13605f30e1aa6c096f4c4ea28ba11bc9"}

[commit-bot: I haz the power, 2017-01-09 22:39:27]

Description was changed from

==========
ONC: Fix handling of invalid proxy scheme.

It is common for a proxy configuration URL to be added as 'foo.com'
instead of 'http://foo.com'. Rather than relying on all UI to
validate the input, we should do so in the ONC conversion.

BUG=677357
==========

to

==========
ONC: Fix handling of invalid proxy scheme.

It is common for a proxy configuration URL to be added as 'foo.com'
instead of 'http://foo.com'. Rather than relying on all UI to
validate the input, we should do so in the ONC conversion.

BUG=677357

Review-Url: https://codereview.chromium.org/2608803002
Cr-Commit-Position: refs/heads/master@{#442378}
Committed:
https://chromium.googlesource.com/chromium/src/+/a8a349eb13605f30e1aa6c096f4c...
==========

[commit-bot: I haz the power, 2017-01-09 22:39:29]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a8a349eb13605f30e1aa6c096f4c...

[Timothy Loh, 2017-01-10 02:50:29]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2619803003/ by timloh@chromium.org.

The reason for reverting is: The modified test is failing on Linux ChromiumOS
Tests (dbg)(1)

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...

An example failure:
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....