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)