Summary: | WebSocket onmessageerror event handler | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fumitoshi Ukai <ukai> | ||||||||
Component: | WebCore JavaScript | Assignee: | Fumitoshi Ukai <ukai> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Fumitoshi Ukai
2010-03-02 03:01:43 PST
Created attachment 49877 [details]
Patch
Comment on attachment 49877 [details]
Patch
Should we call onmessageerror on frame length overflow for frames 0x80...0xff?
WebSocket::didReceiveMessageError() probably needs a LOG - we have it even in close(), surely an error condition should have one.
Looks good to me otherwise.
Created attachment 49973 [details]
Patch
(In reply to comment #3) > Created an attachment (id=49973) [details] > Patch Ah, onmessageerror has been changed to onerror http://lists.whatwg.org/pipermail/commit-watchers-whatwg.org/2010/003979.html Comment on attachment 49973 [details]
Patch
The test depends on WebSocket.onclose patch, correct? I'm still thinking about that idea, not sure if I like it or not.
Created attachment 49990 [details]
Patch
(In reply to comment #5) > (From update of attachment 49973 [details]) > The test depends on WebSocket.onclose patch, correct? I'm still thinking about > that idea, not sure if I like it or not. no, it doesn't depend on other patch. Which patch do you mean? OK. I was thinking about bug 35573 (CloseEvent). Comment on attachment 49990 [details] Patch > +void WebSocket::didReceiveMessageError() > +{ > + LOG(Network, "WebSocket %p didReceiveErrorMessage", this); > + if (m_state != OPEN || !scriptExecutionContext()) > + return; Can both these conditions really occur? Seems acceptable to be a little defensive, but maybe there should also be ASSERTs if this isn't supposed to happen. Otherwise, these checks can confuse someone about how this code works. r=me Committed r55573: <http://trac.webkit.org/changeset/55573> (In reply to comment #9) > (From update of attachment 49990 [details]) > > +void WebSocket::didReceiveMessageError() > > +{ > > + LOG(Network, "WebSocket %p didReceiveErrorMessage", this); > > + if (m_state != OPEN || !scriptExecutionContext()) > > + return; > > Can both these conditions really occur? Seems acceptable to be a little > defensive, but maybe there should also be ASSERTs if this isn't supposed to > happen. Otherwise, these checks can confuse someone about how this code works. Ok. I think scriptExecutionContext() should be non null, but m_state might be CLOSED if script closing the WebSocket and events from network happened same time. make ASSERT(scriptExecutionContext()) and leave "if (m_state != OPEN)". > > r=me |