WebSocket onmessageerror event handler is added to report unexpected frames. http://lists.whatwg.org/pipermail/commit-watchers-whatwg.org/2010/003975.html
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