[webkit-reviews] review denied: [Bug 172630] Add support for wss:// websockets to wincairo : [Attachment 311335] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied
isaac+webkit at devinesystems.co.nz's request for review:
Bug 172630: Add support for wss:// websockets to wincairo
https://bugs.webkit.org/show_bug.cgi?id=172630

Attachment 311335: Patch

https://bugs.webkit.org/attachment.cgi?id=311335&action=review




--- Comment #2 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 311335
  --> https://bugs.webkit.org/attachment.cgi?id=311335
Patch

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

> Source/WebCore/ChangeLog:12
> +	   Add support for wss:// websockets to wincairo

This is a great thing.	Sorry I hadn't noticed this patch before now.  Let's
polish up this code and get it in.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:50
> +static CString certificatePath()

This code exists already in CurlContext.cpp, and that's probably a better
version because it correctly surrounds the CF code with #if USE(CF).  If we
need to use it here, let's instead put String certificatePath() in a header
somewhere and use the one copy of the code.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:78
> +    , m_certificatePath(certificatePath())

This already exists on CurlContext::singleton(). There's no need for a
SocketStreamHandleImpl to keep the path of the certificate file.

> Source/WebCore/platform/network/curl/SocketStreamHandleImplCurl.cpp:216
> +	   callOnMainThread([this, ret] {
> +	       // Check reference count to fix a crash.
> +	       // When the call is invoked on the main thread after all other
references are released,
> +	       // the SocketStreamClient is already deleted. Accessing the
SocketStreamClient will then cause a crash.
> +	       if (refCount() > 1) {

This is bad.  If we need to prevent use-after-free bugs when calling something
on the main thread, then we should capture protectedThis = makeRef(*this) to
keep the SocketStreamHandleImpl alive.	I'm not sure this is a good design,
though.  I see you're just moving code, but I think this should be improved.