Bug 76521 – WebSocket: MessageEvent fired during send() on workers

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

[Yuta Kitamura]

Chromium bug: http://code.google.com/p/chromium/issues/detail?id=108626

There is a bug in WorkerThreadableWebSocketChannel that it may fire a MessageEvent while it is waiting for the result of a synchronous operation (WebSocket.send() or WebSocket.bufferedAmount). The event should not be fired until the user script exits the current cycle of the event loop.

[Yuta Kitamura]

Created attachment 122886 [details]
Patch

[David Levin]

Comment on attachment 122886 [details]
Patch

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

> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:190
> +    if (!m_syncMethodDone) {

Why go through all of this?

If you post a task without a mode, then it won't run until the nested loop is done.

[Yuta Kitamura]

Comment on attachment 122886 [details]
Patch

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

>> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:190
>> +    if (!m_syncMethodDone) {
> 
> Why go through all of this?
> 
> If you post a task without a mode, then it won't run until the nested loop is done.

Sorry, I digged the code a bit but I still don't get the idea of what the "mode" is.

WorkerThreadableWebSocketChannel posts tasks with m_taskMode specified as the mode (which is generated within ThreadableWebSocketChannel::create()). Is this wrong?

[David Levin]

(In reply to comment #3)
> (From update of attachment 122886 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122886&action=review
> 
> >> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:190
> >> +    if (!m_syncMethodDone) {
> > 
> > Why go through all of this?
> > 
> > If you post a task without a mode, then it won't run until the nested loop is done.
> 
> Sorry, I digged the code a bit but I still don't get the idea of what the "mode" is.
> 
> WorkerThreadableWebSocketChannel posts tasks with m_taskMode specified as the mode (which is generated within ThreadableWebSocketChannel::create()). Is this wrong?

When a nested loop runs in a worker, it runs in a "mode". The mode is just a string but that string acts as a filter. It only allows tasks that are posted using that "mode" (the same string) to run during that nested loop. If a task is posted with no mode, then the task isn't run until the nested loop is done.

In general, the code should posts tasks using the m_taskMode (or else they won't run at all during this nested message loop.). However, in this case, for this one task, it sounds like you want to run a task but not until the nested loop is done, so if the code uses "postTask" instead of "postTaskForMode" (or uses "" as the mode if necessary), then you will get the desired result.

Hope that helps.

[Yuta Kitamura]

Comment on attachment 122886 [details]
Patch

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

>>>> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:190
>>>> +    if (!m_syncMethodDone) {
>>> 
>>> Why go through all of this?
>>> 
>>> If you post a task without a mode, then it won't run until the nested loop is done.
>> 
>> Sorry, I digged the code a bit but I still don't get the idea of what the "mode" is.
>> 
>> WorkerThreadableWebSocketChannel posts tasks with m_taskMode specified as the mode (which is generated within ThreadableWebSocketChannel::create()). Is this wrong?
> 
> When a nested loop runs in a worker, it runs in a "mode". The mode is just a string but that string acts as a filter. It only allows tasks that are posted using that "mode" (the same string) to run during that nested loop. If a task is posted with no mode, then the task isn't run until the nested loop is done.
> 
> In general, the code should posts tasks using the m_taskMode (or else they won't run at all during this nested message loop.). However, in this case, for this one task, it sounds like you want to run a task but not until the nested loop is done, so if the code uses "postTask" instead of "postTaskForMode" (or uses "" as the mode if necessary), then you will get the desired result.
> 
> Hope that helps.

Okay, I think I got your point. I'm going to update the patch.

[Yuta Kitamura]

Comment on attachment 122886 [details]
Patch

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

>>>>> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:190
>>>>> +    if (!m_syncMethodDone) {
>>>> 
>>>> Why go through all of this?
>>>> 
>>>> If you post a task without a mode, then it won't run until the nested loop is done.
>>> 
>>> Sorry, I digged the code a bit but I still don't get the idea of what the "mode" is.
>>> 
>>> WorkerThreadableWebSocketChannel posts tasks with m_taskMode specified as the mode (which is generated within ThreadableWebSocketChannel::create()). Is this wrong?
>> 
>> When a nested loop runs in a worker, it runs in a "mode". The mode is just a string but that string acts as a filter. It only allows tasks that are posted using that "mode" (the same string) to run during that nested loop. If a task is posted with no mode, then the task isn't run until the nested loop is done.
>> 
>> In general, the code should posts tasks using the m_taskMode (or else they won't run at all during this nested message loop.). However, in this case, for this one task, it sounds like you want to run a task but not until the nested loop is done, so if the code uses "postTask" instead of "postTaskForMode" (or uses "" as the mode if necessary), then you will get the desired result.
>> 
>> Hope that helps.
> 
> Okay, I think I got your point. I'm going to update the patch.

After some investigation, I started to think postTask'ing pending tasks (e.g. did*Callback()) is infeasible because we can't call these callbacks when the worker is suspended. We need to process these events in the chronological order, and if we postTask'ed each event it would be difficult to handle suspension correctly. That's why there's m_pendingTasks, I guess.

(Maybe the responsibility of suspension handling should exist in WebSocket class, not here; but fixing it would require another work.)

[David Levin]

Comment on attachment 122886 [details]
Patch

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

Thanks for the explanation. (I think I led you astray.)

> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:188
> +    if (m_suspended)

Since you are adding this check, perhaps in another patch (not this patch), you could change the many places that do this


if (!m_suspended)
    processPendingTasks();

to

     processPendingTasks();

[Yuta Kitamura]

Thanks. I somehow missed you r+ed my patch (sorry!), so I'm trying CQ now...

[WebKit Review Bot]

Comment on attachment 122886 [details]
Patch

Clearing flags on attachment: 122886

Committed r107788: <http://trac.webkit.org/changeset/107788>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.