Bug 163806 – URL::port should return Optional<uint16_t>

• Component: Platform
• Status: RESOLVED FIXED

[Alex Christensen]

URL::port should return Optional<uint16_t>

[Alex Christensen]

Created attachment 292385 [details]
Patch

[Build Bot]

Comment on attachment 292385 [details]
Patch

Attachment 292385 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2339936

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 292392 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292385 [details]
Patch

Attachment 292385 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2339935

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 292393 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292385 [details]
Patch

Attachment 292385 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2339946

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 292394 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Alex Christensen]

Created attachment 292428 [details]
Patch

[Alex Christensen]

Created attachment 292432 [details]
Patch

[Alex Christensen]

Created attachment 292443 [details]
Patch

[Alex Christensen]

Created attachment 292446 [details]
Patch

[Darin Adler]

Comment on attachment 292446 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.h:44
> +        m_databaseName = String(WTF::HashTableDeletedValue);

This should use member initialization, not assignment.

> Source/WebCore/editing/cocoa/DataDetection.mm:159
> +    return [softLink_DataDetectorsCore_DDURLTapAndHoldSchemes() containsObject:(NSString *)downcast<HTMLAnchorElement>(element).href().protocol().toString().convertToASCIILowercase()];

Call sites like this one can use toStringWithoutCopying rather than toString, because the nature of the code guarantees that no one is going to make a reference to the string and keep it alive after the expression is done. Not sure if it’s critical to use that function, but that is what it is intended for.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:485
> +    if (document().page() && !SchemeRegistry::shouldTreatURLSchemeAsLocal(document().page()->mainFrame().document()->baseURL().protocol().toString()) && document().page()->settings().autostartOriginPlugInSnapshottingEnabled())

Ditto.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:704
> +    if (!SchemeRegistry::shouldTreatURLSchemeAsLocal(m_loadedUrl.protocol().toString()) && !m_loadedUrl.host().isEmpty() && m_loadedUrl.host() == document().page()->mainFrame().document()->baseURL().host()) {

Ditto.

> Source/WebCore/loader/CrossOriginAccessControl.cpp:148
> +    return SchemeRegistry::shouldTreatURLSchemeAsCORSEnabled(redirectURL.protocol().toString())

Ditto.

> Source/WebCore/loader/DocumentLoader.cpp:780
> +    return !documentLoader.substituteData().isValid() && !SchemeRegistry::shouldTreatURLSchemeAsLocal(documentLoader.request().url().protocol().toString());

Ditto.

> Source/WebCore/loader/DocumentLoader.cpp:1489
> +    bool shouldLoadEmpty = !m_substituteData.isValid() && (m_request.url().isEmpty() || SchemeRegistry::shouldLoadURLSchemeAsEmptyDocument(m_request.url().protocol().toString()));
> +    if (!shouldLoadEmpty && !frameLoader()->client().representationExistsForURLScheme(m_request.url().protocol().toString()))

Ditto.

> Source/WebCore/loader/DocumentLoader.cpp:1498
> +    String mimeType = shouldLoadEmpty ? "text/html" : frameLoader()->client().generatedMIMETypeForURLScheme(m_request.url().protocol().toString());

Ditto.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:148
> +    if (!SchemeRegistry::shouldTreatURLSchemeAsCORSEnabled(request.url().protocol().toString())) {

Ditto.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:112
> +    if (!SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol().toString()))

Ditto.

> Source/WebCore/loader/cache/CachedResource.cpp:477
> +            if (m_type == MainResource || SchemeRegistry::shouldAlwaysRevalidateURLScheme(protocol.toString()))

Ditto.

> Source/WebCore/page/Page.cpp:1073
> +    if (SchemeRegistry::shouldTreatURLSchemeAsLocal(url.protocol().toString()))

Ditto.

> Source/WebCore/page/SecurityOrigin.cpp:97
> +    if (SchemeRegistry::shouldTreatURLSchemeAsNoAccess(innerURL.protocol().toString()))

Ditto.

> Source/WebCore/page/SecurityOrigin.cpp:118
> +    if (m_port && isDefaultPortForProtocol(m_port.value(), StringView(m_protocol)))

Peculiar that we have to explicitly convert to StringView here.

> Source/WebCore/page/SecurityOrigin.cpp:204
> +    if (!url.isValid() || SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol().toString()))

Can be toStringWithoutCopying.

> Source/WebCore/page/SecurityOrigin.cpp:208
> +    if (shouldUseInnerURL(url) && SchemeRegistry::shouldTreatURLSchemeAsSecure(extractInnerURL(url).protocol().toString()))

Ditto.

