Bug 157220 – On platforms that support it, use a SecTrustRef as the basis of CertificateInfo instead of a chain of SecCertificateRefs.

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

[Sam Weinig]

On platforms that support it, use a SecTrustRef as the basis of CertificateInfo instead of a chain of SecCertificateRefs.

[Sam Weinig]

Created attachment 277776 [details]
Patch

[WebKit Commit Bot]

Attachment 277776 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/Authentication/mac/AuthenticationManager.mac.mm:57:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 277776 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Sam Weinig]

Created attachment 277794 [details]
Patch 2

[WebKit Commit Bot]

Attachment 277794 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/Authentication/mac/AuthenticationManager.mac.mm:57:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 277794 [details]
Patch 2

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

> Source/WebCore/platform/network/cf/CertificateInfo.h:39
> +    CertificateInfo()
> +    {
> +    }

Maybe "= default"? Would be nice to keep these function bodies out of the class definition so it’s easier to read.

> Source/WebCore/platform/network/cf/CertificateInfo.h:53
> +    explicit CertificateInfo(RetainPtr<SecTrustRef> trust)
> +        : m_trust(trust)
> +    {
> +    }

Just SecTrustRef for argument type? Or WTFMove to avoid retain churn? Maybe rvalue reference for argument type?

> Source/WebCore/platform/network/cf/CertificateInfo.h:61
>      CertificateInfo(RetainPtr<CFArrayRef> certificateChain)
>          : m_certificateChain(certificateChain)
> -    { }
> +    {
> +    }

Same questions.

> Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:109
> +    return CertificateInfo(adoptCF((CFArrayRef)certificateChain));

Why do we need the cast to CFArrayRef? Ambiguous or something?

> Source/WebCore/platform/network/mac/CertificateInfoMac.mm:55
> +        RetainPtr<NSMutableArray> array = adoptNS([[NSMutableArray alloc] init]);
> +        for (CFIndex i = 0, size = SecTrustGetCertificateCount(trust()); i < size; ++i)
> +            [array addObject:(id)SecTrustGetCertificateAtIndex(trust(), i)];
> +        m_certificateChain = (CFArrayRef)array.get();

This is  the same as the end of ResourceResponse::platformCertificateInfo. Can these share code?

> Source/WebCore/platform/network/mac/CertificateInfoMac.mm:59
> +    return m_certificateChain.get();

Kind of strange to fall into this in the HAVE(SEC_TRUST_SERIALIZATION) case. It’s just a less efficient way to return nullptr.

> Source/WebCore/platform/network/mac/CertificateInfoMac.mm:67
> +        for (CFIndex i = 0, size = SecTrustGetCertificateCount(trust()) - 1; i < size; ++i) {

What guarantees count is not zero?

> Source/WebCore/platform/network/mac/CertificateInfoMac.mm:68
> +            SecCertificateRef certificate = SecTrustGetCertificateAtIndex(trust(), i);

auto?

> Source/WebCore/platform/network/mac/CertificateInfoMac.mm:80
> +        for (CFIndex i = 0, size = CFArrayGetCount(m_certificateChain.get()) - 1; i < size; ++i) {

What guarantees the array count is not zero?

> Source/WebKit2/Shared/Authentication/mac/AuthenticationManager.mac.mm:56
> +    NSMutableArray *array = [NSMutableArray array];

arrayWithCapacity would be better

[Darin Adler]

Looks like the Windows build failed.

[Sam Weinig]

Committed r200463: <http://trac.webkit.org/changeset/200463>