Allow url::SchemeHostPort to hold non-file scheme without port

Allow url::SchemeHostPort to hold non-file scheme without port

WebSockets use url::Origin to pass origin info between renderer and
browser. Currently, it cannot hold an origin with non-file scheme and
no port. Chrome extensions have been using such origins, so we need
to keep the channel to convey origin info work for such origins.

BUG=516971
R=sleevi,brettw

Committed: https://crrev.com/1ac9ec7bccd1b5178b18338b10149f36292f5fb6
Cr-Commit-Position: refs/heads/master@{#343895}

Committed: https://crrev.com/11a7c9fe93850341043844ab48bf379c485d05b1
Cr-Commit-Position: refs/heads/master@{#344181}

[tyoshino (SeeGerritForStatus), 2015-08-06 03:22:55]

tyoshino@chromium.org changed reviewers:
+ rsleevi@chromium.org

[tyoshino (SeeGerritForStatus), 2015-08-06 03:24:28]

Hi Ryan,

It's WIP (needs tests). But first I'd like you to review this briefly to see if
the approach looks good to you.

[Ryan Sleevi, 2015-08-07 01:35:35]

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

[Ryan Sleevi, 2015-08-07 01:35:36]

Adding Mike as well.

This Not LGTM. A big part of the goal here was to avoid hardcoding such lists,
and instead follow the generic URL construction.

Are chrome-extension urls handling themselves properly as non-standard URLs
(chrome-extension:blah)? Or are they misappropriating the generic URI syntax
(chrome-extension://blah) ?

Within the generic URI syntax, port is entirely optional (c.f.
https://tools.ietf.org/html/rfc3986#section-3.2 ).

Mike, I feel like this issue argues for ability to push scheme helpers to GURL,
like we'd previously discussed. What do you think?

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc
File url/scheme_host_port.cc (left):

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#old...
url/scheme_host_port.cc:53: bool isUnsupportedScheme =
blah. How'd I miss these variable names during review. They're all wrong :(

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc
File url/scheme_host_port.cc (right):

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#new...
url/scheme_host_port.cc:38: }
We already have such a registry. It's called a Standard URL in GURL - aka one
that conforms to the generic URL syntax.

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#new...
url/scheme_host_port.cc:49: void
SchemeHostPort::CanonicalizeHost(base::StringPiece host,
STYLE: Declaration order should match definition order. This should be moved.

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#new...
url/scheme_host_port.cc:75: if (!url::IsStandard(scheme.data(),
You've removed all of the documentation that explains this logic. Please provide
ample and clear reasoning for each of these.

[tyoshino (SeeGerritForStatus), 2015-08-07 02:47:54]

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc
File url/scheme_host_port.cc (right):

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#new...
url/scheme_host_port.cc:38: }
On 2015/08/07 01:35:36, Ryan Sleevi wrote:
> We already have such a registry. It's called a Standard URL in GURL - aka one
> that conforms to the generic URL syntax.

url::IsStandard() which is used by GURL is no longer working as a method to
check if URLs with the given scheme is expected to follow the generic URI
syntax. It may still be working for checking "syntax" (the reg-name production),
but at least the host component doesn't always have a string representing a
host. In content::RegisterContentSchemes, various Chrome specific addresses are
registered as standard scheme. On binaries where ChromeContentClient is used,
chrome-native, chrome-search, chrome-extension-resource, chrome-distiller,
chrome-extension are registered as standard schemes to url_util.

Like Blob URL, the filesystem URI also have non-host data in the authority
subcomponent when parsed following the generic URI syntax. But it's in the
default list of standard schemes used by DoIsStandard().

So, to know if we want to apply host canonicalization algorithm or not, we
shouldn't use IsStandard(), but have something else. This could be placed in
url_util.h.

[tyoshino (SeeGerritForStatus), 2015-08-07 02:53:38]

On 2015/08/07 01:35:36, Ryan Sleevi wrote:
> Adding Mike as well.
> 
> This Not LGTM. A big part of the goal here was to avoid hardcoding such lists,
> and instead follow the generic URL construction.
> 
> Are chrome-extension urls handling themselves properly as non-standard URLs
> (chrome-extension:blah)? Or are they misappropriating the generic URI syntax
> (chrome-extension://blah) ?

They're using the generic URI syntax. WebSockets and CORS enabled (actually
more. See https://github.com/whatwg/fetch/issues/91) have been sending origins
in the latter format for long time.

[tyoshino (SeeGerritForStatus), 2015-08-07 06:10:23]

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc
File url/scheme_host_port.cc (right):

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#new...
url/scheme_host_port.cc:38: }
On 2015/08/07 02:47:54, tyoshino wrote:
> On 2015/08/07 01:35:36, Ryan Sleevi wrote:
> > We already have such a registry. It's called a Standard URL in GURL - aka
one
> > that conforms to the generic URL syntax.
> 
> url::IsStandard() which is used by GURL is no longer working as a method to
> check if URLs with the given scheme is expected to follow the generic URI
> syntax. It may still be working for checking "syntax" (the reg-name
production),
> but at least the host component doesn't always have a string representing a
> host. In content::RegisterContentSchemes, various Chrome specific addresses
are
> registered as standard scheme. On binaries where ChromeContentClient is used,
> chrome-native, chrome-search, chrome-extension-resource, chrome-distiller,
> chrome-extension are registered as standard schemes to url_util.
> 
> Like Blob URL, the filesystem URI also have non-host data in the authority
> subcomponent when parsed following the generic URI syntax. But it's in the
> default list of standard schemes used by DoIsStandard().
> 

This is not a problem in SchemeHostPort as it doesn't accept filesystem URLs.

> So, to know if we want to apply host canonicalization algorithm or not, we
> shouldn't use IsStandard(), but have something else. This could be placed in
> url_util.h.

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#new...
url/scheme_host_port.cc:49: void
SchemeHostPort::CanonicalizeHost(base::StringPiece host,
On 2015/08/07 01:35:36, Ryan Sleevi wrote:
> STYLE: Declaration order should match definition order. This should be moved.

Done.

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#new...
url/scheme_host_port.cc:75: if (!url::IsStandard(scheme.data(),
On 2015/08/07 01:35:36, Ryan Sleevi wrote:
> You've removed all of the documentation that explains this logic. Please
provide
> ample and clear reasoning for each of these.

The comment in the .h file also explains almost the same thing as the comment we
had here. And, after refactoring, the method calls newly introduced helped
improve descriptiveness of the code. So, I removed it.

I've re-added some to have better readability.

[tyoshino (SeeGerritForStatus), 2015-08-07 07:50:08]

On 2015/08/07 02:53:38, tyoshino wrote:
> On 2015/08/07 01:35:36, Ryan Sleevi wrote:
> > Adding Mike as well.
> > 
> > This Not LGTM. A big part of the goal here was to avoid hardcoding such
lists,
> > and instead follow the generic URL construction.
> > 
> > Are chrome-extension urls handling themselves properly as non-standard URLs
> > (chrome-extension:blah)? Or are they misappropriating the generic URI syntax
> > (chrome-extension://blah) ?
> 
> They're using the generic URI syntax. WebSockets and CORS enabled (actually
> more. See https://github.com/whatwg/fetch/issues/91) have been sending origins
> in the latter format for long time.

To make the Origin header useful for Chrome extensions, we need to include the
extension ID in the Origin header. Then, the developer can serve data only to
the extension by authors trusted by the service provider, as far as the user's
extension installation (webstore system, network, uniqueness of the app ID, etc.
are all) is not compromised. The only place in the origin mechanism to hold this
info is the host part.

Whether this is working correctly or not from security point of view could be
revisited later by the security team with more detailed investigation. But
fixing the regression is P0, I think.

If the chrome-extension URLs were represented in the form of
"chrome-extension:blah", the host component is considered to be missing from
RFC3986's point of view. So, we need to extend RFC 6454 to include the path
"blah" into the origin.

But it's been used in the "chrome-extension://blah" form for long time. We
cannot change it now. From RFCs' point of view, what we're doing now could be
justified as:
- "blah" is of an extended authority type
- The extended authority type to hold a chrome-extension app ID should be passed
to RFC 6454 as if it were a host

Anyway, we're, in reality, using an extended version of the URL or one of the
Web Origin now.

[tyoshino (SeeGerritForStatus), 2015-08-07 07:57:39]

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc
File url/scheme_host_port.cc (right):

https://codereview.chromium.org/1272113002/diff/1/url/scheme_host_port.cc#new...
url/scheme_host_port.cc:38: }
On 2015/08/07 06:10:23, tyoshino wrote:
> On 2015/08/07 02:47:54, tyoshino wrote:
> > On 2015/08/07 01:35:36, Ryan Sleevi wrote:
> > > We already have such a registry. It's called a Standard URL in GURL - aka
> one
> > > that conforms to the generic URL syntax.
> > 

To be clear,

GURL is using url::Canonicalize() to set up parsed_. KURL is doing so, too.
url::Canonicalize() is using url::IsStandard() to switch between Parse.*URL
functions defined in third_party/mozilla/url_parse.h. Not based on whether "//"
is include or not. Once ParseStandardURL() is called, it ignores "//". This
means that GURL/KURL are using IsStandard() as a way to check if the following
URL string follows the

  "//" authority path-abempty

construction defined in the generic URI syntax or not. Not a way to check if the
following URL string conforms to any of the hier-part constructions.

Let's say like "conforms to the URI construction with authority" or something to
avoid confusion.

> > url::IsStandard() which is used by GURL is no longer working as a method to
> > check if URLs with the given scheme is expected to follow the generic URI
> > syntax. It may still be working for checking "syntax" (the reg-name
> production),
> > but at least the host component doesn't always have a string representing a
> > host. In content::RegisterContentSchemes, various Chrome specific addresses
> are
> > registered as standard scheme. On binaries where ChromeContentClient is
used,
> > chrome-native, chrome-search, chrome-extension-resource, chrome-distiller,
> > chrome-extension are registered as standard schemes to url_util.
> > 
> > Like Blob URL, the filesystem URI also have non-host data in the authority
> > subcomponent when parsed following the generic URI syntax. But it's in the
> > default list of standard schemes used by DoIsStandard().
> > 
> 
> This is not a problem in SchemeHostPort as it doesn't accept filesystem URLs.
> 
> > So, to know if we want to apply host canonicalization algorithm or not, we
> > shouldn't use IsStandard(), but have something else. This could be placed in
> > url_util.h.
>

[Ryan Sleevi, 2015-08-07 20:28:21]

On 2015/08/07 07:50:08, tyoshino (off on Aug 10) wrote:
> On 2015/08/07 02:53:38, tyoshino wrote:
> > On 2015/08/07 01:35:36, Ryan Sleevi wrote:
> > > Adding Mike as well.
> > > 
> > > This Not LGTM. A big part of the goal here was to avoid hardcoding such
> lists,
> > > and instead follow the generic URL construction.
> > > 
> > > Are chrome-extension urls handling themselves properly as non-standard
URLs
> > > (chrome-extension:blah)? Or are they misappropriating the generic URI
syntax
> > > (chrome-extension://blah) ?
> > 
> > They're using the generic URI syntax. WebSockets and CORS enabled (actually
> > more. See https://github.com/whatwg/fetch/issues/91) have been sending
origins
> > in the latter format for long time.
> 
> To make the Origin header useful for Chrome extensions, we need to include the
> extension ID in the Origin header. Then, the developer can serve data only to
> the extension by authors trusted by the service provider, as far as the user's
> extension installation (webstore system, network, uniqueness of the app ID,
etc.
> are all) is not compromised. The only place in the origin mechanism to hold
this
> info is the host part.
> 
> Whether this is working correctly or not from security point of view could be
> revisited later by the security team with more detailed investigation. But
> fixing the regression is P0, I think.
> 
> If the chrome-extension URLs were represented in the form of
> "chrome-extension:blah", the host component is considered to be missing from
> RFC3986's point of view. So, we need to extend RFC 6454 to include the path
> "blah" into the origin.
> 
> But it's been used in the "chrome-extension://blah" form for long time. We
> cannot change it now. From RFCs' point of view, what we're doing now could be
> justified as:
> - "blah" is of an extended authority type
> - The extended authority type to hold a chrome-extension app ID should be
passed
> to RFC 6454 as if it were a host
> 
> Anyway, we're, in reality, using an extended version of the URL or one of the
> Web Origin now.

Apologies for the confusion I may have caused, but I don't think this analysis
is correct.

It sounds like chrome-extension://blah is following the generic URI (Standard
URI) syntax. The generic URI syntax _does not_ require a port number be
specified, and the 'host' used here is a registered name:
   A host identified by a registered name is a sequence of characters
   usually intended for lookup within a locally defined host or service
   name registry, though the URI's scheme-specific semantics may require
   that a specific registry (or fixed name table) be used instead.

Similarly, 3.2.3 defines port as optional.

To add clarification to the remark I made to Mike, this question of what to do
with default ports was something we discussed when developing the
url::SchemeHostPort, url::Origin, and url::GURL split. The presumption (proved
incorrect by chrome extensions) was that port would only be optional for file://
URLs.

We also discussed whether or not GURL should 'know' about schemes like blob: or
filesystem:, or whether GURL should be 'ignorant' of the Web Platform concepts,
and instead have those dependency injected in. For example, we discussed having
a way to register a callback of some sort to allow a caller to add a scheme as a
standard scheme, but then define how its Origin was produced and serialized. We
didn't think this was immediately necessary, since the 'ship had sailed' on GURL
knowing about filesystem: and blob:, but I think it's readily apparent that we
need some sort of extensibility for the Chrome schemes.

That was the broader context for the Not LG, and why I added Mike.

So the question is now, what can we do about this regression, and what are the
viable short and long term solutions for this.

I think we're in agreement, and supported by RFC 3986, of a few conditions:
 - Some standard schemes have a default port (that is a valid port). Examples
are HTTP/80 or HTTPS/443.
 - Some standard schemes have an optional port, which if present, must be valid,
but if absent, is OK. The canonical example of this is file/0, but other file
URLs may have a port.
 - Some standard schemes never have a port. Here, chrome-extension://foo:0 would
be ill-defined, but chrome-extension://foo is not (even though the default port
for chrome-extension:// *is* 0)

We currently don't have a way of expressing this in url. I think in an ideal
world, we'd be able to express via a policy configuration object, when adding a
standard scheme, what standard productions are valid. For example, we'd express
whether or not port was required, what its default value was, and whether or not
it was permitted to be included (in production or parsing). On this policy
object, we might provide a function for indicating how to produce an Origin for
a given URL - such as looking at the content of a blob: or filesystem: URL and
parsing the inner content. Things of that nature.

Is this something you need to merge? Or is this something we could explore, as a
larger change? In particular, if we change url::AddStandardScheme to require a
policy options object, that might be the best, and aligned with the long-term
pony-dream of mine of extricating Origin production for blob/filesystem out of
GURL. Alternatively, we might add url::AddStandardSchemeWithOptions as an
additional function, with the existing url::AddStandardScheme having a certain
presumption of behaviour.

Finally, we might determine, from all the calls to url::AddStandardScheme, that
the default is that anything added via that method should not require a port
number at all. I'm least inclined to like this solution, since I think other,
non-Chromium embedders use this mechanism (e.g. CEF), so it's hard to appreciate
the full set of concerns there.

As far as determining how this all interacts, I think we'd move such a registry
to be part of url/url_util, rather than SchemeHostPort. SchemeHostPort would
call in to url_util to determine whether or not, for a given scheme, the port is
valid, and use that to inform SchemeHostPort.IsInvalid().

Does that provide ample enough context for the discussions and where we might
want to do? I guess my biggest concern is understanding what constraints you
have - but I agree, we absolutely want to fix this.

[tyoshino (SeeGerritForStatus), 2015-08-11 11:19:20]

On 2015/08/07 20:28:21, Ryan Sleevi wrote:
> On 2015/08/07 07:50:08, tyoshino (off on Aug 10) wrote:
> > On 2015/08/07 02:53:38, tyoshino wrote:
> > > On 2015/08/07 01:35:36, Ryan Sleevi wrote:
> > > > Adding Mike as well.
> > > > 
> > > > This Not LGTM. A big part of the goal here was to avoid hardcoding such
> > lists,
> > > > and instead follow the generic URL construction.
> > > > 
> > > > Are chrome-extension urls handling themselves properly as non-standard
> URLs
> > > > (chrome-extension:blah)? Or are they misappropriating the generic URI
> syntax
> > > > (chrome-extension://blah) ?
> > > 
> > > They're using the generic URI syntax. WebSockets and CORS enabled
(actually
> > > more. See https://github.com/whatwg/fetch/issues/91) have been sending
> origins
> > > in the latter format for long time.
> > 
> > To make the Origin header useful for Chrome extensions, we need to include
the
> > extension ID in the Origin header. Then, the developer can serve data only
to
> > the extension by authors trusted by the service provider, as far as the
user's
> > extension installation (webstore system, network, uniqueness of the app ID,
> etc.
> > are all) is not compromised. The only place in the origin mechanism to hold
> this
> > info is the host part.
> > 
> > Whether this is working correctly or not from security point of view could
be
> > revisited later by the security team with more detailed investigation. But
> > fixing the regression is P0, I think.
> > 
> > If the chrome-extension URLs were represented in the form of
> > "chrome-extension:blah", the host component is considered to be missing from
> > RFC3986's point of view. So, we need to extend RFC 6454 to include the path
> > "blah" into the origin.
> > 
> > But it's been used in the "chrome-extension://blah" form for long time. We
> > cannot change it now. From RFCs' point of view, what we're doing now could
be
> > justified as:
> > - "blah" is of an extended authority type
> > - The extended authority type to hold a chrome-extension app ID should be
> passed
> > to RFC 6454 as if it were a host
> > 
> > Anyway, we're, in reality, using an extended version of the URL or one of
the
> > Web Origin now.
> 
> Apologies for the confusion I may have caused, but I don't think this analysis
> is correct.
> 
> It sounds like chrome-extension://blah is following the generic URI (Standard
> URI) syntax. The generic URI syntax _does not_ require a port number be
> specified, and the 'host' used here is a registered name:
>    A host identified by a registered name is a sequence of characters
>    usually intended for lookup within a locally defined host or service
>    name registry, though the URI's scheme-specific semantics may require
>    that a specific registry (or fixed name table) be used instead.
> 
> Similarly, 3.2.3 defines port as optional.
> 

Sorry for the unclear text. By "extended", I didn't mean that a port must be
attached to a generic URI. The chrome-extension URI does match with the reg-name
ABNF, yes, but the applying host name canonicalization on the app ID is strange.
That's I wanted to say. CanonicalizeHost() doesn't have any harmful effect on
the sequence of alphabet letters used by the app ID syntax, but I tried to apply
it to only ones known to have IP/reg-name representing an address of a host. We
can also take an approach to consider that we have extended usage of the generic
URI like chrome-extension URLs but keep such an extensions to have authority
component conforming even to the non-syntax expectations (e.g. canonicalization
made by CanonicalizeHost). I want to discuss what's the best with you guys, yes.

I don't have any strong opinion about which component should be responsible for
syntax and which is for grammar, etc. If we give url::IsStandard() a role to
check only syntax, it's fine. But then, it wouldn't work for determining whether
we should apply CanonicalizeHost() or not, ... I thought.

> To add clarification to the remark I made to Mike, this question of what to do
> with default ports was something we discussed when developing the
> url::SchemeHostPort, url::Origin, and url::GURL split. The presumption (proved
> incorrect by chrome extensions) was that port would only be optional for
file://
> URLs.

Right!

> 
> We also discussed whether or not GURL should 'know' about schemes like blob:
or
> filesystem:, or whether GURL should be 'ignorant' of the Web Platform
concepts,
> and instead have those dependency injected in. For example, we discussed
having
> a way to register a callback of some sort to allow a caller to add a scheme as
a
> standard scheme, but then define how its Origin was produced and serialized.
We
> didn't think this was immediately necessary, since the 'ship had sailed' on
GURL
> knowing about filesystem: and blob:, but I think it's readily apparent that we
> need some sort of extensibility for the Chrome schemes.
> 
> That was the broader context for the Not LG, and why I added Mike.

Sounds good to have such a mechanism. Yeah.

> 
> So the question is now, what can we do about this regression, and what are the
> viable short and long term solutions for this.
> 
> I think we're in agreement, and supported by RFC 3986, of a few conditions:
>  - Some standard schemes have a default port (that is a valid port). Examples
> are HTTP/80 or HTTPS/443.

Right.

>  - Some standard schemes have an optional port, which if present, must be
valid,
> but if absent, is OK. The canonical example of this is file/0, but other file
> URLs may have a port.
>  - Some standard schemes never have a port. Here, chrome-extension://foo:0
would
> be ill-defined, but chrome-extension://foo is not (even though the default
port
> for chrome-extension:// *is* 0)

Right.

> 
> We currently don't have a way of expressing this in url. I think in an ideal
> world, we'd be able to express via a policy configuration object, when adding
a
> standard scheme, what standard productions are valid. For example, we'd
express
> whether or not port was required, what its default value was, and whether or
not
> it was permitted to be included (in production or parsing). On this policy
> object, we might provide a function for indicating how to produce an Origin
for
> a given URL - such as looking at the content of a blob: or filesystem: URL and
> parsing the inner content. Things of that nature.
> 
> Is this something you need to merge? Or is this something we could explore, as
a
> larger change? In particular, if we change url::AddStandardScheme to require a
> policy options object, that might be the best, and aligned with the long-term
> pony-dream of mine of extricating Origin production for blob/filesystem out of
> GURL. Alternatively, we might add url::AddStandardSchemeWithOptions as an
> additional function, with the existing url::AddStandardScheme having a certain
> presumption of behaviour.

All the solutions proposed here look good and I really want to explore, after
... making some quicker fix.

> 
> Finally, we might determine, from all the calls to url::AddStandardScheme,
that
> the default is that anything added via that method should not require a port
> number at all. I'm least inclined to like this solution, since I think other,
> non-Chromium embedders use this mechanism (e.g. CEF), so it's hard to
appreciate
> the full set of concerns there.
> 
> As far as determining how this all interacts, I think we'd move such a
registry
> to be part of url/url_util, rather than SchemeHostPort. SchemeHostPort would
> call in to url_util to determine whether or not, for a given scheme, the port
is
> valid, and use that to inform SchemeHostPort.IsInvalid().
> 
> Does that provide ample enough context for the discussions and where we might
> want to do? I guess my biggest concern is understanding what constraints you
> have - but I agree, we absolutely want to fix this.

This works. I don't have any constraint except for timeframe.

[Ryan Sleevi, 2015-08-11 22:44:52]

On 2015/08/11 11:19:20, tyoshino wrote:
> Sorry for the unclear text. By "extended", I didn't mean that a port must be
> attached to a generic URI. The chrome-extension URI does match with the
reg-name
> ABNF, yes, but the applying host name canonicalization on the app ID is
strange.
> That's I wanted to say. CanonicalizeHost() doesn't have any harmful effect on
> the sequence of alphabet letters used by the app ID syntax, but I tried to
apply
> it to only ones known to have IP/reg-name representing an address of a host.
We
> can also take an approach to consider that we have extended usage of the
generic
> URI like chrome-extension URLs but keep such an extensions to have authority
> component conforming even to the non-syntax expectations (e.g.
canonicalization
> made by CanonicalizeHost). I want to discuss what's the best with you guys,
yes.
> 
> I don't have any strong opinion about which component should be responsible
for
> syntax and which is for grammar, etc. If we give url::IsStandard() a role to
> check only syntax, it's fine. But then, it wouldn't work for determining
whether
> we should apply CanonicalizeHost() or not, ... I thought.
> 

the chrome-extension:// (and seemingly the other standard schemes) do seem to
conform to the DNS hostname syntax re: CanonicalizeHost, so I don't see the
fundamental issue? I think this is something we'd already have issue with, with
regards to the URL spec ( https://url.spec.whatwg.org ), if we didn't, so that's
not an unreasonable requirement.

> All the solutions proposed here look good and I really want to explore, after
> ... making some quicker fix.

I don't see why url::AddStandardSchemeWithOptions, if viable, wouldn't meet that
criteria? Even AddStandardScheme could be quickly done, it's just a greater
merge risk.

> This works. I don't have any constraint except for timeframe.

Well, I think we still have an issue that I think we want url::SchemeHostPort
largely ignorant of the scheme-related policy; that is, that should be separate
from the SchemeHostPort implementation, and SHP should just ask whether or not
an absent/invalid port invalidates the SHP.

I think that probably hangs in url_util.h, and SHP calls into that. Overall, I
think we're in agreement as to the what needs to happen (some schemes require
ports, some require valid ports if present, some don't expect ports), it's just
a question of where. Hopefully the previously reply gave enough context as to
desirable approaches. If there's still confusion, ping me on gchat when you get
in today and we can iterate more real time. Happy to stay as late as needed.

[tyoshino (SeeGerritForStatus), 2015-08-12 03:40:22]

On 2015/08/11 22:44:52, Ryan Sleevi wrote:
> On 2015/08/11 11:19:20, tyoshino wrote:
> > Sorry for the unclear text. By "extended", I didn't mean that a port must be
> > attached to a generic URI. The chrome-extension URI does match with the
> reg-name
> > ABNF, yes, but the applying host name canonicalization on the app ID is
> strange.
> > That's I wanted to say. CanonicalizeHost() doesn't have any harmful effect
on
> > the sequence of alphabet letters used by the app ID syntax, but I tried to
> apply
> > it to only ones known to have IP/reg-name representing an address of a host.
> We
> > can also take an approach to consider that we have extended usage of the
> generic
> > URI like chrome-extension URLs but keep such an extensions to have authority
> > component conforming even to the non-syntax expectations (e.g.
> canonicalization
> > made by CanonicalizeHost). I want to discuss what's the best with you guys,
> yes.
> > 
> > I don't have any strong opinion about which component should be responsible
> for
> > syntax and which is for grammar, etc. If we give url::IsStandard() a role to
> > check only syntax, it's fine. But then, it wouldn't work for determining
> whether
> > we should apply CanonicalizeHost() or not, ... I thought.
> > 
> 
> the chrome-extension:// (and seemingly the other standard schemes) do seem to
> conform to the DNS hostname syntax re: CanonicalizeHost, so I don't see the
> fundamental issue? I think this is something we'd already have issue with,
with

I said the same above. It does conform to the ABNF production. Applying
CanonicalizeHost() is not harmful. It does nothing.

I just wondered if it's intentionally designed to look like (conform to the ABNF
of) the host in the generic URI or not but accidentally conforming to. If it is,
it's reasonable to apply CanonicalizeHost(). If not, it's more reasonable to
categorize the chrome-extension into the same group as the filesystem scheme,
etc. for which DoCanonicalize() takes special care of.

> regards to the URL spec ( https://url.spec.whatwg.org ), if we didn't, so
that's
> not an unreasonable requirement.
> 
> > All the solutions proposed here look good and I really want to explore,
after
> > ... making some quicker fix.
> 
> I don't see why url::AddStandardSchemeWithOptions, if viable, wouldn't meet
that
> criteria? Even AddStandardScheme could be quickly done, it's just a greater
> merge risk.
> 

I'm not objecting to try it out. I just repeated my opinion that fixing it
quickly is important, and implied worry about merge risk.

> > This works. I don't have any constraint except for timeframe.
> 
> Well, I think we still have an issue that I think we want url::SchemeHostPort
> largely ignorant of the scheme-related policy; that is, that should be
separate
> from the SchemeHostPort implementation, and SHP should just ask whether or not
> an absent/invalid port invalidates the SHP.
> 

Yeah, that's easy to do and I'm doing it.

> I think that probably hangs in url_util.h, and SHP calls into that. Overall, I
> think we're in agreement as to the what needs to happen (some schemes require
> ports, some require valid ports if present, some don't expect ports), it's
just
> a question of where. Hopefully the previously reply gave enough context as to
> desirable approaches. If there's still confusion, ping me on gchat when you
get
> in today and we can iterate more real time. Happy to stay as late as needed.

Again, I'm not objecting to it. Thanks for your review. I'll post PTAL once it's
done.

[tyoshino (SeeGerritForStatus), 2015-08-12 09:54:58]

Done. PTAL

[tyoshino (SeeGerritForStatus), 2015-08-13 15:00:02]

Added unittests for the newly introduced url_util functions.

[tyoshino (SeeGerritForStatus), 2015-08-13 16:05:13]

Patchset #10 (id:180001) has been deleted

[tyoshino (SeeGerritForStatus), 2015-08-13 18:09:44]

I'll fix the build failures tomorrow. At least chrome compiles now.

[Ryan Sleevi, 2015-08-13 22:32:12]

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

[Ryan Sleevi, 2015-08-13 22:32:13]

LGTM; you'll want Brett as url/ OWNERs to review.

https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.cc
File url/scheme_host_port.cc (right):

https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.c...
url/scheme_host_port.cc:25: const Component raw_host_component(0,
static_cast<int>(host.length()));
Same DANGER remark applies below.

https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.c...
url/scheme_host_port.cc:50: Component(0, static_cast<int>(scheme.length())),
DANGER: I missed this in the original review, but because |scheme| might have
come from IPC (thus hostile input), it's possible that scheme.length() >
std::numeric_limits<int>::max(), thus creating an invalid component.

It may make more sense to do base::checked_cast<int>(scheme.length()), which
will still crash the browser process on an invalid IPC, but would be strictly
safer.

(Or we could rely on the fact that the IPC filter will have filtered these out
as being messages that are too large, but the nature of this constructor, and
the inherent danger, makes me think we're better off with checked_cast<>'s)

https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.c...
url/scheme_host_port.cc:76: // Return an invalid object if the scheme is 'file'
and the port is
This comment seems out of date/incorrect; that is, it doesn't just apply to
'file' schemed objects.

https://codereview.chromium.org/1272113002/diff/220001/url/url_util.cc
File url/url_util.cc (right):

https://codereview.chromium.org/1272113002/diff/220001/url/url_util.cc#newcode25
url/url_util.cc:25: {kFileScheme, SCHEME_WITHOUT_PORT},  // Yes, file urls can
have a hostname!
nit: It took me a while to find out why this is 'true'

RFC 3986 defines the generic syntax, but the IANA registry for file (
www.iana.org/assignments/uri-schemes/uri-schemes.xhtml ) refers to
http://tools.ietf.org/html/rfc1738#section-3.10 , but that's then updated by
http://tools.ietf.org/html/rfc3986 

I'm not 100% sure this is universally true, but it's consistent enough with the
previous implementation, so yolo.

[tyoshino (SeeGerritForStatus), 2015-08-14 06:29:01]

Thanks!

https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.cc
File url/scheme_host_port.cc (right):

https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.c...
url/scheme_host_port.cc:25: const Component raw_host_component(0,
static_cast<int>(host.length()));
On 2015/08/13 22:32:12, Ryan Sleevi (out until 8-24) wrote:
> Same DANGER remark applies below.

Done.

https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.c...
url/scheme_host_port.cc:50: Component(0, static_cast<int>(scheme.length())),
On 2015/08/13 22:32:12, Ryan Sleevi (out until 8-24) wrote:
> DANGER: I missed this in the original review, but because |scheme| might have
> come from IPC (thus hostile input), it's possible that scheme.length() >
> std::numeric_limits<int>::max(), thus creating an invalid component.
> 
> It may make more sense to do base::checked_cast<int>(scheme.length()), which
> will still crash the browser process on an invalid IPC, but would be strictly
> safer.
> 
> (Or we could rely on the fact that the IPC filter will have filtered these out
> as being messages that are too large, but the nature of this constructor, and
> the inherent danger, makes me think we're better off with checked_cast<>'s)

Agreed. Changed to checked_cast.

https://codereview.chromium.org/1272113002/diff/220001/url/scheme_host_port.c...
url/scheme_host_port.cc:76: // Return an invalid object if the scheme is 'file'
and the port is
On 2015/08/13 22:32:12, Ryan Sleevi (out until 8-24) wrote:
> This comment seems out of date/incorrect; that is, it doesn't just apply to
> 'file' schemed objects.

Updated

https://codereview.chromium.org/1272113002/diff/220001/url/url_util.cc
File url/url_util.cc (right):

https://codereview.chromium.org/1272113002/diff/220001/url/url_util.cc#newcode25
url/url_util.cc:25: {kFileScheme, SCHEME_WITHOUT_PORT},  // Yes, file urls can
have a hostname!
On 2015/08/13 22:32:12, Ryan Sleevi (out until 8-24) wrote:
> nit: It took me a while to find out why this is 'true'
> 
> RFC 3986 defines the generic syntax, but the IANA registry for file (
> http://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml ) refers to
> http://tools.ietf.org/html/rfc1738#section-3.10 , but that's then updated by
> http://tools.ietf.org/html/rfc3986 
> 
> I'm not 100% sure this is universally true, but it's consistent enough with
the
> previous implementation, so yolo.

Ya. At least we won't break anything new, I think.

[tyoshino (SeeGerritForStatus), 2015-08-14 06:30:01]

Brett, please take a look. This fix is for a stable blocker bug.

[tyoshino (SeeGerritForStatus), 2015-08-14 13:10:35]

Now the builds are green.

[brettw, 2015-08-14 18:05:02]

LGTM, I didn't look at all of the details.

[tyoshino (SeeGerritForStatus), 2015-08-14 21:42:51]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-08-14 21:42:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1272113002/#ps240001
(title: "Addressed #19. Fixed build.")

[commit-bot: I haz the power, 2015-08-14 21:43:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272113002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272113002/240001

[commit-bot: I haz the power, 2015-08-14 21:45:09]

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

[commit-bot: I haz the power, 2015-08-14 21:45:10]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mike West, 2015-08-17 22:31:12]

On 2015/08/14 at 21:45:10, commit-bot wrote:
> Try jobs failed on following builders:
>   ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
>   mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Looks like this needed a rebase. Mind trying again, tyoshino@?

[tyoshino (SeeGerritForStatus), 2015-08-18 04:32:25]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-08-18 04:32:26]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1272113002/#ps260001
(title: "Rebase")

[tyoshino (SeeGerritForStatus), 2015-08-18 04:32:57]

On 2015/08/17 22:31:12, Mike West (traveling) wrote:
> On 2015/08/14 at 21:45:10, commit-bot wrote:
> > Try jobs failed on following builders:
> >   ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
> >   mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
> >   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> 
> Looks like this needed a rebase. Mind trying again, tyoshino@?

Sorry, I was OOO on Monday. Yes. I've sent it to the CQ again after rebasing.

[commit-bot: I haz the power, 2015-08-18 04:34:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272113002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272113002/260001

[commit-bot: I haz the power, 2015-08-18 04:54:36]

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

[commit-bot: I haz the power, 2015-08-18 04:54:37]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)

[tyoshino (SeeGerritForStatus), 2015-08-18 05:27:39]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-08-18 05:27:40]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1272113002/#ps280001
(title: "Fix include")

[commit-bot: I haz the power, 2015-08-18 05:27:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272113002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272113002/280001

[commit-bot: I haz the power, 2015-08-18 05:51:17]

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

[commit-bot: I haz the power, 2015-08-18 05:51:18]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[tyoshino (SeeGerritForStatus), 2015-08-18 06:16:35]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-08-18 06:16:35]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1272113002/#ps300001
(title: "Fix iOS build")

[commit-bot: I haz the power, 2015-08-18 06:17:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272113002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272113002/300001

[commit-bot: I haz the power, 2015-08-18 07:06:39]

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

[commit-bot: I haz the power, 2015-08-18 07:06:40]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tyoshino (SeeGerritForStatus), 2015-08-18 07:28:04]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2015-08-18 07:28:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272113002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272113002/300001

[commit-bot: I haz the power, 2015-08-18 08:31:32]

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

[commit-bot: I haz the power, 2015-08-18 08:31:33]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tyoshino (SeeGerritForStatus), 2015-08-18 11:17:28]

I accidentally used a global array of non-POD type objects (SchemeWithType
struct with constructor). Fixing now.

[tyoshino (SeeGerritForStatus), 2015-08-18 11:29:32]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-08-18 11:29:32]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1272113002/#ps320001
(title: "Make SchemeWithType POD type")

[commit-bot: I haz the power, 2015-08-18 11:29:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272113002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272113002/320001

[commit-bot: I haz the power, 2015-08-18 12:50:53]

Committed patchset #16 (id:320001)

[commit-bot: I haz the power, 2015-08-18 12:51:26]

Patchset 16 (id:??) landed as
https://crrev.com/1ac9ec7bccd1b5178b18338b10149f36292f5fb6
Cr-Commit-Position: refs/heads/master@{#343895}

[msramek, 2015-08-18 13:03:45]

A revert of this CL (patchset #16 id:320001) has been created in
https://codereview.chromium.org/1301563003/ by msramek@chromium.org.

The reason for reverting is: Broken build.

http://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/buil...
.

[tyoshino (SeeGerritForStatus), 2015-08-19 04:05:33]

Prepared a patchset for relanding. Checking iOS Device ninja rel

[tyoshino (SeeGerritForStatus), 2015-08-19 06:44:51]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-08-19 06:44:51]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1272113002/#ps380001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-19 06:45:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272113002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272113002/380001

[commit-bot: I haz the power, 2015-08-19 08:51:53]

Committed patchset #19 (id:380001)

[commit-bot: I haz the power, 2015-08-19 08:52:29]

Patchset 19 (id:??) landed as
https://crrev.com/11a7c9fe93850341043844ab48bf379c485d05b1
Cr-Commit-Position: refs/heads/master@{#344181}