> Source/WebCore/page/SecurityOrigin.cpp:539
>      stringBuilder.append(separatorCharacter);
> -    stringBuilder.appendNumber(m_port);
> +    stringBuilder.appendNumber(m_port.valueOr(0));

Is this what we want? Or should we leave out the separator and port when m_port is not present?

> Source/WebCore/page/SecurityOriginData.cpp:52
> +    return makeString(protocol, "://", host, port ? makeString(":", String::number(port.value())) : String());

This is an inefficient idiom, but presumably OK if this is just for debugging.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:456
> +    if (SchemeRegistry::schemeShouldBypassContentSecurityPolicy(url.protocol().toString()))

Can be toStringWithoutCopying.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:472
> +    if (SchemeRegistry::schemeShouldBypassContentSecurityPolicy(url.protocol().toString()))

Ditto.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:486
> +    if (SchemeRegistry::schemeShouldBypassContentSecurityPolicy(url.protocol().toString()))

Ditto.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:549
> +    if (SchemeRegistry::schemeShouldBypassContentSecurityPolicy(url.protocol().toString()))

Ditto.

> Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:410
> -        port = 0;
> +        ASSERT(port == Nullopt);

This seems kind of ugly. I think we want to write:

    port = Nullopt;

This is an out argument. It’s not the correct style to rely on the value being in a known state before calling the function, even if in practice it is guaranteed.

> Source/WebCore/platform/SchemeRegistry.h:36
> +// FIXME: Make HashSet<String>::contains(StringView) work and use StringViews here.
>  typedef HashSet<String, ASCIICaseInsensitiveHash> URLSchemesMap;

If you are making tweaks and changes like this, then I suggest changing this to "using" instead of "typedef".

> Source/WebCore/platform/URL.cpp:717
> +    if (m_hostEnd >= m_portEnd - 1)

Does not seem safe to subtract 1 from m_portEnd without checking it for zero.

> Source/WebCore/platform/URL.cpp:723
> +    if (m_string.isNull())
> +        return Nullopt;

I don’t see how we could get here for a null string. Maybe if there is a bug in the m_hostEnd/m_portEnd check above?

> Source/WebCore/platform/network/ResourceHandle.cpp:91
> +    if (auto constructor = builtinResourceHandleConstructorMap().get(request.url().protocol().toString()))

Can be toStringWithoutCopying.

> Source/WebCore/platform/network/ResourceHandle.cpp:135
> +    if (auto constructor = builtinResourceHandleSynchronousLoaderMap().get(request.url().protocol().toString())) {

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4426
> +    if (SchemeRegistry::shouldLoadURLSchemeAsEmptyDocument(request.url().protocol().toString()))

Ditto.

[Alex Christensen]

(In reply to comment #12)
> Comment on attachment 292446 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292446&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.h:44
> > +        m_databaseName = String(WTF::HashTableDeletedValue);
> 
> This should use member initialization, not assignment.
> 
> > Source/WebCore/editing/cocoa/DataDetection.mm:159
> > +    return [softLink_DataDetectorsCore_DDURLTapAndHoldSchemes() containsObject:(NSString *)downcast<HTMLAnchorElement>(element).href().protocol().toString().convertToASCIILowercase()];
> 
> Call sites like this one can use toStringWithoutCopying rather than
> toString, because the nature of the code guarantees that no one is going to
> make a reference to the string and keep it alive after the expression is
> done. Not sure if it’s critical to use that function, but that is what it is
> intended for.
Ok.  Ideally we would replace all calls to toStringWithoutCopying with using StringViews instead of Strings.  That'd be a lot of refactoring.
> > Source/WebCore/page/SecurityOrigin.cpp:539
> >      stringBuilder.append(separatorCharacter);
> > -    stringBuilder.appendNumber(m_port);
> > +    stringBuilder.appendNumber(m_port.valueOr(0));
> 
> Is this what we want? Or should we leave out the separator and port when
> m_port is not present?
This is what we want for the database identifier to be backwards-compatible with existing persistent storage.
> > Source/WebCore/platform/URL.cpp:717
> > +    if (m_hostEnd >= m_portEnd - 1)
> 
> Does not seem safe to subtract 1 from m_portEnd without checking it for zero.
Will add check.
> 
> > Source/WebCore/platform/URL.cpp:723
> > +    if (m_string.isNull())
> > +        return Nullopt;
> 
> I don’t see how we could get here for a null string. Maybe if there is a bug
> in the m_hostEnd/m_portEnd check above?
Aha!  That's what's going on!  Adding the zero check to m_portEnd should make this unnecessary.  Null strings are always invalid URLs, and m_hostEnd and m_portEnd would both be zero in that case.

[Alex Christensen]

Created attachment 292630 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/changeset/207769