Bug 35570 - WebSocket onmessageerror event handler
Summary: WebSocket onmessageerror event handler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Fumitoshi Ukai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-02 03:01 PST by Fumitoshi Ukai
Modified: 2010-03-05 01:37 PST (History)
1 user (show)

See Also:


Attachments
Patch (8.56 KB, patch)
2010-03-02 21:42 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (9.14 KB, patch)
2010-03-03 18:26 PST, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Patch (8.69 KB, patch)
2010-03-03 23:28 PST, Fumitoshi Ukai
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 2010-03-02 03:01:43 PST
WebSocket onmessageerror event handler is added to report unexpected frames.
http://lists.whatwg.org/pipermail/commit-watchers-whatwg.org/2010/003975.html
Comment 1 Fumitoshi Ukai 2010-03-02 21:42:11 PST
Created attachment 49877 [details]
Patch
Comment 2 Alexey Proskuryakov 2010-03-03 14:42:32 PST
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.
Comment 3 Fumitoshi Ukai 2010-03-03 18:26:46 PST
Created attachment 49973 [details]
Patch
Comment 4 Fumitoshi Ukai 2010-03-03 19:37:36 PST
(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 5 Alexey Proskuryakov 2010-03-03 22:18:25 PST
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.
Comment 6 Fumitoshi Ukai 2010-03-03 23:28:16 PST
Created attachment 49990 [details]
Patch
Comment 7 Fumitoshi Ukai 2010-03-03 23:29:31 PST
(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?
Comment 8 Alexey Proskuryakov 2010-03-04 00:08:59 PST
OK. I was thinking about bug 35573 (CloseEvent).
Comment 9 Alexey Proskuryakov 2010-03-04 10:46:46 PST
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
Comment 10 Fumitoshi Ukai 2010-03-05 01:35:41 PST
Committed r55573: <http://trac.webkit.org/changeset/55573>
Comment 11 Fumitoshi Ukai 2010-03-05 01:37:53 PST
(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