[webkit-reviews] review granted: [Bug 57707] WebKit2: Specify the certificate store in WKBundleSetClientCertificate() : [Attachment 87980] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Jeff Miller <jeffm at apple.com>'s request for review:
Bug 57707: WebKit2: Specify the certificate store in
WKBundleSetClientCertificate()
https://bugs.webkit.org/show_bug.cgi?id=57707

Attachment 87980: Patch
https://bugs.webkit.org/attachment.cgi?id=87980&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=87980&action=review

> Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:114
> +    ASSERT(!certificateSystemStoreName.isEmpty());

You can use ASSERT_ARG here.

> Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:122
> +    ASSERT(certStore);

I don’t know this Windows API, but I’m not sure it’s correct to assert this. At
the very least it asserts that the bundle code, which is not part of WebKit,
isn’t sending a bogus store name. But even if the store name was correct at
some point, isn’t it possible that by the time we reach this statement, the
store has been deleted, renamed, or otherwise made un-openable? Maybe
LOG_ERROR() is more appropriate than ASSERT().

> Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:127
> +    ASSERT(realCert);

Similar comment here.

> Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:134
> +    // We can't close certStore here, since the certificate is still in use.


So we never close it. Is this OK?