Don't call lower() on KURL protocol/host

Don't call lower() on KURL protocol/host

KURL should already canonicalize the scheme and host. This is just
needless work and causes confusion to readers who may get the
wrong impression about code invariants.

Additionally, calling lower() on the host is a bug, because we
canonicalize escape characters to uppercase, but lower() reverts
that and brings them back to lowercase, effecively rendering the
host not canonical anymore!

This is a pre-req for removing CaseFoldingHash from SchemeRegistry.

BUG=659550, 348655
Committed: https://crrev.com/2874fa423262879ed27036f716f3e96a5f8e863f
Cr-Commit-Position: refs/heads/master@{#428762}

[Charlie Harrison, 2016-10-25 22:28:24]

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

[commit-bot: I haz the power, 2016-10-25 22:28:47]

Dry run: 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, 2016-10-26 00:58:15]

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

[commit-bot: I haz the power, 2016-10-26 00:58:16]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-10-26 01:12:53]

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

[commit-bot: I haz the power, 2016-10-26 01:13:14]

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

[Charlie Harrison, 2016-10-26 01:17:59]

Description was changed from

==========
Dont sanity check KURL::protocol::lower()

KURL should already canonicalize the scheme. This is just needless work
and confuses the code.

BUG=
==========

to

==========
Dont sanity check KURL::protocol::lower()

KURL should already canonicalize the scheme. This is just needless work
and confuses the code.

This is a pre-req for removing CaseFoldingHash from SchemeRegistry.

BUG=348655
==========

[Charlie Harrison, 2016-10-26 01:22:32]

Description was changed from

==========
Dont sanity check KURL::protocol::lower()

KURL should already canonicalize the scheme. This is just needless work
and confuses the code.

This is a pre-req for removing CaseFoldingHash from SchemeRegistry.

BUG=348655
==========

to

==========
Don't call KURL::protocol::lower()

KURL should already canonicalize the scheme. This is just needless work
and confuses the code.

This is a pre-req for removing CaseFoldingHash from SchemeRegistry.

BUG=348655
==========

[Charlie Harrison, 2016-10-26 01:24:03]

csharrison@chromium.org changed reviewers:
+ brettw@chromium.org

[Charlie Harrison, 2016-10-26 01:24:03]

Brett, PTAL. Did KURL used to not do this canonicalization?

[brettw, 2016-10-26 01:54:27]

LGTM

https://codereview.chromium.org/2447293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/2447293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:115:
m_host(url.host().lower()),
Since you're here, you can delete this lower() call on the host for the same
reason.

[Charlie Harrison, 2016-10-26 01:58:32]

https://codereview.chromium.org/2447293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/2447293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:115:
m_host(url.host().lower()),
On 2016/10/26 01:54:27, brettw (ping on IM after 24h) wrote:
> Since you're here, you can delete this lower() call on the host for the same
> reason.

Actually, I tried that and hit a DCHECK failure in this case:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Fram...

I added a similar DCHECK_EQ(host(), host().lower()) that failed because the %3E 
changed %3e. This is after changing the constructor to KURL(KURL(),
"http://f--.com-->").

[commit-bot: I haz the power, 2016-10-26 02:48:17]

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

[commit-bot: I haz the power, 2016-10-26 02:48:19]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[brettw, 2016-10-26 05:14:32]

On 2016/10/26 01:58:32, Charlie Harrison wrote:
>
https://codereview.chromium.org/2447293002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):
> 
>
https://codereview.chromium.org/2447293002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:115:
> m_host(url.host().lower()),
> On 2016/10/26 01:54:27, brettw (ping on IM after 24h) wrote:
> > Since you're here, you can delete this lower() call on the host for the same
> > reason.
> 
> Actually, I tried that and hit a DCHECK failure in this case:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/Fram...
> 
> I added a similar DCHECK_EQ(host(), host().lower()) that failed because the
%3E 
> changed %3e. This is after changing the constructor to KURL(KURL(),
> "http://f--.com-->").

I guess we can fix this case later then, but GURL canonicalizes escaped
characters to uppercase so that this should be safe. I'm actually hoping to
remove the escaped characters in hostnames altogether.

[Charlie Harrison, 2016-10-26 11:56:37]

Ah, so actually calling lower() on a host from KURL doesn't really make sense as
it could ruin your canonicalization if you have escaped characters? Does this
happen in GURL or url parsing in general (shared w/ KURL).

If that is happening today w/ KURL, calling lower() on the host here seems like
a bug.

