Bug 136219 – Websocket state should be set to closed in didReceiveMessage call back

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Shivakumar J M]

According to W3C specification link: http://www.w3.org/TR/websockets/ , Websocket state should be set to closed in didReceiveMessage call back

[Shivakumar J M]

Created attachment 237076 [details]
Patch

Set the Websocket state to closed in didReceiveMessage call back

[Darin Adler]

Comment on attachment 237076 [details]
Patch

WebKit bug fixes require either a test case or an explanation of why they don’t have one. Please add a regression test.

[Shivakumar J M]

Created attachment 237144 [details]
Patch-Updated

Updated patch with new layout test

[Darin Adler]

Comment on attachment 237144 [details]
Patch-Updated

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:536
> +    m_state = CLOSED;

Do we also need to call m_channel->disconnect()? Do we also need to call ActiveDOMObject::unsetPendingActivity?

[Shivakumar J M]

(In reply to comment #4)
> (From update of attachment 237144 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237144&action=review
> 
> > Source/WebCore/Modules/websockets/WebSocket.cpp:536
> > +    m_state = CLOSED;
> 
> Do we also need to call m_channel->disconnect()? Do we also need to call ActiveDOMObject::unsetPendingActivity?

Dear Darin,

     Thanks for inputs, i rechecked again, i think we no need to close the channel object here, also ActiveDOMObject.

[Darin Adler]

Comment on attachment 237144 [details]
Patch-Updated

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

>>> Source/WebCore/Modules/websockets/WebSocket.cpp:536
>>> +    m_state = CLOSED;
>> 
>> Do we also need to call m_channel->disconnect()? Do we also need to call ActiveDOMObject::unsetPendingActivity?
> 
> Dear Darin,
> 
>      Thanks for inputs, i rechecked again, i think we no need to close the channel object here, also ActiveDOMObject.

I’m glad you think so. Why do you think so?

[Shivakumar J M]

(In reply to comment #6)
> (From update of attachment 237144 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237144&action=review
> 
> >>> Source/WebCore/Modules/websockets/WebSocket.cpp:536
> >>> +    m_state = CLOSED;
> >> 
> >> Do we also need to call m_channel->disconnect()? Do we also need to call ActiveDOMObject::unsetPendingActivity?
> > 
> > Dear Darin,
> > 
> >      Thanks for inputs, i rechecked again, i think we no need to close the channel object here, also ActiveDOMObject.
> 
> I’m glad you think so. Why do you think so?


Dear Darin,

Here is my analysis, When i ran the new test cases error-event-ready-state.html, first we get onerror() call back, where the proper state will be set, after these onclose() will get called, so here the channel object will get closed and ActiveDOMObject as well. so we no need to release the resources in didReceiveMessageError() call back.

I tried by closing channel object in didReceiveMessageError() call back. but it was failing some other tests, with error: FAIL: Timed out waiting for notifyDone to be called.

pls share your inputs, if my understanding is not correct.

[Darin Adler]

Alexey, you know this code better than I do. Could you review?

[Alexey Proskuryakov]

Comment on attachment 237144 [details]
Patch-Updated

The existing code is somewhat confusing, but I think that the patch a step in the right direction.

The key consideration is that there is only one place where an error event is dispatched per the spec, and that's right before a close event.

[WebKit Commit Bot]

Comment on attachment 237144 [details]
Patch-Updated

Clearing flags on attachment: 237144

Committed r173642: <http://trac.webkit.org/changeset/173642>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.