Bug 40945 – WebSocket errors should be logged to console

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexey Proskuryakov]

Currently, there is no explanation of why a WebSocket connection is not established - all a developer gets is a "close" event.

While this is correct behavior for Web content, it's extremely unfriendly to developers.

[Alexey Proskuryakov]

One particularly mysterious case that we need to report is failing to establish a connection due to SSL failures.

For CFNetwork, we can use APIs such as CFErrorCopyDescription() to report the errors.

[Yuta Kitamura]

I will take care of this.

Here is rough sketch of my implementation plan:

* Add some members (failingURL and localizedDescription for example, as in ResourceError) to SocketStreamError.
* Pass the reason of a failure to SocketStreamError instance (in each port), and notify it via SocketStreamHandleClient::didFail().
* Log the reason of failure in WebSocketChannel::didFail handler.

[Yuta Kitamura]

Created attachment 76866 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 76866 [details]
Patch

+ws://nonexistent.domain.invalid:80/WebSocket network error: error code 2 (Operation could not be completed. (kCFErrorDomainCFNetwork error 2.))

It isn't great to print the almost useless error code twice.

This bug is originally about invalid SSL certificates. Did you check that a useful message is printed for those? How does it look?

+#ifdef BUILDING_ON_TIGER

Please put modern code first, and Tiger specific in a fallback branch (i.e. please reverse both ifdefs).

This is definitely an improvement, r=me. Please do check that this addresses the original issue reported here (printing an error for SSL certificates).

[Yuta Kitamura]

(In reply to comment #4)
> (From update of attachment 76866 [details])
> +ws://nonexistent.domain.invalid:80/WebSocket network error: error code 2 (Operation could not be completed. (kCFErrorDomainCFNetwork error 2.))
> 
> It isn't great to print the almost useless error code twice.

I will change the code so that it will not show errorCode when localizedDescription is provided.

> 
> This bug is originally about invalid SSL certificates. Did you check that a useful message is printed for those? How does it look?

I tried, and it looked like:
>WebSocket network error: error code -9812 (Operation could not be completed. (OSStatus Error -9812))
...which sounds less helpful.

It looks like this status code comes from SecureTransport.h: 
>errSSLUnknownRootCert		= -9812,	/* valid cert chain, untrusted root */

Do you have any idea about how to improve this message? I do not have a good experience on Mac programming, and do not know a portable way to convert this status code into human-readable description.


> 
> +#ifdef BUILDING_ON_TIGER
> 
> Please put modern code first, and Tiger specific in a fallback branch (i.e. please reverse both ifdefs).

Sure.

> 
> This is definitely an improvement, r=me. Please do check that this addresses the original issue reported here (printing an error for SSL certificates).

Thank you for your review!

[Yuta Kitamura]

On Windows, the error message about SSL certificate will look like:

> WebSocket network error: error code -2146762487 (The operation couldn’t be completed. (OSStatus error -2146762487.))

I have no idea about where this value comes from...

[Alexey Proskuryakov]

For errors in kCFErrorDomainOSStatus domain, GetMacOSStatusCommentString() might print a useful message. I'd be slightly surprised if that worked when CFErrorCopyDescription() didn't, but seems worth a try.

[Alexey Proskuryakov]

Oops, I just noticed a memory leak:

+        RetainPtr<CFErrorRef> error(CFReadStreamCopyError(m_readStream.get()));

This should be (in two places):

+        RetainPtr<CFErrorRef> error(AdoptCF, CFReadStreamCopyError(m_readStream.get()));

[Alexey Proskuryakov]

And same for description, of course - CFErrorCopyDescription is also a "copy" function.

[Yuta Kitamura]

Created attachment 76994 [details]
Patch v2

[Yuta Kitamura]

I'm requesting a review again because I made significant changes to the previous patch:
- Changed error handling in SocketStreamHandle to handle OSStatus
- Added a test to check invalid SSL certificate.

[Alexey Proskuryakov]

Comment on attachment 76994 [details]
Patch v2

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

> WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:599
> +void SocketStreamHandle::notifyCFErrorToClient(CFErrorRef error)

The name of this function is not grammatically correct. I'd say "reportErrorToClient" or just "reportError" - there is no need to encode argument type in funticon name.

[Yuta Kitamura]

Committed r74390: <http://trac.webkit.org/changeset/74390>

[Alexey Proskuryakov]

I've filed <rdar://problem/8787916> about making CFErrorCopyDescription use a better string for Security framework's OSStatus errors.