[Charlie Harrison, 2016-10-26 15:21:49]

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

[commit-bot: I haz the power, 2016-10-26 15:22:03]

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

[Charlie Harrison, 2016-10-26 15:33:27]

Description was changed from

==========
Don't call KURL::protocol::lower()

KURL should already canonicalize the scheme. This is just needless work
and confuses the code.

This is a pre-req for removing CaseFoldingHash from SchemeRegistry.

BUG=348655
==========

to

==========
Don't call lower() on KURL protocol/host

KURL should already canonicalize the scheme and host. This is just
needless work and causes confusion to readers who may get the
wrong impression about code invariants.

Additionally, calling lower() on the host is a bug, because we
canonicalize escape characters to uppercase, but lower() reverts
that and brings them back to lowercase, effecively rendering the
host not canonical anymore!


This is a pre-req for removing CaseFoldingHash from SchemeRegistry.

BUG=659550,348655
==========

[Charlie Harrison, 2016-10-26 16:20:31]

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

[commit-bot: I haz the power, 2016-10-26 16:20:53]

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

[Charlie Harrison, 2016-10-26 17:40:27]

csharrison@chromium.org changed reviewers:
+ jochen@chromium.org

[Charlie Harrison, 2016-10-26 17:40:27]

Looks like I was right about host().lower() being buggy :)

Feel free to take another look if you like.

+jochen can you look at //content and core/dom?

[commit-bot: I haz the power, 2016-10-26 17:49:25]

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

[commit-bot: I haz the power, 2016-10-26 17:49:26]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-10-26 17:51:46]

The canonicalzation works the same between GURL and KURL (this being consistent
is entirely the point).

https://codereview.chromium.org/2447293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2447293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4406: // TODO(csharrison):
Should probably ensure newDomain is canonical rather than
definitely!

[Charlie Harrison, 2016-10-26 18:10:59]

https://codereview.chromium.org/2447293002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2447293002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4406: // TODO(csharrison):
Should probably ensure newDomain is canonical rather than
On 2016/10/26 17:51:46, brettw (ping on IM after 24h) wrote:
> definitely!

Yes, it looks like we are missing step 4 here (parse the host):
https://html.spec.whatwg.org/multipage/browsers.html#dom-document-domain

Do you have a sense for how critical this error is? Can we fix it in a follow
up? I think there is no functionality in Blink to canonicalize hosts (there is a
anon function in SchemeHostPort that we will likely have to share).

[Charlie Harrison, 2016-10-26 18:16:11]

csharrison@chromium.org changed reviewers:
+ mkwst@chromium.org

[Charlie Harrison, 2016-10-26 18:16:12]

also adding mkwst, who might be an expert on setting the domain on a document.

[brettw, 2016-10-26 19:41:05]

On 2016/10/26 18:10:59, Charlie Harrison wrote:
>
https://codereview.chromium.org/2447293002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2447293002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:4406: // TODO(csharrison):
> Should probably ensure newDomain is canonical rather than
> On 2016/10/26 17:51:46, brettw (ping on IM after 24h) wrote:
> > definitely!
> 
> Yes, it looks like we are missing step 4 here (parse the host):
> https://html.spec.whatwg.org/multipage/browsers.html#dom-document-domain
> 
> Do you have a sense for how critical this error is? Can we fix it in a follow
> up? I think there is no functionality in Blink to canonicalize hosts (there is
a
> anon function in SchemeHostPort that we will likely have to share).

You can call url::CanonicalizeHost in url/url_canon.h (this is what backs KURL
in the end).

[Charlie Harrison, 2016-10-26 20:09:23]

Let me throw up a dependent patch which adds a step to canonicalize the host to
Document.

[Charlie Harrison, 2016-10-26 20:10:50]

On 2016/10/26 20:09:23, Charlie Harrison wrote:
> Let me throw up a dependent patch which adds a step to canonicalize the host
to
> Document.

NVM scratch that. I think landing one without the other will cause subtle bugs
with lower().

[Charlie Harrison, 2016-10-26 20:58:58]

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

[commit-bot: I haz the power, 2016-10-26 20:59:36]

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

[Charlie Harrison, 2016-10-26 22:23:59]

I've added `String SecurityOrigin::canonicalizeHost(const String&)` with some
tests. This should be ready for another review. Sorry the CL got a bit bigger.

