Bug 211358 – A URL cannot have a username/password/port if its host is null

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Rob Buis]

A URL cannot have a username/password/port if its host is null [1], so
adjust URL.cpp accordingly.

Behavior matches Chrome and Firefox.

[1] https://url.spec.whatwg.org/#cannot-have-a-username-password-port

[Rob Buis]

Created attachment 398327 [details]
Patch

[Daniel Bates]

Comment on attachment 398327 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398327&action=review

Patch looks good.

> Source/WTF/wtf/URL.cpp:453
> +    if (hostStart() == m_hostEnd || protocolIs("file"))

Ok as-is. No change needed. Spec also mentions about checking cannot-be-a-base-URL flag. This is equivalent to checking !isHierarchical() (very quick read + intuition) and the optimal solution would do that. <--- function assets there is :, which I think is covered by m_isValid. If not, then need to check that too to avoid the assert triggering in debug.

[Daniel Bates]

Comment on attachment 398327 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398327&action=review

>> Source/WTF/wtf/URL.cpp:453
>> +    if (hostStart() == m_hostEnd || protocolIs("file"))
> 
> Ok as-is. No change needed. Spec also mentions about checking cannot-be-a-base-URL flag. This is equivalent to checking !isHierarchical() (very quick read + intuition) and the optimal solution would do that. <--- function assets there is :, which I think is covered by m_isValid. If not, then need to check that too to avoid the assert triggering in debug.

Also optimal solution is to move this check into a private member  function marked inline in .cpp instead of duplicating it, especially if the check will have 3 disjunction <-- meaning it's becoming complicated.

[Rob Buis]

Created attachment 398355 [details]
Patch

[Rob Buis]

Created attachment 398379 [details]
Patch

[Rob Buis]

Comment on attachment 398327 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398327&action=review

>>> Source/WTF/wtf/URL.cpp:453
>>> +    if (hostStart() == m_hostEnd || protocolIs("file"))
>> 
>> Ok as-is. No change needed. Spec also mentions about checking cannot-be-a-base-URL flag. This is equivalent to checking !isHierarchical() (very quick read + intuition) and the optimal solution would do that. <--- function assets there is :, which I think is covered by m_isValid. If not, then need to check that too to avoid the assert triggering in debug.
> 
> Also optimal solution is to move this check into a private member  function marked inline in .cpp instead of duplicating it, especially if the check will have 3 disjunction <-- meaning it's becoming complicated.

Thanks for the review! Today I found out cannot-be-a-base-URL is already handled in URLDecomposition, so I moved my code there.

[Rob Buis]

Created attachment 398388 [details]
Patch

[Darin Adler]

Comment on attachment 398388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398388&action=review

> Source/WTF/wtf/URL.h:169
> +    WTF_EXPORT_PRIVATE bool hasEmptyHost() const;

This is not significantly more efficient than writing:

    fullURL.host().isEmpty()

That’s one of the benefits of StringView. Not sure we need to add the new function.

The name seems to imply some difference between having "no host" and "empty host".

> Source/WebCore/html/URLDecomposition.cpp:61
> +    if (fullURL.hasEmptyHost() || fullURL.cannotBeABaseURL() || fullURL.protocolIs("file"))

Starting to think we need a URL::protocolIsFile function. Also irritated that there’s no concept here, like "cannot be base URL".

[Rob Buis]

Created attachment 398510 [details]
Patch

[EWS]

Committed r261173: <https://trac.webkit.org/changeset/261173>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398510 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62889969>