Improve OriginAccessEntry support for IP addresses.

Improve OriginAccessEntry support for IP addresses.

The current IP address check in OriginAccessEntry is hardly robust; it
just verifies that the last character in the given host is a digit. This
patch brings in the processing from GURL::HostIsIPAddress, and adds a
number of tests to verify the behavior.

It also notes clearly that the 'TreatIPAddressAsDomain' should never be
used outside of 'document.domain' layout tests, and even there we should
get rid of it ASAP.

BUG=477578
R=jochen@chromium.org, rsleevi@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195410

[Mike West, 2015-05-13 10:27:57]

Ryan, does this patch make you happier about
https://codereview.chromium.org/1133223002? :)

Jochen, WDYT about the `url/` inclusions? We're including the same files in KURL
(also in platform/weborigin), so I think we're probably ok from a layering
standpoint (at least as ok as KURL is...)

-mike

[Mike West, 2015-05-13 14:36:55]

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

[commit-bot: I haz the power, 2015-05-13 14:37:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128913004/1

[commit-bot: I haz the power, 2015-05-13 14:39:55]

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

[commit-bot: I haz the power, 2015-05-13 14:39:56]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2015-05-13 23:23:35]

Holy crap, yeah, that's scary behaviour!

Overall LG, with some nits, but jochen knows more about what's allowed or not.

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
File Source/platform/weborigin/OriginAccessEntry.cpp (right):

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntry.cpp:38: #include
<url/third_party/mozilla/url_parse.h>
what the wat?

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntry.cpp:56: url::Component
hostComponent(7, host.length());
grumble-nit - It's not immediately obvious that the 7 is tied to line 50, or
that someone won't come along and helpfully change things to https:// because
http:// is bad and then bad things happen :)

Make these two static constants in the function?

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntry.cpp:58: return
hostInfo.IsIPAddress();
This is basically duplicating GURL::HostIsIPAddress.

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
File Source/platform/weborigin/OriginAccessEntryTest.cpp (right):

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntryTest.cpp:110: for (auto test :
inputs) {
s/auto/const auto&/

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntryTest.cpp:110: for (auto test :
inputs) {
SCOPED_TRACE(test) ? (assuming Test had a PrintTo, which it doesn't). Or
something to indicate which test is failing?

In general, I hate tests that do things in loops vs parameterized tests, but
when they do loops, I prefer there be some indicator about which input failed.

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntryTest.cpp:149: for (auto test :
inputs) {
const auto&

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntryTest.cpp:183: for (auto test :
inputs) {
const auto&

[jochen (gone - plz use gerrit), 2015-05-15 14:44:13]

lgtm

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
File Source/platform/weborigin/OriginAccessEntry.cpp (right):

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntry.cpp:45: bool HostIsIPAddress(const
String& host)
no indent

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntry.cpp:56: url::Component
hostComponent(7, host.length());
On 2015/05/13 at 23:23:34, Ryan Sleevi wrote:
> grumble-nit - It's not immediately obvious that the 7 is tied to line 50, or
that someone won't come along and helpfully change things to https:// because
http:// is bad and then bad things happen :)
> 
> Make these two static constants in the function?

why not make it https:// already now :)

[Mike West, 2015-05-15 14:45:17]

WDYT, Jochen?

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
File Source/platform/weborigin/OriginAccessEntry.cpp (right):

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntry.cpp:38: #include
<url/third_party/mozilla/url_parse.h>
On 2015/05/13 at 23:23:34, Ryan Sleevi wrote:
> what the wat?

This is where 'url::Component' is defined. Don't ask me why, I'm just the guy
copy/pasting from KURL and GURL.

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntry.cpp:56: url::Component
hostComponent(7, host.length());
On 2015/05/13 at 23:23:34, Ryan Sleevi wrote:
> grumble-nit - It's not immediately obvious that the 7 is tied to line 50, or
that someone won't come along and helpfully change things to https:// because
http:// is bad and then bad things happen :)

I _should_ change this to HTTPS! It'll be tons more secure.

> Make these two static constants in the function?

Done.

https://codereview.chromium.org/1128913004/diff/1/Source/platform/weborigin/O...
Source/platform/weborigin/OriginAccessEntry.cpp:58: return
hostInfo.IsIPAddress();
On 2015/05/13 at 23:23:34, Ryan Sleevi wrote:
> This is basically duplicating GURL::HostIsIPAddress.

Noted.

[Mike West, 2015-05-15 14:45:56]

On 2015/05/15 at 14:45:17, Mike West (holiday in DE) wrote:
> WDYT, Jochen?

Wow. You're fast!

[Mike West, 2015-05-15 14:46:58]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2015-05-15 14:46:59]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1128913004/#ps40001
(title: "Feedback.")

[commit-bot: I haz the power, 2015-05-15 14:47:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128913004/40001

[commit-bot: I haz the power, 2015-05-15 16:16:40]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195410

[Mike West, 2015-05-16 04:59:01]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1135493005/ by mkwst@chromium.org.

The reason for reverting is: ASAN and ClusterFuzz do not like this patch.

BUG=488783.