To clarify my above fears: if we stop calling lower() on hosts coming into
SecurityOrigin but *don't* stop calling lower() on domains coming from
setDomain, we could lead to mismatch with URLs with escape characters.

[commit-bot: I haz the power, 2016-10-26 23:00:27]

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

[commit-bot: I haz the power, 2016-10-26 23:00:28]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-10-27 07:30:35]

Canonicalizing the input to `document.domain` makes sense to me. I think layout
tests covering the edge cases you're fixing would be helpful, though, as noted
below.

https://codereview.chromium.org/2447293002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2447293002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4404: "' could not be parsed
properly.");
Can you add a layout test that runs through `document.domain` assignments where
this would fail?

[Mike West, 2016-10-27 07:30:53]

(The general approach LGTM; if you add tests, I'm happy.)

[jochen (gone - plz use gerrit), 2016-10-27 07:45:20]

(I think Brett and Mike cover everything here)

[Charlie Harrison, 2016-10-27 13:51:45]

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

[commit-bot: I haz the power, 2016-10-27 13:52:06]

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

[Charlie Harrison, 2016-10-27 14:11:30]

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

[commit-bot: I haz the power, 2016-10-27 14:11:53]

Dry run: 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, 2016-10-27 15:30:20]

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

[commit-bot: I haz the power, 2016-10-27 15:30:21]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-10-27 15:31:04]

Added some layout tests. Brett, would you mind double checking the host
canonicalization code in SecurityOrigin?

[brettw, 2016-10-31 16:55:25]

The canonicalizeHost function looks right to me. I'm not aulified to say
anything about whether the callers of that function are in the right place.

[Charlie Harrison, 2016-10-31 17:00:19]

Thanks Brett, I'll go ahead and land this with the assumption that Mike's
l-g-t-m covers that missing qualification.

Thanks again, everyone.

[Charlie Harrison, 2016-10-31 17:00:51]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-10-31 17:00:52]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2447293002/#ps120001
(title: "add canonicalization layout test")

[commit-bot: I haz the power, 2016-10-31 17:01:16]

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, 2016-10-31 18:52:28]

Description was changed from

==========
Don't call lower() on KURL protocol/host

KURL should already canonicalize the scheme and host. This is just
needless work and causes confusion to readers who may get the
wrong impression about code invariants.

Additionally, calling lower() on the host is a bug, because we
canonicalize escape characters to uppercase, but lower() reverts
that and brings them back to lowercase, effecively rendering the
host not canonical anymore!


This is a pre-req for removing CaseFoldingHash from SchemeRegistry.

BUG=659550,348655
==========

to

==========
Don't call lower() on KURL protocol/host

KURL should already canonicalize the scheme and host. This is just
needless work and causes confusion to readers who may get the
wrong impression about code invariants.

Additionally, calling lower() on the host is a bug, because we
canonicalize escape characters to uppercase, but lower() reverts
that and brings them back to lowercase, effecively rendering the
host not canonical anymore!


This is a pre-req for removing CaseFoldingHash from SchemeRegistry.

BUG=659550,348655
==========

[commit-bot: I haz the power, 2016-10-31 18:52:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-31 19:18:51]

Description was changed from

==========
Don't call lower() on KURL protocol/host

KURL should already canonicalize the scheme and host. This is just
needless work and causes confusion to readers who may get the
wrong impression about code invariants.

Additionally, calling lower() on the host is a bug, because we
canonicalize escape characters to uppercase, but lower() reverts
that and brings them back to lowercase, effecively rendering the
host not canonical anymore!


This is a pre-req for removing CaseFoldingHash from SchemeRegistry.

BUG=659550,348655
==========

to

==========
Don't call lower() on KURL protocol/host

KURL should already canonicalize the scheme and host. This is just
needless work and causes confusion to readers who may get the
wrong impression about code invariants.

Additionally, calling lower() on the host is a bug, because we
canonicalize escape characters to uppercase, but lower() reverts
that and brings them back to lowercase, effecively rendering the
host not canonical anymore!

This is a pre-req for removing CaseFoldingHash from SchemeRegistry.

BUG=659550,348655

Committed: https://crrev.com/2874fa423262879ed27036f716f3e96a5f8e863f
Cr-Commit-Position: refs/heads/master@{#428762}
==========

[commit-bot: I haz the power, 2016-10-31 19:18:52]

Patchset 7 (id:??) landed as
https://crrev.com/2874fa423262879ed27036f716f3e96a5f8e863f
Cr-Commit-Position: refs/heads/master@{#428762}