| Summary: | URL::port should return Optional<uint16_t> | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||||||
| Component: | Platform | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, darin, rniwa | ||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Alex Christensen
2016-10-21 12:42:43 PDT
Created attachment 292385 [details]
Patch
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. 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
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. 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
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. 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
Created attachment 292428 [details]
Patch
Created attachment 292432 [details]
Patch
Created attachment 292443 [details]
Patch
Created attachment 292446 [details]
Patch
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. (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. Created attachment 292630 [details]
Patch
|