Bug 160144 - Add specialization for encoding/decoding WebCore::CertificateInfos in the Network Cache
Summary: Add specialization for encoding/decoding WebCore::CertificateInfos in the Net...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-24 10:54 PDT by Sam Weinig
Modified: 2016-07-25 11:47 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.77 KB, patch)
2016-07-24 10:56 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (9.80 KB, patch)
2016-07-24 16:00 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (9.65 KB, patch)
2016-07-24 16:23 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2016-07-24 10:54:24 PDT
Add specialization for encoding/decoding WebCore::CertificateInfos in the Network Cache
Comment 1 Sam Weinig 2016-07-24 10:56:27 PDT
Created attachment 284447 [details]
Patch
Comment 2 Sam Weinig 2016-07-24 10:57:21 PDT
<rdar://problem/27409315>
Comment 3 Chris Dumez 2016-07-24 11:33:53 PDT
Comment on attachment 284447 [details]
Patch

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

r=me % nits.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.cpp:169
> +    data = adoptCF(CFDataCreate(0, vector.data(), vector.size()));

0 -> nullptr

> Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.cpp:200
> +    if (!trustData)

Looks like this could be an assertion instead? decodeCFData() would have returned value if trustData were null.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.cpp:225
> +        RetainPtr<CFDataRef> data = adoptCF(SecCertificateCopyData((SecCertificateRef)values[i]));

Could use auto

> Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.cpp:236
> +    RetainPtr<CFMutableArrayRef> array = adoptCF(CFArrayCreateMutable(0, 0, &kCFTypeArrayCallBacks));

could use auto.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheCoders.cpp:247
> +    certificateChain = adoptCF(array.leakRef());

certificateChain = WTFMove(array);

may be a bit nicer.
Comment 4 Sam Weinig 2016-07-24 16:00:04 PDT
Created attachment 284452 [details]
Patch
Comment 5 Sam Weinig 2016-07-24 16:23:35 PDT
Created attachment 284454 [details]
Patch
Comment 6 Sam Weinig 2016-07-24 17:50:46 PDT
Fixed in https://trac.webkit.org/r203671.
Comment 7 Ryan Haddad 2016-07-25 10:11:37 PDT
Debug WK2 tests are exiting early after failing an assertion added with this change:
ASSERT(CFGetTypeID(values[i]) != SecCertificateGetTypeID()); 

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r203684%20(6840)/results.html
Comment 8 Sam Weinig 2016-07-25 11:47:13 PDT
Fixed assertion in https://trac.webkit.org/r203692.