WebKit Bugzilla
In order to display an invalid certificate, the UI process needs access to the certificate from the web process on Windows. Keep track of the certificate in the WebCore::ResourceError and add support for encoding this in the message that's sent to the UI process.
Created attachment 87092 [details] Patch
Comment on attachment 87092 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87092&action=review Seems worth another go-round. But this looks great! > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:142 > + CFErrorRef cfError = resourceError.cfError(); > + if (!cfError) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + } > + > + RetainPtr<CFDictionaryRef> userInfo(AdoptCF, CFErrorCopyUserInfo(cfError)); > + if (!userInfo) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + } > + > + PCCERT_CONTEXT certificate = static_cast<PCCERT_CONTEXT>(wkGetSSLPeerCertificateDataPtr(userInfo.get())); > + if (!certificate) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + } Why can't we just use ResourceError::m_certificate here? > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:152 > + CertFreeCertificateContext(reinterpret_cast<PCCERT_CONTEXT>(ptr)); static_cast should work fine. > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:158 > + PCCERT_CONTEXT certCopy = 0; You can wait to declare this until you initialize it on line 165. > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:164 > + if (!certDealloc) { > + CFAllocatorContext allocContext = { > + 0, 0, 0, 0, 0, 0, 0, deallocCertContext, 0 > + }; > + certDealloc = CFAllocatorCreate(kCFAllocatorDefault, &allocContext); > + } Having a separate createCertContextDeallocator function would clean this up a bit. Then you could just assign its result into the certDealloc variable and not need an if. > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:187 > + resourceError = WebCore::ResourceError(domain, errorCode, failingURL, localizedDescription, copyCert(certificateChain[0])); I personally prefer .first() instead of [0].
(In reply to comment #2) > (From update of attachment 87092 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87092&action=review > > Seems worth another go-round. But this looks great! > > > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:142 > > + CFErrorRef cfError = resourceError.cfError(); > > + if (!cfError) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > + > > + RetainPtr<CFDictionaryRef> userInfo(AdoptCF, CFErrorCopyUserInfo(cfError)); > > + if (!userInfo) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > + > > + PCCERT_CONTEXT certificate = static_cast<PCCERT_CONTEXT>(wkGetSSLPeerCertificateDataPtr(userInfo.get())); > > + if (!certificate) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > Why can't we just use ResourceError::m_certificate here? Good idea, I added a public certificate() method so we can do that. I also addressed all your other comments in the patch I'm about to upload.
Created attachment 87164 [details] Patch
Attachment 87164 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 WebKitLibraries/ChangeLog:15: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 87164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87164&action=review > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:130 > + CFErrorRef cfError = resourceError.cfError(); > + if (!cfError) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + } I don't think this is needed anymore. > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:144 > + CFDataRef certificateData = resourceError.certificate(); > + if (!certificateData) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + } > + > + PCCERT_CONTEXT certificate = reinterpret_cast<PCCERT_CONTEXT>(CFDataGetBytePtr(certificateData)); > + if (!certificate) { > + encoder->encode(WebKit::PlatformCertificateInfo()); > + return; > + } > + > + encoder->encode(WebKit::PlatformCertificateInfo(certificate)); Maybe ResourceError should have an accessor that returns a PCCERT_CONTEXT? And/or maybe we should have a function that returns a PlatformCertificateInfo given a ResourceError? What you have now seems fine, too, but these other functions might make it cleaner.
I filed https://bugs.webkit.org/show_bug.cgi?id=57250 for the check-webkit-style failure, which I think is a false positive.
(In reply to comment #6) > (From update of attachment 87164 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87164&action=review > > > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:130 > > + CFErrorRef cfError = resourceError.cfError(); > > + if (!cfError) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > I don't think this is needed anymore. > > > Source/WebKit2/Shared/win/WebCoreArgumentCodersWin.cpp:144 > > + CFDataRef certificateData = resourceError.certificate(); > > + if (!certificateData) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > + > > + PCCERT_CONTEXT certificate = reinterpret_cast<PCCERT_CONTEXT>(CFDataGetBytePtr(certificateData)); > > + if (!certificate) { > > + encoder->encode(WebKit::PlatformCertificateInfo()); > > + return; > > + } > > + > > + encoder->encode(WebKit::PlatformCertificateInfo(certificate)); > > Maybe ResourceError should have an accessor that returns a PCCERT_CONTEXT? And/or maybe we should have a function that returns a PlatformCertificateInfo given a ResourceError? What you have now seems fine, too, but these other functions might make it cleaner. PlatformCertificateInfo is part of WebKit, and ResourceError is in WebCore, and I didn't want to cross the streams. However, I agree I could make this better, which I'll look at doing in another patch.
Committed r82137: <http://trac.webkit.org/changeset/82137>
http://trac.webkit.org/changeset/82137 might have broken Leopard Intel Release (Tests)