WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
40945
WebSocket errors should be logged to console
https://bugs.webkit.org/show_bug.cgi?id=40945
Summary
WebSocket errors should be logged to console
Alexey Proskuryakov
Reported
2010-06-21 14:22:30 PDT
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.
Attachments
Patch
(10.29 KB, patch)
2010-12-17 04:23 PST
,
Yuta Kitamura
no flags
Details
Formatted Diff
Diff
Patch v2
(13.82 KB, patch)
2010-12-20 06:20 PST
,
Yuta Kitamura
ap
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1
2010-06-21 14:27:42 PDT
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
Comment 2
2010-12-06 03:57:58 PST
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
Comment 3
2010-12-17 04:23:26 PST
Created
attachment 76866
[details]
Patch
Alexey Proskuryakov
Comment 4
2010-12-17 09:16:16 PST
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
Comment 5
2010-12-19 19:13:39 PST
(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
Comment 6
2010-12-19 19:38:24 PST
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
Comment 7
2010-12-19 20:27:19 PST
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
Comment 8
2010-12-19 20:51:02 PST
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
Comment 9
2010-12-19 20:52:05 PST
And same for description, of course - CFErrorCopyDescription is also a "copy" function.
Yuta Kitamura
Comment 10
2010-12-20 06:20:06 PST
Created
attachment 76994
[details]
Patch v2
Yuta Kitamura
Comment 11
2010-12-20 06:22:55 PST
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 12
2010-12-20 08:44:45 PST
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
Comment 13
2010-12-20 21:44:28 PST
Committed
r74390
: <
http://trac.webkit.org/changeset/74390
>
Alexey Proskuryakov
Comment 14
2010-12-20 21:55:26 PST
I've filed <
rdar://problem/8787916
> about making CFErrorCopyDescription use a better string for Security framework's OSStatus errors.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug